-
Notifications
You must be signed in to change notification settings - Fork 20
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
We will use the regional endpoint with STS whenever possible. #45
Conversation
The global endpoint is used in case the region cannot be found. #44
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.
Overall LGTM. Minor comments added.
...-mapper-application/src/main/java/com/amazon/aws/emr/credentials/STSCredentialsProvider.java
Show resolved
Hide resolved
...-mapper-application/src/main/java/com/amazon/aws/emr/credentials/STSCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...-mapper-application/src/main/java/com/amazon/aws/emr/credentials/STSCredentialsProvider.java
Outdated
Show resolved
Hide resolved
ApplicationConfiguration object to create the AWS STS client.
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.
Made the STS client instantiation configurable. Based on the app props now we will hit the regional/global endpoint.
public boolean isRegionalStsEnabled() { | ||
return Boolean.parseBoolean(properties.getProperty(Constants.REGIONAL_STS_ENABLED, | ||
String.valueOf("true"))); | ||
} | ||
|
||
|
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/ Add some comments to explain this config
2/ nit: regionalStsEnabled -> regionalStsEndpointEnabled (same comment applies to other relevant places)
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.
Done
region = Regions.getCurrentRegion(); | ||
regionString = region.getName(); | ||
} catch (Exception e) { | ||
log.error("Cannot determine the AWS region. Defaulting to {}", regionString); |
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 the code cannot get the regionString, how can subsequent code uses the regional endpoint?
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.
It's set to "us-west-2" in the beginning. So we will always use us-west-2 in this case.
- Renamed constant to use regional STS endpoint - Added some comments about the constant
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.
Incorporated the code review comments.
- Renamed the constant
- Added some comment to enable the regional STS endpoint
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.
LGTM
The global endpoint is used in case the region cannot be found.
#44
Issue #, if available:
Testing:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.