search

cruise-automation / rbacsync

Automatically sync groups into Kubernetes RBAC
Apache License 2.0
240 stars 33 forks source link

controller: allow RBACSyncConfig to bind to ClusterRole #13

Closed sbunce closed 5 years ago

sbunce commented 5 years ago

Issue: https://github.com/cruise-automation/rbacsync/issues/12

Context: There are valid use cases for binding a namespaced user or group to a cluster role. This change removes the validation that was preventing this. See the k8s documentation on this. https://kubernetes.io/docs/reference/access-authn-authz/rbac

Testing: The previous test for binding invalid roles was modified to have a purely made up role type name. A new test was added for testing binding a cluster role.

mattlandis commented 5 years ago

Can you take a look through the docs and make sure they are accurate after this change. For instance:


For namespace-scoped RBACSyncConfig, the behavior is nearly identical except for the following:

RBACSyncConfig must be defined with a namespace.
RBACSyncConfig can only reference Roles.
All RoleBindings created as a result of the RBACSyncConfig will be in the same namespace.
stevvooe commented 5 years ago

Code/Test LGTM

Please update the readme. Also double check against the RBAC docs and make sure we are using the same language to describe this scenario.

sbunce commented 5 years ago

I gave the RBAC docs a re-read.

I updated the README.md to get rid of the point about only being able to reference a role from a RBACSyncConfig. I added an extra paragraph in the README.md about the meaning of referencing a ClusterRole from a RBACSyncConfig. This paragraph is almost identical to what is in the RBAC docs.

Also updated that comment in the controller role ref type check to be more descriptive.

sbunce commented 5 years ago

I squashed. Ready for merge.