Add support for remaining config variables from the old to-be-deprecated Config interface and add a new config class - #758
Conversation
arandito
left a comment
There was a problem hiding this comment.
Thanks @ubaskota! I left a couple comments but my biggest concern is how we are resolving environment and profile credentials during config resolution. Config resolution should only handle in-code credentials and defer env/profile credentials to the new IdentityChain. Let me know if you have any questions!
jonathan343
left a comment
There was a problem hiding this comment.
Thanks Ujjwal. I let some comments on the areas I'm most concerned about. Let me know if you have any questions.
Also, you need to rebase this PR with the latest from develop.
I'm still investigating some additional cleanup that probably should be done, but wanted to get you some feedback so you have something to work on in the meantime.
| $3C | ||
| self._config = config or $1T() | ||
|
|
||
| client_plugins: list[$2T] = [ |
There was a problem hiding this comment.
Why did you decide to move client_plugins this out of the client constructor? It's now generated inside of every operation which means we are re-allocating every time. Unless there is a good reason, I think this should stay in the class constructor as it exists today.
There was a problem hiding this comment.
With the new design for lazy resolution, I moved both client_plugins and operation_plugins into the operation methods. But you're right that client_plugins can remain inside the init function. I will move it back.
| writer.writeDocs("The protocol to serialize and deserialize requests with.", context); | ||
| writer.write(""); |
There was a problem hiding this comment.
There are multiple config options that get generated with trailing whitespace in their docstrings:
"""The protocol to serialize and deserialize requests with. """
This should be:
"""The protocol to serialize and deserialize requests with. """
Can you investigate this bug and compare with the existing Config object to see why there is this difference?
There was a problem hiding this comment.
This package should have a brief but descriptive changelog entry for the changes being made in this PR. Our packages get version bumped based on pending entries. Right now you're relying on existing entries to get version bumped which we shouldn't do.
There was a problem hiding this comment.
Should also add an entry here to ensure this is released with the other changes.
| """; | ||
|
|
||
| // Variant for services without a generated async config, which must not be referenced. | ||
| private static final String USER_AGENT_PLUGIN_SYNC_ONLY = """ |
There was a problem hiding this comment.
Which services won't have an async config? Shouldn't they all have the async config right now?
There was a problem hiding this comment.
All AWS services have async config. This is for non-AWS Smithy services (those without @aws.api#service), which don't get an async config generated, getAsyncConfigSymbol returns Optional.empty() for them.
| if (asyncConfigForPlugin.isPresent()) { | ||
| writer.write("$L: TypeAlias = Callable[[$T | $T], None]", | ||
| plugin.getName(), config, asyncConfigForPlugin.get()); | ||
| } else { | ||
| writer.write("$L: TypeAlias = Callable[[$T], None]", plugin.getName(), config); | ||
| } |
There was a problem hiding this comment.
The plugin API introduced in this PR doesn't make sense to me. Currently I see the following get generated:
AsyncBedrockRuntimePlugin: TypeAlias = Callable[[AsyncBedrockRuntimeConfig], None]
"""
A callable that allows customizing the async config object on each
request.
"""
Plugin: TypeAlias = Callable[[Config | AsyncBedrockRuntimeConfig], None]
"""A callable that allows customizing the config object on each request."""- I don't see
AsyncBedrockRuntimePluginactually being used or referenced anywhere. - The
Asyncnaming prefix seems misleading since there is no async work done by the plugins. - The
Callable[[Config | AsyncBedrockRuntimeConfig], None]signature is not what we want. This will make all plugins need to accept both config options. See below for what I think it should be.
IMO, during migration, we should generate the following:
Plugin: TypeAlias = (
Callable[[Config], None]
| Callable[[AsyncBedrockRuntimeConfig], None]
)After we remove support for Config it should just become:
Plugin: TypeAlias = Callable[[AsyncBedrockRuntimeConfig], None]There was a problem hiding this comment.
After internal discussion, we decided to drop support for the old Config for AWS services and only generate Async<ServiceId>Config. This simplifies the plugin type as Plugin is now Callable[[AsyncBedrockRuntimeConfig], None], and AsyncBedrockRuntimePlugin has been removed.
| } | ||
|
|
||
| // Write _FIELDS class variable with service-specific defaults | ||
| writer.openBlock("_FIELDS: ClassVar[dict[str, $T]] = {", fieldSpecSymbol); |
There was a problem hiding this comment.
This emits something like below:
_FIELDS: ClassVar[dict[str, FieldSpec]] = {
"aws_credentials_identity_resolver": FieldSpec(default=None),
"region": FieldSpec(default=None),
"aws_access_key_id": FieldSpec(default=None),
"aws_secret_access_key": FieldSpec(default=None),
"aws_session_token": FieldSpec(default=None),
"user_agent_extra": FieldSpec(default=None),
"sdk_ua_app_id": FieldSpec(default=None),
**AsyncAwsConfig._FIELDS,
"endpoint_uri": FieldSpec(
default=None, resolver=EndpointUriResolver("bedrock_runtime")
),
"endpoint_resolver": FieldSpec(
default_factory=lambda: StandardRegionalEndpointsResolver(
endpoint_prefix="bedrock-runtime"
)
),
"protocol": FieldSpec(
default_factory=lambda: RestJsonClientProtocol(
_SCHEMA_AMAZON_BEDROCK_FRONTEND_SERVICE
)
),
"auth_schemes": FieldSpec(
default_factory=lambda: {
ShapeID("aws.auth#sigv4"): SigV4AuthScheme(service="bedrock")
}
),
"auth_scheme_resolver": FieldSpec(default_factory=HTTPAuthSchemeResolver),
"transport": FieldSpec(default_factory=lambda: AWSCRTHTTPClient()),
}It's not clean to my why we're emitting inherited fields here that I though would be covered by **AsyncAwsConfig._FIELDS,.
I was expecting to see something closer to:
_FIELDS = {
**AsyncAwsConfig._FIELDS,
"endpoint_uri": ...,
"endpoint_resolver": ...,
"protocol": ...,
"auth_schemes": ...,
"auth_scheme_resolver": ...,
"transport": ...,
}There was a problem hiding this comment.
This is intentional. The base class (AsyncAwsConfig in aws_config.py) declares common config fields with their resolvers and validators. When AwsAsyncConfigIntegration.java generates the service-specific config, it iterates all registered integration plugins (like AwsAuthIntegration,AwsUserAgentIntegration) to emit config fields they contribute. Some of those plugins contribute fields that already exist in the base class, just like the duplicates here.
Functionally, these duplicates are harmless because if a config var is declared in the base class it will overwrite the duplicate ones with the proper specs (resolvers and validators). Fields that are unique to a service are not overwritten and will be used in config resolution. One option to prevent this was by filtering based on the config vars that are already in the base class, but that'd require us to hardcode the list of those config vars in codegen. That meant creating a second source of truth that needs to be in sync with the variables in the base class. For now, I chose to keep duplicates rather than have two sources of truth.
…ted Config interface # Conflicts: # packages/smithy-core/tests/unit/aio/test_retries.py
7653aea to
089c52f
Compare
| """ | ||
|
|
||
| aws_session_token: str | None = field(default=None, repr=False) | ||
| """An access key ID that identifies temporary security credentials.""" |
There was a problem hiding this comment.
nit - This docstring describes a session token as "An access key ID," is incorrect and confusing since (an access key id is aws_access_key_id). I know this description comes from the existing config object, but something like The session token used with temporary AWS security credentials. might be more correct here.
| ) | ||
|
|
||
| if result.value is not UNSET: | ||
| result = Resolved(value=result.value.lower(), source=result.source) |
There was a problem hiding this comment.
retry_mode case-handling is inconsistent between env/profile and in-code overrides. Values coming from AWS_RETRY_MODE/profile are lowercased here before validation, so AWS_RETRY_MODE=STANDARD works. In-code overrides skip this path: resolve(retry_mode=...) sets the raw value and validate_retry_mode compares case-sensitively against ("standard",), so resolve(retry_mode="Standard") (or "STANDARD") raises ConfigValidationError even though the same string is accepted from the environment. Consider lowercasing the override too (or documenting the constraint).
I will validate this finding and update w/ my own feedback
| rendered = ", ".join( | ||
| f"{f.name}={getattr(self, f.name)!r}" | ||
| for f in fields(self) | ||
| if f.repr and f.name not in _CREDENTIAL_FIELDS |
There was a problem hiding this comment.
__repr__ still exposes credential material via aws_credentials_identity_resolver. The filter redacts the three string credential fields but not the resolver field, which holds the same secret (the auto-wired StaticCredentialsResolver is built from aws_access_key_id/aws_secret_access_key). It happens not to leak today only because StaticCredentialsResolver has no custom __repr__ and AWSCredentialsIdentity is only reachable through it — but AWSCredentialsIdentity is a plain @dataclass whose repr prints secret_access_key. Any resolver that reprs its identity (or a future dataclass-style resolver) would defeat the stated goal ("without exposing credential material").
I will validate this finding and update w/ my own feedback
| // This class is only deprecated where an async replacement is generated to point | ||
| // at. For services without one it remains the supported config class. | ||
| var asyncConfigSymbol = CodegenUtils.getAsyncConfigSymbol(context.settings(), context.model()); | ||
| if (asyncConfigSymbol.isPresent()) { |
There was a problem hiding this comment.
Dead branch: this deprecated-Config path is unreachable. generateConfig(...) is only invoked when getAsyncConfigSymbol(...).isEmpty() (non-AWS services), yet this branch runs only when asyncConfigSymbol.isPresent() — recomputed from the same inputs. So the .. deprecated:: shim + DeprecationWarning is never emitted. If the intent is to fully drop old Config for AWS services (per the commit message), this whole if branch is dead code and can be removed; if the intent was to still emit a deprecation shim, it is silently not happening.
I will validate this finding and update w/ my own feedback
Issue #, if available:
Description of changes:
Adds the remaining AWS-shared config fields to
AsyncAwsConfig, bringing it to parity with the generated service Config class.endpoint_uri, aws_access_key_id, aws_secret_access_key, aws_session_token, sdk_ua_app_id, user_agent_extra, interceptors, http_request_config, transport, retry_strategy, aws_credentials_identity_resolver. Resolvable fields wire into theenv > profile > defaultresolution pipeline.Async<ServiceId>Config(AsyncAwsConfig)with service-specific_FIELDSthat override the base class example:endpoint_uriuses a service-aware resolver that checksAWS_ENDPOINT_URL_<SERVICE_ID>and the services config section before falling back to global sources.Config(with a deprecation warning) and the newAsync<ServiceId>Config, so existing users continue to work while new users adopt the async resolution path. The generated client accepts either type viaisinstancedispatch.get_service_config()onMergedConfigfor services-section lookups, and updatesRetryStrategyResolverto acceptretry_mode/max_attemptsfallbacks from the config layer.Testing:
EndpointUriResolvercovering the full precedence chain:service-specific env var > global env var > service config section > global profile > unset.MergedConfig.get_service_config()covering all lookup paths (profile missing, services key missing, service section not found, multiple services).RetryStrategyResolverfallback behavior:retry_mode/max_attemptsparams used whenretry_strategyis None, explicit strategy takes precedence over fallbacks.Example Usage:
Resolve service config and inspect provenance:
Invalid profile raises a clear error:
Refer to #751 for more examples.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.