Skip to content

add traits for custom subprocess implementations to aws-types and aws-config#4590

Open
codeshaunted wants to merge 4 commits into
smithy-lang:mainfrom
BoundaryML:avery/process-provider
Open

add traits for custom subprocess implementations to aws-types and aws-config#4590
codeshaunted wants to merge 4 commits into
smithy-lang:mainfrom
BoundaryML:avery/process-provider

Conversation

@codeshaunted
Copy link
Copy Markdown
Contributor

Motivation and Context

After #4570 was merged I realized that to support the full default provider chain with virtualized OS operations we would also need to provide custom implementations for subprocess spawning in credential-process.

Description

Adds a public ProvideProcess trait and Process shim to aws_types::os_shim_internal, following the same pattern as ProvideEnv/Env and ProvideFs/Fs. This enables custom implementations of process execution for credential_process, allowing users to provide custom backends (e.g., sandboxed environments, remote execution, WASM) and enabling tests to mock process execution without spawning real subprocesses.

Also updates ProviderConfig and ConfigLoader in aws_config to accept a Process, and updates CredentialProcessProvider to use the Process abstraction. The default tokio-based process provider is behind the new default-process feature (enabled by default), mirroring the default-https-client pattern. Users can disable it and provide their own Process implementation.

Open Questions

Technically this will not be used outside of aws-config, but I put the interface in os_shim_internal in aws-types for consistency with Fs and Env. Should this be moved to aws-config altogether?

Testing

./gradlew :aws:sdk:cargoTest
cargo semver-checks in aws/rust-runtime

Checklist

  • For changes to the AWS SDK, generated SDK code, or SDK runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "aws-sdk-rust" in the applies_to key.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codeshaunted codeshaunted requested review from a team as code owners April 3, 2026 18:58
Copy link
Copy Markdown
Collaborator

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

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

Generally looks reasonable to me. One concern about the newly added feature in aws-config

Comment thread aws/rust-runtime/aws-config/Cargo.toml Outdated
credentials-process = ["tokio/process"]
default = ["default-https-client", "rt-tokio", "credentials-process", "sso"]
credentials-process = []
default-process = ["credentials-process", "tokio/process"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am hesitant to add a new feature for this functionality. We use cargo hack to test the powerset of feature combinations for this crate. So each new feature we add has a substantial time cost in CI.

Also think this could be a breaking change for users using no-default-features but who were relying on the existing credential process implementation.

Copy link
Copy Markdown
Contributor Author

@codeshaunted codeshaunted Apr 6, 2026

Choose a reason for hiding this comment

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

Yeah I think you're right, with that restriction though I'm not sure if there's a way to prevent pulling in tokio/process. I guess we can live with this restriction, I'll update to remove the feature for it soon.

@codeshaunted codeshaunted force-pushed the avery/process-provider branch from 510a8da to c65223a Compare April 7, 2026 05:07
@codeshaunted codeshaunted force-pushed the avery/process-provider branch from c65223a to cc91574 Compare April 9, 2026 18:57
@ysaito1001
Copy link
Copy Markdown
Collaborator

ysaito1001 commented Apr 10, 2026

Thanks for the work on this PR, appreciate the effort.

After seeing this PR, stepping back and looking at the bigger picture, we'd like to reconsider the direction here. The goal is to mirror a default credentials provider chain in a virtual environment, as noted in the previous PR

Implementing ProvideCredentials would work, but one of our goals is to mirror the default provider chain as closely as possible, and we'd like to avoid maintaining a largely identical copy.

The concern here is that it expands the public trait surface to support virtual environments, and that surface will only grow as more credential sources are added. We'd prefer not to commit to that path.

Sorry for the back-and-forth — to make things atomic, would it be ok to revert smithy-rs#4570 (since it introduces public API changes that would ship in the next release) and instead start from detailed problem descriptions and functional requirements? We feel we need to spend more time designing.

@codeshaunted
Copy link
Copy Markdown
Contributor Author

codeshaunted commented Apr 13, 2026

Thanks for the work on this PR, appreciate the effort.

After seeing this PR, stepping back and looking at the bigger picture, we'd like to reconsider the direction here. The goal is to mirror a default credentials provider chain in a virtual environment, as noted in the previous PR

Implementing ProvideCredentials would work, but one of our goals is to mirror the default provider chain as closely as possible, and we'd like to avoid maintaining a largely identical copy.

The concern here is that it expands the public trait surface to support virtual environments, and that surface will only grow as more credential sources are added. We'd prefer not to commit to that path.

Sorry for the back-and-forth — to make things atomic, would it be ok to revert smithy-rs#4570 (since it introduces public API changes that would ship in the next release) and instead start from detailed problem descriptions and functional requirements? We feel we need to spend more time designing.

Reverting it is fine, we can just use a fork for now while design is being finalized. I'm open to alternative solutions, just wary about re-implementing the entire default chain, as it seems quite involved.

For the design I'm also available for a call to discuss if that would be helpful for you all!

@ysaito1001
Copy link
Copy Markdown
Collaborator

ysaito1001 commented Apr 14, 2026

Thank you for your reply. Again, apologize for going back to square one.

We'll first create an issue to track the requested functionality of mirroring a default credentials provider chain in a virtual environment. In the issue, it'd be great to provide a high-level description of the problem along with concrete use cases that led to pain points with today's codebase. It not only clarifies the problem space but also serves as useful context for an agent to derive artifacts in the solution space (e.g. functional spec, implementation)

ysaito1001 added a commit that referenced this pull request Apr 14, 2026
ysaito1001 added a commit that referenced this pull request Apr 14, 2026
Per
#4590 (comment)

[Issue](#4601) has been
created to track the desired functionality.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
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.

3 participants