HBASE-23312 HBase Thrift SPNEGO configs (HBASE-19852) should be backwards compatible #850
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
Contributor
Author
|
FYI @busbey @esteban @joshelser (I'm not sure how to tag reviewers or get feedback on the PR - doesn't look like the HBase book was updated? https://hbase.apache.org/book.html#submitting.patches). |
|
💔 -1 overall
This message was automatically generated. |
risdenk
force-pushed
the
HBASE-23312
branch
2 times, most recently
from
853991a to
a4fb8d2
Compare
Contributor
Author
|
Pushed change that addresses the license and race condition with the tests |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Pushed whitespace fix. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
joshelser
requested changes
Nov 19, 2019
Member
joshelser
left a comment
joshelser
left a comment
There was a problem hiding this comment.
Looks reasonable! Appears to restore functionality so that hbase.thrift.keytab.file can continue to be used in the absence of hbase.thrift.spnego.keytab.file.
A couple of minor polishing requests.
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Outdated
Show resolved
Hide resolved
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Show resolved
Hide resolved
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Outdated
Show resolved
Hide resolved
risdenk
force-pushed
the
HBASE-23312
branch
2 times, most recently
from
096a2db to
8e24d7a
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
busbey
requested changes
Nov 22, 2019
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private String getSpengoPrincipal(Configuration conf, String host) throws IOException { | ||
| String principal = conf.get(THRIFT_SPNEGO_PRINCIPAL_KEY); |
Contributor
There was a problem hiding this comment.
We're doing this ourselves instead of using Hadoop Configuration's deprecation mechanisms because we want a different fall back order? Or we can't set deprecation soon enough for some reason?
We should have comments proactively letting future folks know the reasoning.
Contributor
Author
There was a problem hiding this comment.
Hmm let me take a look at what Hadoop Configuration's deprecation options are - I didn't know that was a fine. The old config is still technically valid just not for SPNEGO. I'll see if that is an option here to fallback with a message.
Contributor
Author
There was a problem hiding this comment.
So I don't think using the Hadoop Configuration deprecations will work here. The existing config key hbase.thrift.kerberos.principal is still valid and needs to be used for Kerberos communication between HBase Thrift Server to backend HBase master/rs. The new config key hbase.thrift.spnego.principal should be used for SPNEGO (handling SPNEGO HTTP only not for backend communication).
So its not really a deprecation per say, but more configuration to split out the principal/keytab used for SPNEGO versus backend communication. In an ideal world, the config hbase.thrift.kerberos.principal would never have done double duty.
Contributor
Author
There was a problem hiding this comment.
I added a comment specifically for this case.
Member
There was a problem hiding this comment.
Given I missed this the first time, the comment clearly explains things to me. Resolving.
…ards compatible HBase Thrift SPNEGO configs should not be required. The `hbase.thrift.spnego.keytab.file` and `hbase.thrift.spnego.principal` configs should fall back to the `hbase.thrift.keytab.file` and `hbase.thrift.kerberos.principal` configs. This will avoid any issues during upgrades. Signed-off-by: Kevin Risden <[email protected]>
Contributor
Author
|
Latest push adds comment about deprecation handling and why not use Hadoop |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@busbey @joshelser I think this is ready for another round of review |
joshelser
approved these changes
Nov 25, 2019
Member
joshelser
left a comment
joshelser
left a comment
There was a problem hiding this comment.
A couple of minor comments, but I can run the tests myself locally to validate my questions :)
I know Busbey is not working this week, so I'm moving ahead without his ack.
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Show resolved
Hide resolved
...-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private String getSpengoPrincipal(Configuration conf, String host) throws IOException { | ||
| String principal = conf.get(THRIFT_SPNEGO_PRINCIPAL_KEY); |
Member
There was a problem hiding this comment.
Given I missed this the first time, the comment clearly explains things to me. Resolving.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
HBase Thrift SPNEGO configs should not be required.
The
hbase.thrift.spnego.keytab.fileandhbase.thrift.spnego.principalconfigs should fallback to the
hbase.thrift.keytab.fileandhbase.thrift.kerberos.principalconfigs. This willavoid any issues during upgrades.
Signed-off-by: Kevin Risden [email protected]