HADOOP-17725. Improve error message for token providers in ABFS #3041
8000
Jump to bottom
Conversation
This file contains hidden or 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
sadikovi
suggested changes
May 21, 2021
There was a problem hiding this comment.
I think it would be good to cover all of the token providers since they are all similar and have the same problem. Here is my patch for this issue (in my own fork, so the lines might be different):
--- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
+++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
@@ -774,16 +774,16 @@ public AccessTokenProvider getTokenProvider() throws TokenAccessProviderExceptio
AccessTokenProvider tokenProvider = null;
if (clientCredsTokenProviderName.equals(tokenProviderClassName)) {
- String authEndpoint = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ENDPOINT);
- String clientId = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
- String clientSecret = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_SECRET);
+ String authEndpoint = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ENDPOINT);
+ String clientId = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
+ String clientSecret = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_SECRET);
tokenProvider =getClientCredsTokenProvider(
authEndpoint, clientId, clientSecret);
LOG.trace("ClientCredsTokenProvider initialized");
} else if (userPasswordTokenProviderName.equals(tokenProviderClassName)) {
- String authEndpoint = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ENDPOINT);
- String username = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_USER_NAME);
- String password = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_USER_PASSWORD);
+ String authEndpoint = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ENDPOINT);
+ String username = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_USER_NAME);
+ String password = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_USER_PASSWORD);
tokenProvider = getUserPasswordTokenProvider(
authEndpoint, username, password);
LOG.trace("UserPasswordTokenProvider initialized");
@@ -791,8 +791,8 @@ public AccessTokenProvider getTokenProvider() throws TokenAccessProviderExceptio
String authEndpoint = getTrimmedPasswordString(
FS_AZURE_ACCOUNT_OAUTH_MSI_ENDPOINT,
AuthConfigurations.DEFAULT_FS_AZURE_ACCOUNT_OAUTH_MSI_ENDPOINT);
- String tenantGuid = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_MSI_TENANT);
- String clientId = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
+ String tenantGuid = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_MSI_TENANT);
+ String clientId = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
String authority = getTrimmedPasswordString(
FS_AZURE_ACCOUNT_OAUTH_MSI_AUTHORITY,
AuthConfigurations.DEFAULT_FS_AZURE_ACCOUNT_OAUTH_MSI_AUTHORITY);
@@ -804,8 +804,8 @@ public AccessTokenProvider getTokenProvider() throws TokenAccessProviderExceptio
String authEndpoint = getTrimmedPasswordString(
FS_AZURE_ACCOUNT_OAUTH_REFRESH_TOKEN_ENDPOINT,
AuthConfigurations.DEFAULT_FS_AZURE_ACCOUNT_OAUTH_REFRESH_TOKEN_ENDPOINT);
- String refreshToken = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_REFRESH_TOKEN);
- String clientId = getPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
+ String refreshToken = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_REFRESH_TOKEN);
+ String clientId = getRequiredPasswordString(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ID);
tokenProvider = getRefreshTokenBasedTokenProvider(authEndpoint,
clientId, refreshToken);
LOG.trace("RefreshTokenBasedTokenProvider initialized");
@@ -1004,9 +1004,27 @@ void setIsNamespaceEnabledAccount(String isNamespaceEnabledAccount) {
this.isNamespaceEnabledAccount = isNamespaceEnabledAccount;
}
+ /**
+ * Returns a value for the key if the value exists and is not null.
+ * Otherwise, throws NullPointerException.
+ * @param key Account-agnostic configuration key
+ * @return value in String form if one exists
+ * @throws NullPointerException when the value is null
+ * @throws IOException
+ */
+ private String getRequiredPasswordString(String key) throws IOException {
+ String value = getPasswordString(key);
+ if (value == null) {
+ throw new ConfigurationPropertyNotFoundException(key);
+ }
+ return value;
+ }IMHO, it is better to do it in AbfsConfiguration since you already have all of the methods there + config keys that the code is trying to look up, so the changes are very concise.
| Preconditions.checkNotNull(clientSecret, "clientSecret"); | ||
| final String clientId, final String clientSecret) | ||
| throws AzureBlobFileSystemException { | ||
| validateClientCredsTokenProvider( |
There was a problem hiding this comment.
That is not going to work. You probably want to keep non-null check here, since this class does not depend on Hadoop configuration.
|
|
||
| this.authEndpoint = authEndpoint; | ||
| this.clientId = clientId; | ||
| this.clientSecret = clientSecret; | ||
| } | ||
|
|
||
| private void validateClientCredsTokenProvider(final String configName, |
There was a problem hiding this comment.
IMHO, it is better to move it to AbfsConfiguration.java. It will allow you to cover other token providers there as well.
There was a problem hiding this comment.
move this to the code which gets the config and instantiates the tokenprovider
| final AbfsConfiguration abfsConf = new AbfsConfiguration(conf, accountName); | ||
|
|
||
| setAuthConfig(abfsConf, true, AuthType.OAuth); | ||
| abfsConf.unset(FS_AZURE_ACCOUNT_OAUTH_CLIENT_ENDPOINT + "." + accountName); |
There was a problem hiding this comment.
Hmm... Can you create some kind of for loop to iterate over the keys, otherwise, the code is duplicated quite a bit. I would also suggest creating separate tests for each.
|
I would appreciate it if you could address the comments, thanks. |
19fe670 to
a67a1b2
Compare
|
🎊 +1 overall
This message was automatically generated. |
Thanks for the review @sadikovi, I missed out of on other token providers initially. Please take a look. |
|
@aajisaka would you like to take a look? |
|
@surendralilhore @goiri @steveloughran Could you please review this PR? |
steveloughran
requested changes
May 27, 2021
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| import org.apache.hadoop.fs.azurebfs.contracts.exceptions |
There was a problem hiding this comment.
usual losing battle on import isolation, split from non org.apache stuffl
| private void testMissingConfigKey(final AbfsConfiguration abfsConf, | ||
| final String confKey) { | ||
| try { | ||
| abfsConf.getTokenProvider().getClass().getTypeName(); |
There was a problem hiding this comment.
Use LambdaTestUtils.intercept.
| * | ||
| * @param key Account-agnostic configuration key | ||
| * @return value if exists | ||
| * @throws IOException if error in fetching password or if value does not |
There was a problem hiding this comment.
+mention ConfigurationPropertyNotFoundException for missing key
which endpoint did you run against? Remember: "no tests: no review" |
a67a1b2 to
c8ed0cc
Compare
Unfortunately, I don't have access to test env as of today (non-tech reasons), sorry about that. However, the reason I offered help for this Jira is because of well written unit tests that I was exploring some time back. I was able to reproduce the exact issue reported by @sadikovi using unit test and then realized that source changes are not complex enough to test on real time test env. Please let me know if this works. |
|
Addressed comments from recent review cycle. |
|
🎊 +1 overall
This message was automatically generated. |
|
I concur, the issue can be easily reproduced in unit tests and the fix is fairly straightforward. Let me run a manual test with this commit in my dev environment. |
|
Tested commit c8ed0cc25bd094240b6274df447bdc33aa93546c. Ran the following code import org.apache.hadoop.conf._
import org.apache.hadoop.fs._
val conf = new Configuration()
conf.set("fs.azure.account.auth.type", "OAuth")
conf.set("fs.azure.account.oauth.provider.type", "org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider")
conf.set("fs.azure.account.oauth2.client.id", "<client-id>")
// conf.set("fs.azure.account.oauth2.client.secret.<account>.dfs.core.windows.net", "<client-secret>")
// conf.set("fs.azure.account.oauth2.client.secret", "<client-secret>")
conf.set("fs.azure.account.oauth2.client.endpoint", "https://login.microsoftonline.com/<endpoint>")
val path = new Path("abfss://<container>@<account>.dfs.core.windows.net/")
val fs = path.getFileSystem(conf)
fs.getFileStatus(path)with the storage account in West US, everything seems to work correctly. If I comment out one or more configs, e.g. client-secret or client-id, the error message is as follows: The code works when commenting out |
Given that a source change which may cause a regression, there is no source change for the hadoop-azure module which doesn't require the submitter to run the integration test suites. It's not about whether the new feature merits i a test, its "what of the existing features have stopped working". As for new features: the tests are there to stop the next person who touches the code from regressing this bit. no test: no review See the section in Testing hadoop-azure @sadikovi if you are set up and willing to test this, are you able to run the |
steveloughran
requested changes
May 28, 2021
| unsetAuthConfig(abfsConf, true); | ||
| } | ||
|
|
||
| private void testMissingConfigKey(final AbfsConfiguration abfsConf, |
There was a problem hiding this comment.
Whenever an exception doesn't match expected, its critical for the test case to throw up that exception so we can debug what's gone wrong from the test report alone. This one loses the stack trace on L399 and L403.
add verifyCause to verify cause type and GenericTestUtils.assertExceptionContains() to process, something like
assertExceptionContains("Configuration property "+confKey+" not found."),
verifyCause(ConfigurationPropertyNotFoundException.class,
intercept(TokenAccessProviderException.class,()->
abfsConf.getTokenProvider().getClass().getTypeName())));There was a problem hiding this comment.
This is addressed @steve F438 loughran. Please take a look when you get a chance. Thanks
c8ed0cc to
2a9849f
Compare
2a9849f to
4b82454
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
I ran the tests using the following: Storage account is in West US and has HNS enabled. I configured all of the auth mechanisms in the I get the following output: I am not quite sure why the integration tests are ignored. Do you know how I can configure them to run? |
|
@sadikovi Thanks a lot for running this. One request for you: could you please also run this one in Here is the reference link. Thanks |
|
I tried running those commands but the tests are still ignored. I triggered Did anyone try running the tests following the guide in the repository? |
|
The error seems to be but I am setting the key in azure-auth-keys.xml. Also, I found that the tests only work/fail if you hardcode "fs.azure.account.key" in It seems https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/site/markdown/testing_azure.md is missing a few configuration points, including installing dependencies to run the scripts on Ubuntu. Would appreciate it if someone could share what I need to change exactly to run the tests. Thanks. |
|
Okay, I think I figured out how to run them. I think I confused mvn test with the runtests.sh, will post the test results soon-ish. |
|
Tested commit 4b82454. NonHNS-SharedKey tests. Storage account is in West US 2. HNS-OAuth tests. Storage account is in West US. The failures are due to the fact that I forgot to configure CheckAccess part of the azure-auth-keys.xml. HNS-SharedKey tests. Storage account is in West US. The failures are due to the fact that I forgot to configure CheckAccess part of the azure-auth-keys.xml. |
CEB7
|
@sadikovi great work, you've done more than anyone could ask for. You're also now set up for testing your own work. |
|
merged into trunk; will cherrypick to branch-3.3 with a recompile to verify all is good. leaving JIRA open until then |
|
..and its done. Thank you everyone for your work. |
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
…he#3041) Contributed by Viraj Jasani.
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.
No description provided.