-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-19406. ABFS: [FNSOverBlob] Support User Delegation SAS for FNS Blob #7523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-19406. ABFS: [FNSOverBlob] Support User Delegation SAS for FNS Blob #7523
Conversation
Test Results============================================================
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
try { | ||
if (abfsConfiguration.getAuthType(abfsConfiguration.getAccountName()) == AuthType.SAS && // Auth type is SAS | ||
!tryGetIsNamespaceEnabled(new TracingContext(initFSTracingContext)) && // Account is FNS | ||
abfsConfiguration.getFsConfiguredServiceType() == DFS && // Service type is DFS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the constant for DFS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is using the AbfsServiceType.DFS constant here
break; | ||
case SASTokenProvider.LIST_OPERATION_BLOB: | ||
sp = "l"; | ||
sr="c"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
final AbfsRestOperation successOp = getSuccessOp( | ||
AbfsRestOperationType.RenamePath, HTTP_METHOD_PUT, | ||
url, requestHeaders); | ||
successOp.setMask(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this performed only in the rename case ? By default we mask for all cases if auth type is SAS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, yes we mask for all SAS cases
isHNSEnabled = this.getConfiguration().getBoolean( | ||
TestConfigurationKeys.FS_AZURE_TEST_NAMESPACE_ENABLED_ACCOUNT, false); | ||
Assume.assumeTrue(isHNSEnabled); | ||
if(!isHNSEnabled){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more tests around user delegation SAS specific to FNS Blob and see it works correctly for all cases of implicit, explicit, root path, container path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some new tests. Please review
|
||
There are three types of SAS supported by Azure Storage: | ||
- [User Delegation SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas): Recommended for use with ABFS Driver with HNS Enabled ADLS Gen2 accounts. It is Identity based SAS that works at blob/directory level) | ||
- [User Delegation SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas): Recommended for use with ABFS Driver with HNS Enabled ADLS Gen2 accounts or HNS-Disabled Blob Storage accounts. It is Identity based SAS that works at blob/directory level) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recommended line should say HNS only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes recommended only for HNS-DFS.
Supported on HNS-DFS and FNS-BLOB
Not supported on FNS-DFS
This should be clearly conveyed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, rectified
#### Known Issues With SAS | ||
- SAS Based Authentication works only with HNS Enabled ADLS Gen2 Accounts which | ||
is a recommended account type to be used with ABFS. | ||
- SAS Based Authentication works with HNS Enabled ADLS Gen2 Accounts (which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does SAS work correctly with FNS DFS ? We should say FNS here only if we confirm the behaviour for both the endpoints
- **Security**: More secure than Shared Key and allows granting limited access | ||
to data without exposing the access key. Recommended to be used only with HNS Enabled, | ||
ADLS Gen 2 storage accounts. | ||
ADLS Gen 2 storage accounts or HNS-Disabled Blob Storage accounts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again remove non HNS for recommended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great patch
Added some thoughts
...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsErrors.java
Outdated
Show resolved
Hide resolved
|
||
There are three types of SAS supported by Azure Storage: | ||
- [User Delegation SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas): Recommended for use with ABFS Driver with HNS Enabled ADLS Gen2 accounts. It is Identity based SAS that works at blob/directory level) | ||
- [User Delegation SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas): Recommended for use with ABFS Driver with HNS Enabled ADLS Gen2 accounts or HNS-Disabled Blob Storage accounts. It is Identity based SAS that works at blob/directory level) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes recommended only for HNS-DFS.
Supported on HNS-DFS and FNS-BLOB
Not supported on FNS-DFS
This should be clearly conveyed
...azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelegationSAS.java
Show resolved
Hide resolved
0fd1ded
to
1676667
Compare
1676667
to
a01beb7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks for taking comments.
Resolve the yetus failures before merge
- [Service SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas): Global and works at container level. | ||
- [Account SAS](https://learn.microsoft.com/en-us/rest/api/storageservices/create-account-sas): Global and works at account level. | ||
|
||
#### Known Issues With SAS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAS-based authentication works with HNS-enabled ADLS Gen2 accounts (recommended for use with ABFS) and is also supported with non-HNS (FNS) Blob accounts. However, it is not supported with FNS-DFS accounts. - better to frame like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken
sp = "d"; | ||
sr = "d"; | ||
sdd = Integer.toString(StringUtils.countMatches(path, "/")); | ||
sdd = path.equals("/")? "0": Integer.toString(StringUtils.countMatches(path, "/")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ROOT constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted it to path.equals(ROOT_PATH)? "0": Integer.toString(StringUtils.countMatches(path, "/"));
Did not change the last "/" part for readability purpose
case SASTokenProvider.LIST_OPERATION: | ||
sp = "l"; | ||
sr = "d"; | ||
sdd = path.equals("/")? "0": Integer.toString(StringUtils.countMatches(path, "/")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
abfsUriQueryBuilder.addQuery(QUERY_PARAM_COMP, BLOCKLIST); | ||
abfsUriQueryBuilder.addQuery(QUERY_PARAM_CLOSE, String.valueOf(isClose)); | ||
String sasTokenForReuse = appendSASTokenToQuery(path, SASTokenProvider.FIXED_SAS_STORE_OPERATION, | ||
String sasTokenForReuse = appendSASTokenToQuery(path, SASTokenProvider.WRITE_OPERATION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using WRITE_OPERATION for the flush call a good idea? Should we create another operation type for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same operation type for DFS flush call as well. We can keep it as it is I think
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
💔 -1 overall
This message was automatically generated. |
d8a2816
to
82ef314
Compare
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
… Blob (apache#7523) Contributed by Manika Joshi Reviewed by Anmol Asrani, Manish Bhatt, Anuj Modi Signed off by Anuj Modi<[email protected]>
… Blob (#7523) (#7649) Contributed by Manika Joshi Reviewed by Anmol Asrani, Manish Bhatt, Anuj Modi Signed off by Anuj Modi<[email protected]>
… Blob (apache#7523) Contributed by Manika Joshi Reviewed by Anmol Asrani, Manish Bhatt, Anuj Modi Signed off by Anuj Modi<[email protected]>
… Blob (apache#7523) Contributed by Manika Joshi Reviewed by Anmol Asrani, Manish Bhatt, Anuj Modi Signed off by Anuj Modi<[email protected]>
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19406
In ABFS Driver, user delegation SAS is currently only supported by hierarchical namespace (HNS) enabled accounts (over DFS endpoint). This PR adds support for User Delegation SAS for non-HNS accounts over Blob endpoint as well.
Refer: https://hadoop.apache.org/docs/current/hadoop-azure/abfs.html#Shared_Access_Signature_.28SAS.29_Token_Provider
Tests added in comments below.