HDFS-15765. WebHdfs: add support for basic auth and custom API path #5447
+312
−11
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
trakos
changed the title
trakos
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saxenapranav
reviewed
Mar 7, 2023
| @@ -120,7 +136,7 @@ public HttpURLConnection configure(HttpURLConnection connection) | |||
| } | |||
| } | |||
|
|
|||
| return conn; | |||
| return new BasicAuthConfigurator(conn, basicAuthCredentials); | |||
There was a problem hiding this comment.
should we have something like:
private ConfigurationConfigurator getConfigurator(ConfigurationConfigurator configurator, String basicAuthCred) {
if(basicAuthCred != null && !basicAuthCred.isEmpty()) {
return new BasicAuthConfigurator(configurator, basicAuthCred);}
else {return configurator;}
}
and instead of creating new object of basicAuthConfigurator, we call this new method. And if an object of BasicAuthConfigurator can be made, method will return that new object.
What you say?
There was a problem hiding this comment.
Sounds reasonable! I've made the change.
| conf.configure(conn); | ||
| Mockito.verify(conn, Mockito.times(1)).setRequestProperty( | ||
| "AUTHORIZATION", | ||
| "Basic dXNlcjpwYXNz" |
There was a problem hiding this comment.
should we have "Basic " + Base64.getEncoder().encodeToString( credentials.getBytes(StandardCharsets.UTF_8)
as its not clear from test how user:pass is converting to dXNlcjpwYXNz
There was a problem hiding this comment.
I've taken that string from testing a basic auth implementation in cURL, but I can see how it's confusing here. I've applied the suggested change.
|
Thanks for the review @saxenapranav, I've applied the suggested changes. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Thank you for taking previous comments. Have added some suggestion.
@Hexiaoqiao, requesting you to kindly review the PR please.
Regards.
| @@ -33,6 +33,16 @@ public class BasicAuthConfigurator implements ConnectionConfigurator { | |||
| private final ConnectionConfigurator parent; | |||
| private final String credentials; | |||
|
|
|||
| static public ConnectionConfigurator getConfigurator( | |||
There was a problem hiding this comment.
Since this method has to be called by URLConnectionFactory which is in same package. Lets have it package-protected and remove public
There was a problem hiding this comment.
Should this method be moved to URLConnectionFactory class only?
Reason being, when external class calls BasicAuthConfigurator.getConfigurator, to developer it can look that we want something out of BasicAuthConfigurator, but instead it either gives BasicAuthConfigurator object or parent-configurator object. What you feel?
There was a problem hiding this comment.
Good point, we might as well move it to URLConnectionFactory and make it private there. I've made the change.
| HdfsClientConfigKeys.DFS_CLIENT_WEBHDFS_USE_BASE_PATH_KEY, | ||
| HdfsClientConfigKeys.DFS_CLIENT_WEBHDFS_USE_BASE_PATH_DEFAULT | ||
| ); | ||
| if (uri.getPath() != null && !uri.getPath().equals("") && useBasePath) { |
There was a problem hiding this comment.
although uri can not be null, but better to have null-check on that.
…eateBasicAuthConfigurator
|
Thanks for the additional look @saxenapranav! I've applied the suggested changes. Hey @Hexiaoqiao, Pranev mentioned that you could help with further review. Please let me know if you'd like me to make additional changes! |
|
💔 -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.
Description of PR
WebHdfsFileSystemdidn't provide any support for HTTP BASIC authentication (username/password). This patch adds that feature. When specifying filesystem URI, the credentials part (user:pass@) is now parsed properly and used forAuthorizationheader.Additionally, base path specified in filesystem URL used to be ignored. This patch adds configuration option
dfs.client.webhdfs.use-base-paththat, when enabled, indicates that this path should be used as API prefix. This allows specifying/gateway/gatewaynamewhen using WebHdfs for Apache Knox. When base path contains/webhdfs/v1, it is ignored, since we always append that.Option
dfs.client.webhdfs.use-base-pathdefaults to false because it could introduce a backward compatibility break. Some WebHdfs users could have typos or something random as path, and before this patch it would simply be ignored. By setting the default to false, we make sure that it won't break any existing setup.Note that issue HDFS-15765 is also about Kerberos auth. This patch only addresses the base path and basic authentication portion, I didn't investigate the Kerberos auth since we don't use in our setup.
How was this patch tested?
I tested it with WebHdfs secured by Apache Knox with basic authorization, but without Kerberos. I had a test script that would perform a file upload. I set
dfs.client.webhdfs.use_basepathto true, and usedswebhdfs://admin:admin-password@localhost:8443/gateway/docker/as filesystem URI. Without my patch, both theadmin:admin-passwordcredentials and/gateway/dockerAPI base path would be ignored. With it, file upload worked.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?