Bug 1746159: Cleanup leftover cross-namespace OwnerReferences #1025
Conversation
openshift-ci-robot
added
the
approved
|
@njhale: This pull request references Bugzilla bug 1746159, 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. In response to this:
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
added
bugzilla/valid-bug
|
@njhale: This pull request references Bugzilla bug 1746159, which is valid. In response to this:
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. |
njhale
changed the title
| update = func() error { | ||
| _, err = c.KubernetesInterface().RbacV1().RoleBindings(v.GetNamespace()).Update(v) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Why doesn't you make the default case log a warning message and then return? Then you can remove the empty update function assignment above.
There was a problem hiding this comment.
SGTM. We could also panic since it's probably a programmer error. This already merged, so I may try to sneak this in with another PR at some point.
| // If this is not a type we care about, do nothing | ||
| return nil | ||
| } | ||
| switch v := obj.(type) { |
There was a problem hiding this comment.
Super paranoid comment here (and slightly contradicts my comment below), but this sort of change almost makes me ponder a v1alpha2 version bump. Then you could easily filter out potential resources that might be problematic. I'm wondering if it's possible that a future backport ends up pulling in a new resource and this code doesn't get updated. I might be way off base here.
There was a problem hiding this comment.
Hopefully we don't add cross-namespace ownerrefs in newer versions, I don't think we have to worry too much about that.
There was a problem hiding this comment.
/lgtm
I left a couple of comments, but nothing serious. Once this merges, can you test an upgrade from an older 4.1 cluster (that has bad ownerrefs) to this, to verify that specifically the packageserver serviceaccount & clusterrole / rolebinding are fixed up properly?
| var cleanRefs []metav1.OwnerReference | ||
| objNamespace := obj.GetNamespace() | ||
| for _, ref := range obj.GetOwnerReferences() { | ||
| if ref.Kind == kind { |
There was a problem hiding this comment.
nit:
if ref.Kind != kind {
continue
}
| // If this is not a type we care about, do nothing | ||
| return nil | ||
| } | ||
| switch v := obj.(type) { |
There was a problem hiding this comment.
Hopefully we don't add cross-namespace ownerrefs in newer versions, I don't think we have to worry too much about that.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
|
@njhale: All pull requests linked via external trackers have merged. Bugzilla bug 1746159 has been moved to the MODIFIED state. In response to this:
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. |
|
/cherry-pick release-4.1 |
|
@njhale: #1025 failed to apply on top of branch "release-4.1": In response to this:
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. |
Adds cross-namespace OwnerReference cleanup logic to OLM startup.
Cross-namespace and cluster-to-namespace scoped OwnerReferences may cause sibling resources in the owner namespace to be deleted sporadically (see CVE-2019-3884). Older versions of OLM use both types of OwnerReference, and in cases where OLM is updated, they must be removed to prevent erroneous deletion of OLM's self-hosted components.