-
Notifications
You must be signed in to change notification settings - Fork 883
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
Adds business metrics for credential providers #5814
base: master
Are you sure you want to change the base?
Conversation
593ec00
to
73242f0
Compare
|
Can you elaborate further on why getting |
Arguments.of(StaticCredentialsProvider.create(SESSION_IDENTITY), "m/D"), | ||
Arguments.of(StaticCredentialsProvider.create(BASIC_IDENTITY), "m/D") |
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.
Probably missed something, but why are we checking for m/D
here? (D
being RETRY_MODE_LEGACY
).
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.
I think there is a pretty large bug in how the user agent is constructed, putting credential provider business metrics in auth source but not in m
.
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.
Why did we remove this test?
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.
Because there is also a SystemSettingCredentialsProvidersTest
that is more extensive 🙄
* @return The credentials provider with permissions derived from the source credentials provider and profile. | ||
*/ | ||
AwsCredentialsProvider create(AwsCredentialsProvider sourceCredentialsProvider, Profile 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.
The javadoc implies this this might better as a List<String>
, any reason it isn't?
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.
See the general comment. I think the question of List or not List will become clearer if I go through the APIs for source again.
When I look at the code again, I think it will work. In this example, ideally the user agent should contain p,0,i for the main call. First, the profile credentials provider is called and in ProfileCredentialsUtils, the use case for named provider is selected based on the settings in Overall the question of whether composite sources should be concatenated String or List - I'm still leaning towards concatenated String. There is a fair amount of refactoring that needs to be done because toString cannot and should not be used to retrieve the business metric. I have to roll back some weird code. |
Motivation and Context
Tracking how credentials were resolved
Modifications
source
on some builders for credential providers. It's the only non-trivial way to link more than one metrics value togetherDeviations from specs:
u
because it doesn't make sense - It's already tracked as legacy via theCREDENTIALS_PROFILE_SSO_LEGACY
t
value. Once in the SSO provider legacy doesn't really matter. The regularCREDENTIALS_PROFILE_SSO
value ofs
can be used for both cases. You can't really set a legacy client without coming from the profilep,0
for the imds call andp,0,i
for the credential metrics: named provider followed by imds followed by assume role. Instead, we'll havep,0
andp,i
for the imds call and the assume roll call respectively.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