rgw/admin: use shared AWS SigV4 signer in API client

[dswarbrick]

Instantiating a single-use signer per request is wasteful, resulting in an additional 35 allocs per request, in addition to a small speed overhead.

$ benchstat old new
goos: linux
goarch: amd64
pkg: github.com/ceph/go-ceph/rgw/admin
cpu: Intel(R) Core(TM) Ultra 7 165U
                 │     old     │                new                 │
                 │   sec/op    │   sec/op     vs base               │
GetBucketInfo-14   106.8µ ± 1%   103.1µ ± 1%  -3.48% (p=0.000 n=10)

                 │     old      │                 new                  │
                 │     B/op     │     B/op      vs base                │
GetBucketInfo-14   23.14Ki ± 0%   20.21Ki ± 0%  -12.66% (p=0.000 n=10)

                 │    old     │                new                 │
                 │ allocs/op  │ allocs/op   vs base                │
GetBucketInfo-14   325.0 ± 0%   290.0 ± 0%  -10.77% (p=0.000 n=10)

Checklist

• Added tests for features and functional changes
• Public functions and types are documented
• Standard formatting is applied to Go code
• Is this a new API? Added a new file that begins with //go:build ceph_preview
• Ran make api-update to record new APIs

[dswarbrick]

I would also highly recommend moving the aws.NewCredentialsCache(...) to the API client factory function, as there is also no reason to instantiate this on every request, and it would result in further performance increase and fewer allocs. My benchmarking of this showed about 10% faster req processing, which is significant when fetching bucket info for tens of thousands of buckets.

However, this would effectively constitute a breaking change, as it would no longer be possible to change the access key / secret key after instantiating the API client. To this I would argue that it was never a very good idea to export credentials on the API struct in the first place.

[phlogistonjohn]

All other fields in this struct are public. For consistency, should this field also be public?

(You could argue that the other fields should have been private but unfortunately that's in the past)

[dswarbrick]

I think it's fairly standard practice in Go to only export struct members if necessary. It's generally a bad idea to export pointers.

[phlogistonjohn]

I would also highly recommend moving the aws.NewCredentialsCache(...) to the API client factory function, as there is also no reason to instantiate this on every request, and it would result in further performance increase and fewer allocs. My benchmarking of this showed about 10% faster req processing, which is significant when fetching bucket info for tens of thousands of buckets.

However, this would effectively constitute a breaking change, as it would no longer be possible to change the access key / secret key after instantiating the API client. To this I would argue that it was never a very good idea to export credentials on the API struct in the first place.

Can we provide a new "preferred" function that does this w/o removing the old way and breaking the API? Regardless, this comment feels like it deserves it's own issue where we can discuss ideas w/o further distracting from the current topic.

[dswarbrick]

Can we provide a new "preferred" function that does this w/o removing the old way and breaking the API? Regardless, this comment feels like it deserves it's own issue where we can discuss ideas w/o further distracting from the current topic.

If we're going to make a bigger departure from the current implementation, then I would suggest adding the auth via a http.RoundTripper. This is currently not so so feasible because the New() func takes a HTTPClient interface, rather than a standard http.Client, which can include a non-default Transport. The auth RoundTripper would need to wrap any such Transport that was passed in by the http.Client.

I see that this HTTPClient interface is also implemented by debugHTTPClient, but looking at what it does, the preferred way to implement it would also be as a RoundTripper.

[anoopcs9]

The change itself lgtm.

One minor note: since signer is only set in New(), anyone building API{} directly via the exported fields will now panic on the first request. Probably a non-issue since New() is the intended path and I don't see anyone constructing it directly - but if we want to be safe, either a lazy if api.signer == nil fallback in call() or a short doc comment on API noting New() is required would cover it.

[anoopcs9]

Regardless, this comment feels like it deserves it's own issue where we can discuss ideas w/o further distracting from the current topic.

I've created #1325 for further discussions.