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

[DPE-5206] Add DIGEST scheme ACL to znode created by zookeeper #97

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

theoctober19th
Copy link
Member

This PR adds an additional ACL with "DIGEST" scheme to the znodes created by zookeeper. This is needed because Kyuubi uses DIGEST ACL to create / write znodes and is not able to access the znodes created by zookeeper otherwise.

@theoctober19th theoctober19th marked this pull request as ready for review August 21, 2024 15:10
Copy link
Contributor

@welpaolo welpaolo left a comment

Choose a reason for hiding this comment

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

LGTM, but I will wait for Marc's opinion on it!

src/managers/quorum.py Show resolved Hide resolved
tests/integration/test_tls.py Outdated Show resolved Hide resolved
src/managers/quorum.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Batalex Batalex left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

LGTM!

I suppose the integration tests for testing this would be in Kyuubi. Would it be very hard to make an integration tests to make sure it works? If it means more than 4h of work, it is not worth it, I believe

acls = kwargs["acls"]
assert len(acls) == 2
assert len([acl for acl in acls if acl.perms == 31 and acl.id.scheme == "sasl"]) != 0
assert len([acl for acl in acls if acl.perms == 31 and acl.id.scheme == "digest"]) != 0
Copy link
Contributor

Choose a reason for hiding this comment

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

question what is the 31 magic number?

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.

4 participants