-
Notifications
You must be signed in to change notification settings - Fork 215
Initial setup for credential resolution feature tracking #4224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial setup for credential resolution feature tracking #4224
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks clearer to hoist AWS SDK features to a upstream crate altogether, as opposed to special casing credentials-related features with AwsSdkCredentialsFeatures
defined in aws-credential-types
.
Great start!
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
builder.set_expiration(expiry); | ||
builder.build().expect("set required fields") | ||
|
||
if let Some(features) = val.get_property::<Vec<AwsSdkFeature>>().cloned() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it doesn't matter where this lives if aws-credential-types
has to know about it. Alternatively could just make a new AwsCredentialFeature
enum inside aws-credential-types
and implement BusinessMetric
conversions for that in aws-runtime
. Would mean we don't need a new crate for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, after thinking about the point of having a repository-like crate, I've started learning towards this idea of defining AwsCredentialFeature
in aws-credential-types
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we are leaning towards a separate enum. I still prefer only having two feature enums (AWS and Smithy) since it seems cleaner, but don't know that that preference is strong enough to justify a new crate. Will make the change today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to an AwsCredentialFeature
enum and removed the aws-features
crate in 6bcea29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, brought up one possible suggestion to avoid a new crate but otherwise looks fine.
A new generated diff is ready to view.
A new doc preview is ready to view. |
// Extract the FrozenLayer placed in the Identity property bag by the From<Credentials> impl. | ||
// This layer contains AwsSdkFeatures for the user agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding comments, but since this is in generic smithy runtime and Identity's frozen layer could be used for something else, so either I'd remove the comments or make them more abstract, so they're not tied specifically to AwsSdkFeatures
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, made comment more generic in b785a6e
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
3965ea8
into
feature/credential-features
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Continuing the work in #4224, adding user-agent feature tracking for Credential Providers. ## Description <!--- Describe your changes in detail --> Added `AwsCredentialFeatures` properties for each implementor of `ProvideCredentials` in `aws-config`. ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> Added integration tests for each of the supported Credential Providers. This required exposing some previously `pub(crate)` utility functions as `pub` under a `test-util` feature flag. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Merging the feature branch containing work from the below two PRs: * #4238 * #4224 ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [x] 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._ --------- Co-authored-by: AWS SDK Rust Bot <[email protected]>
Motivation and Context
Initial setup for tracking credential/identity resolution based features.
Description
The biggest change here is the addition of a new
aws-features
crate containing thesdk_features
module that used to live inaws-runtime
. This was done so that theAwsSdkFeature
type could be referenced in theaws-credential-types
crate without causing a circular dependency.Initially I was just going to pass along the types without downcasting, but they would have had to be downcast eventually, since in the
Credentials
typemap they areVec<AwsSdkFeature>
but to add them to aLayer
we need to extract the individualAwsSdkFeature
s from theVec
. This could either happen inaws-credential-types
whereFrom<Credentials> for Identity
is implemented or inaws-smithy-runtime
in the orchestrator where theIdentity
is resolved. Since these credentials types are AWS specific it seemed to make more sense to keep it inaws/rust-runtime
.Other changes:
Credentials
(and manually implement the previously derived traits for it) to carry the feature informationFrom<Credentials> for Identity
implementation to extract theAwsSdkFeatures
and pass them in aLayer
to theIdentity
's typemap.resolve_identity
in the orchestrator to extract theFrozenLayer
fromIdentity
and insert it in theConfigBag
so that theUserAgentInterceptor
can extract it later.Testing
Added new tests around
Credentials
equality and the updatedFrom<Credentials> for Identity
implementation.Note on failing semver test: Failing because the
UnwindSafe
traits are no longer auto implemented forCredentials
This is due to us adding a
HashMap<TypeId, TypeErasedBox>
toCredentials
. We could likely wrap this in anArc<Mutex<>>
to keep it mutable and get back the unwind safety, but that doesn't feel like it justifies the added complexity. Open to debate on this one though.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.