Bug 1819457: Services should not have duplicate ownerrefs #1855

ecordell · 2020-11-09T16:40:32Z

Description of the change:
This change only adds a unit test to verify that services created for apiservices installed via OLM do not have multiple ownerrefs added.

The test harness is bulky, but because this is intended to be backported, I did not attempt to refactor the related apiservice management to make testing simpler.

Motivation for the change:
This is to verify that the changes from this commit (1476ff1) are included in master.

This set of changes + the above commit will be backported to older versions, to address a report of this bug in 4.5

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 /docs
Commit messages sensible and descriptive

ownerref added twice

openshift-ci-robot · 2020-11-09T16:40:39Z

@ecordell: This pull request references Bugzilla bug 1819457, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1819457: Services should not have duplicate ownerrefs

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-11-09T16:40:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell

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]

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

openshift-ci-robot · 2020-11-09T17:02:08Z

@ecordell: This pull request references Bugzilla bug 1819457, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.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 1819457: Services should not have duplicate ownerrefs

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 · 2020-11-09T18:29:44Z

pkg/lib/operatorlister/lister.go

+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_service_account_lister.go k8s.io/client-go/listers/core/v1.ServiceAccountLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_service_account_namespace_lister.go k8s.io/client-go/listers/core/v1.ServiceAccountNamespaceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_service_lister.go k8s.io/client-go/listers/core/v1.ServiceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_service_namespace_lister.go k8s.io/client-go/listers/core/v1.ServiceNamespaceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_secret_lister.go k8s.io/client-go/listers/core/v1.SecretLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_v1_secret_namespace_lister.go k8s.io/client-go/listers/core/v1.SecretNamespaceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_rbac_v1_role_lister.go k8s.io/client-go/listers/rbac/v1.RoleLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_rbac_v1_role_namespace_lister.go k8s.io/client-go/listers/rbac/v1.RoleNamespaceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_rbac_v1_rolebinding_lister.go k8s.io/client-go/listers/rbac/v1.RoleBindingLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_rbac_v1_rolebinding_namespace_lister.go k8s.io/client-go/listers/rbac/v1.RoleBindingNamespaceLister
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o ../../fakes/client-go/listers/fake_rbac_v1_clusterrolebinding_lister.go k8s.io/client-go/listers/rbac/v1.ClusterRoleBindingLister


pkg/controller/install/certresources_test.go

ecordell · 2020-11-09T18:43:54Z

pkg/controller/install/certresources_test.go

+						Name:      "test-service",
+						Namespace: owner.GetNamespace(),
+						OwnerReferences: []metav1.OwnerReference{
+							ownerutil.NonBlockingOwner(owner),


this is a service that has the ownerref set

ecordell · 2020-11-09T18:45:00Z

pkg/controller/install/certresources_test.go

+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: owner.GetNamespace(),
+						OwnerReferences: []metav1.OwnerReference{
+							ownerutil.NonBlockingOwner(owner),


this sets the "existing" service on the cluster to already have the ownerref

ecordell · 2020-11-09T18:46:11Z

pkg/controller/install/certresources_test.go

+						Selector: selector(t, "test=label").MatchLabels,
+					},
+				}
+				mockOpClient.EXPECT().CreateService(&service).Return(&service, nil)


and then this mock is expected to be called with a service object with that exact spec - i.e. only one owner ref

if you remove the commit that fixes the bug, this mock fails because the service passed to CreateService will have two ownerrefs and fail mock validation.

Ah, so if you don't pass a single ownerref service, the mock will return nil and I assume that would cause a failure -- subtle.

njhale · 2020-11-09T18:52:14Z

/lgtm
/retest

openshift-bot · 2020-11-09T21:29:36Z

/retest

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

openshift-ci-robot · 2020-11-09T23:25:59Z

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

operator-framework/operator-lifecycle-manager#1855

Bugzilla bug 1819457 has been moved to the MODIFIED state.

In response to this:

Bug 1819457: Services should not have duplicate ownerrefs

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.

ecordell added 3 commits November 4, 2020 19:53

chore: generate fakes with inline go:generate directives

94cf709

test: scaffold a basic unit test for certresources

f74d837

test(certs): add a test the ensures services do not get the same

35827c6

ownerref added twice

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Nov 9, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 9, 2020

openshift-ci-robot requested review from awgreene and kevinrizza November 9, 2020 16:40

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

njhale reviewed Nov 9, 2020

View reviewed changes

pkg/controller/install/certresources_test.go Show resolved Hide resolved

ecordell commented Nov 9, 2020

View reviewed changes

openshift-ci-robot assigned njhale Nov 9, 2020

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

openshift-merge-robot merged commit 161c86b into operator-framework:master Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1819457: Services should not have duplicate ownerrefs #1855

Bug 1819457: Services should not have duplicate ownerrefs #1855

ecordell commented Nov 9, 2020 •

edited

openshift-ci-robot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

njhale Nov 9, 2020

ecordell Nov 9, 2020

ecordell Nov 9, 2020

ecordell Nov 9, 2020

njhale Nov 9, 2020

njhale commented Nov 9, 2020

openshift-bot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

Bug 1819457: Services should not have duplicate ownerrefs #1855

Bug 1819457: Services should not have duplicate ownerrefs #1855

Conversation

ecordell commented Nov 9, 2020 • edited

openshift-ci-robot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

njhale Nov 9, 2020

Choose a reason for hiding this comment

ecordell Nov 9, 2020

Choose a reason for hiding this comment

ecordell Nov 9, 2020

Choose a reason for hiding this comment

ecordell Nov 9, 2020

Choose a reason for hiding this comment

njhale Nov 9, 2020

Choose a reason for hiding this comment

njhale commented Nov 9, 2020

openshift-bot commented Nov 9, 2020

openshift-ci-robot commented Nov 9, 2020

ecordell commented Nov 9, 2020 •

edited