Skip to content

Add tenant filter, verbose logging, and default api version#29

Open
jmattheus wants to merge 4 commits into
Azure:mainfrom
jmattheus:AddOptions
Open

Add tenant filter, verbose logging, and default api version#29
jmattheus wants to merge 4 commits into
Azure:mainfrom
jmattheus:AddOptions

Conversation

@jmattheus
Copy link
Copy Markdown

No description provided.

@legra-ms
Copy link
Copy Markdown
Contributor

legra-ms commented Oct 29, 2020

While it might help a little, I think specifying the api version of ARM isn't the best implementation. That's because API versions vary by resource type, so a VM may have a different list of API versions for each resource. That 2018 date was selected because the original version of Stormspotter was written in early 2019 so getting resources from an API from that far back isn't ideal. Also, I'm not sure that user really needs any control over the api version, since we generally just want them to have the most updated API, especially since a lot of the older ones have a different structure for additional_properties or a few other things.

So the options would be to either specify a new common API across resources by default, or just build the cache as you mentioned in Teams. I'm a bigger fan of the cache, since that has a greater chance of reducing the amount of requests made with longer runs.

Alternatively, we could create a dictionary of resources and their latest api versions. I'm not sure if that info is easily accessible but we can probably just parse https://github.com/Azure/azure-sdk-for-python to grab the latest API versions from each management client and load that info at runtime.

@legra-ms
Copy link
Copy Markdown
Contributor

legra-ms commented Nov 1, 2020

So it turns out that we can list the resource providers per subscription and get the API versions from there. It's actually already part of the same package already being used, which is perfect. I think listing these providers and then grabbing the defaultApiVersion or potentially the first in the list of the apiVersions which may or may not be a preview (not sure if it matters but in most cases, newer is better) is the best way to go. Here's an example of the Storage Account provider:

...
      {
        "aliases": null,
        "apiProfiles": [
          {
            "apiVersion": "2017-10-01",
            "profileVersion": "2019-03-01-hybrid"
          },
          {
            "apiVersion": "2016-01-01",
            "profileVersion": "2017-03-09-profile"
          },
          {
            "apiVersion": "2016-01-01",
            "profileVersion": "2018-03-01-hybrid"
          },
          {
            "apiVersion": "2017-10-01",
            "profileVersion": "2018-06-01-profile"
          }
        ],
        "apiVersions": [
          "2020-08-01-preview",
          "2019-06-01",
          "2019-04-01",
          "2018-11-01",
          "2018-07-01",
          "2018-03-01-preview",
          "2018-02-01",
          "2017-10-01",
          "2017-06-01",
          "2016-12-01",
          "2016-05-01",
          "2016-01-01",
          "2015-06-15",
          "2015-05-01-preview"
        ],
        "capabilities": "CrossResourceGroupResourceMove, CrossSubscriptionResourceMove, SystemAssignedResourceIdentity, SupportsTags, SupportsLocation",
        "defaultApiVersion": "2019-06-01",
        "locations": [
          "East US",
          "East US 2",
          "West US",
          "West Europe",
          "East Asia",
          "Southeast Asia",
          "Japan East",
          "Japan West",
          "North Central US",
          "South Central US",
          "Central US",
          "North Europe",
          "Brazil South",
          "Australia East",
          "Australia Southeast",
          "South India",
          "Central India",
          "West India",
          "Canada East",
          "Canada Central",
          "West US 2",
          "West Central US",
          "UK South",
          "UK West",
          "Korea Central",
          "Korea South",
          "France Central",
          "Australia Central",
          "South Africa North",
          "UAE North",
          "Switzerland North",
          "Germany West Central",
          "Norway East"
        ],
        "properties": null,
        "resourceType": "storageAccounts"
      },
...

I say newer is better because last I checked, there are a few API versions for some resources that were not with the expected structures, so I have no issue using the preview versions.

@jmattheus
Copy link
Copy Markdown
Author

I agree. I actually have half a branch that gets gets the version from the RP but I didn't stop to look at the parsing logic to understand if the latest was really what we wanted rather than just a fallback.

I think ripping the arm version arg out of this PR then adding the version caching layer in a subsequent PR is the right approach.

@legra-ms
Copy link
Copy Markdown
Contributor

legra-ms commented Nov 4, 2020

I just realized this might cause some unwanted behavior on the AAD side. There won't be an issue with SPN credentials, since those require that the tenant be specified in the arguments (and the tenants argument here will be ignored), but additional logic is going to have to be implemented to handle CLI credentials or possibly others in the future.

AAD enumeration is done via REST APIs. The REST endpoints include the tenant ID in them (i.e. https://graph.windows.net/<tenantID>/users). By default, the tenant is set to myorganization which is associated with the tenant of the users principal name. So user@stormspotter.onmicrosoft.com would just check stormspotter tenant.

If a user has access to multiple tenants (they might be a guest in another tenant), they may expect to enumerate all the tenants they have access to. The AAD logic needs to account for this. The gather tasks would need to reflect that there are multiple tenants to enumerate.

@legra-ms
Copy link
Copy Markdown
Contributor

Doing some work on Stormspotter this week so I'm going to revisit this idea and separate them into different PRs.

@AkechiShiro
Copy link
Copy Markdown

@legra-ms any news is this project currently maintained or in low maintenance?

Is it just abandoned because, I've seen the last commit on main is from 3 years ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants