HADOOP-17053. ABFS: Fix Account-specific OAuth config setting parsing #2034
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Accounts are in East US2 region. HNS Account: |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bilaharith
reviewed
May 27, 2020
| * @return Highest-precedence Class object that was found | ||
| */ | ||
| public <U> Class<? extends U> getClass(String name, Class<? extends U> defaultValue, Class<U> xface) { | ||
| public <U> Class<? extends U> getTokenProviderClass(AuthType authType, |
There was a problem hiding this comment.
nit: I could see it from the javadocs, still naming it something like authTypeForAccount could improve readability
There was a problem hiding this comment.
This method fetches TokenProviderClass instance based on the input AuthType. It is in sync with the naming followed by other methods that resolve account-specific config and in it's absence default to account-agnostic value. Will retain the naming.
There was a problem hiding this comment.
I meant for the parameter currently named authType
There was a problem hiding this comment.
Inputs AuthType, name of the relevant TokenProvider config key, xface (interface) are derived by the caller of getTokenProviderClass based on account-specific config settings. As it applies to all the inputs equally and is clear from the calling method's perspective, will retain the naming.
bilaharith
reviewed
May 27, 2020
| import org.apache.hadoop.conf.Configuration; | ||
|
|
There was a problem hiding this comment.
Nit: This new line can be removed
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When AuthType and Auth token provider configs are set for both generic (account-agnostic) and account-specific config, as below:
// account agnostic
fs.azure.account.auth.type=CUSTOM
fs.azure.account.oauth.provider.type=ClassExtendingCustomTokenProviderAdapter
// account specific
fs.azure.account.auth.type.account_name=OAuth
fs.azure.account.oauth.provider.type.account_name=ClassExtendingAccessTokenProvider
Effective token provider for 'account_name' is expected to be ClassExtendingAccessTokenProvider.
But currently, when the token provider class is being read from the config, account-agnostic config setting is read first in the assumption that it can serve as default if account-specific config setting is absent.
This logic leads to failure such as in above case where AuthType for account-specific and AuthType for account-agnostic are different. This is because the Interface implementing the token provider is different for various Auth Types (For Custom AuthType it is CustomTokenProviderAdapter and for OAuth it is AccessTokenProvider). This leads to a Runtime exception when trying to create the oAuth access token provider.
In this PR, getTokenProvider() is fixed to check for account-specific token provider configuration first. In its absence, account-agnostic token provider configuration is read only if account-specific and account-agnostic AuthType setting is same (otherwise expected token provider interface is different). The same logic applies while determining SAS Token provider too, hence that has been updated as well.
Testing is done with combinations of Custom and OAuth settings in account-specific and account-agnostic configs and determining which is in effect. Tested getSASTokenProvider which uses the same logic to find the effective token provider with SAS set for both account-specific and account-agnostic configs.