-
Notifications
You must be signed in to change notification settings - Fork 939
Add business metrics support for STS and Profile credential providers #6426
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
base: feature/master/feature-ids-implementation
Are you sure you want to change the base?
Add business metrics support for STS and Profile credential providers #6426
Conversation
077e209
to
685e7ac
Compare
|
.build(); | ||
} | ||
|
||
/** | ||
* Create the SSO credentials provider based on the related profile properties. | ||
*/ | ||
private AwsCredentialsProvider ssoProfileCredentialsProvider() { | ||
validateRequiredPropertiesForSsoCredentialsProvider(); | ||
boolean isLegacy = validateRequiredPropertiesForSsoCredentialsProvider(); |
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.
Don't we throw exception if it's invalid in validateRequiredPropertiesForSsoCredentialsProvider
?
* previous credentials providers that are chained with this one. | ||
* @return The credentials provider with permissions derived from the source credentials provider and profile. | ||
*/ | ||
AwsCredentialsProvider create(AwsCredentialsProvider sourceCredentialsProvider, Profile profile, String source); |
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.
Can we create a wrapper object for those parameters? I'm trying to avoid the situation where we need to add a fourth parameter in the future and have to create another constructor.
Something like
AwsCredentialsProvider create(XXX input) //naming to be determined
Also, we should throw unsupportedoperation exception here otherwise it'd may break customers
if (!StringUtils.isEmpty(this.source)) { | ||
providerName = String.format("%s,%s", this.source, providerName); | ||
} |
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.
nit: can we move this logic to ctor so that we just compute this once and not every time? Also, can we use plain string concatenation since String.format is a bit expensive? String.format is okay for exceptional messages/logging statement because the impact is minimal (doesn't affect every request), it seems unnecessary here.
Same for other places
@@ -48,6 +49,10 @@ public AssumeRoleWithWebIdentityRequest get() { | |||
return request.toBuilder().webIdentityToken(getToken(webIdentityTokenFile)).build(); | |||
} | |||
|
|||
public String source() { |
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.
If it's nullable, we should use Optional<String>
@@ -145,6 +155,15 @@ public Builder refreshRequest(Consumer<AssumeRoleRequest.Builder> assumeRoleRequ | |||
return refreshRequest(AssumeRoleRequest.builder().applyMutation(assumeRoleRequest).build()); | |||
} | |||
|
|||
/** | |||
* An optional string list of {@link BusinessMetricFeatureId} denoting previous credentials providers |
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.
BusinessMetricFeatureId
is a protected API and it's not intended for users to use directly. Can we update the javadoc to mention this method is primarily intended for use by AWS SDK internal components?
Same for other places
AwsCredentials awsCredentials = credentialsProvider.resolveCredentials(); | ||
if (awsCredentials instanceof AwsSessionCredentials) { | ||
AwsSessionCredentials sessionCredentials = (AwsSessionCredentials) awsCredentials; | ||
Optional<String> providerName = awsCredentials.providerName(); | ||
if (providerName.isPresent()) { | ||
return sessionCredentials.copy(s -> s.providerName(providerName.get() + "," + PROVIDER_NAME)); | ||
} | ||
return sessionCredentials; | ||
} | ||
return awsCredentials; | ||
} |
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.
Question: is there any reason we have special handling for this class? It seems for other providers, we just update the providerName method to include source
assertThat(userAgent).doesNotContain("o"); | ||
assertThat(userAgent).doesNotContain("n"); | ||
|
||
} catch (Exception e) { |
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.
We probably should not silently swallow exception. Same for other places
Adds business metric tracking for credentials
Motivation and Context
Keeping track of how users are providing credentials to SDKs and which credentials providers are being used.
Modifications
This PR adds business metrics support for these credential providers:
Key Technical Changes
Source Propagation: Introduces
source
parameter on credential provider builders to track credential provider chainsm/n,g,i
(profile + env vars + assume role)Provider Name Updates: Changes existing
providerName()
methods to return business metric codes instead of full class namesChain Tracking: Supports credential scenarios like:
credential_source = Ec2InstanceMetadata
Example User-Agent Output
m/g
m/n,o,i
m/g
(only successful provider shown)Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License