diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index 2bb23a555e..f0841953ab 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -8,8 +8,6 @@ import ( "sync" "time" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" - "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -42,11 +40,14 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/informers/externalversions" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/overrides" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins" "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clients" csvutility "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/csv" diff --git a/pkg/controller/operators/olm/operator_test.go b/pkg/controller/operators/olm/operator_test.go index a28a67bdd4..c117cbffb6 100644 --- a/pkg/controller/operators/olm/operator_test.go +++ b/pkg/controller/operators/olm/operator_test.go @@ -20,7 +20,6 @@ import ( "github.com/google/go-cmp/cmp" configfake "github.com/openshift/client-go/config/clientset/versioned/fake" - hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,6 +43,7 @@ import ( metadatafake "k8s.io/client-go/metadata/fake" "k8s.io/client-go/pkg/version" "k8s.io/client-go/rest" + clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" @@ -54,7 +54,6 @@ import ( operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" "github.com/operator-framework/api/pkg/operators/v1alpha1" opregistry "github.com/operator-framework/operator-registry/pkg/registry" - clienttesting "k8s.io/client-go/testing" "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" @@ -64,6 +63,7 @@ import ( resolvercache "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/clientfake" csvutility "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/csv" + hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/labeler" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" @@ -5050,7 +5050,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), &rbacv1.Role{ @@ -5155,7 +5160,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), &rbacv1.Role{ @@ -5312,7 +5322,12 @@ func TestSyncOperatorGroups(t *testing.T) { }, targetNamespace: { withLabels( - withAnnotations(targetCSV.DeepCopy(), map[string]string{operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace}), + withAnnotations(targetCSV.DeepCopy(), map[string]string{ + operatorsv1.OperatorGroupAnnotationKey: "operator-group-1", + operatorsv1.OperatorGroupNamespaceAnnotationKey: operatorNamespace, + "olm.operatorframework.io/nonStatusCopyHash": "9ZxHAHQTkxzAQd7Qkk4Qjz3VAkA8lXwuX9mDX6", + "olm.operatorframework.io/statusCopyHash": "bedtcmN999WBSJ1RHvM7JfN2NJITrUjJ0g0MoH", + }), labels.Merge(targetCSV.GetLabels(), map[string]string{v1alpha1.CopiedLabelKey: operatorNamespace}), ), }, diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 8429a59ce8..3d05bf967a 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -360,7 +360,7 @@ func (a *Operator) pruneProvidedAPIs(group *operatorsv1.OperatorGroup, groupProv } // Prune providedAPIs annotation if the cluster has fewer providedAPIs (handles CSV deletion) - //if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) { + // if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) { if len(intersection) < len(groupProvidedAPIs) { difference := groupProvidedAPIs.Difference(intersection) logger := logger.WithFields(logrus.Fields{ @@ -790,6 +790,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, return newHash, originalHash, nil } +const ( + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" +) + // If returned error is not nil, the returned ClusterServiceVersion // has only the Name, Namespace, and UID fields set. func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, nsFrom, nsTo, nonstatus, status string) (*v1alpha1.ClusterServiceVersion, error) { @@ -803,6 +808,7 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns existing, err := a.copiedCSVLister.Namespace(nsTo).Get(prototype.GetName()) if apierrors.IsNotFound(err) { + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus created, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Create(context.TODO(), prototype, metav1.CreateOptions{}) if err != nil { return nil, fmt.Errorf("failed to create new CSV: %w", err) @@ -811,6 +817,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), created, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update status on new CSV: %w", err) } + prototype.Annotations[statusCopyHashAnnotation] = status + if _, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: created.Name, @@ -825,11 +835,15 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion prototype.UID = existing.UID - existingNonStatus := existing.Annotations["$copyhash-nonstatus"] - existingStatus := existing.Annotations["$copyhash-status"] + // Get the non-status and status hash of the existing copied CSV + existingNonStatus := existing.Annotations[nonStatusCopyHashAnnotation] + existingStatus := existing.Annotations[statusCopyHashAnnotation] var updated *v1alpha1.ClusterServiceVersion + // Always set the in-memory prototype's nonstatus annotation: + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus if existingNonStatus != nonstatus { + // include updates to the non-status hash annotation if there is a mismatch if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update: %w", err) } @@ -843,6 +857,17 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns if _, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { return nil, fmt.Errorf("failed to update status: %w", err) } + // Update the status first if the existing copied CSV status hash doesn't match what we expect + // to prevent a scenario where the hash annotations match but the contents do not. + // We also need to update the CSV itself in this case to ensure we set the status hash annotation. + prototype.Annotations[statusCopyHashAnnotation] = status + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } + } else { + // Even if they're the same, ensure the returned prototype is annotated. + prototype.Annotations[statusCopyHashAnnotation] = status + updated = prototype } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -939,7 +964,6 @@ func namespacesChanged(clusterNamespaces []string, statusNamespaces []string) bo func (a *Operator) getOperatorGroupTargets(op *operatorsv1.OperatorGroup) (map[string]struct{}, error) { selector, err := metav1.LabelSelectorAsSelector(op.Spec.Selector) - if err != nil { return nil, err } diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index bb328c72cc..035543347f 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -6,19 +6,72 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/client-go/metadata/metadatalister" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/metadata/metadatalister" ktesting "k8s.io/client-go/testing" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake" ) +// fakeCSVNamespaceLister implements metadatalister.NamespaceLister +type fakeCSVNamespaceLister struct { + namespace string + items []*metav1.PartialObjectMetadata +} + +func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { + var result []*metav1.PartialObjectMetadata + for _, item := range n.items { + if item != nil && item.Namespace == n.namespace { + result = append(result, item) + } + } + return result, nil +} + +func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata, error) { + for _, item := range n.items { + if item != nil && item.Namespace == n.namespace && item.Name == name { + return item, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) +} + +// fakeCSVLister implements the full metadatalister.Lister interface +// so that Operator.copiedCSVLister = &fakeCSVLister{...} works. +type fakeCSVLister struct { + items []*metav1.PartialObjectMetadata +} + +// List returns all CSV metadata items, ignoring namespaces. +func (f *fakeCSVLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { + return f.items, nil +} + +// Get returns the CSV by name, ignoring namespaces. +func (f *fakeCSVLister) Get(name string) (*metav1.PartialObjectMetadata, error) { + for _, item := range f.items { + if item != nil && item.Name == name { + return item, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) +} + +// Namespace returns a namespace-scoped lister wrapper. +func (f *fakeCSVLister) Namespace(ns string) metadatalister.NamespaceLister { + return &fakeCSVNamespaceLister{ + namespace: ns, + items: f.items, + } +} + func TestCopyToNamespace(t *testing.T) { gvr := v1alpha1.SchemeGroupVersion.WithResource("clusterserviceversions") @@ -44,9 +97,12 @@ func TestCopyToNamespace(t *testing.T) { Name: "copy created if does not exist", FromNamespace: "from", ToNamespace: "to", + Hash: "hn-1", + StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -55,11 +111,16 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }, + // No ExistingCopy: means there's nothing in "to" namespace yet. ExpectedActions: []ktesting.Action{ + // Create the new CSV with nonStatusCopyHashAnnotation ktesting.NewCreateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -68,10 +129,33 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + // UpdateStatus: note that name/namespace remain "name"/"to", + // and we still have nonStatusCopyHashAnnotation: "hn-1". ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal Update for the statusCopyHashAnnotation = "hs" + // We still keep the "hn-1" annotation as well, plus Name/Namespace as is. + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + "olm.operatorframework.io/statusCopyHash": "hs", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -96,7 +180,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -112,25 +197,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn-2", // differs => triggers normal Update + statusCopyHashAnnotation: "hs", // same => no status update }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // Non-status differs => 1 normal Update ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + // We'll set the new nonStatusCopyHashAnnotation = "hn-1" + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -140,6 +223,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "copy status updated if status hash differs", @@ -149,7 +239,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -165,25 +256,43 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs-2", + // non-status matches => no normal update + nonStatusCopyHashAnnotation: "hn", + // status differs => subresource + normal update + statusCopyHashAnnotation: "hs-2", }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // UpdateStatus (we set the new status, and the in-memory CSV includes the matching nonStatus) ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal Update to set statusCopyHashAnnotation = "hs-1" + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -193,6 +302,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "copy and copy status updated if both hashes differ", @@ -202,7 +318,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -218,25 +335,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs-2", + // Both nonStatus and status mismatch + nonStatusCopyHashAnnotation: "hn-2", + statusCopyHashAnnotation: "hs-2", }, }, }, - ExpectedResult: &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "to", - UID: "uid", - }, - }, ExpectedActions: []ktesting.Action{ + // Normal update for the non-status mismatch ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -245,12 +360,35 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + // UpdateStatus because status hash mismatch ktesting.NewUpdateSubresourceAction(gvr, "status", "to", &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", UID: "uid", ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Replaces: "replacee", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + // Normal update for the new statusCopyHashAnnotation + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + statusCopyHashAnnotation: "hs-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -260,6 +398,13 @@ func TestCopyToNamespace(t *testing.T) { }, }), }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, + }, }, { Name: "no action taken if neither hash differs", @@ -269,7 +414,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", + Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{ @@ -278,8 +424,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs", }, }, }, @@ -290,18 +436,28 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", }, }, + ExpectedActions: nil, // no update calls if neither hash differs }, } { t.Run(tc.Name, func(t *testing.T) { + // Create a new fake clientset populated with the "existing copy" if any client := fake.NewSimpleClientset() var lister metadatalister.Lister + + // If we have an existing CSV in that target namespace, add it to the slice + items := []*metav1.PartialObjectMetadata{} if tc.ExistingCopy != nil { - client = fake.NewSimpleClientset(&v1alpha1.ClusterServiceVersion{ + existingObj := &v1alpha1.ClusterServiceVersion{ ObjectMeta: tc.ExistingCopy.ObjectMeta, - }) - lister = FakeClusterServiceVersionLister{tc.ExistingCopy} - } else { - lister = FakeClusterServiceVersionLister{{}} + // ... if you want to set Spec/Status in the client, you can + } + client = fake.NewSimpleClientset(existingObj) + items = []*metav1.PartialObjectMetadata{tc.ExistingCopy} + } + + // Create the full Lister + lister = &fakeCSVLister{ + items: items, } logger, _ := test.NewNullLogger() @@ -311,13 +467,22 @@ func TestCopyToNamespace(t *testing.T) { logger: logger, } - result, err := o.copyToNamespace(tc.Prototype.DeepCopy(), tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) + proto := tc.Prototype.DeepCopy() + result, err := o.copyToNamespace(proto, tc.FromNamespace, tc.ToNamespace, tc.Hash, tc.StatusHash) if tc.ExpectedError == nil { require.NoError(t, err) + + // Ensure the in-memory 'proto' has the correct final annotations + annotations := proto.GetObjectMeta().GetAnnotations() + require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation], + "proto should have the non-status hash annotation set") + require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation], + "proto should have the status hash annotation set") } else { require.EqualError(t, err, tc.ExpectedError.Error()) } + if diff := cmp.Diff(tc.ExpectedResult, result); diff != "" { t.Errorf("incorrect result: %v", diff) } @@ -332,78 +497,3 @@ func TestCopyToNamespace(t *testing.T) { }) } } - -type FakeClusterServiceVersionLister []*metav1.PartialObjectMetadata - -func (l FakeClusterServiceVersionLister) List(selector labels.Selector) ([]*metav1.PartialObjectMetadata, error) { - var result []*metav1.PartialObjectMetadata - for _, csv := range l { - if !selector.Matches(labels.Set(csv.GetLabels())) { - continue - } - result = append(result, csv) - } - return result, nil -} - -func (l FakeClusterServiceVersionLister) Namespace(namespace string) metadatalister.NamespaceLister { - var filtered []*metav1.PartialObjectMetadata - for _, csv := range l { - if csv.GetNamespace() != namespace { - continue - } - filtered = append(filtered, csv) - } - return FakeClusterServiceVersionLister(filtered) -} - -func (l FakeClusterServiceVersionLister) Get(name string) (*metav1.PartialObjectMetadata, error) { - for _, csv := range l { - if csv.GetName() == name { - return csv, nil - } - } - return nil, errors.NewNotFound(v1alpha1.Resource("clusterserviceversion"), name) -} - -var ( - _ metadatalister.Lister = FakeClusterServiceVersionLister{} - _ metadatalister.NamespaceLister = FakeClusterServiceVersionLister{} -) - -func TestCSVCopyPrototype(t *testing.T) { - src := v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Namespace: "foo", - Annotations: map[string]string{ - "olm.targetNamespaces": "a,b,c", - "kubectl.kubernetes.io/last-applied-configuration": "{}", - "preserved": "yes", - }, - Labels: map[string]string{ - "operators.coreos.com/foo": "", - "operators.coreos.com/bar": "", - "untouched": "fine", - }, - }, - } - var dst v1alpha1.ClusterServiceVersion - csvCopyPrototype(&src, &dst) - assert.Equal(t, v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{ - "preserved": "yes", - }, - Labels: map[string]string{ - "untouched": "fine", - "olm.copiedFrom": "foo", - }, - }, - Status: v1alpha1.ClusterServiceVersionStatus{ - Message: "The operator is running in foo but is managing this namespace", - Reason: v1alpha1.CSVReasonCopied, - }, - }, dst) -}