Topic permission 191 #456
Topic permission 191 #456
Conversation
ChunyiLyu
force-pushed
the
topic-permission-191
branch
from
c06c27d to
5d85042
Compare
ChunyiLyu
force-pushed
the
topic-permission-191
branch
from
5d85042 to
6d9a793
Compare
| ) | ||
|
|
||
| // TopicPermissionSpec defines the desired state of TopicPermission | ||
| type TopicPermissionSpec struct { |
There was a problem hiding this comment.
We noticed that TopicPermissionSpec struct is identical to PermissionSpec of the Permission type. It would be possible to reuse it or maybe is good to have them separate for future extensions?
There was a problem hiding this comment.
Structure of their the permission configurations are different: https://github.com/rabbitmq/messaging-topology-operator/blob/topic-permission-191/api/v1beta1/permission_types.go#L30-L37
User permissions needs configrue read and write, whereas topic permission is name of the exchange, read and write.
| Expect(apierrors.IsInvalid(invalidPermission.ValidateCreate())).To(BeTrue()) | ||
| }) | ||
|
|
||
| It("does not allow both spec.rabbitmqClusterReference.name and spec.rabbitmqClusterReference.connectionSecret be configured", func() { |
There was a problem hiding this comment.
We were noticing that there are not reference of "connectionSecret" in topic_permission_webhook.go, where is this situation managed?
There was a problem hiding this comment.
It's validated as a method on RabbitmqClusterReference so it's shared across all webhooks: https://github.com/rabbitmq/messaging-topology-operator/blob/topic-permission-191/api/v1beta1/topicpermission_webhook.go#L41
| @@ -0,0 +1,7 @@ | |||
| apiVersion: rabbitmq.com/v1beta1 | |||
There was a problem hiding this comment.
Should this file be here? We don't see the folder created on the repo.
There was a problem hiding this comment.
This looks good! Thank you very much. it looks in line with the other controllers. We just noticed that there is a file in samples/rabbitmq.com_v1beta1_topicpermission.yaml that shouldn't be there. From Daniele and @ablease!
This closes #191
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
topicpermissions.rabbitmq.comand tested in integration and system testsvhost,exchange, anduserinformation are immutable and requiredspec.userReferenceis the same as what's inPermissionReconciler, methodr.getUserFromReferencewas refactored togetUsernameFromUserto reuse in topic permission reconciler.Will squash when merging to clean up history.
Additional Context