Filter ClusterRole and ClusterRoleBindings on our labels by runcom · Pull Request #817 · openshift/machine-config-operator

runcom · 2019-06-05T10:47:38Z

This is a subset of #815 and #813 for ClusterRole and ClusterRoleBindings

manifests: remove namespace from ClusterRole/ClusterRoleBinding …
ClusterRole(s) and ClusterRoleBinding(s) aren't namespaced anyway, see
https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole

Signed-off-by: Antonio Murdaca runcom@linux.com

pkg/operator: use filters for ClusterRole/Bindings informers …
Since ClusterRole and ClusterRoleBindings aren't namespaced, we're
triggering a resync for every ClusterRole/Binding that don't pertain us.
Use a custom filtered informers for objects with specific MCO labels.

Signed-off-by: Antonio Murdaca runcom@linux.com

ClusterRole(s) and ClusterRoleBinding(s) aren't namespaced anyway, see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole Signed-off-by: Antonio Murdaca <runcom@linux.com>

Since ClusterRole and ClusterRoleBindings aren't namespaced, we're triggering a resync for every ClusterRole/Binding that don't pertain us. Use a custom filtered informers for objects with specific MCO labels. Signed-off-by: Antonio Murdaca <runcom@linux.com>

openshift-ci-robot · 2019-06-05T10:48:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [runcom]

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

runcom · 2019-06-05T10:54:07Z

+		}
+		opts.LabelSelector = labels.Merge(ls, map[string]string{"mco-built-in/cluster-role": "", "mco-built-in/cluster-role-binding": ""}).String()
+	}
+	clusterRoleAndBindingsInformer := informers.NewFilteredSharedInformerFactory(kubeClient, resyncPeriod()(), targetNamespace, filterClusterRoleAndClusterRoleBindings)


I might just name this mcoBuiltInInformer and we keep adding labels to the map above when we need specific objects

runcom · 2019-06-05T11:02:11Z

/hold

cause testing and fixing and all 🎉

runcom · 2019-06-05T11:26:00Z

  name: machine-config-controller
-  namespace: {{.TargetNamespace}}
+  labels:
+    "mco-built-in/cluster-role": ""


this must be in the form <component>.openshift.io/<functionality> (mco.openshift.io/something-something)

A lot of other operators are going to be installing roles too right? I wonder if we should have a standard annotation for this like openshift.io/operator-managed: ""?

we could yeah, Maceij suggested the above as a common standard pattern to use for labels but I'm open to a more generalized way as well

I think we can agree on mco.openshift.io/managed?

I think we can agree on mco.openshift.io/managed?

I like this label. Much clearer than the previous label.
Anyone have any opposing thoughts?

I think most operators have the CVO create the resources for their operators (CRDs, RBAC, namespaces, etc) directly and doesn't need any sort of label for filtering or ownership designations. Is there a reason the MCO is different and has to do it itself rather than using the CVO?

I did a quick git log install/ which turned up:

commit 3792c944f2ced1be4fe8c2758f390d097a0b7927 Author: Abhinav Dahiya <abhinav.dahiya@redhat.com> AuthorDate: Tue Aug 7 12:42:10 2018 -0700 Commit: Abhinav Dahiya <abhinav.dahiya@redhat.com> CommitDate: Mon Aug 13 11:32:07 2018 -0700 *: add operator sync and manifests manifests/ -> are all the object that operator continuously manages. install/ -> are all the objects that need to be installed but not maned by the operator.

Which...hmm, is some sort of hint? I think part of the idea is that we may need to specially handle upgrades for some of our objects?

I am pretty sure there's a real reason for this but it's escaping me at the moment...

I think part of the idea is that we may need to specially handle upgrades for some of our objects?

Yes, CVO is designed to create and manage objects that are required for the operator to run correctly and not any of it's operands. So it made most sense to control objects so that MCO can at late stage perform migrations for these objects during upgrade...

Based on this I'd personally lean towards a generic openshift.io/operator-managed: "" and try to propagate that to other codebases as well?

Alrighty, I like it and it makes total sense, let's go for it, I'll change this and Steve can change the other PR accordingly

runcom · 2019-06-05T11:26:21Z

+		}
+		opts.LabelSelector = labels.Merge(ls, map[string]string{"mco-built-in/cluster-role": "", "mco-built-in/cluster-role-binding": ""}).String()
+	}
+	clusterRoleAndBindingsInformer := informers.NewFilteredSharedInformerFactory(kubeClient, resyncPeriod()(), targetNamespace, filterClusterRoleAndClusterRoleBindings)


NewSharedInformerFactoryWithOptions instead of NewFilteredSharedInformerFactory

Signed-off-by: Antonio Murdaca <runcom@linux.com>

cgwalters · 2019-06-05T16:02:06Z

LGTM pending resolution of the label.

kikisdeliveryservice · 2019-06-13T16:15:47Z

/skip

openshift-ci-robot · 2019-07-23T22:00:29Z

@runcom: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2019-11-30T13:31:07Z

@runcom: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-op	`b245f1a`	link	`/test e2e-aws-op`
ci/prow/e2e-aws	`b245f1a`	link	`/test e2e-aws`
ci/prow/e2e-etcd-quorum-loss	`b245f1a`	link	`/test e2e-etcd-quorum-loss`
ci/prow/e2e-aws-disruptive	`b245f1a`	link	`/test e2e-aws-disruptive`
ci/prow/e2e-gcp-op	`b245f1a`	link	`/test e2e-gcp-op`
ci/prow/e2e-aws-upgrade	`b245f1a`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-gcp-upgrade	`b245f1a`	link	`/test e2e-gcp-upgrade`
ci/prow/e2e-vsphere	`b245f1a`	link	`/test e2e-vsphere`
ci/prow/build-rpms-from-tar	`b245f1a`	link	`/test build-rpms-from-tar`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kikisdeliveryservice · 2019-12-03T22:46:06Z

In an effort to clean up the MCO repo, closing old open PRs with no recent activity.

Feel free to reopen.

runcom added 2 commits June 5, 2019 12:22

manifests: remove namespace from ClusterRole/ClusterRoleBinding

60b6f06

ClusterRole(s) and ClusterRoleBinding(s) aren't namespaced anyway, see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole Signed-off-by: Antonio Murdaca <runcom@linux.com>

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 5, 2019

openshift-ci-robot requested review from LorbusChris and kikisdeliveryservice June 5, 2019 10:47

runcom changed the title ~~Nsed stuff~~ Filter ClusterRole and ClusterRoleBindings on our labels Jun 5, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019

runcom assigned sgreene570 Jun 5, 2019

runcom commented Jun 5, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2019

runcom commented Jun 5, 2019

View reviewed changes

x

b245f1a

Signed-off-by: Antonio Murdaca <runcom@linux.com>

sgreene570 mentioned this pull request Jun 6, 2019

pkg/operator Filter out non-MCO CRDs when re-syncing modified MCO CRDs #832

Merged

runcom mentioned this pull request Jun 24, 2019

WIP: Use resource{apply,merge,read} functions provided by library-go where possible #829

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2019

sgreene570 mentioned this pull request Aug 13, 2019

[4.3] pkg/daemon: MCD constants fixup #1058

Merged

kikisdeliveryservice added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 3, 2019

kikisdeliveryservice closed this Dec 3, 2019

Conversation

runcom commented Jun 5, 2019

Uh oh!

openshift-ci-robot commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Jun 5, 2019

Uh oh!

kikisdeliveryservice commented Jun 13, 2019

Uh oh!

openshift-ci-robot commented Jul 23, 2019

Uh oh!

openshift-ci-robot commented Nov 30, 2019

Uh oh!

kikisdeliveryservice commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants