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

Able to reference a User from permissions.rabbitmq.com crd #177

Merged
merged 4 commits into from
Jul 1, 2021

Conversation

ChunyiLyu
Copy link
Contributor

This closes #165

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

  • add a field spec.userReference (type LocalObjectReference) to permissions.rabbitmq.com
  • preserve spec.user to minimize disruption to our users
  • spec.user is no longer a mandatory field. However, permissions webhook will make sure that one of spec.userReference and spec.user must be provided; webhook also returns an invalid error if both fields are specified.
  • tested in system tests
  • a small refactor in integration tests to use SatisfyAll where it makes sense

Why reference user in permissions.rabbitmq.com but not the other around?

To assign permissions for a users, we need to have the username, permissions configurations, and the vhost. Users are not vhost specific and can have different permissions configurations in different vhosts. If we were to add permissions configurations in users.rabbitmq.com, updates that revokes permissions in certain vhosts could be hard to perform (operator will need to keep track of the previous list of vhosts and check if there were deletion on every updates).

Copy link
Member

@MirahImage MirahImage 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 integration tests are failing

@ChunyiLyu ChunyiLyu merged commit 9b36238 into main Jul 1, 2021
@ChunyiLyu ChunyiLyu deleted the user_permissions branch July 1, 2021 15:07
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.

Associate permissions to autogenerated users
2 participants