feat: make region optional #970
Conversation
| @@ -124,7 +124,7 @@ public class ProfileCredentialsProvider( | |||
| } | |||
| } | |||
|
|
|||
| private suspend fun LeafProvider.toCredentialsProvider(region: LazyAsyncValue<String>): CredentialsProvider = | |||
| private suspend fun LeafProvider.toCredentialsProvider(region: LazyAsyncValue<String?>): CredentialsProvider = | |||
There was a problem hiding this comment.
correctness: Should region be optional everywhere or just at the client config level? I'm not sure these credential provider components ever work correctly without region.
There was a problem hiding this comment.
It's now optional in the underlying service clients (i.e., the STS client) so I saw no reason to keep it required in the credentials provider. I'd assumed that STS would default to the global endpoint but I now see that's not the case.
If we leave this as optional in the profile creds provider and it attempts to make a call to STS which fails because of missing region, then the provider will throw an exception. If its used as part of a chain (e.g., the default chain), then that exception will cause the the next provider in the list to be called. I think that's correct since the next provide may not require a region.
| assertFailsWith<IllegalArgumentException> { | ||
| S3Client { } | ||
| }.message.shouldContain("region is a required configuration property") | ||
| // Should _not_ throw an exception since region is optional |
There was a problem hiding this comment.
question: What is the behavior now if region isn't set? Should we have a test case for that?
There was a problem hiding this comment.
The S3 client can now be instantiated just fine without a region. The first place we'd encounter an error is invoking an operation. We can expand this test to attempt an operation invocation and assert on the failure.
As written, we will now consider it to be an error when resolving an endpoint during operation invocation. For example, the S3 client will throw an error in the endpoint provider: |
|
Kudos, SonarCloud Quality Gate passed!
|
| // FIXME - we likely need a way to let customizations modify/override this | ||
| // FIXME - we also need a way to tie in config properties added via integrations that need to influence the context |
There was a problem hiding this comment.
Are these two FIXMEs solved with interceptors?
There was a problem hiding this comment.
// FIXME - we likely need a way to let customizations modify/override this
I'm unclear what "customization" means in this context. Certainly interceptors can be used to modify the execution context.
// FIXME - we also need a way to tie in config properties added via integrations that need to influence the context
I wouldn't call this fixed by interceptors. Integrations could certainly add an interceptor but it seems like a boilerplate workaround. There's no common solution for it here yet, which I think is what the FIXME is about.
Issue #
Closes #969
Description of changes
Makes region an optional client config parameter to support MRAP and future multi-region use cases. Note that failing to provide a region in single-region cases will still fail but will throw an exception at operation invocation instead of during client construction.
Companion PR: smithy-kotlin#884
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.