clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin #1219

abhinavdahiya · 2020-11-25T05:47:32Z

xref: https://issues.redhat.com/browse/CO-1282

Any role bindings in the namespace of the clusterpool that refer the ClusterRole hive-cluster-pool-admin
will be used to provide the subjects the same permission in the namespaces created for various
clusterdeployments for the clusterpool.

The controller performs these additional actions for each reconcile,

lists all the rolebindings in clusterpool's namespace that refer the ClusterRole hive-cluster-pool-admin
to collect all the subjects.
This makes sure that no work needs to be done when there are such rolebindings. And since k8s ensures that a rolebinding
cannot be created when the ref doesn't exist, this will ensure that only when the ClusterRole exists would the controller
run the next steps.
lists all the namespaces attached to the clusterpool by using the constants.ClusterPoolNameLabel label selector.
creates/updates a rolebinding hive-cluster-pool-admin-binding in each namespace binding to the same ClusterRole.

The controller also adds new watchers,

changes to rolebindings in the clusterdeployment namespace
changes to any rolebindings with ref to ClusterRole hive-cluster-pool-admin triggers resync for all clusterpools
in that namespace.

NOTE: the controller can only pass on the permissions that it has i.e. the current hive-cluster-pool-admin cluster role is a subset of the hive-controller's permission. If the controller permissions are less that what the hive-cluster-pool-admin cluster role, then when creating a rolebinding in the clusterdeployment namespace we see an error like

time="2020-11-30T21:21:11.129Z" level=error msg="could not create rolebinding" clusterPool=default/adahiya-pool-1 controller=clusterpool error="rolebindings.rbac.authorization.k8s.io \"hive-cluster-pool-admin-binding\" is forbidden: user \"system:serviceaccount:hive:hive-controllers\" (groups=[\"system:serviceaccounts\" \"system:serviceaccounts:hive\" \"system:authenticated\"]) is attempting to grant RBAC permissions not currently held:\n{APIGroups:[\"\"], Resources:[\"pods/log\"], Verbs:[\"get\" \"list\" \"watch\"]}\n{APIGroups:[\"apiextensions.k8s.io\"], Resources:[\"customresourcedefinitions\"], Verbs:[\"get\" \"list\" \"watch\"]}" reconcileID=gn7pt9sd rolebinding=adahiya-pool-1-kpkbg/hive-cluster-pool-admin-binding

dgoodwin

Looks awesome.

In config/rbac/ you'll see we ship a few utility roles already, you just need to add them to the list of things to apply likely here: https://github.com/openshift/hive/blob/master/pkg/operator/hive/hive.go#L271. The hive_frontend_role.yaml is a good starting point, that's almost what we want here, just bound to a namespace and maybe with less write perms.

This would imply we fully control that role definition and users can't modify it. Does this seem good or should we let them override the role name in HiveConfig, which would then pass that as an env var to the controllers deployment?

dgoodwin · 2020-11-25T12:12:54Z

pkg/controller/clusterpool/clusterpool_controller.go

+		if o.Meta.GetName() != resourceName {
+			return nil
+		}
+		clusterName := o.Meta.GetNamespace()


I hope this ClusterDeployment.Name == ClusterDeployment.Namespace assumption holds up over time, it's convenient here.

pkg/controller/clusterpool/clusterpool_controller.go

dgoodwin · 2020-11-25T12:37:47Z

pkg/controller/clusterpool/clusterpool_controller.go

+	observed.Subjects = desired.Subjects
+	observed.RoleRef = desired.RoleRef
+
+	logger.Info("updating rolebinding")


Please also log the subjects and roleref.

hmm, any reason why that would be useful here?

This would be to log an audit trail of what your code determined the desired subjects and role reference is. I'd actually be tempted to log the observed vs desired as well. This is to help debug in a world where external modifications and hotloops can happen.

done with d851c86

dgoodwin · 2020-11-25T12:39:06Z

pkg/controller/clusterpool/clusterpool_controller_test.go

+		name:     "no binding referring to cluster role",
+		existing: []runtime.Object{},
+	}, {
+		name: "no binding referring to cluster role",


name is duplicated with the test above.

the name is same because technically it's testing the same thing but just a different permutation really..

go test -v ./pkg/controller/clusterpool/... -run "TestReconcileRBAC/no_binding_referring_to_cluster_role" === RUN TestReconcileRBAC === RUN TestReconcileRBAC/no_binding_referring_to_cluster_role === RUN TestReconcileRBAC/no_binding_referring_to_cluster_role#01 --- PASS: TestReconcileRBAC (0.00s) --- PASS: TestReconcileRBAC/no_binding_referring_to_cluster_role (0.00s) --- PASS: TestReconcileRBAC/no_binding_referring_to_cluster_role#01 (0.00s) PASS ok github.com/openshift/hive/pkg/controller/clusterpool 0.031s

and go test automatically adds number to the same test name.

should i add one explicitly here?

Peculiar, we don't do this anywhere today afaik and typically try to give the tests unique names, this could get a little funky for anyone looking to just go run -test for a specific test, but probably not a huge deal. I think my vote would still be to give them unique names that cover the intent though.

dgoodwin · 2020-11-25T12:39:42Z

pkg/controller/clusterpool/clusterpool_controller_test.go

+			},
+		},
+	}, {
+		name: "binding referring to cluster role but no namespace for pool",


Several more dupe test names here.

dgoodwin · 2020-11-25T12:41:16Z

pkg/controller/clusterpool/clusterpool_controller_test.go

+			},
+		},
+	}, {
+		name: "multiple binding referring to cluster role with one namespace for pool",


dgoodwin · 2020-11-25T12:41:53Z

pkg/controller/clusterpool/clusterpool_controller_test.go

+			},
+		},
+	}, {
+		name: "pre existing role bindings that are different",


Dupe test name.

abhinavdahiya · 2020-11-30T21:50:39Z

did some manual testing..

add a rolebinding in the namespace of the clusterpool, see the rolebinding created in the namespace of the cd..

$ oc get rolebindings hive-cluster-pool-admin -oyaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: "2020-11-30T21:20:28Z"
  managedFields:
  - apiVersion: rbac.authorization.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:roleRef:
        f:apiGroup: {}
        f:kind: {}
        f:name: {}
      f:subjects: {}
    manager: oc
    operation: Update
    time: "2020-11-30T21:20:28Z"
  name: hive-cluster-pool-admin
  namespace: default
  resourceVersion: "40762"
  selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/default/rolebindings/hive-cluster-pool-admin
  uid: a0dec8ba-411a-48ee-92f3-432701d96655
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: hive-cluster-pool-admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: abhinavdahiya
$ oc get rolebindings hive-cluster-pool-admin-binding -n adahiya-pool-1-kpkbg -oyaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: "2020-11-30T21:40:23Z"
  managedFields:
  - apiVersion: rbac.authorization.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:roleRef:
        f:apiGroup: {}
        f:kind: {}
        f:name: {}
      f:subjects: {}
    manager: manager
    operation: Update
    time: "2020-11-30T21:40:22Z"
  name: hive-cluster-pool-admin-binding
  namespace: adahiya-pool-1-kpkbg
  resourceVersion: "46557"
  selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/adahiya-pool-1-kpkbg/rolebindings/hive-cluster-pool-admin-binding
  uid: fa3e9bfa-56bb-4938-bdbe-051108da68c6
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: hive-cluster-pool-admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: abhinavdahiya
➜  hive git:(cluster_pool_admin) oc adm policy add-role-to-group hive-cluster-pool-admin hive-team
Warning: Group 'hive-team' not found
clusterrole.rbac.authorization.k8s.io/hive-cluster-pool-admin added: "hive-team"
➜  hive git:(cluster_pool_admin) oc get rolebindings hive-cluster-pool-admin -oyaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: "2020-11-30T21:20:28Z"
  managedFields:
  - apiVersion: rbac.authorization.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:roleRef:
        f:apiGroup: {}
        f:kind: {}
        f:name: {}
      f:subjects: {}
    manager: oc
    operation: Update
    time: "2020-11-30T21:20:28Z"
  name: hive-cluster-pool-admin
  namespace: default
  resourceVersion: "40762"
  selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/default/rolebindings/hive-cluster-pool-admin
  uid: a0dec8ba-411a-48ee-92f3-432701d96655
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: hive-cluster-pool-admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: abhinavdahiya

Add say a group to rolebinding in the same namespace, it is synced to the clusterdeployment rolebinding.

$ oc adm policy add-role-to-group hive-cluster-pool-admin hive-team
Warning: Group 'hive-team' not found
clusterrole.rbac.authorization.k8s.io/hive-cluster-pool-admin added: "hive-team"
$ oc get rolebindings hive-cluster-pool-admin hive-cluster-pool-admin-0 -oyaml
apiVersion: v1
items:
- apiVersion: rbac.authorization.k8s.io/v1
  kind: RoleBinding
  metadata:
    creationTimestamp: "2020-11-30T21:20:28Z"
    managedFields:
    - apiVersion: rbac.authorization.k8s.io/v1
      fieldsType: FieldsV1
      fieldsV1:
        f:roleRef:
          f:apiGroup: {}
          f:kind: {}
          f:name: {}
        f:subjects: {}
      manager: oc
      operation: Update
      time: "2020-11-30T21:20:28Z"
    name: hive-cluster-pool-admin
    namespace: default
    resourceVersion: "40762"
    selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/default/rolebindings/hive-cluster-pool-admin
    uid: a0dec8ba-411a-48ee-92f3-432701d96655
  roleRef:
    apiGroup: rbac.authorization.k8s.io
    kind: ClusterRole
    name: hive-cluster-pool-admin
  subjects:
  - apiGroup: rbac.authorization.k8s.io
    kind: User
    name: abhinavdahiya
- apiVersion: rbac.authorization.k8s.io/v1
  kind: RoleBinding
  metadata:
    creationTimestamp: "2020-11-30T21:43:37Z"
    managedFields:
    - apiVersion: rbac.authorization.k8s.io/v1
      fieldsType: FieldsV1
      fieldsV1:
        f:roleRef:
          f:apiGroup: {}
          f:kind: {}
          f:name: {}
        f:subjects: {}
      manager: oc
      operation: Update
      time: "2020-11-30T21:43:37Z"
    name: hive-cluster-pool-admin-0
    namespace: default
    resourceVersion: "47433"
    selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/default/rolebindings/hive-cluster-pool-admin-0
    uid: a4351b94-069d-4ff3-9e37-1ac90171ec84
  roleRef:
    apiGroup: rbac.authorization.k8s.io
    kind: ClusterRole
    name: hive-cluster-pool-admin
  subjects:
  - apiGroup: rbac.authorization.k8s.io
    kind: Group
    name: hive-team
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""
$ oc get rolebindings hive-cluster-pool-admin-binding -n adahiya-pool-1-kpkbg -oyaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  creationTimestamp: "2020-11-30T21:40:23Z"
  managedFields:
  - apiVersion: rbac.authorization.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:roleRef:
        f:apiGroup: {}
        f:kind: {}
        f:name: {}
      f:subjects: {}
    manager: manager
    operation: Update
    time: "2020-11-30T21:43:38Z"
  name: hive-cluster-pool-admin-binding
  namespace: adahiya-pool-1-kpkbg
  resourceVersion: "47436"
  selfLink: /apis/rbac.authorization.k8s.io/v1/namespaces/adahiya-pool-1-kpkbg/rolebindings/hive-cluster-pool-admin-binding
  uid: fa3e9bfa-56bb-4938-bdbe-051108da68c6
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: hive-cluster-pool-admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: Group
  name: hive-team
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: abhinavdahiya

abhinavdahiya · 2020-11-30T21:53:59Z

/assign @dgoodwin
/cc @abutcher

dgoodwin

Looks great, just one noteworthy request left, could you write up a short blurb in docs/clusterpools.md covering how to use this? We did decide recently to try to put docs in with the PRs that add the features going forward.

config/controllers/hive_controllers_role.yaml

dgoodwin · 2020-12-01T18:49:29Z

pkg/controller/clusterpool/clusterpool_controller_test.go

+		name:     "no binding referring to cluster role",
+		existing: []runtime.Object{},
+	}, {
+		name: "no binding referring to cluster role",


Peculiar, we don't do this anywhere today afaik and typically try to give the tests unique names, this could get a little funky for anyone looking to just go run -test for a specific test, but probably not a huge deal. I think my vote would still be to give them unique names that cover the intent though.

…ster-pool-admin Any role bindings in the namespace of the clusterpool that refer the ClusterRole `hive-cluster-pool-admin` will be used to provide the subjects the same permission in the namespaces created for various clusterdeployments for the clusterpool. The controller performs these additional actions for each reconcile, - lists all the rolebindings in clusterpool's namespace that refer the ClusterRole `hive-cluster-pool-admin` to collect all the subjects. This makes sure that no work needs to be done when there are such rolebindings. And since k8s ensures that a rolebinding cannot be created when the ref doesn't exist, this will ensure that only when the ClusterRole exists would the controller run the next steps. - lists all the namespaces attached to the clusterpool by using the constants.ClusterPoolNameLabel label selector. - creates/updates a rolebinding `hive-cluster-pool-admin-binding` in each namespace binding to the same ClusterRole. The controller also adds new watchers, - changes to rolebindings in the clusterdeployment namespace - changes to any rolebindings with ref to ClusterRole `hive-cluster-pool-admin` triggers resync for all clusterpools in that namespace.

the hive controller must have the permissions to give that permission in the clusterdeployments namespaces when created for a cluster pool.

dgoodwin · 2020-12-02T13:45:43Z

/lgtm

openshift-ci-robot · 2020-12-02T13:46:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, dgoodwin

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

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

openshift-bot · 2020-12-02T13:59:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

abhinavdahiya · 2020-12-02T18:03:40Z

/retest

abhinavdahiya · 2020-12-02T19:52:42Z

/retest

openshift-bot · 2020-12-03T11:39:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-12-03T20:19:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-12-03T21:37:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2020

openshift-ci-robot requested review from abutcher and akhil-rane November 25, 2020 05:47

abhinavdahiya force-pushed the cluster_pool_admin branch from ccfd747 to 431d823 Compare November 25, 2020 05:57

dgoodwin reviewed Nov 25, 2020

View reviewed changes

abhinavdahiya changed the title ~~WIP clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin~~ clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin Nov 30, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2020

openshift-ci-robot assigned dgoodwin Nov 30, 2020

abhinavdahiya force-pushed the cluster_pool_admin branch 2 times, most recently from 5b33fd1 to fce428f Compare December 1, 2020 18:15

dgoodwin reviewed Dec 1, 2020

View reviewed changes

abhinavdahiya added 3 commits December 1, 2020 11:37

config/rbac: add hive-cluster-pool-admin clusterrole

43ff061

the hive controller must have the permissions to give that permission in the clusterdeployments namespaces when created for a cluster pool.

doc/clusterpools.md: document how to define cluster pool adminswq

b61ee6b

abhinavdahiya force-pushed the cluster_pool_admin branch from fce428f to b61ee6b Compare December 1, 2020 19:53

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020

openshift-merge-robot merged commit 2f59e7c into openshift:master Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin #1219

clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin #1219

abhinavdahiya commented Nov 25, 2020 •

edited

dgoodwin left a comment •

edited

dgoodwin Nov 25, 2020

dgoodwin Nov 25, 2020

abhinavdahiya Nov 30, 2020

dgoodwin Dec 1, 2020

abhinavdahiya Dec 1, 2020

dgoodwin Nov 25, 2020

abhinavdahiya Dec 1, 2020

dgoodwin Dec 1, 2020

dgoodwin Nov 25, 2020

dgoodwin Nov 25, 2020

dgoodwin Nov 25, 2020

abhinavdahiya commented Nov 30, 2020

abhinavdahiya commented Nov 30, 2020

dgoodwin left a comment

dgoodwin Dec 1, 2020

dgoodwin commented Dec 2, 2020

openshift-ci-robot commented Dec 2, 2020

openshift-bot commented Dec 2, 2020

abhinavdahiya commented Dec 2, 2020

abhinavdahiya commented Dec 2, 2020

openshift-bot commented Dec 3, 2020

openshift-bot commented Dec 3, 2020

openshift-bot commented Dec 3, 2020

clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin #1219

clusterpool: ensure clusterdeployments namespace has same rbac as cluster-pool-admin #1219

Conversation

abhinavdahiya commented Nov 25, 2020 • edited

dgoodwin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Nov 30, 2020

abhinavdahiya commented Nov 30, 2020

dgoodwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoodwin commented Dec 2, 2020

openshift-ci-robot commented Dec 2, 2020

openshift-bot commented Dec 2, 2020

abhinavdahiya commented Dec 2, 2020

abhinavdahiya commented Dec 2, 2020

openshift-bot commented Dec 3, 2020

openshift-bot commented Dec 3, 2020

openshift-bot commented Dec 3, 2020

abhinavdahiya commented Nov 25, 2020 •

edited

dgoodwin left a comment •

edited