Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDK tweaks #276

Merged
merged 7 commits into from
Feb 4, 2023
Merged

SDK tweaks #276

merged 7 commits into from
Feb 4, 2023

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Feb 1, 2023

  • Environments: tidy up endpoints and resource identifiers
  • Environments: try to support environments with older schemas like USGovernment which has reverted to an older ARM Metadata schema version (see below)
  • Claims: should not be an internal package
  • Auth: export the tenant ID when using CLI authentication, as it is auto detected when unspecified
  • Add Microsoft Graph base client

On failure to parse metadata, debug logs should contain the metadata response for troubleshooting purposes

/opt/homebrew/Cellar/go/1.19.5/libexec/bin/go tool test2json -t /private/var/folders/24/xsk_v43x59j9gvchfjp91j7w0000gq/T/GoLand/___endpoint_refresh_test_go.test -test.v -test.paniconexit0 -test.run ^\QTestEndpointRefresh_China\E|\QTestEndpointRefresh_Public\E|\QTestEndpointRefresh_USGovernment\E$
=== RUN   TestEndpointRefresh_China
--- PASS: TestEndpointRefresh_China (7.66s)
=== RUN   TestEndpointRefresh_Public
--- PASS: TestEndpointRefresh_Public (0.93s)
=== RUN   TestEndpointRefresh_USGovernment
2023/02/02 12:29:15 [DEBUG] Unrecognised metadata response for https://management.usgovcloudapi.net/metadata/endpoints?api-version=2022-09-01: [{"portal":"https://portal.azure.com","authentication":{"loginEndpoint":"https://login.microsoftonline.com/","audiences":["https://management.core.windows.net/","https://management.azure.com/"],"tenant":"common","identityProvider":"AAD"},"media":"https://rest.media.azure.net","graphAudience":"https://graph.windows.net/","graph":"https://graph.windows.net/","name":"AzureCloud","suffixes":{"azureDataLakeStoreFileSystem":"azuredatalakestore.net","acrLoginServer":"azurecr.io","sqlServerHostname":"database.windows.net","azureDataLakeAnalyticsCatalogAndJob":"azuredatalakeanalytics.net","keyVaultDns":"vault.azure.net","storage":"core.windows.net","azureFrontDoorEndpointSuffix":"azurefd.net"},"batch":"https://batch.core.windows.net/","resourceManager":"https://management.azure.com/","vmImageAliasDoc":"https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-compute/quickstart-templates/aliases.json","activeDirectoryDataLake":"https://datalake.azure.net/","sqlManagement":"https://management.core.windows.net:8443/","gallery":"https://gallery.azure.com/"},{"portal":"https://portal.azure.cn","authentication":{"loginEndpoint":"https://login.chinacloudapi.cn","audiences":["https://management.core.chinacloudapi.cn","https://management.chinacloudapi.cn"],"tenant":"common","identityProvider":"AAD"},"media":"https://rest.media.chinacloudapi.cn","graphAudience":"https://graph.chinacloudapi.cn","graph":"https://graph.chinacloudapi.cn","name":"AzureChinaCloud","suffixes":{"acrLoginServer":"azurecr.cn","sqlServerHostname":"database.chinacloudapi.cn","keyVaultDns":"vault.azure.cn","storage":"core.chinacloudapi.cn","azureFrontDoorEndpointSuffix":""},"batch":"https://batch.chinacloudapi.cn","resourceManager":"https://management.chinacloudapi.cn","vmImageAliasDoc":"https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-compute/quickstart-templates/aliases.json","sqlManagement":"https://management.core.chinacloudapi.cn:8443","gallery":"https://gallery.chinacloudapi.cn"},{"portal":"https://portal.azure.us","authentication":{"loginEndpoint":"https://login.microsoftonline.us","audiences":["https://management.core.usgovcloudapi.net","https://management.usgovcloudapi.net"],"tenant":"common","identityProvider":"AAD"},"media":"https://rest.media.usgovcloudapi.net","graphAudience":"https://graph.windows.net","graph":"https://graph.windows.net","name":"AzureUSGovernment","suffixes":{"acrLoginServer":"azurecr.us","sqlServerHostname":"database.usgovcloudapi.net","keyVaultDns":"vault.usgovcloudapi.net","storage":"core.usgovcloudapi.net","azureFrontDoorEndpointSuffix":""},"batch":"https://batch.core.usgovcloudapi.net","resourceManager":"https://management.usgovcloudapi.net","vmImageAliasDoc":"https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-compute/quickstart-templates/aliases.json","sqlManagement":"https://management.core.usgovcloudapi.net:8443","gallery":"https://gallery.usgovcloudapi.net"}]
2023/02/02 12:29:15 [DEBUG] Falling back to ARM Metadata version 2019-05-01 for https://management.usgovcloudapi.net
--- PASS: TestEndpointRefresh_USGovernment (3.11s)
PASS

@manicminer manicminer requested a review from a team as a code owner February 1, 2023 20:42
@manicminer manicminer changed the title SDK improvements SDK tweaks Feb 1, 2023
@manicminer manicminer force-pushed the sdk-improvements branch 2 times, most recently from 8614a5f to e8599ea Compare February 1, 2023 22:45
@manicminer manicminer added bug Something isn't working enhancement New feature or request labels Feb 1, 2023
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments inline but otherwise 👍

sdk/client/msgraph/client.go Outdated Show resolved Hide resolved
}

req.URL.RawQuery = query.Encode()
//req.RetryFunc = client.RequestRetryAny(defaultRetryFunctions...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be exposing EnableRetries if it does nothing at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each service client will pass in its own retryfunc since the conditions for retrying are subtly different between endpoints, so this will take affect as clients are built.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok - is it worth the constructor taking a RetryPolicy (or rather an interface, which returns it) then?

func NewMsGraphClient(api environments.Api, apiVersion ApiVersion, tenantId string, retryConfiguration RetryConfiguration) (*Client, error) {
      // ..
}

type RetryConfiguration interface {
  RetryPolicy() []client.RequestRetryFunc
}

That'd allow us to offer some default retry policies, but also override that as needed?

Copy link
Contributor Author

@manicminer manicminer Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that, although this would also need to be different per method. For example, the retry conditions for creating a service principal are different to getting a service principal. At the moment the RetryFuncs are stored in the Request object, with the client deciding whether or not to use them.

sdk/environments/from_endpoint_test.go Outdated Show resolved Hide resolved
sdk/internal/metadata/client.go Outdated Show resolved Hide resolved
@manicminer manicminer merged commit 64f3be5 into main Feb 4, 2023
@manicminer manicminer deleted the sdk-improvements branch February 4, 2023 17:54
@manicminer manicminer added the release-once-merged The SDK should be released once this PR is merged label Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants