Refactor signing config and set a default signing config for s3 client #545
Conversation
monthonk
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
monthonk
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
monthonk
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
monthonk
had a problem deploying
to
PR integration tests
GitHub Actions
Failure
Signed-off-by: Monthon Klongklaew <[email protected]>
monthonk
force-pushed
the
refactor_client
branch
from
40afee6 to
39f5eb7
Compare
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
changed the title
| auth_scheme.signing_region(), | ||
| auth_scheme.scheme_name(), |
There was a problem hiding this comment.
Reason we were not keeping it optional is because when we were resolving endpoint, we were explicitly failing when we were not getting any value for any of the fields of AuthScheme. So, all these options should have some value set.
There was a problem hiding this comment.
That's fine if we want to fail, we can do that at resolve_for_bucket(..). But can't we create a signing config without resolving endpoint? Looking at the CRT function, I think what we really need are region and credentials_provider, the others are optional.
There was a problem hiding this comment.
Yes, we dont need other arguments in the CRT function. But, the signing config is only created after resolving the endpoint ( in s3_crt_client ). I am not sure if we need to set signing config other than making request where endpoint should be resolved.
There was a problem hiding this comment.
Yeah, that is a signing config at request level. We can also set it at client level which I added in this change (see s3_crt_client.rs line 272-279), and I don't think we want to resolve the endpoint there.
| @@ -32,6 +33,49 @@ impl Debug for SigningConfigInner { | |||
| } | |||
| } | |||
|
|
|||
| impl SigningConfigInner { | |||
| /// Create a new [SigningConfig] with default options. | |||
| pub fn new(region: &str, credentials_provider: CredentialsProvider) -> Self { | |||
There was a problem hiding this comment.
Why did we create this constructor for SigningConfigInner when we were doing the same with default_init_siginig_config()?
There was a problem hiding this comment.
Well, some of the configs are now optional. I think this might be easier to read.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description of change
This PR refactors the signing config and also set a default signing config for s3 client to prepare for upcoming changes.
Does this change impact existing behavior?
No.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).