Closed Cynocracy closed 4 years ago
@Cynocracy: The label(s) kind/bug
cannot be applied, because the repository doesn't have them
The primary reason the operator is given cluster-admin
privs is because the knative-serving release manifest employs aggregated clusterroles. At the time, this feature required cluster-admin
privs. I'm not sure if that's still the case or not. /cc @pmorie
Thanks for the context! That makes sense.
It looks like we may be able to have the Operator config set up the aggregated clusterrole, and grant the Operator service account bind
on that role.
hi @Cynocracy, unfortunately you need */*
permissions in order to use cluster role aggregation:
Thanks for the link, I'm still very new to the space so it's quite helpful for me :)
For now, I'll start a PR adding a comment to the */*
permission. It'd be nice if we can get some kind of fix upstream that removes the req, but at least with the current setup, I found the very explicit list followed by */*
to be quite confusing.
Circling back to this, a couple of different things:
@pmorie it looks like https://github.com/kubernetes/kubernetes/blob/3b618af0d435628feedf06f97bd1c69340d07d95/pkg/registry/rbac/clusterrole/policybased/storage.go#L69 early-exits that function when the user has escalate
on clusterroles
, so theoretically we could give the Operator the power to grant all powers, but not directly grant it everything (which would be nice from a we-don't-accidentally-reconcile-the-world perspective).
What I was thinking was a little different actually, if we could:
cluster-admin
(or have escalate
on clusterroles
).bind
the aggregated clusterrole
, which is later authz'd by https://github.com/kubernetes/kubernetes/blob/3b618af0d435628feedf06f97bd1c69340d07d95/pkg/registry/rbac/clusterrolebinding/policybased/storage.go#L70cluster-admin
!/assign Cynocracy
- Remove the clusterrole creation from the manifestival install (via label, maybe?).
This needs to be addressed upstream. The embedded manifest is taken directly from upstream's official release, and the only edits we should ever make to our embedded copy are those we know will be included in upstream's next release (ideally referencing a merged PR). Otherwise, we complicate our upgrade process (even more than it already is) by having to post process the official manifest for every future release.
Ack, I think the change to grant escalate
is a little less invasive, and still an improvement (in order to use the Operator as a confused deputy, you would need it to create a role and rolebinding, whereas today you merely need to get it to apply something for you).
Do you agree? If so, I can go ahead and start testing out that change.
Describe the bug Today the operator is by default granted all permissions on all resources clusterwide by a blanket ClusterRole
Expected behavior Only specifically those permissions which are necessary in order to create a functional Knative serving setup should be added, using escalate and/or bind to create roles or rolebindings that the operator itself does not need explicitly, but which are needed transitively by knative serving.
To Reproduce