[
https://issues.apache.org/jira/browse/HADOOP-19736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18034945#comment-18034945
]
ASF GitHub Bot commented on HADOOP-19736:
-----------------------------------------
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
> ABFS: Support for new auth type: User-bound SAS
> -----------------------------------------------
>
> Key: HADOOP-19736
> URL: https://issues.apache.org/jira/browse/HADOOP-19736
> Project: Hadoop Common
> Issue Type: Task
> Components: fs/azure
> Affects Versions: 3.4.1, 3.4.2
> Reporter: Manika Joshi
> Assignee: Manika Joshi
> Priority: Major
> Labels: pull-request-available
>
> Adding support for new authentication type: user bound SAS
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
