steveloughran commented on a change in pull request #3412:
URL: https://github.com/apache/hadoop/pull/3412#discussion_r705321302
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -436,7 +444,7 @@ public void initialize(URI name, Configuration originalConf)
initializeStatisticsBinding();
// If CSE-KMS method is set then CSE is enabled.
isCSEEnabled = S3AUtils.lookupPassword(conf,
Review comment:
this flag will be true if any algorithm is set. Is this what you want?
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java
##########
@@ -449,9 +450,32 @@ private Constants() {
* May be set within a JCEKS file.
* Value: "{@value}".
*/
+ @Deprecated
public static final String SERVER_SIDE_ENCRYPTION_KEY =
"fs.s3a.server-side-encryption.key";
+ /**
+ * Set S3-server side encryption(SSE) or S3-Client side encryption(CSE)
+ * algorithm. Check {@link S3AEncryptionMethods} for valid options.
+ * <br>
+ * value: {@value}
+ */
+ public static final String S3_ENCRYPTION_ALGORITHM =
+ "fs.s3a.encryption-algorithm";
Review comment:
I think we should call this "fs.s3a.encryption.algorithm" for
consistency with the key.
It's a bigger change than before, but we could also add a deprecation entry
for fs.s3a.encryption-algorithm in case people got it wrong
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java
##########
@@ -39,6 +39,8 @@
import java.util.stream.Collectors;
import com.amazonaws.services.s3.model.MultipartUpload;
+
+import org.apache.hadoop.fs.s3a.Constants;
Review comment:
nit: move to the "real" org.apache imports
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/delegation/ITestSessionDelegationInFileystem.java
##########
@@ -142,22 +143,22 @@ protected Configuration createConfiguration() {
assumeSessionTestsEnabled(conf);
disableFilesystemCaching(conf);
String s3EncryptionMethod =
- conf.getTrimmed(SERVER_SIDE_ENCRYPTION_ALGORITHM,
+ conf.getTrimmed(Constants.S3_ENCRYPTION_ALGORITHM,
S3AEncryptionMethods.SSE_KMS.getMethod());
- String s3EncryptionKey = conf.getTrimmed(SERVER_SIDE_ENCRYPTION_KEY, "");
+ String s3EncryptionKey = conf.getTrimmed(Constants.S3_ENCRYPTION_KEY, "");
removeBaseAndBucketOverrides(conf,
DELEGATION_TOKEN_BINDING,
- SERVER_SIDE_ENCRYPTION_ALGORITHM,
- SERVER_SIDE_ENCRYPTION_KEY);
+ Constants.S3_ENCRYPTION_ALGORITHM,
Review comment:
include the old option names
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestSSEConfiguration.java
##########
@@ -181,14 +181,14 @@ private S3AEncryptionMethods getAlgorithm(String
algorithm, String key)
private Configuration buildConf(String algorithm, String key) {
Configuration conf = emptyConf();
if (algorithm != null) {
- conf.set(SERVER_SIDE_ENCRYPTION_ALGORITHM, algorithm);
+ conf.set(Constants.S3_ENCRYPTION_ALGORITHM, algorithm);
} else {
- conf.unset(SERVER_SIDE_ENCRYPTION_ALGORITHM);
+ conf.unset(Constants.S3_ENCRYPTION_ALGORITHM);
Review comment:
unset the old option too
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/delegation/ITestSessionDelegationInFileystem.java
##########
@@ -348,8 +349,8 @@ public void testDelegatedFileSystem() throws Throwable {
// this is to simulate better a remote deployment.
removeBaseAndBucketOverrides(bucket, conf,
ACCESS_KEY, SECRET_KEY, SESSION_TOKEN,
- SERVER_SIDE_ENCRYPTION_ALGORITHM,
- SERVER_SIDE_ENCRYPTION_KEY,
+ Constants.S3_ENCRYPTION_ALGORITHM,
Review comment:
+old option names
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/scale/ITestS3AHugeFilesSSECDiskBlocks.java
##########
@@ -48,12 +48,12 @@ public void setup() throws Exception {
@Override
protected Configuration createScaleConfiguration() {
Configuration conf = super.createScaleConfiguration();
- removeBaseAndBucketOverrides(conf, SERVER_SIDE_ENCRYPTION_KEY,
- SERVER_SIDE_ENCRYPTION_ALGORITHM);
+ removeBaseAndBucketOverrides(conf, S3_ENCRYPTION_KEY,
Review comment:
+old ones
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/S3ATestUtils.java
##########
@@ -1533,14 +1534,20 @@ public static S3AFileStatus innerGetFileStatus(
}
/**
- * Skip a test if CSE KMS key id is not set.
+ * Skip a test if encryption algo or encryption key is not set.
Review comment:
nit: algorithm
--
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]
