Fix the lease coordination table permissions #5097
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Souvik Bose <souvbose@amazon.com>
sb2k16
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
graytaylor0
previously approved these changes
Oct 22, 2024
dlvenable
requested changes
Oct 22, 2024
| @@ -65,6 +74,10 @@ public Map<String, String> getStsHeaderOverrides() { | |||
| return stsHeaderOverrides; | |||
| } | |||
|
|
|||
| public boolean isUseDefaultCredentials() { | |||
There was a problem hiding this comment.
We already have a concept of default in the plugin configuration. So using "default" here is rather confusing.
So I think we should use a different name here.
One idea: isUseDefaultCredentialsProvider to clarify that this is coupled with the SDK's DefaultCredentialsProvider.
Thoughts?
There was a problem hiding this comment.
Thanks @dlvenable for your input. I will make the change.
| @@ -49,6 +54,10 @@ public static AwsCredentialsOptions defaultOptions() { | |||
| return DEFAULT_OPTIONS; | |||
| } | |||
|
|
|||
| public static AwsCredentialsOptions defaultOptionsWithDefaultCreds() { | |||
There was a problem hiding this comment.
Suggested change
| public static AwsCredentialsOptions defaultOptionsWithDefaultCreds() { | |
| public static AwsCredentialsOptions defaultOptionsWithDefaultCredentialsProvider() { |
There was a problem hiding this comment.
Thanks @dlvenable. I have made the name change.
| * @param useDefaultCredentials | ||
| * @return The {@link Builder} for continuing to build | ||
| */ | ||
| public Builder withUseDefaultCredentials(final boolean useDefaultCredentials) { |
There was a problem hiding this comment.
Suggested change
| public Builder withUseDefaultCredentials(final boolean useDefaultCredentials) { | |
| public Builder withUseDefaultCredentialsProvider(final boolean useDefaultCredentialsProvider) { |
There was a problem hiding this comment.
Thanks @dlvenable. I have made the name change.
Signed-off-by: Souvik Bose <souvbose@amazon.com>
graytaylor0
approved these changes
Oct 22, 2024
dlvenable
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is to fix the credentials required for Lease Coordination table access. The fix is to use the default credentials instead of the pipeline role when Data Prepper would access the dynamoDb table.
Issues Resolved
Resolves #1082
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.