Uh oh!
There was an error while loading. Please reload this page.
HBASE-25051 DIGEST based auth broken for rpc based ConnectionRegistry - #5631
Merged
Conversation
Apache-HBase
commented
Jan 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 24, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 24, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 24, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 25, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 25, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 25, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 26, 2024
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Jan 27, 2024
ContributorAuthor
Good, all green! |
Apache-HBase
commented
Jan 29, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 29, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 29, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 30, 2024
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Jan 31, 2024
ContributorAuthor
@bbeaudreault Mind taking a look at this? It fixes one of the security problems for RpcConnectionRegistry. And I will also fix HBASE-28321 based on this patch. Hope we could get this done in 2.6.0 release. Thanks. |
bbeaudreault
commented
Feb 1, 2024
Contributor
@Apache9 This is on my list, trying to get to it soon |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good, but I had some questions and suggestions around the organization of the StubsHolder.
It might make sense to get another review here, since the code is quite complex. Maybe @bharathv if he's around, since he commented on the jira in the past?
| } | ||
| LOG.debug("Going to refresh stubs"); | ||
| assert rpcClient != null; | ||
| addr2Stub = createStubs(rpcClient, fetchEndpoints.get()); |
Contributor
There was a problem hiding this comment.
do we not need to synchronize the setting of addr2Stub? Looks like we are doing it in 2 places, here and above, and we're also reading it in a synchronized block above.
Contributor
There was a problem hiding this comment.
I now see above that we are synchronizing in the callback of createStubs, but still seems like we need it here too
ContributorAuthor
There was a problem hiding this comment.
No, we don't. It is the different createStubs method, where we just update the addr2Stubs field without setting up any real rpc connections. It is a bit confusing as we use the same method name. Let me see if we can change this a bit.
Contributor
There was a problem hiding this comment.
My feeling is that if we're going to synchronize writes to addr2Stub, we should synchronize all writes to it. I also wonder if we should do something like this:
if (addr2Stub == null) {
Log.debug(...);
return;
}
Collection<ServerName> endpoints = fetchEndpoints.get();
synchronized(this) {
if (addr2Stub == null) {
return;
}
addr2Stubs = createStubs(rpcClient, endpoints);
}Just to minimize the scope of synchronization, since fetchEndpoints is a network call. Also, rather than pass in an IOExceptionSupplier, should we just pass in a future?
ContributorAuthor
There was a problem hiding this comment.
Let me check if we can change the code like this.
For future vs. IOExce[tionSupplier, the intention here is that we do not need asynchronous call here, we are in a background thread, we just fetch the result in a blocking way and update the addr2Stub, so we do not need a CompletableFuture here, the only thing is that we use an async client in the caller method so actually the implementation is a future.get.
ContributorAuthor
There was a problem hiding this comment.
We do not need to synchronize here.
The synchronization is for protecting the initialization of addr2Stub, and after initialization, addr2Stub will never be null, so we will never enter the synchronization block in getStubs method any more. And since addr2Stub is volatile, in refreshStubs, we just update the reference to addr2Stub at once, without changing its content, it is safe to not do any synchronization. Is this clear enough? Or we could introduce a initLock and synchronized on this object in getStubs method to make the logic more clear?
Contributor
There was a problem hiding this comment.
Ok thanks, that makes sense as is
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // out, only a preamble connection header, but if we pass null as user, there will be NPE in | ||
| // some code paths... | ||
| RpcChannel channel = | ||
| getConnectionRegistryRpcClient.createRpcChannel(bootstrapServers.get(index), user, 0); |
Contributor
There was a problem hiding this comment.
Even if the rpcTimeout is not used, I think it's preferable to pass a real value in. We have a real value in this class, let's pass it in? Maybe something changes down the line, I don't like passing a 0 value for timeout
ContributorAuthor
There was a problem hiding this comment.
This is intentional, I do not want to let developers think the timeout will take effect, as not everyone will look at the comment unless there is something unusual...
| // cluster id, which is the old behavior | ||
| LOG.debug("Failed to get connection registry info, should be an old server," | ||
| + " fallback to use null cluster id", controller.getFailed()); | ||
| createStubsAndComplete(null, future); |
Contributor
There was a problem hiding this comment.
I wonder, do we want to try the next bootstrap server? We can fall through to old behavior if all bootstrap servers fail preamble?
ContributorAuthor
There was a problem hiding this comment.
We could but this will make the code even more complicated, since we need to record all the failures, and have to consider what should be done if we got different exceptions from different servers...
And if a server returns us this exception, it means that the cluster is with old server, or at least still under upgrading, then we keep the old behavior is acceptable?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
-206
to
+131
| // exceptionCaught method can not help us finish the call, so here we need to catch the | ||
| // exception and finish it | ||
| // And in netty, the decoding the frame based, when reaching here we have already read a full | ||
| // In netty, the decoding the frame based, when reaching here we have already read a full | ||
| // frame, so hitting exception here does not mean the stream decoding is broken, thus we do | ||
| // not need to throw the exception out and close the connection. | ||
| call.setException(e); | ||
| LOG.warn("failed to process response", e); |
Contributor
There was a problem hiding this comment.
I'm not sure I follow this change -- why is it ok to now remove the call.setException? was it never necessary, or what part of the other changes here negate the need?
ContributorAuthor
There was a problem hiding this comment.
This is already done in RpcConnection.readResponse method, you can check the code there. It will catch the exception for finishCall method and call the setException method. And then it will throw the exception out, for BlockingRpcClient we may close the connetion and for netty, we will just ignore it.
| } | ||
| // on backup masters, this request may be blocked since we need to fetch it from filesystem, | ||
| // but since it is just backup master, it is not a critical problem | ||
| String clusterId = ((ConnectionRegistryEndpoint) rpcServer.server).getClusterId(); |
Contributor
There was a problem hiding this comment.
is there no way to protect this from blocking? maybe in a followup since this PR is already large.
my concern is that backup masters may soon become active, we don't want them blocked.
ContributorAuthor
There was a problem hiding this comment.
The request is blocked, not the backup masters are blocked.
Apache9
commented
Feb 3, 2024
ContributorAuthor
Thanks for the review. I could see if I can split the ConnectionRegistryRpcStubHolder too. |
Apache-HBase
commented
Feb 3, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 3, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 3, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 3, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 3, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 3, 2024
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Feb 4, 2024
ContributorAuthor
OK, good, all green. |
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
Just one more common to resolve on my side. Thanks
| } | ||
| LOG.debug("Going to refresh stubs"); | ||
| assert rpcClient != null; | ||
| addr2Stub = createStubs(rpcClient, fetchEndpoints.get()); |
Contributor
There was a problem hiding this comment.
My feeling is that if we're going to synchronize writes to addr2Stub, we should synchronize all writes to it. I also wonder if we should do something like this:
if (addr2Stub == null) {
Log.debug(...);
return;
}
Collection<ServerName> endpoints = fetchEndpoints.get();
synchronized(this) {
if (addr2Stub == null) {
return;
}
addr2Stubs = createStubs(rpcClient, endpoints);
}Just to minimize the scope of synchronization, since fetchEndpoints is a network call. Also, rather than pass in an IOExceptionSupplier, should we just pass in a future?
bbeaudreault
approved these changes
Feb 5, 2024
Apache9 added a commit
to Apache9/hbase
that referenced
this pull request
Feb 6, 2024
…apache#5631) Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit 16de74c)
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
No description provided.