bhattmanish98 commented on code in PR #7461:
URL: https://github.com/apache/hadoop/pull/7461#discussion_r1988830293
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,43 @@ public void testBothProviderFixedTokenConfigured() throws
Exception {
}
}
+ /**
+ * Helper method to get the Fixed SAS token value
+ */
+ private String getFixedSASToken(AbfsConfiguration config) throws Exception {
+ return config.getSASTokenProvider().getSASToken(this.getAccountName(),
this.getFileSystemName(), getMethodName(),
+ readPermission);
+ }
+
+ /**
+ * Tests the implementation sequence if all fixed SAS configs are set.
+ * The expected sequence is Container Specific Fixed SAS, Account Specific
Fixed SAS, Account Agnostic Fixed SAS.
+ * @throws IOException
+ */
+ @Test
+ public void testFixedTokenPreference() throws Exception {
+ AbfsConfiguration testAbfsConfig = new AbfsConfiguration(
+ getRawConfiguration(), this.getAccountName(),
this.getFileSystemName(), getAbfsServiceType());
+
+ // setting all types of Fixed SAS configs (container-specific,
account-specific, account-agnostic)
+ removeAnyPresetConfiguration(testAbfsConfig);
+ testAbfsConfig.set(containerProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getFileSystemName(), this.getAccountName()), containerSAS);
+ testAbfsConfig.set(accountProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getAccountName()), accountSAS);
+ testAbfsConfig.set(FS_AZURE_SAS_FIXED_TOKEN, accountSAS);
+
+ // Assert that Container Specific Fixed SAS is used
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("sr=c");
Review Comment:
Please add description for this.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,43 @@ public void testBothProviderFixedTokenConfigured() throws
Exception {
}
}
+ /**
+ * Helper method to get the Fixed SAS token value
+ */
+ private String getFixedSASToken(AbfsConfiguration config) throws Exception {
+ return config.getSASTokenProvider().getSASToken(this.getAccountName(),
this.getFileSystemName(), getMethodName(),
+ readPermission);
+ }
+
+ /**
+ * Tests the implementation sequence if all fixed SAS configs are set.
+ * The expected sequence is Container Specific Fixed SAS, Account Specific
Fixed SAS, Account Agnostic Fixed SAS.
+ * @throws IOException
+ */
+ @Test
+ public void testFixedTokenPreference() throws Exception {
+ AbfsConfiguration testAbfsConfig = new AbfsConfiguration(
+ getRawConfiguration(), this.getAccountName(),
this.getFileSystemName(), getAbfsServiceType());
+
+ // setting all types of Fixed SAS configs (container-specific,
account-specific, account-agnostic)
+ removeAnyPresetConfiguration(testAbfsConfig);
+ testAbfsConfig.set(containerProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getFileSystemName(), this.getAccountName()), containerSAS);
+ testAbfsConfig.set(accountProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getAccountName()), accountSAS);
+ testAbfsConfig.set(FS_AZURE_SAS_FIXED_TOKEN, accountSAS);
+
+ // Assert that Container Specific Fixed SAS is used
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("sr=c");
+
+ // Assert that Account Specific Fixed SAS is used if container SAS isn't
set
+ testAbfsConfig.unset(containerProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getFileSystemName(), this.getAccountName()));
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("ss=bf");
+
+ //Assert that Account-Agnostic fixed SAS is used if no other fixed SAS
configs are set.
+ // The token is the same as the Account Specific Fixed SAS.
+ testAbfsConfig.unset(accountProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getAccountName()));
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("ss=bf");
Review Comment:
Same as above.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java:
##########
@@ -322,6 +322,10 @@ public static String accountProperty(String property,
String account) {
return property + "." + account;
}
+ public static String containerProperty(String property, String fsName,
String account) {
+ return property + "." + fsName + "." + account;
Review Comment:
We can use DOT constant here (AbfsHttpConstants)
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,43 @@ public void testBothProviderFixedTokenConfigured() throws
Exception {
}
}
+ /**
+ * Helper method to get the Fixed SAS token value
+ */
+ private String getFixedSASToken(AbfsConfiguration config) throws Exception {
+ return config.getSASTokenProvider().getSASToken(this.getAccountName(),
this.getFileSystemName(), getMethodName(),
+ readPermission);
+ }
+
+ /**
+ * Tests the implementation sequence if all fixed SAS configs are set.
+ * The expected sequence is Container Specific Fixed SAS, Account Specific
Fixed SAS, Account Agnostic Fixed SAS.
+ * @throws IOException
+ */
+ @Test
+ public void testFixedTokenPreference() throws Exception {
+ AbfsConfiguration testAbfsConfig = new AbfsConfiguration(
+ getRawConfiguration(), this.getAccountName(),
this.getFileSystemName(), getAbfsServiceType());
+
+ // setting all types of Fixed SAS configs (container-specific,
account-specific, account-agnostic)
+ removeAnyPresetConfiguration(testAbfsConfig);
+ testAbfsConfig.set(containerProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getFileSystemName(), this.getAccountName()), containerSAS);
+ testAbfsConfig.set(accountProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getAccountName()), accountSAS);
+ testAbfsConfig.set(FS_AZURE_SAS_FIXED_TOKEN, accountSAS);
+
+ // Assert that Container Specific Fixed SAS is used
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("sr=c");
+
+ // Assert that Account Specific Fixed SAS is used if container SAS isn't
set
+ testAbfsConfig.unset(containerProperty(FS_AZURE_SAS_FIXED_TOKEN,
this.getFileSystemName(), this.getAccountName()));
+ Assertions.assertThat(getFixedSASToken(testAbfsConfig)).contains("ss=bf");
Review Comment:
Same as above, please add description in all the assert.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##########
@@ -589,6 +608,16 @@ public String accountConf(String key) {
return key + "." + accountName;
}
+ /**
+ * Appends the container, account name to a configuration key yielding the
+ * container-specific form.
+ * @param key Account-agnostic configuration key
+ * @return Container-specific configuration key
+ */
+ public String containerConf(String key) {
+ return key + "." + fsName + "." + accountName;
Review Comment:
We can use DOT constant here (AbfsHttpConstants)
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,43 @@ public void testBothProviderFixedTokenConfigured() throws
Exception {
}
}
+ /**
+ * Helper method to get the Fixed SAS token value
+ */
+ private String getFixedSASToken(AbfsConfiguration config) throws Exception {
+ return config.getSASTokenProvider().getSASToken(this.getAccountName(),
this.getFileSystemName(), getMethodName(),
Review Comment:
Please review the format. This line has too many characters.
--
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]
