feat: add rbac support #29
Conversation
There was a problem hiding this comment.
This looks great, thanks for working on this and especially for adding unit tests!
It might be preferable to actually create a dedicated service account for the catalog sync service, and bind to that, rather than adding the sync role to all service accounts. That is the approach we took with #37.
Would that be okay? We can make the changes if necessary but want to inquire why you didn't go with that approach since you might have reasons.
values.yaml
Outdated
| # Enabling rbac will create cluster roles and cluster role bindings to allow the | ||
| # syncCatalog service to communicate with the kubernetes api to get/create services | ||
| rbac: | ||
| enabled: false |
There was a problem hiding this comment.
I think this should probably go under the syncCatalog section. I can see the easiest way would be:
syncCatalog:
rbac:
enabled: false/true
|
I've been deploying this helm chart to a dedicated namespace, hence why I didn't think to add a service account. I agree with that approach however, and will integrate it with this PR. |
|
PR updated to include the creation of a service account |
|
I tested this out and the use of |
|
Fixed |
| - watch | ||
| - update | ||
| - patch | ||
| - delete |
There was a problem hiding this comment.
| - delete | |
| - delete | |
| - create |
I was getting error="services is forbidden: User "system:serviceaccount:platform:consul-sync-catalog" cannot create services in the namespace "platform"" without this. Once added, the consul service was synced to k8s.
There was a problem hiding this comment.
Nice. Yep.
It seems possible that we should create two roles here. One for k8s => Consul and one for Consul => k8s, since either one-way can be disabled/enabled and k8s => Consul only requires read-only privileges which is a nice to have.
There was a problem hiding this comment.
Actually decided to just add -create for now and keep the simple thing, we can split it out later if its useful.
|
Thank you very much @tomwganem! I just ran it and it worked great. I added the change @jipperinbham recommended as well. Merged! |
closes #20