[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r326731189
##
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
##
@@ -343,9 +343,43 @@ private Constants() {
public static final String SERVER_SIDE_ENCRYPTION_KEY =
"fs.s3a.server-side-encryption.key";
- //override signature algorithm used for signing requests
+ /**
+ * List of custom Signers. The signer class will be loaded, and the signer
+ * name will be associated with this signer class in the S3 SDK. e.g. Single
+ * CustomSigner -> 'CustomSigner:org.apache...CustomSignerClass Multiple
+ * CustomSigners ->
'CSigner1:CustomSignerClass1,CSigner2:CustomerSignerClass2
+ */
+ public static final String CUSTOM_SIGNERS = "fs.s3a.custom.signers";
+
+ /**
+ * There's 3 parameters that can be used to specify a non-default signing
+ * algorithm. fs.s3a.signing-algorithm - This property has existed for the
+ * longest time. If specified, without either of the other 2 properties being
+ * specified, this signing algorithm will be used for S3 and DDB (S3Guard).
+ * The other 2 properties override this value for S3 or DDB.
Review comment:
also: other uses like STS. Maybe say "non S3 requests, such as to DDB (for
S3Guard) or to STS."
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r326731189
##
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
##
@@ -343,9 +343,43 @@ private Constants() {
public static final String SERVER_SIDE_ENCRYPTION_KEY =
"fs.s3a.server-side-encryption.key";
- //override signature algorithm used for signing requests
+ /**
+ * List of custom Signers. The signer class will be loaded, and the signer
+ * name will be associated with this signer class in the S3 SDK. e.g. Single
+ * CustomSigner -> 'CustomSigner:org.apache...CustomSignerClass Multiple
+ * CustomSigners ->
'CSigner1:CustomSignerClass1,CSigner2:CustomerSignerClass2
+ */
+ public static final String CUSTOM_SIGNERS = "fs.s3a.custom.signers";
+
+ /**
+ * There's 3 parameters that can be used to specify a non-default signing
+ * algorithm. fs.s3a.signing-algorithm - This property has existed for the
+ * longest time. If specified, without either of the other 2 properties being
+ * specified, this signing algorithm will be used for S3 and DDB (S3Guard).
+ * The other 2 properties override this value for S3 or DDB.
Review comment:
also: other uses like STS.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326730426 ## File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/SignerManager.java ## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.fs.s3a; + +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; +import java.io.Closeable; Review comment: usual and predictable comments about imports 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r326730224
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSignerManager.java
##
@@ -0,0 +1,129 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.s3a;
+
+import com.amazonaws.SignableRequest;
+import com.amazonaws.auth.AWSCredentials;
+import com.amazonaws.auth.Signer;
+import com.amazonaws.auth.SignerFactory;
+import java.util.concurrent.TimeUnit;
+import org.assertj.core.api.Assertions;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+import org.apache.hadoop.classification.InterfaceAudience.Private;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.LambdaTestUtils;
+
+import static org.apache.hadoop.fs.s3a.Constants.CUSTOM_SIGNERS;
+
+/**
+ * Tests for the SignerManager.
+ */
+public class TestSignerManager {
Review comment:
Going to highlight that the completely optional
org.apache.hadoop.test.HadoopTestBase class can act as a base class, offering
setup of the test timeouts and automatic naming of the junit thread to the
current method. I would encourage your use of it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326729375 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSignerManager.java ## @@ -0,0 +1,129 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +import com.amazonaws.SignableRequest; +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; +import java.util.concurrent.TimeUnit; Review comment: go on, pull this up into its own block... 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326729184 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java ## @@ -19,17 +19,24 @@ package org.apache.hadoop.fs.s3a; import com.amazonaws.ClientConfiguration; +import com.amazonaws.SignableRequest; +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.S3ClientOptions; +import java.io.IOException; Review comment: actually, given there's an existing java. block down below, just move it to at least be consistent 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326728757 ## File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/DynamoDBClientFactory.java ## @@ -27,6 +27,7 @@ import com.amazonaws.services.dynamodbv2.AmazonDynamoDBClientBuilder; import com.google.common.base.Preconditions; import org.apache.commons.lang3.StringUtils; +import org.apache.hadoop.fs.s3a.Constants; Review comment: import location 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326728677 ## File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/SessionTokenBinding.java ## @@ -31,6 +31,7 @@ import com.amazonaws.auth.AWSSessionCredentials; import com.amazonaws.services.securitytoken.AWSSecurityTokenService; import com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.fs.s3a.Constants; Review comment: import location 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r326728095
##
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
##
@@ -343,9 +343,43 @@ private Constants() {
public static final String SERVER_SIDE_ENCRYPTION_KEY =
"fs.s3a.server-side-encryption.key";
- //override signature algorithm used for signing requests
+ /**
+ * List of custom Signers. The signer class will be loaded, and the signer
+ * name will be associated with this signer class in the S3 SDK. e.g. Single
+ * CustomSigner -> 'CustomSigner:org.apache...CustomSignerClass Multiple
+ * CustomSigners ->
'CSigner1:CustomSignerClass1,CSigner2:CustomerSignerClass2
+ */
+ public static final String CUSTOM_SIGNERS = "fs.s3a.custom.signers";
+
+ /**
Review comment:
you are going to have to add these to the aws docs I'm afraid. This is
probably time to start a new "signing" section
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326305880 ## File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/STSClientFactory.java ## @@ -31,6 +31,7 @@ import com.amazonaws.services.securitytoken.model.Credentials; import com.amazonaws.services.securitytoken.model.GetSessionTokenRequest; import com.google.common.base.Preconditions; +import org.apache.hadoop.fs.s3a.Constants; Review comment: import location 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r326209693 ## File path: hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/SignerManager.java ## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.fs.s3a; + +import static org.apache.hadoop.fs.s3a.Constants.CUSTOM_SIGNERS; + +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; +import java.io.Closeable; +import java.io.IOException; +import org.apache.hadoop.conf.Configuration; Review comment: imports still a bit confused here 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r325567156 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSignerManager.java ## @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +import static org.apache.hadoop.fs.s3a.Constants.CUSTOM_SIGNERS; Review comment: all statics in the same block at the bottom of the imports 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r325566547
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSignerManager.java
##
@@ -0,0 +1,124 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.s3a;
+
+import static org.apache.hadoop.fs.s3a.Constants.CUSTOM_SIGNERS;
+
+import com.amazonaws.SignableRequest;
+import com.amazonaws.auth.AWSCredentials;
+import com.amazonaws.auth.Signer;
+import com.amazonaws.auth.SignerFactory;
+import java.util.concurrent.TimeUnit;
+import org.apache.hadoop.classification.InterfaceAudience.Private;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.LambdaTestUtils;
+import org.assertj.core.api.Assertions;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+public class TestSignerManager {
+
+ @Rule
+ public Timeout testTimeout = new Timeout(
+10_000L, TimeUnit.MILLISECONDS
Review comment:
indentation.
FWIW, for any ITest, the values in S3ATestConstants are the ones to refer
to, not any other ones. Why so? When you are debugging things in the IDE you
can edit those values in one place and know that your debug session won't
suddenly finish 5-10 minutes in. I usually end up doing exactly that after my
first debug session times out
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r325566952 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSignerManager.java ## @@ -0,0 +1,124 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.fs.s3a; + +import static org.apache.hadoop.fs.s3a.Constants.CUSTOM_SIGNERS; + +import com.amazonaws.SignableRequest; +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.classification.InterfaceAudience.Private; Review comment: all org.aoache stuff needs to go in its own block after the block of non-asf imports 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324782298
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##
@@ -617,4 +624,135 @@ public void testSecurityCredentialPropagationEndToEnd()
throws Exception {
"override,base");
}
+ @Test(timeout = 10_000L)
+ public void testS3SpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String s3SignerOverride = "testS3Signer";
+
+// Default SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ @Test(timeout = 10_000L)
+ public void testDdbSpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String ddbSignerOverride = "testDdbSigner";
+
+// Default SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ // Expecting generic Exception.class to handle future implementation changes.
+ // For now, this is an NPE
+ @Test(timeout = 10_000L, expected = Exception.class)
+ public void testCustomSignerFailureIfNotRegistered() {
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
+ }
+
+ @Test(timeout = 10_000L)
+ public void testCustomSignerInitialization() {
+Configuration config = new Configuration();
+SignerForTest1.reset();
+SignerForTest2.reset();
+config.set(CUSTOM_SIGNERS, "testsigner1:" +
SignerForTest1.class.getName());
+initCustomSigners(config);
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
+s1.sign(null, null);
+Assert.assertEquals(true, SignerForTest1.initialized);
+ }
+
+ @Test(timeout = 10_000L)
+ public void testMultipleCustomSignerInitialization() {
+Configuration config = new Configuration();
+SignerForTest1.reset();
+SignerForTest2.reset();
+config.set(CUSTOM_SIGNERS,
+"testsigner1:" + SignerForTest1.class.getName() + "," + "testsigner2:"
++ SignerForTest2.class.getName());
+initCustomSigners(config);
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
+s1.sign(null, null);
+Assert.assertEquals(true, SignerForTest1.initialized);
+
+Signer s2 = SignerFactory.createSigner("testsigner2", null);
+s2.sign(null, null);
+Assert.assertEquals(true, SignerForTest2.initialized);
Review comment:
assertTrue, with error message.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail:
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324781956
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##
@@ -617,4 +624,135 @@ public void testSecurityCredentialPropagationEndToEnd()
throws Exception {
"override,base");
}
+ @Test(timeout = 10_000L)
+ public void testS3SpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String s3SignerOverride = "testS3Signer";
+
+// Default SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ @Test(timeout = 10_000L)
+ public void testDdbSpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String ddbSignerOverride = "testDdbSigner";
+
+// Default SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ // Expecting generic Exception.class to handle future implementation changes.
+ // For now, this is an NPE
+ @Test(timeout = 10_000L, expected = Exception.class)
+ public void testCustomSignerFailureIfNotRegistered() {
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
Review comment:
Prefer `LambaTestUtils.intercept` which lets you assert the text of the
caught exception, and which includes the toString value of the result of the
l-expression when it did not fail
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324782221
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##
@@ -617,4 +624,135 @@ public void testSecurityCredentialPropagationEndToEnd()
throws Exception {
"override,base");
}
+ @Test(timeout = 10_000L)
+ public void testS3SpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String s3SignerOverride = "testS3Signer";
+
+// Default SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ @Test(timeout = 10_000L)
+ public void testDdbSpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String ddbSignerOverride = "testDdbSigner";
+
+// Default SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
+
+// Configured base SIGNING_ALGORITHM, overridden for S3
+config = new Configuration();
+config.set(SIGNING_ALGORITHM, signerOverride);
+config.set(SIGNING_ALGORITHM_DDB, ddbSignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertEquals(ddbSignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert
+.assertEquals(signerOverride, clientConfiguration.getSignerOverride());
+ }
+
+ // Expecting generic Exception.class to handle future implementation changes.
+ // For now, this is an NPE
+ @Test(timeout = 10_000L, expected = Exception.class)
+ public void testCustomSignerFailureIfNotRegistered() {
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
+ }
+
+ @Test(timeout = 10_000L)
+ public void testCustomSignerInitialization() {
+Configuration config = new Configuration();
+SignerForTest1.reset();
+SignerForTest2.reset();
+config.set(CUSTOM_SIGNERS, "testsigner1:" +
SignerForTest1.class.getName());
+initCustomSigners(config);
+Signer s1 = SignerFactory.createSigner("testsigner1", null);
+s1.sign(null, null);
+Assert.assertEquals(true, SignerForTest1.initialized);
Review comment:
assertTrue, with error message.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324781226
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##
@@ -617,4 +624,135 @@ public void testSecurityCredentialPropagationEndToEnd()
throws Exception {
"override,base");
}
+ @Test(timeout = 10_000L)
+ public void testS3SpecificSignerOverride() throws IOException {
+ClientConfiguration clientConfiguration = null;
+Configuration config;
+
+String signerOverride = "testSigner";
+String s3SignerOverride = "testS3Signer";
+
+// Default SIGNING_ALGORITHM, overridden for S3 only
+config = new Configuration();
+config.set(SIGNING_ALGORITHM_S3, s3SignerOverride);
+clientConfiguration = S3AUtils.createAwsConfForS3(config, "dontcare");
+Assert.assertEquals(s3SignerOverride,
+clientConfiguration.getSignerOverride());
+clientConfiguration = S3AUtils.createAwsConfForDdb(config, "dontcare");
+Assert.assertNull(clientConfiguration.getSignerOverride());
Review comment:
Needs an error message. FWIW, when writing tests you suspect I will be
reviewing, always add an error message. one possible exception, assertEquals
-but even there I would prefer one.
When coding, Think to yourself "Jenkins just failed and all I have is this
stack trace of the failure -is that enough to debug what went wrong?". If not:
log more and expand the assertion message.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324780120
##
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##
@@ -617,4 +624,135 @@ public void testSecurityCredentialPropagationEndToEnd()
throws Exception {
"override,base");
}
+ @Test(timeout = 10_000L)
Review comment:
There's already a test timeout ruleeout so you don't need `timeout = 10_000`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r324779446 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java ## @@ -19,17 +19,24 @@ package org.apache.hadoop.fs.s3a; import com.amazonaws.ClientConfiguration; +import com.amazonaws.SignableRequest; +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.S3ClientOptions; +import java.io.IOException; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.hadoop.classification.InterfaceAudience.Private; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.contract.ContractTestUtils; import org.apache.hadoop.fs.s3native.S3xLoginHelper; import org.apache.hadoop.test.GenericTestUtils; +import org.junit.Assert; Review comment: FWIW, We are moving to assertJ when we think it makes for better assertions. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB URL: https://github.com/apache/hadoop/pull/1332#discussion_r324779091 ## File path: hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java ## @@ -19,17 +19,24 @@ package org.apache.hadoop.fs.s3a; import com.amazonaws.ClientConfiguration; +import com.amazonaws.SignableRequest; +import com.amazonaws.auth.AWSCredentials; +import com.amazonaws.auth.Signer; +import com.amazonaws.auth.SignerFactory; import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.S3ClientOptions; +import java.io.IOException; Review comment: should go at the top In its own little block 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow separate custom signing algorithms for S3 and DDB
steveloughran commented on a change in pull request #1332: HADOOP-16445. Allow
separate custom signing algorithms for S3 and DDB
URL: https://github.com/apache/hadoop/pull/1332#discussion_r324778945
##
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##
@@ -300,6 +300,8 @@ public void initialize(URI name, Configuration
originalConf)
LOG.debug("Initializing S3AFileSystem for {}", bucket);
// clone the configuration into one with propagated bucket options
Configuration conf = propagateBucketOptions(originalConf, bucket);
+// Initialize any custom signers
+initCustomSigners(conf);
Review comment:
I guess this is the first thing which needs to be done, before anything else
even thinks about talking to AWS services. If so, this is probably the right
place. But I would put it after `patchSecurityCredentialProviders`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
