ahmarsuhail commented on code in PR #6615:
URL: https://github.com/apache/hadoop/pull/6615#discussion_r1523052340
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ATemporaryCredentials.java:
##########
@@ -325,16 +356,13 @@ public void testSessionCredentialsRegionNoEndpoint()
throws Throwable {
@Test
public void testSessionCredentialsRegionBadEndpoint() throws Throwable {
describe("Create a session with a bad region and expect fast failure");
- IllegalArgumentException ex
+ IOException ex
Review Comment:
revert to IllegalArgumentException
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/DelegationConstants.java:
##########
@@ -160,6 +160,18 @@ public final class DelegationConstants {
*/
public static final String STS_STANDARD = "sts.amazonaws.com";
+ /**
+ * The format of the STS Endpoint
+ */
Review Comment:
you can move both of these into STSClientFactory as they are only relevant
for that class.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/DelegationConstants.java:
##########
@@ -160,6 +160,18 @@ public final class DelegationConstants {
*/
public static final String STS_STANDARD = "sts.amazonaws.com";
+ /**
+ * The format of the STS Endpoint
+ */
+ public static final String STS_ENDPOINT_URI_PATTERN =
"^sts\\..*\\.amazonaws\\.com$";
Review Comment:
your pattern will need to handle china as well.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/STSClientFactory.java:
##########
@@ -125,6 +125,14 @@ public static StsClientBuilder builder(
public static StsClientBuilder builder(final AwsCredentialsProvider
credentials,
final Configuration conf, final String stsEndpoint, final String
stsRegion,
final String bucket) throws IOException {
+ // If an STS endpoint is provided and if it is not STS_STANDARD
(sts.amazonaws.com)
+ // it should match E_INVALID_STS_ENDPOINT_PATTERN.
+ if (!isEmpty(stsEndpoint) &&
+ !STS_STANDARD.equals(stsEndpoint) &&
+ !stsEndpoint.matches(STS_ENDPOINT_URI_PATTERN)) {
+ throw new
IOException(String.format(E_INVALID_STS_ENDPOINT_PATTERN,stsEndpoint));
Review Comment:
Yes, please use `IllegalArgumentException`
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ATemporaryCredentials.java:
##########
@@ -163,6 +163,37 @@ public void testSTS() throws IOException {
}
}
+ /**
+ * Test use of Invalid STS for requesting temporary credentials.
+ *
+ * The property test.sts.endpoint can be set to point this at different
+ * STS endpoints. This test will use the AWS credentials (if provided) for
+ * S3A tests to request temporary credentials, then attempt to use those
+ * credentials instead.
+ *
+ * @throws IOException failure
+ */
+ @Test
+ public void testSTSInvalid() throws IOException {
+ Configuration conf = getContract().getConf();
+ S3AFileSystem testFS = getFileSystem();
+ credentials = getS3AInternals().shareCredentials("testSTS");
+
+ String bucket = testFS.getBucket();
+ try {
Review Comment:
Look at the other tests in this class,
`testSessionCredentialsRegionBadEndpoint()` for example. We can write this
similarly.
You will also need to add a test case to check if China endpoint works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
