[
https://issues.apache.org/jira/browse/HADOOP-19471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17934169#comment-17934169
]
ASF GitHub Bot commented on HADOOP-19471:
-----------------------------------------
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.
> ABFS: Support Fixed SAS token at container level
> -------------------------------------------------
>
> Key: HADOOP-19471
> URL: https://issues.apache.org/jira/browse/HADOOP-19471
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.4.0, 3.4.1
> Reporter: Manika Joshi
> Assignee: Manika Joshi
> Priority: Major
> Labels: pull-request-available
>
> The ABFS driver currently lacks support for multiple SAS tokens for the same
> storage account across different containers.
> We are now introducing this support.
> To use fixed SAS token at container level the configuration to be used is:
> {quote}fs.azure.sas.fixed.token.<container-name>.<storage-account-name>
> {quote}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
