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

Change the way ZkClientUriDomainMappingHelper add watch #111

Merged
merged 5 commits into from
Dec 17, 2022

Conversation

mgao0
Copy link
Collaborator

@mgao0 mgao0 commented Dec 13, 2022

Description

ZkClientUriDomainMappingHelper instance is registered as a watcher during the instantiation process, so it will get the notification every time there's a change in the client URI domain mapping, and it can update each session that are connected to this zookeeper server for the updated access control metatdata.
However, in ZooKeeper server code, there is an implicit assumption that the objects registered as a watcher are ServerCnxn objects, while ZkClientUriDomainMappingHelper is not an instance of ServerCnxn. This incorrect cast of ZkClientUriDomainMappingHelper to ServerCnxn can lead to code failure.
This commit leveraged a dummy server cnxn class DumbWatcher which actually functions as a watcher, to prevent this error.

Tests

The following tests are written for this issue:
ZkClientUriDomainMappingHelperTest.testB_GetWatches

The following is the result of the "mvn test" command on the appropriate module:
[INFO] Results:
[INFO]
[WARNING] Tests run: 3158, Failures: 0, Errors: 0, Skipped: 3
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 18:39 min
[INFO] Finished at: 2022-12-13T13:41:21-08:00
[INFO] ------------------------------------------------------------------------

Changes that Break Backward Compatibility (Optional)

My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)

Documentation (Optional)

In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)

@mgao0 mgao0 marked this pull request as ready for review December 13, 2022 21:24
@rahulrane50 rahulrane50 self-requested a review December 14, 2022 00:02
Copy link
Collaborator

@rahulrane50 rahulrane50 left a comment

Choose a reason for hiding this comment

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

Thanks Molly for working on this unique issue. I was blown away that there is such this implicit assumption that watcher will be extending ServerCxnx class. Anyways i know how absurd or hacky to use DumbWatcher here but surely it will work for us. Some digging around show me that there is another way to do this. What if we use something like this. Here we have cnxn object available here since we it in handleAuthentication module and have zks object in constructor. Let me know what you think.
(I haven't reviews tests since we have to finalize on approach first)

@mgao0
Copy link
Collaborator Author

mgao0 commented Dec 16, 2022

Thanks Molly for working on this unique issue. I was blown away that there is such this implicit assumption that watcher will be extending ServerCxnx class. Anyways i know how absurd or hacky to use DumbWatcher here but surely it will work for us. Some digging around show me that there is another way to do this. What if we use something like this. Here we have cnxn object available here since we it in handleAuthentication module and have zks object in constructor. Let me know what you think. (I haven't reviews tests since we have to finalize on approach first)

Let me make sure I understand your proposal correctly. Are you suggesting every time we call handleAuthentication in X509ZNodeGroupAclProvider(so we have cnxn object), we instantiate a ZkClientUriDomainMappingHelper which will add a watcher during instantiation? This technically is feasible, but it is not an optimal way.
The reason is that: Right now we try to abstract and isolate all the ClientURI-domain mapping logic in ZkClientUriDomainMappingHelper. The scope is one ZkClientUriDomainMappingHelper per X509ZNodeGroupAclProvider -> per ZooKeeperServer, meaning if we add the watcher in ZkClientUriDomainMappingHelper constructor, we will have one watcher for the client uri - domain mapping per zookeeper server (this is also the reason why we don't have cnxn object available for adding watcher), and there's centralized logic in ZkClientUriDomainMappingHelper to handle the parsing of mapping after an event is fired. However, if we add watches in handleAuthentication (scope is one handleAuthentication call for each connection), it means we will have a lot more (# of connections) watchers only for monitoring client uri-domain mapping, which can impact the zk server performance.
Please let me know if my understanding is correct, thanks.

@rahulrane50
Copy link
Collaborator

@mgao0 one more thing let's verify our changes with watch command if it's working now.

Copy link
Collaborator

@rahulrane50 rahulrane50 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again @mgao0 for fixing this.

@mgao0
Copy link
Collaborator Author

mgao0 commented Dec 17, 2022

This PR is ready to be merged. Approved by @rahulrane50 . Thanks.

@mgao0
Copy link
Collaborator Author

mgao0 commented Dec 17, 2022

CI shows Server module tests all passed
[INFO] Apache ZooKeeper - Server .......................... SUCCESS [ 01:12 h]

@mgao0 mgao0 merged commit 558281b into linkedin:li-dev/base-3.6.3-znode-group-security Dec 17, 2022
@mgao0 mgao0 deleted the getWatch branch December 17, 2022 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants