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

Expose configuration of instantiated service clients. #871

Closed
oakad opened this issue Nov 2, 2020 · 13 comments · Fixed by #2398
Closed

Expose configuration of instantiated service clients. #871

oakad opened this issue Nov 2, 2020 · 13 comments · Fixed by #2398
Assignees
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue v1-sync

Comments

@oakad
Copy link

oakad commented Nov 2, 2020

Application manipulating APIs in multiple regions often need to adjust their behavior depending on whether different assets are located in the same or different regions.

To this end, it was handy to obtain the configured region directly from various service clients. Unfortunately, new client API hides region information in the private options object and provides no access to it.

Kindly return the region getter to the client object.

@oakad oakad added the bug This issue is a bug. label Nov 2, 2020
@jasdel jasdel added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels Nov 12, 2020
@jasdel
Copy link
Contributor

jasdel commented Nov 12, 2020

Thanks for reaching out this this issue @oakad. Currently the API client's don't provide a way to extract some or all of the options that the client was created with. One way to address this would be to add a GetAPIClientOptions (or similarly unique named method that won't clash with API operations) to the client. This method would call Copy on the client's option and eturn that copy.

func (c *Client) GetAPIClientOptions() Options {
     return c.options.Copy()
}

@oakad
Copy link
Author

oakad commented Nov 13, 2020

Not all options are equally useful (in the sense, that at some point we can simply tug the config along with the client). But region is of importance - we have dozens of region checks in our aws apps.

@jasdel jasdel changed the title It must be possible to obtain a region from client object Add way to get the region of an API client. Mar 3, 2021
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 4, 2022
@oakad
Copy link
Author

oakad commented Mar 4, 2022

I still believe region and actual API endpoint targeted should be exposed in the client interface. For example, gRPC client offers "Target()" method, so it's possible to check where is it connected to at any time.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 5, 2022
@RanVaknin RanVaknin added p3 This is a minor priority issue s Effort estimation: small labels Oct 31, 2022
@aajtodd
Copy link
Contributor

aajtodd commented Feb 21, 2023

so it's possible to check where is it connected to at any time.

Note this isn't possible for many services as the endpoint is dynamic based on the operation input (e.g. s3 bucket names are hoisted to the URL hostname when virtual host addressing is enabled).

@oakad
Copy link
Author

oakad commented Feb 22, 2023

And yet, regions are critically important when dealing with S3. My s3 code is choke full of bucket region code queries. :-)

@lucix-aws lucix-aws added p2 This is a standard priority issue v1-sync p1 This is a high priority issue and removed p3 This is a minor priority issue p2 This is a standard priority issue s Effort estimation: small labels Sep 21, 2023
@lucix-aws
Copy link
Contributor

Increasing the prio on this in the interest of parity with v1, where such a thing is possible:

fmt.Println(*svc.Config.Region) // service clients embed client.Client, which has aws.Config

I'm not inclined to "guard" access to Options in any way, though:

  1. I don't see a compelling reason to do so. Go as a language doesn't put a lot of emphasis on "guard rails" or object access control outside of the ability to choose whether to export a field. When usage/access semantics matter, it's indicated via documentation.
  2. The method space on Client is currently "pure" in the sense that we only generate modeled operations as methods. The alternative of keeping Options unexported and exposing something like func (*Client) Options() breaks that purity and adopting exposure of methods like that as a paradigm puts us in a position to hit naming collisions in the future (as unlikely as that might be).

To wit I think the following is all we need here:

  // Client provides the API client to make operations call for Amazon Simple
  // Storage Service.
+ //
+ // DO NOT instantiate a client directly - see [New] and [NewFromConfig].
  type Client struct {
-     options Options
+     // API client options.
+     //
+     // DO NOT modify client options directly - either use functional options
+     // on a per-operation basis, or instantiate a separate client.
+     Options Options
  }

@oakad
Copy link
Author

oakad commented Sep 25, 2023

At this point of time, the right thing may be to add the region to each and every request, autopopulated from config. Long gone are the days when everything was stuck in "us-east-1". :-)

@lucix-aws
Copy link
Contributor

Are you asking for the ability to override region per-request? That's possible today, on every service operation.

svc.ListBuckets(context.Background(), nil, func (o *s3.Options) {
    o.Region = "wherever"
})

@oakad
Copy link
Author

oakad commented Sep 25, 2023

Does this mean I can have a single client for all the regions and the client config region will only be used for defaults?
No performance penalty, no hidden catches?

@lucix-aws
Copy link
Contributor

lucix-aws commented Sep 25, 2023

Does this mean I can have a single client for all the regions and the client config region will only be used for defaults?

Yes.

No performance penalty, no hidden catches?

None. Per-operation overrides will actually be more economical than multiple clients since canonically "expensive" resources will be shared. As of this writing that will mostly apply to the HTTP client, assuming we're defaulting it for you:

// assuming cfg.HTTPClient is not set, each of these clients gets their own default HTTP client
// each with their own instance of http.Transport
svcI := s3.NewFromConfig(cfg, func (o *s3.Options) {
    o.Region = "us-east-1"
})
svcJ := s3.NewFromConfig(cfg, func (o *s3.Options) {
    o.Region = "us-east-2"
})

The above example at scale requires n transports to be created for n regions.

regions := loadEveryAWSRegion() // []string
svcK := s3.NewFromConfig(cfg, func (o *s3.Options) {
    o.Region = "us-east-1"
})

for _, region := range regions {
    svcK.DoSomething(ctx, ..., func (o *s3.Options) {
        o.Region = region
    })
}

The above is 1 transport for n regions.

Notice that the func(*Options) input is variadic, you can build up your own logic around this as makes sense for your application, if for example you want to change other parts of the config on a per-operation basis.

Note that if you're using the timestream service(s) you'll run into #2163 if you try to change the region.

[EDIT: code typo]

@lucix-aws lucix-aws changed the title Add way to get the region of an API client. Expose configuration of instantiated service clients. Sep 25, 2023
@oakad
Copy link
Author

oakad commented Sep 25, 2023

I'd say, this stuff should be added to the docs near the beginning (in some sort of best practices section). After all these years I suddenly learn that we don't have to maintain a bunch of client objects. :-)

@lucix-aws lucix-aws added p2 This is a standard priority issue and removed p1 This is a high priority issue labels Oct 11, 2023
@lucix-aws lucix-aws self-assigned this Nov 28, 2023
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p2 This is a standard priority issue v1-sync
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants