anujmodi2021 commented on code in PR #8051:
URL: https://github.com/apache/hadoop/pull/8051#discussion_r2485253373
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java:
##########
@@ -363,6 +363,21 @@ public AbfsClient(final URL baseUrl,
this.sasTokenProvider = sasTokenProvider;
}
+ public AbfsClient(final URL baseUrl,
Review Comment:
Should we simply combine the 3 constructors to accept both
AccessTokenProvider, SASTokenProvider and the caller can set what it has and
null as other?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/AbfsHttpConstants.java:
##########
@@ -201,7 +202,7 @@ public String toString() {
}
public static ApiVersion getCurrentVersion() {
- return NOV_04_2024;
+ return JULY_05_2025;
Review Comment:
Are we bumping up version for all the requests?
Wasn't this version bump only needed for Auth related APIs?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##########
@@ -1474,6 +1474,49 @@ public SASTokenProvider getSASTokenProvider() throws
AzureBlobFileSystemExceptio
}
}
+ /**
+ * Returns the SASTokenProvider implementation to be used to generate
user-bound SAS token.<br>
+ * Custom implementation of {@link SASTokenProvider} under th config
+ * "fs.azure.sas.token.provider.type" needs to be provided.<br>
+ * @return sasTokenProvider object based on configurations provided
+ * @throws AzureBlobFileSystemException
+ */
+ public SASTokenProvider getSASTokenProviderForUserBoundSAS() throws
AzureBlobFileSystemException {
Review Comment:
nit: Can be renamed as `getUserBoundSASTokenProvider`
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java:
##########
@@ -1474,6 +1474,49 @@ public SASTokenProvider getSASTokenProvider() throws
AzureBlobFileSystemExceptio
}
}
+ /**
+ * Returns the SASTokenProvider implementation to be used to generate
user-bound SAS token.<br>
+ * Custom implementation of {@link SASTokenProvider} under th config
+ * "fs.azure.sas.token.provider.type" needs to be provided.<br>
+ * @return sasTokenProvider object based on configurations provided
+ * @throws AzureBlobFileSystemException
+ */
+ public SASTokenProvider getSASTokenProviderForUserBoundSAS() throws
AzureBlobFileSystemException {
+ AuthType authType = getEnum(FS_AZURE_ACCOUNT_AUTH_TYPE_PROPERTY_NAME,
AuthType.SharedKey);
+ if (authType != AuthType.UserboundSASWithOAuth) {
+ throw new SASTokenProviderException(String.format(
+ "Invalid auth type: %s is being used, expecting user-bound SAS.",
authType));
+ }
+
+ try {
+ Class<? extends SASTokenProvider> customSasTokenProviderImplementation =
+ getTokenProviderClass(authType, FS_AZURE_SAS_TOKEN_PROVIDER_TYPE,
+ null, SASTokenProvider.class);
+
+ if (customSasTokenProviderImplementation == null) {
Review Comment:
Similar check for OAuth also needed right?
For UBS, Cx must configure OAuth as well?
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsDfsClient.java:
##########
@@ -174,6 +174,17 @@ public AbfsDfsClient(final URL baseUrl,
encryptionContextProvider, abfsClientContext, AbfsServiceType.DFS);
}
+ public AbfsDfsClient(final URL baseUrl,
Review Comment:
Is this only for DFS Client?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/constants/TestConfigurationKeys.java:
##########
@@ -55,6 +55,9 @@ public final class TestConfigurationKeys {
public static final String FS_AZURE_TEST_APP_SERVICE_PRINCIPAL_OBJECT_ID =
"fs.azure.test.app.service.principal.object.id";
+ public static final String FS_AZURE_END_USER_TENANT_ID =
"fs.azure.test.end.user.tenant.id";
Review Comment:
I think TEST should come first. Check for how other test related configs are
defined. TestConfigurationKeys.java have them
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java:
##########
@@ -1741,7 +1741,15 @@ private void initializeClient(URI uri, String
fileSystemName,
} else if (authType == AuthType.SAS) {
LOG.trace("Fetching SAS Token Provider");
sasTokenProvider = abfsConfiguration.getSASTokenProvider();
- } else {
+ } else if(authType == AuthType.UserboundSASWithOAuth){
Review Comment:
Here also, it can be combined into a single call with both passed.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientHandler.java:
##########
@@ -154,7 +173,15 @@ private AbfsDfsClient createDfsClient(final URL baseUrl,
final EncryptionContextProvider encryptionContextProvider,
final AbfsClientContext abfsClientContext) throws IOException {
URL dfsUrl = changeUrlFromBlobToDfs(baseUrl);
- if (tokenProvider != null) {
+ if (tokenProvider != null && sasTokenProvider != null) {
Review Comment:
Apart from UDS, can there be a case where caller can send both as not null?
Makes ure no flow leads to this and also add a test around it.
##########
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClientHandler.java:
##########
@@ -80,6 +80,25 @@ public AbfsClientHandler(final URL baseUrl,
abfsClientContext);
}
+ public AbfsClientHandler(final URL baseUrl,
Review Comment:
Same as above. We can combine into a single constructor.
But do check if there are any caveats there or a risk of running into NPE
--
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]
