Skip to content

Add support for remaining config variables from the old to-be-deprecated Config interface and add a new config class - #758

Open
ubaskota wants to merge 2 commits into
smithy-lang:developfrom
ubaskota:config_var_support_implementation
Open

Add support for remaining config variables from the old to-be-deprecated Config interface and add a new config class#758
ubaskota wants to merge 2 commits into
smithy-lang:developfrom
ubaskota:config_var_support_implementation

Conversation

@ubaskota

@ubaskota ubaskota commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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.

  • New fields: Adds support for 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 the env > profile > default resolution pipeline.
  • Service-specific codegen: Generates Async<ServiceId>Config(AsyncAwsConfig) with service-specific _FIELDS that override the base class example: endpoint_uri uses a service-aware resolver that checks AWS_ENDPOINT_URL_<SERVICE_ID> and the services config section before falling back to global sources.
  • Dual config support: The generated config module now contains both the old Config (with a deprecation warning) and the new Async<ServiceId>Config, so existing users continue to work while new users adopt the async resolution path. The generated client accepts either type via isinstance dispatch.
  • Supporting changes: Adds get_service_config() on MergedConfig for services-section lookups, and updates RetryStrategyResolver to accept retry_mode/max_attempts fallbacks from the config layer.

Testing:

  • Added unit tests for:
    • EndpointUriResolver covering 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).
    • RetryStrategyResolver fallback behavior: retry_mode/max_attempts params used when retry_strategy is None, explicit strategy takes precedence over fallbacks.

Example Usage:

Resolve service config and inspect provenance:

# With AWS_REGION=us-east-1 set in the environment
# and ~/.aws/config containing:
#   [profile default]
#   services = my-services
#
#   [services my-services]
#   bedrock_runtime =
#     endpoint_url = https://bedrock-runtime.us-east-1.amazonaws.com
import asyncio
from aws_sdk_bedrock_runtime.config import AsyncBedrockRuntimeConfig

async def main():
  config = await AsyncBedrockRuntimeConfig.resolve()

  print(config.region)                    # "us-east-1"
  print(config.source_of("region"))       # ENV
  print(config.endpoint_uri)              # "https://bedrock-runtime.us-east-1.amazonaws.com"
  print(config.source_of("endpoint_uri")) # PROFILE

asyncio.run(main())

Invalid profile raises a clear error:

import asyncio
from aws_sdk_bedrock_runtime.config import AsyncBedrockRuntimeConfig

async def main():
  config = await AsyncBedrockRuntimeConfig.resolve(profile="non-existent")
  # raises ProfileNotFoundError:
  #   Profile 'non-existent' (from the profile argument) not found in config file.

asyncio.run(main())

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.

@ubaskota
ubaskota requested a review from a team as a code owner July 30, 2026 04:25
@ubaskota ubaskota changed the title Add support for remaining config variables from the old to-be-deprecated Config interface Add support for remaining config variables from the old to-be-deprecated Config interface and add a new config class Jul 30, 2026

@arandito arandito left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

if isinstance(config, $6T):
self._config: $1T = config # type: ignore[assignment]
elif isinstance(config, $1T) or config is None:
self._config = config or $1T()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default behavior constructs the deprecated Config object and subsequently throws a deprecation warning to users even though they never explicitly use it. IMO it's an awkward user interface to raise deprecation warnings for default behavior, especially when we are trying to get customers to start using the new config. What's stopping us from defaulting to AsyncAwsConfig.resolve() or Async<ServiceId>Config.resolve()?"

.orElse(context.settings().service().getName());

// Import AsyncAwsConfig base class
writer.addDependency(SmithyPythonDependency.SMITHY_AWS_CORE);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import couples our generic code generator with AWS specifics and forces a smithy-aws-core import. Can we use interceptor system and an AWS integration to write this section instead?

$3C
self._config = config or $1T()
if isinstance(config, $6T):
self._config: $1T = config # type: ignore[assignment]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generates:

if isinstance(config, AsyncBedrockRuntimeConfig):
    self._config: Config = config  # type: ignore[assignment]

If the instance is a AsyncBedrockRuntimeConfig object, why are we setting the self._config type hint to the deprecated Config object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Config type annotation was added to suppress pyright errors downstream. I'll change it to Config | AsyncBedrockRuntimeConfig instead.

writer.write("");
writer.write("@dataclass(kw_only=True)");
writer.openBlock("class $L($T):", asyncConfigSymbol.getName(), asyncAwsConfigSymbol);
writer.write("\"\"\"$L configuration (async-resolved).\"\"\"", serviceId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We should use writeDocs here.

writer.write("\"\"\"$L configuration (async-resolved).\"\"\"", serviceId);
writer.write("");

// Write service-specific field declarations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields are missing docstrings like the old Config object.

Adding them will help with IDE support and for documentation.

retry_strategy = await self._retry_strategy_resolver.resolve_retry_strategy(
retry_strategy=config.retry_strategy
retry_strategy=config.retry_strategy,
retry_mode=getattr(config, "retry_mode", None),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not config.retry_mode and config.max_attempts instead of getattr?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because retry_mode and max_attempts exist on AsyncBedrockRuntimeConfig (inherited from AsyncAwsConfig) but not on the old Config. Since the generated client must work with both old and new configs, getattr returns None when the attribute doesn't exist instead of raising an AttributeError.

aws_credentials_identity_resolver: "IdentityResolver[AWSCredentialsIdentity, AWSIdentityProperties] | None" = None
sdk_ua_app_id: str | None = None
user_agent_extra: str | None = None
interceptors: list[Any] = field(default_factory=list) # type: ignore

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the interceptors list type depends on service specific interceptors, should we define this field in the service config object instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its simpler to leave it in the base class because every service uses it with the same [] default. The only benefit of moving it would be narrower type hints, which can always be aded on the generated subclass later. If you see more benefits on moving it, let’s discuss.

- If both aws_access_key_id and aws_secret_access_key are overridden,
resolve normally
- If only one credential is overridden, raise ConfigValidationError.
- Otherwise, resolve atomically: if both key and secret are present in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must not attempt to resolve env/profile credentials during config resolution. The IdentityChain handles both environment and profile credentials. Config resolution should only set and validate in-code credentials if aws_access_key_id and aws_secret_access_key are set.

If we do this, we would skip any credential source that must be checked after environment and before profile credentials (i.e. STS Assume Role with Web Identity).

What we could do if customers set credentials in-code is to automatically set config.aws_credentials_identity_resolver = StaticCredentialsResolver() for them. Currently, when customers want to define in-code credentials they also have to set their resolver to StaticCredentialsResolver(). To make the user experience easier, we can detect the in-code credentials, validate both are present and automatically set the resolver field to the static resolver. Let me know what you think about this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time of this implementation, the understanding was that aws_access_key_id and aws_secret_access_key would be resolved through the config pipeline, as the credential chain was still under development. I'll update it to only validate in-code credentials and auto-set StaticCredentialsResolver when both are present, leaving env/profile credential resolution to IdentityChain.

class EndpointUriResolver:
"""Service-aware endpoint URI resolver.

Resolution order (first match wins):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do other SDKs also match the global env first before the service specific config file? Seems odd we take the global over a service specific url.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this matches botocore's order. Environment variables (both service-specific and global) take precedence over config file entries.

@@ -37,6 +50,17 @@ class AsyncAwsConfig:
region: str | None = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these options have docstrings. Are we planning on adding them to ensure we have IDE support and documentation support?

Curious how much of a pain its going to be since this repo uses the old rst docstring format while our clients use the google style format.

def __init__(
self,
config: $1T | $6T | None = None,
plugins: list[$2T] | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I realized after reviewing #733 is that we need to start accepting plugins that modify the new async config object and satisfy the new async plugin type. However, that raises an issue: how do we avoid breaking current plugins that use Config.

At the very least, we'll need to update our plugins to not strictly accept Config (example) or else existing clients will likely fail type checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants