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

cloud.Configuration support for endpoints (AzureChinaCloud, AzureGovernmentCloud, AzurePrivateCloud) #235

Open
mblaschke opened this issue Aug 6, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@mblaschke
Copy link

instead of using adapter.SetBaseUrl("https://microsoftgraph.chinaclouapi.cn/v1.0") (related to #26) maybe adapt the way how the azure-sdk-for-go is using cloud.Configuration for configuring endpoints.

json representation of cloud.Configuration content for AzurePublicCloud:

{
    "activeDirectoryAuthorityHost": "https://login.microsoftonline.com/",
    "services": {
        "resourceManager": {
            "audience": "https://management.core.windows.net/",
            "endpoint": "https://management.azure.com"
        },
        // this could be the service configuration for msgraph-sdk-go
        "microsoftGraph": {
            "audience": "https://graph.microsoft.com",
            "endpoint": "https://graph.microsoft.com"
        }
    }
}

which is initialised here: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azcore/arm/arm.go

Inside the client creation (eg armresoruces, armauthorization, ...) the azure-sdk-for-go is using the AzurePublic endpoints by default and overwrites the endpoint if a cloud configuration is passed in the client options:

func NewClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*Client, error) {
	if options == nil {
		options = &arm.ClientOptions{}
	}
	ep := cloud.AzurePublic.Services[cloud.ResourceManager].Endpoint
	if c, ok := options.Cloud.Services[cloud.ResourceManager]; ok {
		ep = c.Endpoint
	}

	// ....
}

see https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/resourcemanager/resources/armresources/zz_generated_client.go#L38

@baywet
Copy link
Member

baywet commented Aug 8, 2022

Hi @mblaschke
Thanks for trying the SDK and for reaching out.
This is an interesting feature request to have the endpoint selection driven by configuration rather than imperatively (and they are not mutually exclusive).
Those kind of features are usually dictated for all of our SDKs (languages) by the design guidelines and we currently don't have any entry for that aspect at the moment.
I'll defer to our PM @maisarissi and our architect @darrelmiller to give some thoughts to this and maybe formalize it in the design guidance.
I want to be transparent on the fact that we are short staffed, and there are other priority items as you know (compilation time), so this one, if accepted, might take a while to be implemented.

@baywet baywet added enhancement New feature or request and removed Needs Triage 🔍 labels Aug 8, 2022
@darrelmiller
Copy link

Thank you for raising this issue. We definitely have some work to do to simplify sovereign cloud support. However, rather than having a cloud configuration document baked into the SDKs, I would rather rely on the cloud endpoints that are advertised in the OpenID connect configuration endpoint.

https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration

When used as a "tenanted endpoint" it can identify which sovereign cloud a tenant is hosted in automatically.

https://login.microsoftonline.com/microsoft.onmicrosoft.com/v2.0/.well-known/openid-configuration

This allows developers to build multi-tenant applications without having to know in advance what clouds those tenants live in.

@mblaschke
Copy link
Author

@darrelmiller
I have users which are using AzureChinaCloud and also private azure cloud so my currently solution is a wrapper to use either the predefined (using env var AZURE_ENVIRONMENT) cloudconfig structs from azure-sdk-for-go or let the users overwrite this structs by using a json definition.

Please stick to the way like azure-sdk-for-go is doing it otherwise it increases the effort to use both SDKs.
Cloudconfig only gives you the root domain for the services eg management.azure.com and others to reach the basic API endpoints and to get a valid token for these services (like in the example above).

@baywet
Copy link
Member

baywet commented Oct 11, 2022

Thanks for the additional feedback. Adding to the conversation as those are questions we're asking ourselves and are trying to get customer evidence to make sure we do the right thing:
Would you rather have one SDK per national cloud that reflects accurately the APIs available in that national cloud or use the "public SDK" against the national cloud, even if that means the public SDK advertises more APIs than actually available in this national cloud?

@mblaschke
Copy link
Author

mblaschke commented Oct 19, 2022

How is this different from now? 🤔
There is also only one azure sdk for china, public and government cloud but the sdk comes with all configuration (base urls) shipped for all the clouds.

My suggestion is to use the same kind of auto configuration like the azure-sdk-for-go is doing by using something similar like the ResourceManager configuration is injected in https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azcore/arm/arm.go

	cloud.AzureChina.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
		Audience: "https://management.core.chinacloudapi.cn",
		Endpoint: "https://management.chinacloudapi.cn",
	}
	cloud.AzureGovernment.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
		Audience: "https://management.core.usgovcloudapi.net",
		Endpoint: "https://management.usgovcloudapi.net",
	}
	cloud.AzurePublic.Services[cloud.ResourceManager] = cloud.ServiceConfiguration{
		Audience: "https://management.core.windows.net/",
		Endpoint: "https://management.azure.com",
	}

So all well known service urls are configured by default (AzurePublicCloud, AzureChinaCloud, AzureGovernmentCloud) and can be set via an predefined cloud configuration struct or an own instance and in "worst case" via adapter.SetBaseUrl()

see documentation here: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azcore/cloud/doc.go

for a story description: As a customer i only want to select one cloud config for azure-sdk-for-go and the msgraph-sdk-for-go to ensure that both SDKs are properly configured with only one struct and don't want to configure the right base url for well known clouds 🙂

@darrelmiller
Copy link

@mblaschke I understand the challenge you have. I agree that it would be helpful to be able to re-use the Azure cloud configuration object to configure the Graph SDKs. I will talk to the team about designing a way take an instance of the Azure cloud object and use it to configure a Graph client.

@ddyett
Copy link
Contributor

ddyett commented Jun 21, 2023

@maisarissi any comments on what to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants