Adding identity provider source to user agent string #5029
Conversation
|
|
||
| for (IdentityProviderNameMapping provider : values()) { | ||
| if (provider.value().equals(value)) { | ||
| return Optional.of(provider.name()); |
There was a problem hiding this comment.
Leaning towards using the lower case variant of the enum, stat instead of STAT.
| } | ||
|
|
||
| private static String sanitizedProviderOrNull(String value) { | ||
| if (containsAllowedCharacters(value) && value.toLowerCase(Locale.US).endsWith("provider")) { |
There was a problem hiding this comment.
Adding provider as a criteria may be too narrow
There was a problem hiding this comment.
We decided offline to remove this criterion
|
|
|
||
| private static <T extends Identity> Optional<String> providerNameFromIdentity(SelectedAuthScheme<T> selectedAuthScheme) { | ||
| CompletableFuture<? extends T> identityFuture = selectedAuthScheme.identity(); | ||
| T identity = CompletableFutureUtils.joinLikeSync(identityFuture); |
There was a problem hiding this comment.
Looks like this is another place where we are making it wait for a future. Do we just want to add a comment indicating that we are blocking here and we eventually would need to remove this ?
There was a problem hiding this comment.
I don't think it's necessary; it's easy to search for this in the code and that's something we'd have to do anyway when we've unblocked async identity handling.
| import software.amazon.awssdk.utils.Logger; | ||
| import software.amazon.awssdk.utils.StringUtils; | ||
| import software.amazon.awssdk.utils.http.SdkHttpUtils; | ||
|
|
||
| /** | ||
| * Apply any custom user agent supplied, otherwise instrument the user agent with info about the SDK and environment. |
There was a problem hiding this comment.
Would help to have some Javadoc
There was a problem hiding this comment.
I think I removed it because I had started a refactoring. Added a new text back.
| */ | ||
| public enum IdentityProviderNameMapping { | ||
|
|
||
| SYS("SystemPropertyCredentialsProvider"), |
There was a problem hiding this comment.
As discussed, would be great to expose these abbreviations to our internal users to avoid any confusions as to which abbreviation maps to which provider (probably in a separate action item)
There was a problem hiding this comment.
This will be addressed when parsing the metrics.
cenedhryn
merged commit 46d3707
into
feature/master/credentials-source
Motivation and Context
This PR adds the identity source (which credentials provider that was used to resolve the identity / credentials) to the user agent string.
The
providerof the identity is added to thecfgpart of the user agent as anauth-source. Auth source is a new section and not ratified in any specification, but should be parseable by the ua metrics system.We choose to add a short form of the credentials provider class as an identifier for now. This can easily be changed as the requirements for the user agent string format and contents are changed, and is backwards compatible.
In this PR, credential providers are represented by the
StaticCredentialsProvider. A follow up PR will have implementations for all other credentials providers.Modifications
StaticCredentialsProviderApplyUserAgentStage, the SDK adds the identity source to the user agent string if present.StaticCredentialsProvider<->STATprovider(debatable)Q. Why not add the short name to the provider class
Most providers contain a
toStringthat prints the name. There's a value in getting the complete name of the provider. In addition, in the future we may not use this short form as the usage of the UA string evolvesQ. How is this addition compatible with the proposed changes to the UA string?
When the SDK adds feature ID to the UA string, the value written to the identity by each provider can be changed to a different value, such as a string digit.