Bug 1863011: Fix checking if ClusterRoleBindings need update #932

paulfantom · 2020-09-17T07:20:19Z

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

Fixing CRB re-creation checks by using the same logic as we have for RoleBindings.

openshift-ci-robot · 2020-09-17T07:20:29Z

@paulfantom: This pull request references Bugzilla bug 1863011, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

WIP: Bug 1863011: Do not use annotations for detecting if resource needs to be reconciled

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 · 2020-09-17T07:21:13Z

@paulfantom: This pull request references Bugzilla bug 1863011, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

WIP: Bug 1863011: Prevent unnecessary recreations of ClusterRoleBindings

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 · 2020-09-17T07:50:55Z

@paulfantom: This pull request references Bugzilla bug 1863011, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

WIP: Bug 1863011: Do not compare annotations to detect if recreation is needed

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.

s-urbaniak · 2020-09-17T09:06:22Z

pkg/client/client.go

@@ -1106,7 +1105,6 @@ func (c *Client) CreateOrUpdateClusterRoleBinding(crb *rbacv1.ClusterRoleBinding
 	changed := reflect.DeepEqual(crb.RoleRef, existing.RoleRef)
 	changed = changed || reflect.DeepEqual(crb.Subjects, existing.Subjects)
 	changed = changed || reflect.DeepEqual(crb.Labels, existing.Labels)
-	changed = changed || reflect.DeepEqual(crb.Annotations, existing.Annotations)


I believe we have to be a little smarter. If we just ignore the annotations then it is fine, but I think we have to preserve the existing annotations of the object that was found, aka crb.Annotations = existing.Annotations.

cc @sttts @p0lyn0mial for verification

you could "merge" the annotation map instead of ignoring it.

please have a look at https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/rbac.go#L58 this is what we use in our operators.

Either merging, or restrict deep equal to the annotation "subdomain" you own, e.g. monitoring.openshift.io. But you have to allow everything else without wiping it, e.g. important.customer.com/foo: bar. It's allowed for every actor to put annotations.

@p0lyn0mial in case of merging annotations, should we follow the same pattern you have of merging whole ObjectMeta? Is there any benefit to such approach?

We don't merge the whole ObjectMeta, just only a few things

func EnsureObjectMeta(modified *bool, existing *metav1.ObjectMeta, required metav1.ObjectMeta) { SetStringIfSet(modified, &existing.Namespace, required.Namespace) SetStringIfSet(modified, &existing.Name, required.Name) MergeMap(modified, &existing.Labels, required.Labels) MergeMap(modified, &existing.Annotations, required.Annotations) }

paulfantom · 2020-09-17T09:24:40Z

Seems like we had an incorrect logic for checking if CRB needs a change. The last commit fixes it by porting logic used in RB to do the same

openshift-ci-robot · 2020-09-17T09:26:06Z

@paulfantom: This pull request references Bugzilla bug 1863011, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1863011: Do not compare annotations to detect if recreation is needed

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.

pkg/client/client.go

openshift-ci-robot · 2020-09-17T11:24:19Z

@paulfantom: This pull request references Bugzilla bug 1863011, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1863011: Fix checking if ClusterRoleBindings need update

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.

paulfantom · 2020-09-17T11:29:15Z

Fixing the annotation issue will be done as part of https://bugzilla.redhat.com/show_bug.cgi?id=1879930

pkg/client/client_test.go

simonpasquier · 2020-09-23T16:06:36Z

Fixing CRB re-creation checks by using the same logic as we have for RoleBindings.

Looking at the code it seems that CreateOrUpdateRoleBinding should be updated too?

cluster-monitoring-operator/pkg/client/client.go

Lines 1056 to 1061 in 40bb0e8

    
           if reflect.DeepEqual(rb.RoleRef, r.RoleRef) && 
        
           	reflect.DeepEqual(rb.Subjects, r.Subjects) && 
        
           	reflect.DeepEqual(rb.Labels, r.Labels) && 
        
           	reflect.DeepEqual(rb.Annotations, r.Annotations) { 
        
           	return nil 
        
           }

paulfantom · 2020-09-24T06:34:41Z

Looking at the code it seems that CreateOrUpdateRoleBinding should be updated too?

Yes, but we also have another BZ to fix this for all resources: https://bugzilla.redhat.com/show_bug.cgi?id=1879930.

That said I updated CreateOrUpdateRoleBindings and rebased.

pkg/client/client_test.go

sttts · 2020-09-24T08:40:47Z

pkg/client/client.go

+	// TODO(paulfantom): currently, we don't set our own annotations for cluster role bindings.
+	// This needs a more elaborated merging logic once we do.
+	// See discussion from https://github.com/openshift/cluster-monitoring-operator/pull/932#discussion_r490089300
+	rb.Annotations = r.Annotations


what happens when you set your own annoations? Having a time bomb known to fail without unit tests or other means to tell is not a good idea.

isn't annotation merge logic simple enough to just do it here rather than adding two TODOs plus those "means" I mention above? Seems like time well spend to make it correct once and for all.

We are aware and I am working on a fix for this in a follow-up BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1879930. Fix would be applied to all resources, not only CRB.

My 2 cents: if the merging logic is not too much effort, I'd pull it in as @sttts suggests. especially given the fact that 4.6 is going to be an LTS release.

@paulfantom do you estimate how much effort it would mean to pull in merging for cluster-role-bindings/role-bindings only? I agree that tackling all the other resources can be done separately as they don't seem to suffer the issue we're observing here.

ok, I added merging Annotations for RBs and CRBs. Other resources will be done in follow-up PR.

I am re-using github.com/imdario/mergo for merging as we already have it in vendor due to transitive dependency on k8s/client-go.

pkg/client/client_test.go

s-urbaniak · 2020-09-24T10:06:35Z

/lgtm once green

s-urbaniak · 2020-09-24T10:07:24Z

/lgtm

sttts · 2020-09-24T10:05:29Z

pkg/client/client.go

-		reflect.DeepEqual(rb.Subjects, r.Subjects) &&
-		reflect.DeepEqual(rb.Labels, r.Labels) &&
-		reflect.DeepEqual(rb.Annotations, r.Annotations) {
+	mergo.Merge(&rb.Annotations, existing.Annotations)


this won't remove old annotations. Is that ok?

At least this must be godoc'ed on this func.

Also this mutates rb.Annotation.

/hold

Yes, this won't remove old annotations. The goal was to merge old/existing ones with what is defined by the operator.

Since rb and crb is what we hardcode in operator, mutating rb.Annotation is how we can update whole object with added Annotations.

I have added a comment to reflect that.

it is very bad style to have side-effects in such a function, especially when named CreateOrUpdateRoleBinding. None of the upstream client funcs does that for a reason. This func should not divert. It is even dangerious if it does because consumers will not expect it and potentially invalidate objects from a cache.

I agree here, for the caller this is unexpected. @paulfantom let's make sure we address this in the follow-up. An easy way forward would be to create a deep copy of rb.

s-urbaniak · 2020-09-24T12:12:33Z

/test e2e-aws-operator
/hold cancel

s-urbaniak · 2020-09-24T12:14:49Z

/lgtm

openshift-ci-robot · 2020-09-24T12:15:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paulfantom, s-urbaniak

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~~ [paulfantom,s-urbaniak]

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-09-24T15:57:11Z

/retest

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

paulfantom · 2020-09-24T18:40:59Z

/retest

openshift-ci-robot · 2020-09-24T20:18:57Z

@paulfantom: All pull requests linked via external trackers have merged:

openshift/cluster-monitoring-operator#932

Bugzilla bug 1863011 has been moved to the MODIFIED state.

In response to this:

Bug 1863011: Fix checking if ClusterRoleBindings need update

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 requested review from pgier and simonpasquier September 17, 2020 07:20

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

paulfantom changed the title ~~WIP: Bug 1863011: Do not use annotations for detecting if resource needs to be reconciled~~ WIP: Bug 1863011: Prevent unnecessary recreations of ClusterRoleBindings Sep 17, 2020

paulfantom force-pushed the rolebinidings branch 2 times, most recently from b96a7ec to 16813f9 Compare September 17, 2020 07:49

paulfantom changed the title ~~WIP: Bug 1863011: Prevent unnecessary recreations of ClusterRoleBindings~~ WIP: Bug 1863011: Do not compare annotations to detect if reconciliation is needed Sep 17, 2020

paulfantom changed the title ~~WIP: Bug 1863011: Do not compare annotations to detect if reconciliation is needed~~ WIP: Bug 1863011: Do not compare annotations to detect if recreation is needed Sep 17, 2020

s-urbaniak reviewed Sep 17, 2020

View reviewed changes

paulfantom changed the title ~~WIP: Bug 1863011: Do not compare annotations to detect if recreation is needed~~ Bug 1863011: Do not compare annotations to detect if recreation is needed Sep 17, 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 Sep 17, 2020

s-urbaniak reviewed Sep 17, 2020

View reviewed changes

pkg/client/client.go Show resolved Hide resolved

paulfantom force-pushed the rolebinidings branch from aa7b164 to 9229c02 Compare September 17, 2020 11:23

paulfantom changed the title ~~Bug 1863011: Do not compare annotations to detect if recreation is needed~~ Bug 1863011: Fix checking if ClusterRoleBindings need update Sep 17, 2020

paulfantom force-pushed the rolebinidings branch 2 times, most recently from da522f0 to fcef60a Compare September 23, 2020 13:41

simonpasquier reviewed Sep 23, 2020

View reviewed changes

pkg/client/client_test.go Show resolved Hide resolved

paulfantom force-pushed the rolebinidings branch from fcef60a to ca45dd1 Compare September 24, 2020 06:34

s-urbaniak reviewed Sep 24, 2020

View reviewed changes

pkg/client/client_test.go Show resolved Hide resolved

paulfantom force-pushed the rolebinidings branch from ddb43f9 to 9e92f35 Compare September 24, 2020 08:30

sttts reviewed Sep 24, 2020

View reviewed changes

paulfantom force-pushed the rolebinidings branch from 9e92f35 to e3a2030 Compare September 24, 2020 09:54

s-urbaniak reviewed Sep 24, 2020

View reviewed changes

pkg/client/client_test.go Outdated Show resolved Hide resolved

paulfantom force-pushed the rolebinidings branch from e3a2030 to 69d7294 Compare September 24, 2020 10:05

openshift-ci-robot assigned s-urbaniak Sep 24, 2020

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

sttts reviewed Sep 24, 2020

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 Sep 24, 2020

paulfantom added 2 commits September 24, 2020 12:21

pkg/client: invert CRB checks

b280d2a

pkg/client: add tests for checking updates of CRBs

819d487

paulfantom force-pushed the rolebinidings branch from 69d7294 to b4446f3 Compare September 24, 2020 10:21

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2020

vendor: revendor

f65020b

paulfantom force-pushed the rolebinidings branch from b4446f3 to f65020b Compare September 24, 2020 10:25

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2020

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

openshift-merge-robot merged commit eb64b73 into openshift:master Sep 24, 2020

paulfantom deleted the rolebinidings branch September 24, 2020 20:48

paulfantom mentioned this pull request Sep 28, 2020

Bug 1879930: Preserve labels and annotations during reconciliation #942

Merged

2 tasks

paulfantom mentioned this pull request Oct 12, 2020

Bug 1887354: Backport fix checking if ClusterRoleBindings need update #956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1863011: Fix checking if ClusterRoleBindings need update #932

Bug 1863011: Fix checking if ClusterRoleBindings need update #932

paulfantom commented Sep 17, 2020 •

edited

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

s-urbaniak Sep 17, 2020

s-urbaniak Sep 17, 2020

p0lyn0mial Sep 17, 2020

sttts Sep 17, 2020 •

edited

paulfantom Sep 17, 2020

p0lyn0mial Sep 17, 2020

paulfantom commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

paulfantom commented Sep 17, 2020

simonpasquier commented Sep 23, 2020

paulfantom commented Sep 24, 2020

sttts Sep 24, 2020 •

edited

sttts Sep 24, 2020

paulfantom Sep 24, 2020

s-urbaniak Sep 24, 2020 •

edited

paulfantom Sep 24, 2020 •

edited

s-urbaniak commented Sep 24, 2020

s-urbaniak commented Sep 24, 2020

sttts Sep 24, 2020

sttts Sep 24, 2020

paulfantom Sep 24, 2020

sttts Sep 24, 2020

s-urbaniak Sep 25, 2020

s-urbaniak commented Sep 24, 2020

s-urbaniak commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

openshift-bot commented Sep 24, 2020

paulfantom commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

Bug 1863011: Fix checking if ClusterRoleBindings need update #932

Bug 1863011: Fix checking if ClusterRoleBindings need update #932

Conversation

paulfantom commented Sep 17, 2020 • edited

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfantom commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

openshift-ci-robot commented Sep 17, 2020

paulfantom commented Sep 17, 2020

simonpasquier commented Sep 23, 2020

paulfantom commented Sep 24, 2020

sttts Sep 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-urbaniak Sep 24, 2020 • edited

Choose a reason for hiding this comment

paulfantom Sep 24, 2020 • edited

Choose a reason for hiding this comment

s-urbaniak commented Sep 24, 2020

s-urbaniak commented Sep 24, 2020

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

s-urbaniak commented Sep 24, 2020

s-urbaniak commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

openshift-bot commented Sep 24, 2020

paulfantom commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

paulfantom commented Sep 17, 2020 •

edited

sttts Sep 17, 2020 •

edited

sttts Sep 24, 2020 •

edited

s-urbaniak Sep 24, 2020 •

edited

paulfantom Sep 24, 2020 •

edited