Provide one client for all operations in one package for control plane

[tadelesh]

Some possible problems:

1. lazy load or create all sub clients when initializing
2. client param need include all sub clients param
3. non-breaking change

[jhendrixMSFT]

Mock-up

type Client struct {}

func NewClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*Client, error) {}

func (c *Client) FooGroup() *FooGroup {}

func (c *Client) BarGroup() *BarGroup {}

[tadelesh]

Some prototype here.

Two possible ways to resolve extra client params besides subscriptionID, cred and options (for now, only three mgmt. RPs have such issue): one is to add to the get method of root client, another is moving extra client params into operation (breaking change).

type ResourceManagerClient struct {
    subscriptionID string
    host           string
    pl             runtime.Pipeline

    resourcesClient             *Client
    deploymentOperationClient   *DeploymentOperationsClient
    ...
}

func NewResourceManagerClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*ResourceManagerClient, error) {
    if options == nil {
        options = &arm.ClientOptions{}
    }
    ep := cloud.AzurePublicCloud.Services[cloud.ResourceManager].Endpoint
    if c, ok := options.Cloud.Services[cloud.ResourceManager]; ok {
        ep = c.Endpoint
    }
    pl, err := armruntime.NewPipeline(moduleName, moduleVersion, credential, runtime.PipelineOptions{}, options)
    if err != nil {
        return nil, err
    }
    return &ResourceManagerClient{
        subscriptionID: subscriptionID,
        host:           ep,
        pl:             pl,
    }, nil
}

func (mgmtClient *ResourceManagerClient) Resources() *Client {
    if mgmtClient.resourcesClient == nil {
        mgmtClient.resourcesClient = &Client{
            subscriptionID: mgmtClient.subscriptionID,
            host:           mgmtClient.host,
            pl:             mgmtClient.pl,
        }
    }
    return mgmtClient.resourcesClient
}

func (mgmtClient *ResourceManagerClient) DeploymentOperations() *DeploymentOperationsClient {
    if mgmtClient.deploymentOperationClient == nil {
        mgmtClient.deploymentOperationClient = &DeploymentOperationsClient{
            subscriptionID: mgmtClient.subscriptionID,
            host:           mgmtClient.host,
            pl:             mgmtClient.pl,
        }
    }
    return mgmtClient.deploymentOperationClient
}

...

[jhendrixMSFT]

My preference would be to add the extra params to the get methods for the affected operation group. Can you paste a link to one of the RPs with such extra params?

[tadelesh]

iotsecurity IotDefenderLocation powerbiprivatelinks ResourceGroupName storageimportexport AcceptLanguage It seems these three params can also change to method level. I don't know there is any specific rule for the client level param with arm.

[jhendrixMSFT]

Thanks for the links.

I don't think we should make them method params as that would mean all methods would take the same param which is redundant. Instead, we should make them params on the "getter" method for the operation group.

[JeffreyRichter]

This pattern seems OK to me. However, it does cause the ResourceManagerClient package to depend on ALL other management library packages. Maybe we're OK with this, maybe not - we'd have to discuss all the implications.

Also, I think the method names would be more like: NewDeploymentOperationsClient to match Go's (and our) pattern used elsewhere when clients create other clients.

[jhendrixMSFT]

To clarify, I'd want one uber client per RP. Not a single client that spans all RPs.

[JeffreyRichter]

Oh, that sounds much better to me then.

[tadelesh]

Yes. One root client per RP. The code is just a prototype for resources RP. I think two name we need to decide. One is the root client name. We cannot use Client directly as we have removed stuttering, so it will collide with create client for resources in resources RP. I propose the name NewResourceManagerClient for all RPs to align with the package path. Customer then will use armcompute.NewResourceManagerClient to create the root client. Another is the name to get sub client. As the main purpose of root client is to help to declare only one client for each RP, I'd like to use Get instead of New to prevent customer define another variable.

client := armcompute.NewResourceManagerClient(subscription, credential, options)
client.GetVirtualMachinesClient().BeginCreateOrUpdate(...)
client.GetDisksClient().BeginCreateOrUpdate(...)

[tadelesh]

One example customer use self-defined client to store all the sub-client: https://github.com/Azure/azure-sdk-for-go/issues/18590

[xboxeer]

Recap this issue based on our recent user study. 5 of 6 participant in our interview expressed that they don't want always to create new client for new azure services they are using in their code, too many clients object there in Azure Golang SDK One prototype I that came across to have just one root client:

rootClient := NewRootClient(cred);
rootClient.GetClient<XXXClient>();

is this something doable? In this case we can even have a pool of XXXClient so that customer don't need to worry about how many client instance is created. One customer do shares his concern that for all the clients object in Azure Golang SDK, are they allowed to have multiple instance or a singleton is preferred CC: @jhendrixMSFT @RickWinter @tadelesh @JeffreyRichter

[JeffreyRichter]

Our mental model is that each client encapsulates a root endpoint, credentials, and options (for retry, logging, transport, etc.). For ARM clients, we default the endpoint so it is just creds and options.

Whenever a customer wants to change ANY of these things, a new client object MUST be created. If the customer is OK with all these things, then they can use the 1 client and it is goroutine safe. NewRootClient should take creds and options (not shown above). Then, I assume that root.Client.GetClient creates a new XxxClient object using the creds/options of the rootclient, right?

From a correctness point of view, it doesn't really matter if GetClient returns a cached singleton or a brand new client. Our usual pattern would be to call this NewClient() and return a new client each time. Having a cache can prolong client lifetime beyond what the customer's app needs.

[xboxeer]

Our mental model is that each client encapsulates a root endpoint, credentials, and options (for retry, logging, transport, etc.). For ARM clients, we default the endpoint so it is just creds and options.

Whenever a customer wants to change ANY of these things, a new client object MUST be created. If the customer is OK with all these things, then they can use the 1 client and it is goroutine safe. NewRootClient should take creds and options (not shown above). Then, I assume that root.Client.GetClient creates a new XxxClient object using the creds/options of the rootclient, right?

From a correctness point of view, it doesn't really matter if GetClient returns a cached singleton or a brand new client. Our usual pattern would be to call this NewClient() and return a new client each time. Having a cache can prolong client lifetime beyond what the customer's app needs.

Customer are ok with storing the whole azure executing context (endpoint, credentials, options) in one rootClient since this is what they want.

For your question: Then, I assume that root.Client.GetClient creates a new XxxClient object using the creds/options of the rootclient, right? The answer is Yes, customer complains that they don't want to pass in creds/options repeatedly to create new XXXClient, for example if you want to create a new ServiceBusQueue in a new resource group, you need ResourceGroupClient, ServiceBusNamespaceClient, ServiceBusQueueClient, too redundant.

I think the NewClient method name makes more sense since it clearly tells customers we are newing something, they can decide if they want to cache it or not in their own way.

Just to make it clear, if this is doable, how difficulty it is to add this in our current SDK and is it something we need add to our autorest.go? CC @tadelesh @jhendrixMSFT @RickWinter

[tadelesh]

@xboxeer Joel mentioned one concern before that if we add such root client in non-breaking way, then customer will have two ways to create client. Will it be confusing for the customers?

[xboxeer]

We can have a blog expressing this new API and add it to our document in at top level

[JeffreyRichter]

In some other packages, we have multiple wya of creating a client. Storage, for example lets you create an accountClient and from that create a container client. Or, you could just create a container client. So, I'm not concerned about multiple ways.

If this "client factory client" is usable for all ARM clients, then perhaps it should go in armcore?

[xboxeer]

In some other packages, we have multiple wya of creating a client. Storage, for example lets you create an accountClient and from that create a container client. Or, you could just create a container client. So, I'm not concerned about multiple ways.

If this "client factory client" is usable for all ARM clients, then perhaps it should go in armcore?

If it is in armcore, does this factory always know the client it needs to create? Since each practical clients are within each module, how would that armcore factory know all those practical clients without importing their corresponding modules, or a can solve this issue? By the way, if this is doable, and we could create any client from this factory in armcore, i would prefer this way

[JeffreyRichter]

I would think we could use generics and reflection to call the ctor (maybe).

[tadelesh]

Two thoughts:

1. For some clients, there are swagger defined client parameters that should be passed when creating client (I gave some the example here). So, NewSubClient function needs to have alterable parameters.
2. We need to know which sub client we should create when calling NewSubClient. So, it's not a generic type instantiation, but a reflection call (maybe), like NewSubClient(clientType reflect.Type, additionalParams ...any). I don't know it is possible to add some constraints for such reflect.Type parameter.

[JeffreyRichter]

1. So, if there are client-specific options, then I don't see how we can offer an all-purpose Client factory at all. We shouldn't offer and promote an abstraction that hides options from customers (especially if some are required or frequently used).
2. I agree that reflection would have issues but Im not sure it's even worth pursuing considering #1 above.

[tadelesh]

1. So, if there are client-specific options, then I don't see how we can offer an all-purpose Client factory at all. We shouldn't offer and promote an abstraction that hides options from customers (especially if some are required or frequently used).
2. I agree that reflection would have issues but Im not sure it's even worth pursuing considering initial #1 above.

I agree that we should not hide required params when creating client. I couldn't find a good way to provide a core-lib level root client. But it's practicable to provide a module level (single RP) root client because we could generate GetXXXSubClient methods separately by code generator.

[JeffreyRichter]

Do ALL clients for the same RP share the exactly the same options? Or, would each GetXxxSubClient method take a specific options structure?

[tadelesh]

Do ALL clients for the same RP share the exactly the same options? Or, would each GetXxxSubClient method take a specific options structure?

Each GetXXXSubClient method will take different params. We could generate the method according to client's client parameters.

[JeffreyRichter]

I think this is worth pursuing. I'd like to see a prototype of it and what the customer code looks like today versus what it would look like if we did this proposal. The code shouldn't actually work - just a prototype is good.

[tadelesh]

A simple prototype (armcompute as an example module) is as follows. Some points:

1. subscriptionID appears in most ARM clients' CTOR. But still some clients (most in armresources and all OperationsClient) do not need it. I trend to promote it to RootClient instead of makes it a param in most GetXXXClient.
2. All GetXXXClient return a new client. Do we need to cache them?
3. For the sub clients with additional client's param, the param will be a method param as shown in GetFakeClient.

type RootClient struct {
    subscriptionID string
    host           string
    pl             runtime.Pipeline
}

func NewRootClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*RootClient, 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
    }
    pl, err := armruntime.NewPipeline(moduleName, moduleVersion, credential, runtime.PipelineOptions{}, options)
    if err != nil {
        return nil, err
    }
    return &RootClient{
        subscriptionID: subscriptionID,
        host:           ep,
        pl:             pl,
    }, nil
}

func (c *RootClient) GetOperationsClient() *OperationsClient {
    return &OperationsClient{
        host: c.host,
        pl:   c.pl,
    }
}

func (c *RootClient) GetVirtualMachinesClient() *VirtualMachinesClient {
    return &VirtualMachinesClient{
        subscriptionID: c.subscriptionID,
        host:           c.host,
        pl:             c.pl,
    }
}

func (c *RootClient) GetFakeClient(fakeAddtionalClientParam string) *FakeClient {
    return &FakeClient{
        subscriptionID:           c.subscriptionID,
        host:                     c.host,
        pl:                       c.pl,
        fakeAddtionalClientParam: fakeAddtionalClientParam,
    }
}

[JeffreyRichter]

1. This code assumes that all clients use the same exact arm.ClientOptions - can we make this assumption?
2. From a code hygiene point of view, it would be better if each of the GetXxxClient() methods internally called the correct NewXxxClient method passing the required parameters. This is better than accessing package-private fields directly. The purpose of a ctor method is to ensure proper construction and just constructing a structure instance and setting private fields directly gives up the guarantee.
3. The above point means that NewRootClient would just store the params in fields and not create any pipline itself. The Root Client is a simple client that is a factory for other clients - period.
4. This probably means that the GetXxxClient methods have to return an error too (which they should).
5. No, the methods should not do any caching.
6. In fact, they should be named NewXxxClient to indicate that they New-up a client and this would also the pattern we have established for all other clients.

[tadelesh]

1. I think in most time all sub clients in one RP module share the same client options. If not, then is should be a different client, either create with root client or use current NewXXXClient to create a separate one.
2. The start point we introduce this root client is that customer does not want to maintain so many clients when using our SDK. If the GetXXXClients will return error, then it means customer needs to use another variable to store the sub client and deal with error. It has no difference with current NewXXXClient. The most convenient thing for RootClient is that after we create it, we could use rootClient.GetXXXClient().DoOperation() to call all operations in all sub-clients directly, with only one client variable.

[JeffreyRichter]

Any clients constructable from the root client MUST use identical options or must take their own specific options. We have to be sure of this in order to make this a replicable/consistent pattern across all RPs. We can't have some RPs with root clients and some without, etc.; it will cause customer confusion.

If the NewXxxClient methods take their own specific options, then caching can never be done (which is just fine with me). If we're sure that after constructing the root client successfully, that constructing any child client will never fail, then I'm OK with the NewXxxClient method not returning an error - we just swallow in in the NewXxxClient method before returning.

[tadelesh]

I think some of my wording may have mis-leading. I mean I prefer any clients constructable from the root client use identical options. This pattern applies for all RPs. Then root client creates the pipeline and shares it with all sub clients. So, when NewXXXClient, there will be no new pipeline, then no error. But from your opinion, each client should be a fully new created client with separated pipeline, right? We just ensure the pipeline creation could be success when creating root client, but not cache that pipeline, then when calling NewXXXClient, we swallow the error and just return the client. Do I understand it right?

[JeffreyRichter]

Yes, you understand what I was saying.

---

From: Chenjie Shi @.> Sent: Wednesday, January 11, 2023 2:08:02 AM To: Azure/autorest.go @.> Cc: Jeffrey Richter @.>; Mention @.> Subject: Re: [Azure/autorest.go] Provide one client for all operations in one package for control plane (Issue #823)

I think some of my wording may have mis-leading. I mean I prefer any clients constructable from the root client use identical options. This pattern applies for all RPs. Then root client creates the pipeline and shares it with all sub clients. So, when NewXXXClient, there will be no new pipeline, then no error. But from your opinion, each client should be a fully new created client with separated pipeline, right? We just ensure the pipeline creation could be success when create root client, but not cache that pipeline, then when calling NewXXXClient, we swallow the error and just return the client. Do I understand it right?

— Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fautorest.go%2Fissues%2F823%23issuecomment-1378511435&data=05%7C01%7Cjeffreyr%40microsoft.com%7C265599c94c9941458c5908daf3bbb9ca%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638090284855910549%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iyFK36viWzb3hLHXSfPIGpyugp8CQvz5bEsA%2FqkgBZg%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAARLJPYG6FT47ZXYATOODK3WR2BAFANCNFSM5VU6A2PQ&data=05%7C01%7Cjeffreyr%40microsoft.com%7C265599c94c9941458c5908daf3bbb9ca%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C638090284855910549%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NZRW49ya7LZ9jrlfo95E849nRVurbcomv8wVBbSBrkA%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

[tadelesh]

Updated prototype as follows. I think we should deep copy the option when create the RootClient, to prevent possible change after we check the error. Then the error swallow could be safe.

type RootClient struct {
    subscriptionID string
    credential     azcore.TokenCredential
    options        *arm.ClientOptions
}

func NewRootClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*RootClient, error) {
    if options == nil {
        options = &arm.ClientOptions{}
    }
    _, err := armruntime.NewPipeline(moduleName, moduleVersion, credential, runtime.PipelineOptions{}, options)
    if err != nil {
        return nil, err
    }
        options = deepcopy(options)
    return &RootClient{subscriptionID, credential, options}, nil
}

func (c *RootClient) NewOperationsClient() (client *OperationsClient) {
    client, _ = NewOperationsClient(c.credential, c.options)
    return
}

func (c *RootClient) NewVirtualMachinesClient() (client *VirtualMachinesClient) {
    client, _ = NewVirtualMachinesClient(c.subscriptionID, c.credential, c.options)
    return
}

func (c *RootClient) GetFakeClient(fakeAddtionalClientParam string) (client *FakeClient) {
    client, _ = NewFakeClient(c.subscriptionID, fakeAddtionalClientParam, c.credential, c.options)
    return
}

[JeffreyRichter]

Yes, this looks great to me! Let me just ask @jhendrixMSFT to do another sanity check on this.

[jhendrixMSFT]

Sorry for the delay.

First, I'm not a fan of RootClient and would prefer something like ClientFactory. But we can have the naming discussion later.

Second, it would be great to avoid having to create stuff then throw it away just for sake of validation. I get why you do this given the current design.

Third, keep in mind that we'll be moving to a shared client. I've just started work on the codegen part. It wouldn't impact your current design though (snippet below).

type AvailabilitySetsClient struct {
    internal       *arm.Client
    subscriptionID string
}

func NewAvailabilitySetsClient(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*AvailabilitySetsClient, error) {
    cl, err := arm.NewClient("armcompute.AvailabilitySetsClient", moduleVersion, credential, options)
    if err != nil {
        return nil, err
    }
    client := &AvailabilitySetsClient{
        subscriptionID: subscriptionID,
        internal:       cl,
    }
    return client, nil
}

[jhendrixMSFT]

Seems like we can codegen this.

type ClientFactory struct {
    subscriptionID string
    credential     azcore.TokenCredential
    options        *arm.ClientOptions
}

func NewClientFactory(subscriptionID string, credential azcore.TokenCredential, options *arm.ClientOptions) (*ClientFactory, error) {
    _, err := arm.NewClient("armcompute.ClientFactory", moduleVersion, credential, options)
    if err != nil {
        return nil, err
    }
    return &ClientFactory{
        subscriptionID: subscriptionID,
        credential:     credential,
        options:        options,
    }, nil
}

func (c *ClientFactory) NewAvailabilitySetsClient() *AvailabilitySetsClient {
    availabilitySetsClient, _ := NewAvailabilitySetsClient(c.subscriptionID, c.credential, c.options)
    return availabilitySetsClient
}

It does assume that client constructors defer all of their logic to the shared client. This is true today and will likely be for the foreseeable future. In fact, with this design, we'd have to make it an invariant or we're hosed.

[jhendrixMSFT]

I just found something a bit concerning.

https://github.com/Azure/autorest.go/blob/main/src/generator/operations.ts#L113-L128

It would appear that for ARM, if a client contains any RP-specific optional params, then we generate an options type for that ctor which embeds arm.ClientOptions. While this makes sense, it's very bad for versioning. Do you know if we have any clients that use their own options type?

[tadelesh]

1. For naming, I prefer NewClientFactory because we use NewXXXClient for all the methods. @xboxeer , what do you think?
2. For the pipeline created only for validation, can we add some validation function in azcore.arm to avoid using NewClient to do the validation?
3. I went through all our mgmt. modules; it seems we do not have such client specific option type for now (2352 clients all use arm.ClientOptions). It will be a breaking change if an optional param is added, though I doubt it will happen for ARM. Can we just remove the logic and throw error if ARM has such pattern?
4. My concern about this approach is it only solve one point from customer: they do not want to pass subId, crendential, options when create the clients, but customer still need to store many sub clients.

[xboxeer]

I'd like to summarize our design as below 2 different solutions

1. SDK provide a thin layer of subclient create, we stores subid, resource group name, etc and directly use them to new subclient, customer will New the client without providing redundant subids, resourcegroups, etc. 1.1 Pro: easy to implement in SDK level, lower chance of seeing issue triggered by SDK 1.2 Cons: Customer may new too much subclient
2. SDK provide a thick layer of subclient create, that we internally create and cache subclient, customer will Get the client 2.1 Pro: We can control the life cycle of subclient, more easy for us to provide new features in the future 2.2 Cons: hard to implement, higher chance of seeing issue triggered by SDK

@JeffreyRichter what's your preference?

[jhendrixMSFT]

Agreed we can remove the optional client params part for now and throw instead. If/when we need this though we'll need to find a solution.

I didn't think my proposal was any different from what was proposed earlier (other than naming and reliance on the shared client). What am I missing?

[JeffreyRichter]

@xboxeer My philosophy is that we are just adding a simple light-weight convenience (client factory) which customers may or may not use. Customers could easily implement this themselves. If we had Get methods that return some internally cached thing, then we still do NOT control the lifetime as Get hands it over to the customer. With the current NewXxxClient method name, customers immediately realize that NewXxx returns a new client each time and they can (or should) cache this themselves if they want to. It is not trivial to implement a caching mechanism - do we ever let a client be GC'd? If not, then we hold on to memory longer than what the customer's app needs.

I prefer your option #1, not #2.

[JeffreyRichter]

@tadelesh: I originally thought that all control plane clients would have the same, exact client options because why would there be any difference and how would our code generator emit these and use them? So, I believe that we could make this assumption for now and forever. As for the validation. I can't remember exactly what this is for: some cloud-related verification? If we can come up with another (not-brittle) way of accomplishing this, I'm fine with that.

[tadelesh]

@jhendrixMSFT Yes. There is no difference. I'm just thinking of how much benefits we could get from this approach. Since customers may do the cache both with and without this factory, and passing client CTOR params has little difference with passing factory, I'd like to keep SDK API simple. But I'll discuss with @xboxeer further.

@JeffreyRichter Since we all agree that control plane clients would have the same options, I'll change the code generator logic. Regarding validation, it is only for cloud endpoint and audience check. So, the validation in factory CTOR could be refined. @jhendrixMSFT correct me if I'm wrong.

[jhendrixMSFT]

@tadelesh there's a bit more validation with the shared client, see https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azcore/arm/client.go#L35-L62.

[tadelesh]

I've discussed with @xboxeer and come to the agreement to add such client factory. @jhendrixMSFT , @JeffreyRichter do you have any further concern? If not, I'll start the codegen change according to this proto.

[JeffreyRichter]

I have no other conerns. Can't wait to see it!