anujmodi2021 commented on code in PR #7461:
URL: https://github.com/apache/hadoop/pull/7461#discussion_r1990563138
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,59 @@ 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(
Review Comment:
We are setting same SAS Token as both account specific and account agnostic.
We should separate them out.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,59 @@ 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))
Review Comment:
This seems good but you can also assert on the whole SAS Token string.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,59 @@ 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 {
Review Comment:
Nit: `testFixedSASTokenProviderPreference'
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:
##########
@@ -126,6 +147,59 @@ 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))
+ .describedAs("Container-specific fixed SAS should've been used.")
+ .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))
+ .describedAs("Account-specific fixed SAS should've been used.")
+ .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))
+ .describedAs("Account-agnostic fixed SAS should've been used.")
+ .contains("ss=bf");
Review Comment:
This assert should be different from other else it won't be able to catch
regressions
--
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]
