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.

Sign up for GitHub

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

Jump to bottom

znode: add options for authentication #5306

Merged
merged 13 commits into from
Oct 5, 2022
Merged

znode: add options for authentication #5306

merged 13 commits into from
Oct 5, 2022

Conversation

henkwiedig
Copy link
Contributor

SUMMARY

This adds the possibility to use zookeeper ACL authentication.
Kazoo supports "digest” and “sasl”.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

znode

ADDITIONAL INFORMATION

@henkwiedig henkwiedig marked this pull request as ready for review September 24, 2022 18:15
@ansibullbot
Copy link
Collaborator

cc @treyperry
click here for bot help

@ansibullbot ansibullbot added clustering feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Sep 24, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 24, 2022
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Sep 24, 2022
@github-actions
Copy link

github-actions bot commented Sep 24, 2022 •
edited
Loading

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 24, 2022
@henkwiedig henkwiedig changed the title add options for authentication znode: add options for authentication Sep 24, 2022
felixfontein
felixfontein reviewed Sep 25, 2022
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I've added a few comments.

changelogs/fragments/5306-add-options-for-authentication.yml Outdated Show resolved Hide resolved
plugins/modules/clustering/znode.py Show resolved Hide resolved
plugins/modules/clustering/znode.py Show resolved Hide resolved
plugins/modules/clustering/znode.py Outdated Show resolved Hide resolved
plugins/modules/clustering/znode.py Outdated Show resolved Hide resolved
plugins/modules/clustering/znode.py Outdated Show resolved Hide resolved
plugins/modules/clustering/znode.py
credential:
description:
- 'The credential value. Depends on scheme (format "digest": user:password, "sasl": user:password).'
type: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering whether it's better to provide a dict here, respectively have two options username and password instead of one (credential).

Copy link
Contributor Author

@henkwiedig henkwiedig Sep 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest i don't know nothing about sasl but reading the wiki about sasl i can imagine that it does not depend on user and password but some other kind of secret based on the actual used sasl implmentation. I'm just exposing the underlying kazoo api here which also dosen't make this distinction. Besides that: this would leave us with a user password dict in case of digest and the credentail option in case of sasl. This might confuse people and increases the code to handle each case. I assume that users of zookeeper are familiar with this kind of notation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤷 I'm not using zookeeper, so no idea. If nobody else chimes in who knows anything about zookeeper, let's keep this as you suggested :)

henkwiedig and others added 6 commits September 25, 2022 17:38
@henkwiedig @felixfontein
Update changelogs/fragments/5306-add-options-for-authentication.yml
7f00d8f 
Co-authored-by: Felix Fontein <felix@fontein.de>
@henkwiedig @felixfontein
Update plugins/modules/clustering/znode.py
9c8f334 
Co-authored-by: Felix Fontein <felix@fontein.de>
@henkwiedig @felixfontein
Update plugins/modules/clustering/znode.py
4367464 
Co-authored-by: Felix Fontein <felix@fontein.de>
@henkwiedig @felixfontein
Update plugins/modules/clustering/znode.py
72fdf59 
Co-authored-by: Felix Fontein <felix@fontein.de>
@henkwiedig @felixfontein
Update plugins/modules/clustering/znode.py
dbc82a8 
Co-authored-by: Felix Fontein <felix@fontein.de>
@henkwiedig
rename scheme to auth_scheme, credential to auth_credential
e76328d
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 25, 2022
This was referenced Nov 20, 2022
Closed
Merged
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Dec 1, 2022
Open
Closed
@renovate renovate bot mentioned this pull request Dec 14, 2022
Closed
1 task
@renovate renovate bot mentioned this pull request Jan 1, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
clustering feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@henkwiedig @ansibullbot @felixfontein