fix: Ensure verifyKvStoreConnection takes into account rootPrefix #5
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
Motivation Instances of KVUtilsFactory are initialized for a particular connection configuration, which for etcd includes a "chroot" rootPrefix. All the utility methods honor this prefix, except the verifyKvConnection method. This can cause problems when role-based access controls are configured in etcd and the client-configured userid doesn't have access outside of the rootPrefix key range. In this case the verifyKvConnection call will fail unexpectedly with a permission denied error. Modifications - Adjust the verifyKvConnection to perform a get on a dummy (likely nonexistent) key within the rootPrefix range - Add auth-based unit test Result KVUtilsFactory#verifyKvConnection will work as expected in key-specific auth cases. Signed-off-by: Nick Hill <[email protected]>
joerunde
reviewed
Aug 9, 2022
|
|
||
| @Override | ||
| public ListenableFuture<Boolean> verifyKvStoreConnection() { | ||
| return Futures.catching(Futures.transform(client.getKvClient().get(TEST_KEY).countOnly() | ||
| return Futures.catching(Futures.transform(client.getKvClient().get(pathToKey(TEST_PATH, false)).countOnly() |
There was a problem hiding this comment.
is the .countOnly() what makes it cool to .get() a key that doesn't exist without throwing an error?
There was a problem hiding this comment.
I think even without that it's cool, it will just return an empty result (the "get" operation is actually generic and takes either a range or single key, as well as other filtering criteria, so it can return zero or more results).
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.
Motivation
Instances of
KVUtilsFactoryare initialized for a particular connection configuration, which for etcd includes a "chroot"rootPrefix. All the utility methods honor this prefix, except theverifyKvConnectionmethod.This can cause problems when role-based access controls are configured in etcd and the client-configured userid doesn't have access outside of the
rootPrefixkey range. In this case theverifyKvConnectioncall will fail unexpectedly with a permission denied error.Modifications
verifyKvConnectionto perform a get on a dummy (likely nonexistent) key within therootPrefixrangeResult
KVUtilsFactory#verifyKvConnectionwill work as expected in key-specific auth cases.