Restrict RBAC for Operator #109

Cynocracy · 2020-02-15T00:48:18Z

Fixes #106

#150 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 Eventing),
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 Eventing resources (or things which can be operated
on by upstream).

Release Note

Change RBAC to minimize permissions of Operator service account.

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 Eventing), 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. Overall, this should keep the Operator roughly operating only on Knative Eventing resources (or things which can be operated on by upstream).

Cynocracy · 2020-02-18T21:25:15Z

This should begin to pass once we regen the eventing YAML

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

/retest

Cynocracy · 2020-03-06T23:39:12Z

{"level":"info","ts":"2020-03-06T23:29:47.135Z","logger":"eventing_operator.knativeeventing-controller.event-broadcaster","caller":"record/event.go:274","msg":"Event(v1.ObjectReference{Kind:"KnativeEventing", Namespace:"operator-tests", Name:"knative-eventing", UID:"cdb13e43-2533-4246-8050-3886920854ef", APIVersion:"operator.knative.dev/v1alpha1", ResourceVersion:"2475", FieldPath:""}): type: 'Warning' reason: 'InternalError' clusterroles.rbac.authorization.k8s.io "source-observer" not found","commit":"025ff25","knative.dev/controller":"knativeeventing-controller"}
{"level":"info","ts":"2020-03-06T23:29:47.135Z","logger":"eventing_operator.knativeeventing-controller","caller":"controller/controller.go:363","msg":"Reconcile failed. Time taken: 3.20852151s.","commit":"025ff25","knative.dev/controller":"knativeeventing-controller","knative.dev/traceid":"26604652-ebba-4703-9fa8-a46d3c00f35a","knative.dev/key":"operator-tests/knative-eventing"}
{"level":"info","ts":"2020-03-06T23:30:28.095Z","logger":"eventing_operator.knativeeventing-controller","caller":"knativeeventing/knativeeventing.go:112","msg":"Reconciling KnativeEventing","commit":"025ff25","knative.dev/controller":"knativeeventing-controller","Request.Namespace":"operator-tests","Request.Name":"knative-eventing","status":{"conditions":[{"type":"InstallSucceeded","status":"Unknown","lastTransitionTime":"2020-03-06T23:28:29Z"},{"type":"Ready","status":"False","lastTransitionTime":"2020-03-06T23:28:36Z","reason":"Manifest Installation","message":"clusterroles.rbac.authorization.k8s.io "source-observer" not found"}]}}
{"level":"error","ts":"2020-03-06T23:30:31.326Z","logger":"eventing_operator.knativeeventing-controller","caller":"controller/controller.go:376","msg":"Reconcile error","commit":"025ff25","knative.dev/controller":"knativeeventing-controller","error":"clusterroles.rbac.authorization.k8s.io "source-observer" not found","stacktrace":"knative.dev/eventing-operator/vendor/knative.dev/pkg/controller.(*Impl).handleErr\n\tknative.dev/eventing-operator/vendor/knative.dev/pkg/controller/controller.go:376\nknative.dev/eventing-operator/vendor/knative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/eventing-operator/vendor/knative.dev/pkg/controller/controller.go:362\nknative.dev/eventing-operator/vendor/knative.dev/pkg/controller.(*Impl).Run.func2\n\tknative.dev/eventing-operator/vendor/knative.dev/pkg/controller/controller.go:310"}

This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in knative#109 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 (Cluster)Role(Bindings) as a final cleanup step. This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in #109 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). * Remove redundant All(), fix conditional.

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

/assign @n3wscott

Cynocracy · 2020-03-17T01:07:53Z

Whoops, big mistake in the existing role. This needs an update and retest.

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

/unassign @n3wscott

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

New issue seems to be the use of the operator-tests namespace, iirc this was dropped for kserving.

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#109 to 'escalate' itself into management of the Knative Eventing installation without a *** clusterrole.

Cynocracy · 2020-03-25T01:23:22Z

Bah humbug, let's see what the problem is.

Cynocracy · 2020-03-25T01:30:05Z

/retest

Cynocracy · 2020-03-25T01:55:39Z

msg: "Event(v1.ObjectReference{Kind:"KnativeEventing", Namespace:"operator-tests", Name:"knative-eventing", UID:"41eff4b4-2aee-478d-99dc-9baf59462f1d", APIVersion:"operator.knative.dev/v1alpha1", ResourceVersion:"2200", FieldPath:""}): type: 'Warning' reason: 'InternalError' serviceaccounts "eventing-controller" is forbidden: User "system:serviceaccount:default:knative-eventing-operator" cannot get resource "serviceaccounts" in API group "" in the namespace "operator-tests""

Aha, right, the namespace thing, maybe I can test it out on a different namespace

Cynocracy · 2020-03-25T02:14:34Z

Very odd that both tests pass locally.

I'm going to tilt at that windmill for a bit.

Cynocracy · 2020-03-25T03:03:44Z

Oh :) That was a silly reason.

…ion.

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

/assign @n3wscott

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

The role defined in role.yaml is a strict subset of that in serving-operator, where in that repo
we have a few 'bind's on roles which are used but not created by the operator (now required)
and a 'cleanup' section where serving operator needs permission to delete older resources it now
routinely cleans up.
In the future, if we move to a job based cleanup, those can be defined as one-offs.

If the Operator repos and/or controllers are to merge, this dramatically simplifies the collapse of
the roles needed.

And knative-eventing namespace to knativeeventings namespaces.

config/role.yaml

n3wscott · 2020-03-25T14:54:13Z

/lgtm
/approve

knative-prow-robot · 2020-03-25T14:54:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [n3wscott]

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

Jon Donovan added 2 commits February 14, 2020 16:44

Merge branch 'master' into restrict-rbac

37601fd

knative-prow-robot added do-not-merge/work-in-progress size/L labels Feb 15, 2020

knative-prow-robot requested review from houshengbo and k4leung4 February 15, 2020 00:48

Cynocracy mentioned this pull request Feb 15, 2020

Create service accounts strictly after roles. knative/eventing#2574

Merged

Cynocracy force-pushed the restrict-rbac branch from b3b5524 to 37601fd Compare February 15, 2020 01:14

Merge branch 'master' into restrict-rbac

dd8fc06

knative-prow-robot added the needs-rebase label Mar 9, 2020

Merge from upstream

013015c

knative-prow-robot removed the needs-rebase label Mar 11, 2020

Jon Donovan added 2 commits March 12, 2020 12:16

Update to correct namespaces for Operator SA and binding.

b8cb470

Readd daemon/replica/stateful/sets.

9bed3cd

Cynocracy mentioned this pull request Mar 12, 2020

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

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 n3wscott 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

Removing 'specific to this operator' and blanket role.

1ab995c

googlebot added the cla: yes label Mar 17, 2020

knative-prow-robot unassigned n3wscott Mar 17, 2020

Cynocracy mentioned this pull request Mar 25, 2020

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

Closed

Jon Donovan added 2 commits March 24, 2020 18:11

Fixing golint

1d77777

Merge branch 'roles-then-bindings' into restrict-rbac

9f1446d

Jon Donovan added 4 commits March 24, 2020 20:29

Role in eventing namespace is no longer required due to role aggregat…

88d29f3

…ion.

TODO suggested improvement.

be230fd

Final update for pretty diff between role in serving and eventing.

0f5453c

Final final update. Add copyright header.

4269e3b

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 n3wscott Mar 25, 2020

Another copyright, serving -> eventing.

3242409

And knative-eventing namespace to knativeeventings namespaces.

n3wscott reviewed Mar 25, 2020

View reviewed changes

config/role.yaml Show resolved Hide resolved

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

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

knative-prow-robot merged commit 44db4eb into knative:master Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict RBAC for Operator #109

Restrict RBAC for Operator #109

Cynocracy commented Feb 15, 2020 •

edited

Loading

Cynocracy commented Feb 18, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 17, 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

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

n3wscott commented Mar 25, 2020

knative-prow-robot commented Mar 25, 2020

Restrict RBAC for Operator #109

Restrict RBAC for Operator #109

Conversation

Cynocracy commented Feb 15, 2020 • edited Loading

Proposed Changes

Cynocracy commented Feb 18, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 6, 2020

Cynocracy commented Mar 17, 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

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

Cynocracy commented Mar 25, 2020

n3wscott commented Mar 25, 2020

knative-prow-robot commented Mar 25, 2020

Cynocracy commented Feb 15, 2020 •

edited

Loading