Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-18516: [ABFS][Authentication] Support Fixed SAS Token for ABFS Authentication #6552
HADOOP-18516: [ABFS][Authentication] Support Fixed SAS Token for ABFS Authentication #6552
Changes from 2 commits
f643e3e
db03c5f
92cb671
2912980
42952a2
3aa4cbf
e91c4dc
595d644
817f7cb
a65cdd7
a79cae1
554fc39
d06fe41
b8a0464
f8e246d
b01a229
7c9d5b4
5db5372
3d5a448
ecd151a
820e2e0
298640f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
better to directly raise a new TokenAccessProviderException() here so that there's no double wrapping of stack traces.
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.
I realized we do have a dedicated exception type for SAS Token related issues.
Using SASTokenProviderException instead.
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.
Instead of returning null, can we create an implementation of SasTokenProvider interface, which implements getSasToken() method and return the value in the configuration
FS_AZURE_SAS_FIXED_TOKEN
. This would simplify code logic, remove null checks, and reduce git diffs.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
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 a . at the end of this and the first sentence of every other javadoc; some java versions require it. Ideally a
{@value}
element too, so the IDEs show what the value isThere 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.
Added everywhere.
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 operationName is not provided here. If the thing is that for container APIs, existing SAS mechanism can not work, we should still prevent them. We might have to add intelligence which SAS implementations are allowed for container APIs.
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.
Nice Catch. Operations name added for file system level operations as well.
Just FYI
The purpose of having operation name here is to determine what all permissions are needed to be set in SAS Token generated. It is up to the custom SAS Token Provider Implementation to use this information and set appropriate permissions. For Example, MockDelegationSASTokenProvider does not allow file system level operations hence it will error out for these operations with SASTokenProviderException.
Where as AccountSASGenerator will have fixed permissions irrespective of operation type.
Operation Name are added here so that if a user wants to define their own implementation, they can choose to consume this information as per their needs and logic. They can also choose to simply ignore them.
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.
appendSASTokenToQuery
was not there before this patch. Is this added for fixed sas token only. If yes, then we would have to add if-condition to keep only fixedSasToken (if in config) applying to the container APIs, and in non-fixedSasToken case,getSASToken
should not be called. Reason being, developers would already have their implementations and those might not be adaptable to the container APIs.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.
As discussed, we don't want to support any container API on SAS Token Authentication. Removed these changes
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.
make the javadoc of the new method
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
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.
import ordering doesn't match style rules for new code.
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.
Modified as per the rules
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.
nit: lines seem too long; if over 100 chars split to multiple lines
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.
Checked Everywhere
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.
Since, these tests may run in parallel with test of other classes. Lets make clone of the configuration object and use it in the test.
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
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.
prefer MockDelegationSASTokenProvider.class.getName()
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
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.
recursive root delete is a funny one. what does abfs do here? does it delete everything? I'm curious now. (s3a fs returns false before even trying to talk to the store).
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.
In case of delete on a root path, ABFS list down all the children of root and delete them individually. In case a child is itself a directory, it will be deleted recursively.
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.
ok. I think for s3a we decided against that on the basis that it was pretty dangerous to do accidentally. nobody ever does "rm -rf /" after all. At least not more than once...
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.
don't catch, you've just lost the entire stack trace. let the test handler catch and report the problem. always
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.
Taken
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 protected?
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.
Made private