microsoftgraph / msgraph-sdk-go

Microsoft Graph SDK for Go
https://docs.microsoft.com/en-us/graph/sdks/sdks-overview
MIT License
214 stars 32 forks source link

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

Open mblaschke opened 1 year ago

mblaschke commented 1 year ago

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 commented 1 year ago

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.

darrelmiller commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@maisarissi any comments on what to do here.