From 94fd8fbb3351a6bbde481888ebf88175e79fca6e Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Thu, 28 Mar 2019 17:42:57 -0400 Subject: [PATCH] fix(annotation): don't annotate deployments that aren't owned by a CSV --- pkg/controller/operators/olm/operator.go | 13 +-- pkg/controller/operators/olm/operator_test.go | 109 +++++++++++++----- 2 files changed, 84 insertions(+), 38 deletions(-) diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 954b712b04..ecf8fae369 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -8,7 +8,6 @@ import ( v1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1" "github.com/sirupsen/logrus" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -1423,22 +1422,14 @@ func (a *Operator) ensureDeploymentAnnotations(logger *logrus.Entry, csv *v1alph return nil } - var depNames []string - for _, dep := range strategyDetailsDeployment.DeploymentSpecs { - depNames = append(depNames, dep.Name) - } - existingDeployments, err := a.lister.AppsV1().DeploymentLister().Deployments(csv.GetNamespace()).List(labels.Everything()) + existingDeployments, err := a.lister.AppsV1().DeploymentLister().Deployments(csv.GetNamespace()).List(ownerutil.CSVOwnerSelector(csv)) if err != nil { return err } // compare deployments to see if any need to be created/updated - existingMap := map[string]*appsv1.Deployment{} - for _, d := range existingDeployments { - existingMap[d.GetName()] = d - } updateErrs := []error{} - for _, dep := range existingMap { + for _, dep := range existingDeployments { if dep.Spec.Template.Annotations == nil { dep.Spec.Template.Annotations = map[string]string{} } diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index 8c8e70a8c6..75bfc0f3a0 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -10,6 +10,7 @@ import ( "fmt" "math" "math/big" + "reflect" "sort" "strings" "testing" @@ -28,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/diff" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/informers" @@ -39,8 +41,7 @@ import ( apiregistrationfake "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" kagg "k8s.io/kube-aggregator/pkg/client/informers/externalversions" - "github.com/operator-framework/operator-lifecycle-manager/pkg/metrics" - "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1" + v1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" @@ -56,6 +57,7 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer" + "github.com/operator-framework/operator-lifecycle-manager/pkg/metrics" ) // Fakes @@ -796,6 +798,7 @@ func TestTransitionCSV(t *testing.T) { } type expected struct { csvStates map[string]csvState + objs []runtime.Object err map[string]error } tests := []struct { @@ -2174,7 +2177,7 @@ func TestTransitionCSV(t *testing.T) { }, }, { - name: "SingleCSVInstallingToSucceeded", + name: "SingleCSVInstallingToSucceeded/UnmanagedDeploymentNotAffected", initial: initial{ csvs: []runtime.Object{ csvWithAnnotations(csv("csv1", @@ -2193,12 +2196,55 @@ func TestTransitionCSV(t *testing.T) { }, objs: []runtime.Object{ deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations), + deployment("extra-dep", namespace, "sa", nil), }, }, expected: expected{ csvStates: map[string]csvState{ "csv1": {exists: true, phase: v1alpha1.CSVPhaseSucceeded}, }, + objs: []runtime.Object{ + deployment("extra-dep", namespace, "sa", nil), + }, + }, + }, + { + name: "SingleCSVSucceededToSucceeded/UnmanagedDeploymentInNamespace", + initial: initial{ + csvs: []runtime.Object{ + withConditionReason(csvWithAnnotations(csv("csv1", + namespace, + "0.0.0", + "", + installStrategy("csv1-dep1", nil, nil), + []*v1beta1.CustomResourceDefinition{crd("c1", "v1", "g1")}, + []*v1beta1.CustomResourceDefinition{}, + v1alpha1.CSVPhaseSucceeded, + ), defaultTemplateAnnotations), v1alpha1.CSVReasonInstallSuccessful), + }, + clientObjs: []runtime.Object{defaultOperatorGroup}, + crds: []runtime.Object{ + crd("c1", "v1", "g1"), + }, + objs: []runtime.Object{ + withLabels( + deployment("csv1-dep1", namespace, "sa", defaultTemplateAnnotations), + map[string]string{ + ownerutil.OwnerKey: "csv1", + ownerutil.OwnerNamespaceKey: namespace, + ownerutil.OwnerKind: "ClusterServiceVersion", + }, + ), + deployment("extra-dep", namespace, "sa", nil), + }, + }, + expected: expected{ + csvStates: map[string]csvState{ + "csv1": {exists: true, phase: v1alpha1.CSVPhaseSucceeded}, + }, + objs: []runtime.Object{ + deployment("extra-dep", namespace, "sa", nil), + }, }, }, { @@ -2859,6 +2905,11 @@ func TestTransitionCSV(t *testing.T) { } } } + + // Verify other objects + if tt.expected.objs != nil { + RequireObjectsInNamespace(t, op.OpClient, op.client, namespace, tt.expected.objs) + } }) } } @@ -3706,35 +3757,39 @@ func TestSyncOperatorGroups(t *testing.T) { assert.Equal(t, tt.expectedStatus, operatorGroup.Status) for namespace, objects := range tt.final.objects { - for _, object := range objects { - var err error - var fetched runtime.Object - switch o := object.(type) { - case *appsv1.Deployment: - fetched, err = op.OpClient.GetDeployment(namespace, o.GetName()) - case *rbacv1.ClusterRole: - fetched, err = op.OpClient.GetClusterRole(o.GetName()) - case *rbacv1.Role: - fetched, err = op.OpClient.GetRole(namespace, o.GetName()) - case *rbacv1.ClusterRoleBinding: - fetched, err = op.OpClient.GetClusterRoleBinding(o.GetName()) - case *rbacv1.RoleBinding: - fetched, err = op.OpClient.GetRoleBinding(namespace, o.GetName()) - case *v1alpha1.ClusterServiceVersion: - fetched, err = op.client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Get(o.GetName(), metav1.GetOptions{}) - case *v1.OperatorGroup: - fetched, err = op.client.OperatorsV1().OperatorGroups(namespace).Get(o.GetName(), metav1.GetOptions{}) - default: - require.Failf(t, "couldn't find expected object", "%#v", object) - } - require.NoError(t, err, "couldn't fetch %s %v", namespace, object) - require.Equal(t, object, fetched, "%s in %s not equal", object.GetObjectKind().GroupVersionKind().String(), namespace) - } + RequireObjectsInNamespace(t, op.OpClient, op.client, namespace, objects) } }) } } +func RequireObjectsInNamespace(t *testing.T, opClient operatorclient.ClientInterface, client versioned.Interface, namespace string, objects []runtime.Object) { + for _, object := range objects { + var err error + var fetched runtime.Object + switch o := object.(type) { + case *appsv1.Deployment: + fetched, err = opClient.GetDeployment(namespace, o.GetName()) + case *rbacv1.ClusterRole: + fetched, err = opClient.GetClusterRole(o.GetName()) + case *rbacv1.Role: + fetched, err = opClient.GetRole(namespace, o.GetName()) + case *rbacv1.ClusterRoleBinding: + fetched, err = opClient.GetClusterRoleBinding(o.GetName()) + case *rbacv1.RoleBinding: + fetched, err = opClient.GetRoleBinding(namespace, o.GetName()) + case *v1alpha1.ClusterServiceVersion: + fetched, err = client.OperatorsV1alpha1().ClusterServiceVersions(namespace).Get(o.GetName(), metav1.GetOptions{}) + case *v1.OperatorGroup: + fetched, err = client.OperatorsV1().OperatorGroups(namespace).Get(o.GetName(), metav1.GetOptions{}) + default: + require.Failf(t, "couldn't find expected object", "%#v", object) + } + require.NoError(t, err, "couldn't fetch %s %v", namespace, object) + require.True(t, reflect.DeepEqual(object, fetched), diff.ObjectDiff(object, fetched)) + } +} + func TestIsReplacing(t *testing.T) { logrus.SetLevel(logrus.DebugLevel) namespace := "ns"