-
Notifications
You must be signed in to change notification settings - Fork 31
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
add controller to create and delete individual usernames in mariadb / galera #166
add controller to create and delete individual usernames in mariadb / galera #166
Conversation
Hi @zzzeek. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a sample file that includes a MariaDBAccount and a Secret
not sure if I should keep this in draft, the folks I wanted to see it first were @dciabrin , @SeanMooney and @gibizer |
/ok-to-test |
/retest |
30bfe83
to
207ba9b
Compare
207ba9b
to
bcdc146
Compare
/retest hold-the-node |
@zzzeek: The
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test mariadb-operator-build-deploy-kuttl hold-the-node |
/test mariadb-operator-build-deploy-kuttl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im conflicted on how to procedd with reviewing this.
we are not meant to use sqash merges in any of the repos which means you should not be breaking out the commits the way you are.
each commit should be indepently correct and when you need to make fixes you should interactivly rebase the fixes into the approate commits.
so I'm not sure there is value in doing an review of the implementation until that is done
i can take a look at the design side i guess but can you try cleaning this set of commits up in the 3-5 that it probably should be.
i would start with having the first commit introduce the API type/CRD
then the controller and samples
followed by kuttle tests and then ideally envtest
i realise that the mariadb-operator currently does not have envtest coverage which is why that should be last and likely in a sepeata pr since it will require some infra to be set up in the repo first
b48a470
to
90b87ab
Compare
670f352
to
4471b1e
Compare
/retest |
I will let @dciabrin merge it but I think it looks good what you have here |
@zzzeek one thing I noticed locally with this. Can you run 'make bundle' and commit the modifications to config/manifests/bases/mariadb-operator.clusterserviceversion.yaml as part of this PR? |
OK will do that now, hopefully it makes it all the way through |
ok thats up now, just added an entry to a yaml file |
OK so I also have to add this API to the PROJECT file, is that generated from something ? edit: nm, operator-sdk did it |
operator-sdk did it already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good I'm OK to land this and do my remaining comments in a follow up.
8bf20ff
to
484a1f6
Compare
OK it should just be ensuring the secret is immutable change subsequent to this |
/lgtm |
/retest |
/lgtm |
@zzzeek: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
/approved |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dciabrin, zzzeek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4cd7b3f
into
openstack-k8s-operators:main
this is a "create /drop account" controller that is separate from the main "create/drop database" controller, for the purpose of producing rotating username/passwords. The background for the change is based on discussions surrounding https://issues.redhat.com/browse/OSPRH-92 where internal control plane services such as Galera , Rabbit, Redis etc. would provide interfaces to add /remove arbitrary usernames, where a "password rotation" would involve adding a new username/password and having services switch there, retiring the old account once all finalizers have been removed.