HBASE-26204 Obtain credential for VerifyReplication with peerQuorumAddress #3591
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Aug 17, 2021
There was a problem hiding this comment.
Looks good, so far. Few remarks:
- Can we add explanation on how to use this in the
printUsagemethod? - What if the remote peer isn't kerberised? I guess
initCredentialsForClustercall would throw an exception? - Since we are making VerifyReplication a general comparison tool (not replication specific anymore), shouldn't we cover all possible scenarios (unsecure/secure, secure/secure, secure/unsecure, unsecure/unsecure)?
- Any chance to add UTs?
|
Thank you for your comments.
I see. I'll check the current usage first.
I think there are several cases as you mentioned, but in general, initCredentialsForCluster shouldn't throw exceptions and do nothing for non-kerberos cluster configuration(connection). I know initCredentialsForCluster might throw some exception if job conf has hbase.security.authentication=simple/kerberos and cluster conf has hbase.security.authentication=kerberos/simple as another issue. Current initCredentialsForCluster won't work for the following case and I'd like to fix it in HBASE-26205 (#3592) as indivisual change (because initCredentialsForCluster is common feature)
Yes, it could handle any combination with configuration for the VerifyReplication execution. For example, these is some examples of the execution for the combinations.
I'll try it. |
Added test case for both is secure. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Added examples |
|
🎊 +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. |
shahrs87
reviewed
Aug 18, 2021
| System.err.println(" To verify the data in TestTable between the secured cluster runs VerifyReplication and insecure cluster-b"); | ||
| System.err.println( | ||
| " $ hbase org.apache.hadoop.hbase.mapreduce.replication.VerifyReplication \\\n" + | ||
| " -D verifyrep.peer.hbase.security.authentication=simple \\\n" + |
There was a problem hiding this comment.
Do we expect a space between D and verifyrep.peer.hbase.security.authentication ?
There was a problem hiding this comment.
Yes, this is a part of the generic option for the Hadoop tool and isn't option for Java command itself
https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-common/CommandsManual.html#Generic_Options
ToolRunner internally process this sort of common options in GenericOptionsParser
https://github.com/apache/hadoop/blob/07627ef19e2bf4c87f12b53e508edf8fee05856a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java#L50-L112
shahrs87
reviewed
Aug 18, 2021
| @@ -755,7 +793,8 @@ public int run(String[] args) throws Exception { | |||
| * @throws Exception When running the job fails. | |||
| */ | |||
| public static void main(String[] args) throws Exception { | |||
| int res = ToolRunner.run(HBaseConfiguration.create(), new VerifyReplication(), args); | |||
| System.exit(res); | |||
| // int res = ToolRunner.run(HBaseConfiguration.create(), new VerifyReplication(), args); | |||
There was a problem hiding this comment.
Oops, embarrassed. Thanks!
shahrs87
reviewed
Aug 18, 2021
| HBaseClassTestRule.forClass(TestVerifyReplicationSecureClusterCredentials.class); | ||
|
|
||
| private static MiniKdc KDC; | ||
| private static final HBaseTestingUtil UTIL1 = new HBaseTestingUtil(); |
There was a problem hiding this comment.
nit: add a comment that UTIL1 is secure cluster and UTIL2 is non-secure cluster.
There was a problem hiding this comment.
Well, both are a secure cluster as setupCluster method apply secure configuration before start cluster.
I'd like to add test case mixing secure cluster and non-secure cluster, but it seems like it's difficult due to some static fields:
#3591 (comment)
#3591 (review)
shahrs87
approved these changes
Aug 18, 2021
shahrs87
reviewed
Aug 18, 2021
| @@ -736,6 +740,40 @@ private static void printUsage(final String errorMsg) { | |||
| System.err.println(" $ hbase " + | |||
| "org.apache.hadoop.hbase.mapreduce.replication.VerifyReplication" + | |||
| " --starttime=1265875194289 --endtime=1265878794289 5 TestTable "); | |||
| System.err.println(); | |||
| System.err.println(" To verify the data in TestTable between the cluster runs VerifyReplication and cluster-b"); | |||
There was a problem hiding this comment.
nice work to add examples on how to run VerifyReplication between non secure and secure environment. It is very helpful.
There was a problem hiding this comment.
Thanks for putting up these examples!
wchevreuil
approved these changes
Aug 19, 2021
There was a problem hiding this comment.
Unfortunately, I couldn't add a test case for non-secure and secure because it's difficult to start non-secure and secure cluster at the same time in a single process.
User (UserGroupInformation) uses static heavily and it can be either SIMPLE auth or Kerberos auth, so either cluster confuse and won't work properly.
If you know how to boot both of non-secure and secure cluster at the same time, please tell me so that I could add test case for it...
Yeah, I had similar problems before when implementing cross realm kerberos support for hashtable/synctable. I believe we are good with the current test, which at least assert we add credentials when peerId is null.
| @@ -736,6 +740,40 @@ private static void printUsage(final String errorMsg) { | |||
| System.err.println(" $ hbase " + | |||
| "org.apache.hadoop.hbase.mapreduce.replication.VerifyReplication" + | |||
| " --starttime=1265875194289 --endtime=1265878794289 5 TestTable "); | |||
| System.err.println(); | |||
| System.err.println(" To verify the data in TestTable between the cluster runs VerifyReplication and cluster-b"); | |||
There was a problem hiding this comment.
Thanks for putting up these examples!
|
🎊 +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. |
bitterfox
added a commit
to bitterfox/hbase
that referenced
this pull request
Aug 25, 2021
…dress (apache#3591) Signed-off-by: Rushabh Shah <shahrs87@gmail.com> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
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-26204
Example for execution that this PR wants to fix.