don't return an error if the singleton rbac doesn't exist #2309

ecordell · 2021-07-30T22:11:23Z

Description of the change:
Doesn't return an error from ensureSingletonRBAC if we can detect that the cache is just a bit behind

Motivation for the change:
Fixes #2091

prior to this change, I could reproduce with:

$ go test ./pkg/controller/operators/olm/... -v -run ^TestSyncOperatorGroups$ -count 10

after this change, I couldn't reproduce with:

$ go test ./pkg/controller/operators/olm/... -v -run ^TestSyncOperatorGroups$ -count 100

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /doc
Commit messages sensible and descriptive

Signed-off-by: Evan <cordell.evan@gmail.com>

timflannagan · 2021-07-30T22:19:04Z

pkg/controller/operators/olm/operatorgroup.go

+				if k8serrors.IsAlreadyExists(err) && ownerutil.IsOwnedByLabel(cr, csv) {
+					continue
+				} else {
+					return err
+				}


nit:

Suggested change

if k8serrors.IsAlreadyExists(err) && ownerutil.IsOwnedByLabel(cr, csv) {

continue

} else {

return err

}

if k8serrors.IsAlreadyExists(err) && ownerutil.IsOwnedByLabel(cr, csv) {

continue

}

return err

timflannagan · 2021-07-30T22:19:34Z

Nice 😎
Holding for another opinion (if needed - feel free to remove) but this looks reasonable.

/lgtm
/hold

dinhxuanvu

/lgtm

dinhxuanvu · 2021-07-31T00:50:02Z

This PR failed one e2e test creation with nodeSelector config.

njhale

I think it's also time to resurrect the flake analyzer

openshift-ci · 2021-07-31T03:42:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, ecordell, njhale

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~~ [ecordell,njhale]

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

njhale · 2021-07-31T03:42:59Z

/lgtm
/hold cancel

…troller Fixes [operator-framework#2091](operator-framework#2091). This is a follow-up to [operator-framework#2309](operator-framework#2309) that attempted to fix the original issue. When checking whether the ClusterRole/ClusterRoleBinding resources already exist, we're also checking whether the existing labels are owned by the CSV we're currently handling. When accessing the "cr" or "crb" variables that the Create calls output, a panic is produced as we're attempting to access the meta.Labels key from those resources, except those resources themselves are nil. Update the check to instead use the static type instead to avoid a panic.

…troller Fixes [operator-framework#2091](operator-framework#2091). This is a follow-up to [operator-framework#2309](operator-framework#2309) that attempted to fix the original issue. When checking whether the ClusterRole/ClusterRoleBinding resources already exist, we're also checking whether the existing labels are owned by the CSV we're currently handling. When accessing the "cr" or "crb" variables that the Create calls output, a panic is produced as we're attempting to access the meta.Labels key from those resources, except those resources themselves are nil. Update the check to instead use the static type instead to avoid a panic. Signed-off-by: timflannagan <timflannagan@gmail.com>

…troller Fixes [operator-framework#2091](operator-framework#2091). This is a follow-up to [operator-framework#2309](operator-framework#2309) that attempted to fix the original issue. When checking whether the ClusterRole/ClusterRoleBinding resources already exist, we're also checking whether the existing labels are owned by the CSV we're currently handling. When accessing the "cr" or "crb" variables that the Create calls output, a panic is produced as we're attempting to access the meta.Labels key from those resources, except those resources themselves are nil. Update the check to verify that the cr/crb variables are not nil before attempting to access those object's labels. The testing fake client may need to be updated in the future to handle returning these resources properly. Signed-off-by: timflannagan <timflannagan@gmail.com>

…troller (#2349) * pkg/controller: Fix panic when creating cluster-scoped RBAC in OG controller Fixes [#2091](#2091). This is a follow-up to [#2309](#2309) that attempted to fix the original issue. When checking whether the ClusterRole/ClusterRoleBinding resources already exist, we're also checking whether the existing labels are owned by the CSV we're currently handling. When accessing the "cr" or "crb" variables that the Create calls output, a panic is produced as we're attempting to access the meta.Labels key from those resources, except those resources themselves are nil. Update the check to verify that the cr/crb variables are not nil before attempting to access those object's labels. The testing fake client may need to be updated in the future to handle returning these resources properly. Signed-off-by: timflannagan <timflannagan@gmail.com> * test(og): de-flake sync unit tests Co-authored-by: Nick Hale <njohnhale@gmail.com>

…troller (#2349) * pkg/controller: Fix panic when creating cluster-scoped RBAC in OG controller Fixes [#2091](operator-framework/operator-lifecycle-manager#2091). This is a follow-up to [#2309](operator-framework/operator-lifecycle-manager#2309) that attempted to fix the original issue. When checking whether the ClusterRole/ClusterRoleBinding resources already exist, we're also checking whether the existing labels are owned by the CSV we're currently handling. When accessing the "cr" or "crb" variables that the Create calls output, a panic is produced as we're attempting to access the meta.Labels key from those resources, except those resources themselves are nil. Update the check to verify that the cr/crb variables are not nil before attempting to access those object's labels. The testing fake client may need to be updated in the future to handle returning these resources properly. Signed-off-by: timflannagan <timflannagan@gmail.com> * test(og): de-flake sync unit tests Co-authored-by: Nick Hale <njohnhale@gmail.com> Upstream-repository: operator-lifecycle-manager Upstream-commit: 188ee1ae265f0a79d90b4cce52d97a26f16afe5a

don't return an error if the singleton rbac doesn't exist

51775f6

Signed-off-by: Evan <cordell.evan@gmail.com>

openshift-ci bot requested review from anik120 and benluddy July 30, 2021 22:11

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2021

timflannagan reviewed Jul 30, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

openshift-ci bot assigned timflannagan Jul 30, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2021

dinhxuanvu approved these changes Jul 30, 2021

View reviewed changes

openshift-ci bot assigned dinhxuanvu Jul 30, 2021

njhale approved these changes Jul 31, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2021

openshift-ci bot assigned njhale Jul 31, 2021

openshift-ci bot merged commit 538b131 into operator-framework:master Jul 31, 2021

timflannagan mentioned this pull request Sep 8, 2021

pkg/controller: Fix panic when creating cluster-scoped RBAC in OG controller #2349

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't return an error if the singleton rbac doesn't exist #2309

don't return an error if the singleton rbac doesn't exist #2309

ecordell commented Jul 30, 2021 •

edited

Loading

timflannagan Jul 30, 2021

timflannagan commented Jul 30, 2021 •

edited

Loading

dinhxuanvu left a comment

dinhxuanvu commented Jul 31, 2021

njhale left a comment

openshift-ci bot commented Jul 31, 2021

njhale commented Jul 31, 2021

don't return an error if the singleton rbac doesn't exist #2309

don't return an error if the singleton rbac doesn't exist #2309

Conversation

ecordell commented Jul 30, 2021 • edited Loading

timflannagan Jul 30, 2021

Choose a reason for hiding this comment

timflannagan commented Jul 30, 2021 • edited Loading

dinhxuanvu left a comment

Choose a reason for hiding this comment

dinhxuanvu commented Jul 31, 2021

njhale left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 31, 2021

njhale commented Jul 31, 2021

ecordell commented Jul 30, 2021 •

edited

Loading

timflannagan commented Jul 30, 2021 •

edited

Loading