-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-19736: ABFS. Support for new auth type: User-bound SAS #8051
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
base: trunk
Are you sure you want to change the base?
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| AUG_03_2023("2023-08-03"), | ||
| NOV_04_2024("2024-11-04"); | ||
| NOV_04_2024("2024-11-04"), | ||
| JULY_05_2025("2025-07-05"); |
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 should follow the same format: JUL_05_2025, what do you think?
| abfsClientContext); | ||
| } | ||
|
|
||
| public AbfsClientHandler(final URL baseUrl, |
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.
Java doc missing for the constructor
| this.sasTokenProvider = sasTokenProvider; | ||
| } | ||
|
|
||
| public AbfsClient(final URL baseUrl, |
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.
Java doc missing
| encryptionContextProvider, abfsClientContext, AbfsServiceType.DFS); | ||
| } | ||
|
|
||
| public AbfsDfsClient(final URL baseUrl, |
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.
Java doc missing
| case UserboundSASWithOAuth: | ||
| httpOperation.setRequestProperty(HttpHeaderConfigurations.AUTHORIZATION, | ||
| client.getAccessToken()); | ||
| httpOperation.setMaskForSAS(); //mask sig/oid from url for logs |
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.
Typo: *sign
| tokenProvider, sasTokenProvider, encryptionContextProvider, | ||
| populateAbfsClientContext()); | ||
| } | ||
| else if (tokenProvider != null) { |
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
|
|
||
| LOG.trace("Initializing AbfsClient for {}", baseUrl); | ||
| if (tokenProvider != null) { | ||
| if(tokenProvider != null && sasTokenProvider != null){ |
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.
Space between if and (
| 3. Deployed in-Azure with the Azure VMs providing OAuth 2.0 tokens to the application, "Managed Instance". | ||
| 4. Using Shared Access Signature (SAS) tokens provided by a custom implementation of the SASTokenProvider interface. | ||
| 5. By directly configuring a fixed Shared Access Signature (SAS) token in the account configuration settings files. | ||
| 6. Using user-bound SAS auth type, which is requires OAuth 2.0 setup (point 2 above) and SAS setup (point 4 above) |
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.
Grammatical mistake: which requires or which is required?
|
|
||
| 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"; |
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.
Rename the variable to FS_AZURE_TEST_END_USER_TENANT_ID
| 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"; | ||
| public static final String FS_AZURE_END_USER_OBJECT_ID = "fs.azure.test.end.user.object.id"; |
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
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19736
Adding support for new authentication type: user bound SAS
How was this patch tested?
Test suite will be run for the patch