Skip to content
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

HDDS-3282. ozone.http.filter.initializers can't be set properly for S… #724

Merged
merged 5 commits into from May 6, 2020

Conversation

xiaoyuyao
Copy link
Contributor

@xiaoyuyao xiaoyuyao commented Mar 26, 2020

What changes were proposed in this pull request?

Change to use the Initializer class from hadoop-common while keeping the ozone initializer configuration key as-is.

More specifically, to enable SPNEGO auth for ozone web console, you need to properly set the auth filter initializer like below.

ozone.http.filter.initializers = org.apache.hadoop.security.AuthenticationFilterInitializer

Standardize the SPNEGO related configurations to follow the SPNEGO filter prefix rules as follows:

ozone.[component].http.auth.type = [simple, kerberos]
ozone.[component].http.auth.kerberos.principal
ozone.[component].http.auth.kerberos.keytab

component should be [om,scm,datanode,s3g,recon]

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3282

How the fix is tested:

Test with ozonesecure docker-compose locally and verified that the change ensure unauthenticated user will get 401 error like below.

URI: | /
-- | --
401
Authentication required
org.eclipse.jetty.servlet.DefaultServlet-1e1b061

@avijayanhwx
Copy link
Contributor

@xiaoyuyao Is it possible to add a negative test case for this in acceptance tests?

@elek
Copy link
Member

elek commented Apr 1, 2020

Thanks the patch @xiaoyuyao

Not clear what is the exact problem with forking? I would like to remove the dependency on Hadoop classes (to be compatible with multiple hadoop version). And interested what was the problem with the forks...

@xiaoyuyao
Copy link
Contributor Author

bq. And interested what was the problem with the forks...
The problem is with the partially forked Filter implementation from hadoop-common, e.g. the SPNEGO implemented by org.apache.hadoop.security.AuthenticationFilterInitializer has assumption on the SPNEGO configuration prefix and key names. When they are not matched in ozone configurations, the AuthenticationFilter will not work as expected. This need larger change then the current PR, I will try to isolate them into small PR with documents.

Copy link
Member

@elek elek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to explain it @xiaoyuyao

Fair enough. Let's merge it now and fork it later properly.

I am working to make Ozone more hadoop-free/independent. My base motivation to provide an easier ozonefs for Hadoop 2.7/2.8 without classpath separation. It requires to reduce the dependencies from Hadoop and that's the reason why (in some cases) prefer to fork and use an own version. (Especially related to the HTTP server where half of the Hadoop code/complexity is not required by Ozone).

But I am fine to revert this part of the previous fork to provide a quick fix and do it fully, later...

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xiaoyuyao for working on this. It seems one of the acceptance test cases (S3 gateway web ui test) is validating the change and failing due to expecting old behavior:

Running command 'curl --negotiate -u : -v http://s3g:9878/static/index.html
'...
Error 401 Authentication required
...' does not contain 'Apache Hadoop Ozone S3'

@vivekratnavel
Copy link
Contributor

@xiaoyuyao Thanks for working on this! Can you please fix the acceptance test failure as @adoroszlai pointed out?

@xiaoyuyao
Copy link
Contributor Author

Still need some additional changes to handle the deprecation of the inconsistent key names related to HTTP auth.

@xiaoyuyao
Copy link
Contributor Author

These two tests are not very reliable they both passed in my local setup but failed on CI.
org.apache.hadoop.ozone.client.rpc.Test2WayCommitInRatis
org.apache.hadoop.fs.ozone.contract.ITestOzoneContractSeek

@adoroszlai adoroszlai dismissed their stale review April 22, 2020 10:19

acceptance test was fixed

Comment on lines +60 to +62
import org.apache.hadoop.http.FilterContainer;
import org.apache.hadoop.http.FilterInitializer;
import org.apache.hadoop.http.lib.StaticUserWebFilter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these classes copied from Hadoop being removed and dependency on Hadoop being re-introduced intentionally? CC @elek

Copy link
Contributor Author

@xiaoyuyao xiaoyuyao Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adoroszlai Yes, these are brought back to get SPNEGO work with our fork of Httpserver2. SPNEGO is implemented by KerberosAuthenticationHandler/KerberosAuthenticator in hadoop-common.

On server side, KerberosAuthenticationHandler is initialized by AuthenticationFilterInitializer that implements org.apache.hadoop.http.FilterInitializer. Unless, we decide to fork more code from hadoop-common, I think we should stick with the FilterContainer/FilterInitilaizer/StaticUserWebFilter from hadoop-common given the fact that there are barely major change in SPNEGO from hadoop 2 to hadoop 3.

@elek
Copy link
Member

elek commented Apr 30, 2020

Still +1

As I wrote earlier, I am fine to remove the (half-made) fork temporary to have a working state and do proper (working) fork later.

Still need some additional changes to handle the deprecation of the inconsistent key names related to HTTP auth.

I assume it also can be done in a follow-up jira if required.

I will merge this issue if no more objections.

@xiaoyuyao
Copy link
Contributor Author

TestBlockOutputStreamWithFailures seems to take long time to finish (10m) with pending flaky issues as query below. Should we consider disable it?

https://issues.apache.org/jira/issues/?jql=text%20~%20%22TestBlockOutputStreamWithFailures%22%20and%20resolution%20%3D%20%22Unresolved%22

@elek
Copy link
Member

elek commented May 6, 2020

I will merge this issue if no more objections.

Green build, no objections --> I am merging it.

@xiaoyuyao xiaoyuyao merged commit 4a9fde5 into apache:master May 6, 2020
@xiaoyuyao
Copy link
Contributor Author

Thanks all for the reviews and discussions. I've merged it to master.

@elek
Copy link
Member

elek commented May 7, 2020

I've merged it to master.

Sorry, I merged it locally as I promised but forget to push it. Thanks to finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants