HBASE-28294 Support to skip Kerberos authentication for metric endpoints #5606
+110
−4
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @eubnara Welcome and thank you for your first PR in Apache HBase.Please have a look at the review comments and please let me know if you need any support. |
NihalJain
reviewed
Jan 6, 2024
| @@ -152,6 +152,9 @@ public class HttpServer implements FilterContainer { | |||
| "hbase.security.authentication.ui.config.protected"; | |||
| public static final String HTTP_UI_NO_CACHE_ENABLE_KEY = "hbase.http.filter.no-store.enable"; | |||
| public static final boolean HTTP_PRIVILEGED_CONF_DEFAULT = false; | |||
| public static final String HTTP_PRIVILEGED_METRICS_KEY = | |||
There was a problem hiding this comment.
Can we have a test case similar to the hadoop PR? Else it's difficult to ensure this doesn't break in future.
There was a problem hiding this comment.
I got it, thanks. I will prepare with that.
There was a problem hiding this comment.
I'm trying to add a test case but I encountered a problem. In order to use "hbase.security.authentication.spnego.kerberos.endpoint.whitelist", HBase should use the hadoop which has HADOOP-18666.
I think HADOOP-18666 will be committed to hadoop >= 3.4.0 according to jira https://issues.apache.org/jira/browse/HADOOP-18666.
There was a problem hiding this comment.
Oh oh, in that case this feature won't even work until we have hadoop-3.4 right/ Maybe we park this PR for later then?
There was a problem hiding this comment.
I think so.
Let's postpone this PR until hbase uses hadoop with HADOOP-18666.
NihalJain
reviewed
Jan 6, 2024
| `false` | ||
|
|
||
|
|
||
| [[hbase.security.authentication.spnego.kerberos.endpoint.whitelist]] |
There was a problem hiding this comment.
Was going through hadoop PR. This property has been implemented in hadoop to handle any endpoint irrespective of whether it is metrics or something else. Why don't we follow same approach.? Do we really need to handle just metrics endpoint and have the other property hbase.security.authentication.ui.metrics.protected? The property will become redundant.
A generic implementation like the one in hadoop sound more useful to me. Also the name of the config will confuse others. As it seems like a generic whitelist.
There was a problem hiding this comment.
If you want to use the name "hadoop.http.authentication.kerberos.endpoint.whitelist", instead of initializing params using configurations that starts with HTTP_SPNEGO_AUTHENTICATION_PREFIX, that name should be used like:
String endpointWhitelist = getOrEmptyString(conf, "hadoop.http.authentication.kerberos.endpoint.whitelist");
if (!endpointWhitelist.isEmpty()) {
params.put("endpoint.whitelist", endpointWhitelist);
}Do you think it is more suitable for HBase code?
I did initialize params using configurations that starts with HTTP_SPNEGO_AUTHENTICATION_PREFIX because already other configurations uses prefix with it.
There was a problem hiding this comment.
If you want to use the name "hadoop.http.authentication.kerberos.endpoint.whitelist"
Hey I did not mean to change name.
A generic implementation like the one in hadoop sound more useful to me.
What I am trying to say is current implementation in PR aims to just whitelist which are under metrics umbrella. But if you look at the hadoop PR, it aims to provide a way to whitelist any endpoint in the passed list. Can we not follow that approach and have a generic implementation where we let the user whitelist any endpoint irrespective of whether it is a metric endpoint or say /logs
Also the name of the config will confuse others. As it seems like a generic whitelist.
I mean nowhere in config name hbase.security.authentication.spnego.kerberos.endpoint.whitelist it is obvious that this can control only metrics endpoints.
There was a problem hiding this comment.
Ignore as my doubt is clear here. Just that I think even conf servlet would also require the whitelisting if one want to access it:
?So is this documentation correct? It is valid even if hbase.security.authentication.ui.config.protected is false and whitelist is set to \conf, isn't it?
There was a problem hiding this comment.
Yes, if someone wants to disable spnego authentication completely on /conf endpoint completely, /conf should also be added to whitelist.
There was a problem hiding this comment.
Maybe then update docs to reflect same?
eubnara
commented
Jan 6, 2024
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java
Outdated
Show resolved
Hide resolved
NihalJain
reviewed
Jan 6, 2024
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
eubnara
commented
Jan 7, 2024
| // FIXME: hadoop >= 3.4.0 (or hadoop with HADOOP-18666) is needed to test with below lines. | ||
| // url = new URL(getServerURL(server), "/prometheus"); | ||
| // conn = (HttpURLConnection) url.openConnection(); | ||
| // assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode()); |
There was a problem hiding this comment.
I added a test case but it cannot be tested properly if you don't use hadoop with HADOOP-18666 😢 .
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
eubnara
force-pushed
the
HBASE-28294
branch
2 times, most recently
from
25150c8
to
09291ac
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +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.
https://issues.apache.org/jira/browse/HBASE-28294
Make HBase support to skip Kerberos authentication for metric endpoints. (e.g. /jvm, /prometheus, /metrics)
Since HBase uses KerberoAuthenticationHandler.java, whitelist configuration can be used like HADOOP-16527.