Restrict RBAC for Operator #291

Cynocracy · 2020-02-11T22:15:11Z

Fixes #282

#374 is a base of this, you can solely review the YAML here
(and not the reconciler change) though you're welcome to review
that as well.

Proposed Changes

By using the escalate verb on clusterroles, we allow the Operator
to have the ability to create any clusterrole. This prevents us from
needing to grant it cluster-admin.

Additionally, by having the Operator create an aggregated clusterrole
for itself (aggregating all clusteroles created by Knative Serving),
we pick up any permissions that the upstream dictate as necessary
by any component.

Any roles that are not created by upstream, but which are bound as
part of install are added with the bind verb to allow the Operator
to bind them without necessarily having the permissions they grant.

Some other permissions are needed to support controller infra
and are explicitly included, as well as some specific to the Operator
itself. An attempt at grouping these by sections is included, though
may be slightly out of sync (in particular: Controller infra / Specific
to this Operator may overlap).

Overall, this should keep the Operator roughly operating only on
Knative Serving resources (or things which can be operated
on by upstream).

Release Note

Change RBAC to minimize permissions of Operator service account.

Cynocracy · 2020-02-11T23:29:00Z

/retest

(seems to be running OK locally)

Cynocracy · 2020-02-12T00:10:04Z

Oh, I see the issue.

Cynocracy · 2020-02-12T01:02:54Z

/retest

:(

Cynocracy · 2020-02-13T00:56:00Z

Found out how to read the logs:

{"ts":"2020-02-12T01:09:32.279Z","knative.dev/controller":"knativeserving-controller","msg":"Reconcile succeeded. Time taken: 11.99198359s.","commit":"0b4d6f8","level":"info","logger":"serving_operator.knativeserving-controller","knative.dev/key":"operator-tests/knative-serving","caller":"controller…

Hmmmm

/retest

so that I can poke around more :)

Cynocracy · 2020-02-13T01:20:34Z

raceid":"fd80c601-0e62-4773-bdd6-1ecb60dbbf78","knative.dev/key":"operator-tests/knative-serving"}
{"level":"info","ts":"2020-02-13T01:16:24.454Z","logger":"serving_operator.knativeserving-controller.event-broadcaster","caller":"record/event.go:274","msg":"Event(v1.ObjectReference{Kind:"KnativeServing", Namespace:"operator-tests", Name:"knative-serving", UID:"5e9ab652-b2da-431a-86cb-211efa5cbd70", APIVersion:"operator.knative.dev/v1alpha1", ResourceVersion:"4761", FieldPath:""}): type: 'Warning' reason: 'InternalError' apiservices.apiregistration.k8s.io "v1beta1.custom.metrics.k8s.io" is forbidden: User "system:serviceaccount:default:knative-serving-operator" cannot update resource "apiservices" in API group "apiregistration.k8s.io" at the cluster scope","commit":"b29f72a","knative.dev/controller":"knativeserving-controller"}

OK, so that was required.

Cynocracy · 2020-02-13T01:54:31Z

This is ready for a preliminary review.

Let me know if more detail is necessary.

trshafer

role.yaml went from 60 lines to 200 lines, worth splitting into multiple files?

config/role.yaml

Cynocracy · 2020-02-13T21:56:21Z

I think splitting it into multiple files makes sense, especially since some of the sections will be drop-in equivalents between the serving and eventing operator.

I'm working on another WIP for that repo, let me take a stab at splitting once I have that working (it will be a good proof of concept that the general idea of 'escalate' + 'bind' maps well).

Cynocracy · 2020-03-06T23:21:14Z

/retest

Cynocracy · 2020-03-06T23:40:23Z

{"level":"info","ts":"2020-03-06T23:30:27.273Z","logger":"serving-operator.knativeserving-controller.event-broadcaster","caller":"record/event.go:274","msg":"Event(v1.ObjectReference{Kind:"KnativeServing", Namespace:"knative-serving", Name:"knative-serving", UID:"cbacee1e-5ed9-4bac-a8ba-d65b0ba9225a", APIVersion:"operator.knative.dev/v1alpha1", ResourceVersion:"2288", FieldPath:""}): type: 'Warning' reason: 'InternalError' serviceaccounts "controller" is forbidden: User "system:serviceaccount:default:knative-serving-operator" cannot get resource "serviceaccounts" in API group "" in the namespace "knative-serving"","commit":"1419ff1","knative.dev/controller":"knativeserving-controller"}

Cynocracy · 2020-03-06T23:42:23Z

Wait.. it should have * * * in knative-serving.

trshafer · 2020-03-09T17:00:37Z

Wait.. it should have * * * in knative-serving.

Are there more error messages or does it need GET serviceaccounts in knative-serving?

Cynocracy · 2020-03-12T19:19:57Z

I believe the issue has been fixed (the issue was that the role and the rolebinding need to be in the relevant namespace, and the subjects needed to point to the default namespace).

This opens up a question I need to resolve though: do we need to support multiple KS installations in the same cluster, or can we assume the default namespace is used?

Cynocracy · 2020-03-12T19:53:55Z

OK, the error this time: deleting fails, because we delete the role that gives us the ability to delete 'admissionregistration.k8s.io' resources before we delete those resources.

Going to take a stab at saving the deletion of the roles/rolebindings until last as we need to 'unbootstrap' ourselves from having these permissions.

This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in knative#291 is approved, this will become necessary. Additionally, it makes sense to remove roles as the final step to allow human Operators to modify any resources they may have permissions on as a result of the Knative installation (that is, we should not remove any access until we are 'almost done' cleaning up).

* Delete roles and rolebindings as the final step of cleanup. This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in #291 is approved, this will become necessary. Additionally, it makes sense to remove roles as the final step to allow human Operators to modify any resources they may have permissions on as a result of the Knative installation (that is, we should not remove any access until we are 'almost done' cleaning up). * Gofmt * More golang idiomatic var names. * Remove redundant All()

Cynocracy · 2020-03-16T23:55:12Z

/retest

Cynocracy · 2020-03-17T01:03:23Z

/assign @jcrossley3

Cynocracy · 2020-03-17T01:16:40Z

WIP as eventing change still is, and I'd like these to go in concurrently if at all.

Cynocracy · 2020-03-17T01:16:49Z

/unassign @jcrossley3

This order, Roles -> RoleBindings -> The Rest, prevents the operand from needing to structure their manifest such that this ordering exists, while allowing the Operator to use the bootstrapping mechanism described in knative#291 to 'escalate' itself into management of the Knative Serving installation without a *** clusterrole.

Cynocracy · 2020-03-25T05:01:32Z

/assign @jcrossley3

Assuming my last minute diff-tweaking didn't break this, this should be ready to review :)

The only remaining diff between the role here and in eventing are the replacement of some labels,
a few explicit 'bind's (which now need to be added to roles the Operator binds to but
does not create), and the resource deletion section at the end.

Cynocracy · 2020-03-25T05:05:25Z

One more comment re: the last deltas.

In the future, if we move to a job based cleanup, deletion roles can be defined one-off.
If the Operator repos and/or controllers are to merge, this dramatically simplifies the collapse of
the roles needed.

* Install from the manifest in a strict order. This order, Roles -> RoleBindings -> The Rest, prevents the operand from needing to structure their manifest such that this ordering exists, while allowing the Operator to use the bootstrapping mechanism described in #291 to 'escalate' itself into management of the Knative Serving installation without a *** clusterrole. * Fixing golint * Just update the manifest as a final step.

By using the escalate verb on clusterroles, we allow the Operator to have the ability to create any clusterrole. This prevents us from needing to grant it cluster-admin. Additionally, by having the Operator create an aggregated clusterrole for itself (aggregating all clusteroles created by Knative Serving), we pick up any permissions that the upstream dictate as necessary by any component. Any roles that are not created by upstream, but which are bound as part of install are added with the bind verb to allow the Operator to bind them without necessarily having the permissions they grant. Some other permissions are needed to support controller infra and are explicitly included, as well as some specific to the Operator itself. An attempt at grouping these by sections is included, though may be slightly out of sync (in particular: Controller infra / Specific to this Operator may overlap). Overall, this should keep the Operator roughly operating only on Knative Serving resources (or things which can be operated on by upstream). This also minimizes the diff between eventing and serving RBAC.

Cynocracy · 2020-03-25T20:56:35Z

Commits now squashed (squished?) into a single one :)

Thanks for the tip jcrossley3

jcrossley3 · 2020-03-26T01:10:33Z

Runs fine on OpenShift, so...
/lgtm

Cynocracy · 2020-03-27T20:12:10Z

To double check the diff, I did

ko apply config
kubectl api-resources -oname | xargs -I% kubectl get -A % -oname | sort | uniq > /tmp/before
Create the KS resource
Wait for it to finish
Delete the KS resource
kubectl api-resources -oname | xargs -I% kubectl get -A % -oname | sort | uniq > /tmp/after
7 diff /tmp/before /tmp/after

Which spat out

https://pastebin.com/tugjzdeU

The only oddness is the activator pod, which seems to be hanging around.

Cynocracy · 2020-03-27T23:20:05Z

https://pastebin.com/bjuPParB

Before, no activator leak, curious.

Cynocracy · 2020-03-28T01:16:58Z

A second attempt at a diff produced https://pastebin.com/N0w4R9QZ

So it's entirely possible I generated the last one while the pod was going down (I only diff the names, so who knows :)

Either way, this is ready for an approval, IMO.

trshafer · 2020-03-30T17:07:03Z

/approve

knative-prow-robot · 2020-03-30T17:07:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cynocracy, trshafer

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:

~~OWNERS~~ [trshafer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot added the do-not-merge/work-in-progress label Feb 11, 2020

googlebot added the cla: yes Author(s) signed a CLA. label Feb 11, 2020

mattmoor reviewed Feb 11, 2020

View reviewed changes

knative-prow-robot added the size/L label Feb 11, 2020

knative-prow-robot requested review from jcrossley3 and trshafer February 11, 2020 22:15

trshafer reviewed Feb 13, 2020

View reviewed changes

config/role.yaml Show resolved Hide resolved

config/role.yaml Show resolved Hide resolved

config/role.yaml Show resolved Hide resolved

knative-prow-robot added needs-rebase and removed needs-rebase labels Mar 5, 2020

Cynocracy mentioned this pull request Mar 12, 2020

Delete (Cluster)Role(Bindings) as a final cleanup step. #359

Merged

Cynocracy changed the title ~~[WIP] Restrict RBAC for Operator~~ Restrict RBAC for Operator Mar 17, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Mar 17, 2020

knative-prow-robot assigned jcrossley3 Mar 17, 2020

Cynocracy changed the title ~~Restrict RBAC for Operator~~ [WIP] Restrict RBAC for Operator Mar 17, 2020

knative-prow-robot added the do-not-merge/work-in-progress label Mar 17, 2020

knative-prow-robot unassigned jcrossley3 Mar 17, 2020

Cynocracy mentioned this pull request Mar 25, 2020

Install from the manifest in a strict(er) order. #374

Merged

Cynocracy changed the title ~~[WIP] Restrict RBAC for Operator~~ Restrict RBAC for Operator Mar 25, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Mar 25, 2020

knative-prow-robot assigned jcrossley3 Mar 25, 2020

Cynocracy force-pushed the restrict-rbac branch from baad931 to 774fe9f Compare March 25, 2020 20:56

knative-prow-robot added the lgtm label Mar 26, 2020

knative-prow-robot added the approved label Mar 30, 2020

knative-prow-robot merged commit 2e3db9d into knative:master Mar 30, 2020

Cynocracy deleted the restrict-rbac branch March 30, 2020 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict RBAC for Operator #291

Restrict RBAC for Operator #291

Cynocracy commented Feb 11, 2020 •

edited

Loading

Cynocracy commented Feb 11, 2020

Cynocracy commented Feb 12, 2020

Cynocracy commented Feb 12, 2020

Cynocracy commented Feb 13, 2020

Cynocracy commented Feb 13, 2020

Cynocracy commented Feb 13, 2020

trshafer left a comment

Cynocracy commented Feb 13, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

trshafer commented Mar 9, 2020

Cynocracy commented Mar 12, 2020

Cynocracy commented Mar 12, 2020

Cynocracy commented Mar 16, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

jcrossley3 commented Mar 26, 2020

Cynocracy commented Mar 27, 2020

Cynocracy commented Mar 27, 2020

Cynocracy commented Mar 28, 2020

trshafer commented Mar 30, 2020

knative-prow-robot commented Mar 30, 2020

Restrict RBAC for Operator #291

Restrict RBAC for Operator #291

Conversation

Cynocracy commented Feb 11, 2020 • edited Loading

Proposed Changes

Cynocracy commented Feb 11, 2020

Cynocracy commented Feb 12, 2020

Cynocracy commented Feb 12, 2020

Cynocracy commented Feb 13, 2020

Cynocracy commented Feb 13, 2020

Cynocracy commented Feb 13, 2020

trshafer left a comment

Choose a reason for hiding this comment

Cynocracy commented Feb 13, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

trshafer commented Mar 9, 2020

Cynocracy commented Mar 12, 2020

Cynocracy commented Mar 12, 2020

Cynocracy commented Mar 16, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 17, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

jcrossley3 commented Mar 26, 2020

Cynocracy commented Mar 27, 2020

Cynocracy commented Mar 27, 2020

Cynocracy commented Mar 28, 2020

trshafer commented Mar 30, 2020

knative-prow-robot commented Mar 30, 2020

Cynocracy commented Feb 11, 2020 •

edited

Loading