From 1df2a4a4d221dede69e4fa5d4459ca7db8dd8e66 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:35:31 -0400 Subject: [PATCH 1/8] (bugfix): reduce frequency of update requests for CSVs by adding annotations to copied CSVs that are populated with hashes of the non-status fields and the status fields. This seems to be how this was intended to work, but was not actually working this way because the annotations never actually existed on the copied CSV. This resulted in a hot loop of update requests being made on all copied CSVs. Signed-off-by: everettraven --- Makefile | 4 ++-- pkg/controller/operators/olm/operatorgroup.go | 22 +++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 7ef00ffafc..fa27dd405c 100644 --- a/Makefile +++ b/Makefile @@ -63,7 +63,7 @@ export CONFIGMAP_SERVER_IMAGE ?= quay.io/operator-framework/configmap-operator-r PKG := github.com/operator-framework/operator-lifecycle-manager IMAGE_REPO ?= quay.io/operator-framework/olm -IMAGE_TAG ?= "dev" +IMAGE_TAG ?= dev # Go build settings # @@ -228,7 +228,7 @@ kind-create: kind-clean #HELP Create a new kind cluster $KIND_CLUSTER_NAME (defa $(KIND) export kubeconfig --name $(KIND_CLUSTER_NAME) .PHONY: deploy -OLM_IMAGE := quay.io/operator-framework/olm:local +OLM_IMAGE := $(IMAGE_REPO):$(IMAGE_TAG) deploy: $(KIND) $(HELM) #HELP Deploy OLM to kind cluster $KIND_CLUSTER_NAME (default: kind-olmv0) using $OLM_IMAGE (default: quay.io/operator-framework/olm:local) $(KIND) load docker-image $(OLM_IMAGE) --name $(KIND_CLUSTER_NAME); \ $(HELM) upgrade --install olm deploy/chart \ diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 8429a59ce8..aa4bfd1ba2 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) { @@ -825,11 +830,14 @@ 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 if existingNonStatus != nonstatus { + // include updates to the non-status hash annotation if there is a mismatch + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus 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 +851,13 @@ 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) + } } return &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -939,7 +954,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 } From 8d15c88a7b9c3196efdde9f3337593e0bc15b6ea Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 15:43:40 -0400 Subject: [PATCH 2/8] update unit tests Signed-off-by: everettraven --- .../operators/olm/operatorgroup_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index bb328c72cc..9af54e449f 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -112,8 +112,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn-2", + statusCopyHashAnnotation: "hs", }, }, }, @@ -165,8 +165,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs-2", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs-2", }, }, }, @@ -218,8 +218,8 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn-2", - "$copyhash-status": "hs-2", + nonStatusCopyHashAnnotation: "hn-2", + statusCopyHashAnnotation: "hs-2", }, }, }, @@ -278,8 +278,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs", }, }, }, From 692a14e356a75b3faaaea79bb268e134f7019a07 Mon Sep 17 00:00:00 2001 From: everettraven Date: Tue, 8 Oct 2024 17:08:32 -0400 Subject: [PATCH 3/8] updates to test so far Signed-off-by: everettraven --- .github/workflows/e2e-tests.yml | 2 +- pkg/controller/operators/olm/operatorgroup.go | 5 ++++ .../operators/olm/operatorgroup_test.go | 24 ++++++++++++++++++- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 8dd4ff9b91..c4b134b6e2 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -25,7 +25,7 @@ jobs: - name: Build controller image run: make e2e-build - name: Save image - run: docker save quay.io/operator-framework/olm:local -o olm-image.tar + run: docker save quay.io/operator-framework/olm:dev -o olm-image.tar - name: Upload Docker image as artifact uses: actions/upload-artifact@v4 with: diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index aa4bfd1ba2..d8524ec1b5 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -808,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) @@ -816,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, diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 9af54e449f..025f696299 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -44,9 +44,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", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -60,6 +63,9 @@ func TestCopyToNamespace(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "name", Namespace: "to", + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -80,6 +86,13 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), + ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + statusCopyHashAnnotation: "hs", + }, + }, + }) }, ExpectedResult: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -97,6 +110,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -150,6 +164,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -203,6 +218,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -270,6 +286,7 @@ func TestCopyToNamespace(t *testing.T) { Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{ @@ -311,10 +328,15 @@ 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) + // if there is no error expected, ensure that the hash annotations are always present on the resulting CSV + annotations := proto.GetObjectMeta().GetAnnotations() + require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation]) + require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation]) } else { require.EqualError(t, err, tc.ExpectedError.Error()) } From 2f0e825503ed53a91f894fa32d8a626f505165f8 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 9 Oct 2024 10:14:21 -0400 Subject: [PATCH 4/8] Small changes Signed-off-by: Brett Tofel --- .github/workflows/e2e-tests.yml | 2 +- Makefile | 4 +- pkg/controller/operators/olm/operator.go | 5 +- pkg/controller/operators/olm/operator_test.go | 25 +- pkg/controller/operators/olm/operatorgroup.go | 7 +- .../operators/olm/operatorgroup_test.go | 328 +++++++++++------- 6 files changed, 230 insertions(+), 141 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index c4b134b6e2..8dd4ff9b91 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -25,7 +25,7 @@ jobs: - name: Build controller image run: make e2e-build - name: Save image - run: docker save quay.io/operator-framework/olm:dev -o olm-image.tar + run: docker save quay.io/operator-framework/olm:local -o olm-image.tar - name: Upload Docker image as artifact uses: actions/upload-artifact@v4 with: diff --git a/Makefile b/Makefile index fa27dd405c..7ef00ffafc 100644 --- a/Makefile +++ b/Makefile @@ -63,7 +63,7 @@ export CONFIGMAP_SERVER_IMAGE ?= quay.io/operator-framework/configmap-operator-r PKG := github.com/operator-framework/operator-lifecycle-manager IMAGE_REPO ?= quay.io/operator-framework/olm -IMAGE_TAG ?= dev +IMAGE_TAG ?= "dev" # Go build settings # @@ -228,7 +228,7 @@ kind-create: kind-clean #HELP Create a new kind cluster $KIND_CLUSTER_NAME (defa $(KIND) export kubeconfig --name $(KIND_CLUSTER_NAME) .PHONY: deploy -OLM_IMAGE := $(IMAGE_REPO):$(IMAGE_TAG) +OLM_IMAGE := quay.io/operator-framework/olm:local deploy: $(KIND) $(HELM) #HELP Deploy OLM to kind cluster $KIND_CLUSTER_NAME (default: kind-olmv0) using $OLM_IMAGE (default: quay.io/operator-framework/olm:local) $(KIND) load docker-image $(OLM_IMAGE) --name $(KIND_CLUSTER_NAME); \ $(HELM) upgrade --install olm deploy/chart \ 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 d8524ec1b5..3d05bf967a 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -840,9 +840,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns 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 - prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus 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) } @@ -863,6 +864,10 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns 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{ diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 025f696299..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") @@ -48,8 +101,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -58,14 +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{ - nonStatusCopyHashAnnotation: "hn-1", - }, + Annotations: map[string]string{ + "olm.operatorframework.io/nonStatusCopyHash": "hn-1", + }, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -74,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", @@ -86,13 +164,6 @@ func TestCopyToNamespace(t *testing.T) { Phase: "waxing gibbous", }, }), - ktesting.NewUpdateAction(gvr, "to", &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - statusCopyHashAnnotation: "hs", - }, - }, - }) }, ExpectedResult: &v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ @@ -109,8 +180,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -126,25 +197,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ - nonStatusCopyHashAnnotation: "hn-2", - statusCopyHashAnnotation: "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", @@ -154,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", @@ -163,8 +239,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -180,25 +256,43 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ + // non-status matches => no normal update nonStatusCopyHashAnnotation: "hn", - statusCopyHashAnnotation: "hs-2", + // 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", @@ -208,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", @@ -217,8 +318,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs-1", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, Spec: v1alpha1.ClusterServiceVersionSpec{ Replaces: "replacee", @@ -234,25 +335,23 @@ func TestCopyToNamespace(t *testing.T) { UID: "uid", ResourceVersion: "42", Annotations: map[string]string{ + // 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", @@ -261,12 +360,16 @@ 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", @@ -275,6 +378,32 @@ func TestCopyToNamespace(t *testing.T) { 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", + }, + Status: v1alpha1.ClusterServiceVersionStatus{ + Phase: "waxing gibbous", + }, + }), + }, + ExpectedResult: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + }, }, }, { @@ -285,8 +414,8 @@ func TestCopyToNamespace(t *testing.T) { StatusHash: "hs", Prototype: v1alpha1.ClusterServiceVersion{ ObjectMeta: metav1.ObjectMeta{ - Name: "name", - Annotations: map[string]string{}, + Name: "name", + Annotations: map[string]string{}, }, }, ExistingCopy: &metav1.PartialObjectMetadata{ @@ -307,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() @@ -333,13 +472,17 @@ func TestCopyToNamespace(t *testing.T) { if tc.ExpectedError == nil { require.NoError(t, err) - // if there is no error expected, ensure that the hash annotations are always present on the resulting CSV + + // Ensure the in-memory 'proto' has the correct final annotations annotations := proto.GetObjectMeta().GetAnnotations() - require.Equal(t, tc.Hash, annotations[nonStatusCopyHashAnnotation]) - require.Equal(t, tc.StatusHash, annotations[statusCopyHashAnnotation]) + 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) } @@ -354,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) -} From 8d46851be57a2656635f5aabfbd7579b1c976758 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 29 Apr 2025 09:07:27 -0400 Subject: [PATCH 5/8] Add metadata drift guard to copyToNamespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we switched to a PartialObjectMetadata cache to save memory, we lost visibility into copied CSV spec and status fields, and the reintroduced nonStatusCopyHash/statusCopyHash annotations only partially solved the problem. Manual edits to a copied CSV could still go undetected, causing drift without reconciliation. This commit adds two new annotations: olm.operatorframework.io/observedGeneration and olm.operatorframework.io/observedResourceVersion. It implements a mechanism to guard agains metadata drift at the top of the existing-copy path in copyToNamespace. If a stored observedGeneration or observedResourceVersion no longer matches the live object, the operator now: • Updates the spec and hash annotations • Updates the status subresource • Records the new generation and resourceVersion in the guard annotations Because the guard only fires when its annotations are already present, all existing unit tests pass unchanged. We preserve the memory benefits of the metadata‐only informer, avoid extra GETs, and eliminate unnecessary API churn. Future work may explore a WithTransform informer to regain full object visibility with minimal memory impact. Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 47 +++++++++++++++++-- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 3d05bf967a..b16a1b589f 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -791,8 +791,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, } const ( - nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" - statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + // annotations for metadata drift guard + observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" + observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" ) // If returned error is not nil, the returned ClusterServiceVersion @@ -828,9 +831,43 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns UID: created.UID, }, }, nil - } else if err != nil { - return nil, err - } + } else if err != nil { + return nil, err + } + // metadata drift guard: detect manual modifications to spec or status + if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { + // full resync for metadata drift + // prepare prototype for update + prototype.Namespace = existing.Namespace + prototype.ResourceVersion = existing.ResourceVersion + prototype.UID = existing.UID + // sync hash annotations + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus + prototype.Annotations[statusCopyHashAnnotation] = status + // update spec and annotations + updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) + } + // update status subresource + updated.Status = prototype.Status + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) + } + // record observed generation and resourceVersion + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updated.Name, + Namespace: updated.Namespace, + UID: updated.UID, + }, + }, nil + } prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion From 22152969e51e8358c3b75096ec1430daa1cdd62f Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Tue, 29 Apr 2025 11:36:29 -0400 Subject: [PATCH 6/8] Tests for metadata guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verifies that exactly three updates (spec, status, guard) are issued when the observedGeneration doesn’t match. Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 84 +++++++++---------- .../operators/olm/operatorgroup_test.go | 70 ++++++++++++++++ 2 files changed, 112 insertions(+), 42 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index b16a1b589f..c6a59989e0 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -791,11 +791,11 @@ func copyableCSVHash(original *v1alpha1.ClusterServiceVersion) (string, string, } const ( - nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" - statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" - // annotations for metadata drift guard - observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" - observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" + nonStatusCopyHashAnnotation = "olm.operatorframework.io/nonStatusCopyHash" + statusCopyHashAnnotation = "olm.operatorframework.io/statusCopyHash" + // annotations for metadata drift guard + observedGenerationAnnotation = "olm.operatorframework.io/observedGeneration" + observedResourceVersionAnnotation = "olm.operatorframework.io/observedResourceVersion" ) // If returned error is not nil, the returned ClusterServiceVersion @@ -831,43 +831,43 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns UID: created.UID, }, }, nil - } else if err != nil { - return nil, err - } - // metadata drift guard: detect manual modifications to spec or status - if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { - // full resync for metadata drift - // prepare prototype for update - prototype.Namespace = existing.Namespace - prototype.ResourceVersion = existing.ResourceVersion - prototype.UID = existing.UID - // sync hash annotations - prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus - prototype.Annotations[statusCopyHashAnnotation] = status - // update spec and annotations - updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) - } - // update status subresource - updated.Status = prototype.Status - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) - } - // record observed generation and resourceVersion - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) - } - return &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: updated.Name, - Namespace: updated.Namespace, - UID: updated.UID, - }, - }, nil - } + } else if err != nil { + return nil, err + } + // metadata drift guard: detect manual modifications to spec or status + if og, orv := existing.Annotations[observedGenerationAnnotation], existing.Annotations[observedResourceVersionAnnotation]; (og != "" && og != fmt.Sprint(existing.GetGeneration())) || (orv != "" && orv != existing.ResourceVersion) { + // full resync for metadata drift + // prepare prototype for update + prototype.Namespace = existing.Namespace + prototype.ResourceVersion = existing.ResourceVersion + prototype.UID = existing.UID + // sync hash annotations + prototype.Annotations[nonStatusCopyHashAnnotation] = nonstatus + prototype.Annotations[statusCopyHashAnnotation] = status + // update spec and annotations + updated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resync spec for metadata drift guard: %w", err) + } + // update status subresource + updated.Status = prototype.Status + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).UpdateStatus(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to resync status for metadata drift guard: %w", err) + } + // record observed generation and resourceVersion + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updated.Name, + Namespace: updated.Namespace, + UID: updated.UID, + }, + }, nil + } prototype.Namespace = existing.Namespace prototype.ResourceVersion = existing.ResourceVersion diff --git a/pkg/controller/operators/olm/operatorgroup_test.go b/pkg/controller/operators/olm/operatorgroup_test.go index 035543347f..fe5d70be4c 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -34,6 +34,76 @@ func (n *fakeCSVNamespaceLister) List(selector labels.Selector) ([]*metav1.Parti return result, nil } +// Test full resync via metadata drift guard when observedGeneration mismatches +func TestCopyToNamespace_MetadataDriftGuard(t *testing.T) { + // Prepare prototype CSV and compute hashes + prototype := v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Annotations: map[string]string{}, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{Replaces: "replacee"}, + Status: v1alpha1.ClusterServiceVersionStatus{Phase: "waxing gibbous"}, + } + nonstatus, status, err := copyableCSVHash(&prototype) + require.NoError(t, err) + + // Existing partial copy with observedGeneration mismatched + existingCopy := &metav1.PartialObjectMetadata{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "to", + UID: "uid", + ResourceVersion: "42", + Generation: 2, + Annotations: map[string]string{ + nonStatusCopyHashAnnotation: nonstatus, + statusCopyHashAnnotation: status, + observedGenerationAnnotation: "1", + observedResourceVersionAnnotation: "42", + }, + }, + } + // Full CSV for fake client + full := &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: existingCopy.Name, + Namespace: existingCopy.Namespace, + UID: existingCopy.UID, + ResourceVersion: existingCopy.ResourceVersion, + Generation: existingCopy.Generation, + Annotations: existingCopy.Annotations, + }, + Spec: prototype.Spec, + Status: prototype.Status, + } + + client := fake.NewSimpleClientset(full) + lister := &fakeCSVLister{items: []*metav1.PartialObjectMetadata{existingCopy}} + logger, _ := test.NewNullLogger() + o := &Operator{copiedCSVLister: lister, client: client, logger: logger} + + protoCopy := prototype.DeepCopy() + result, err := o.copyToNamespace(protoCopy, "from", "to", nonstatus, status) + require.NoError(t, err) + require.Equal(t, "name", result.GetName()) + require.Equal(t, "to", result.GetNamespace()) + require.Equal(t, "uid", string(result.GetUID())) + + actions := client.Actions() + // Expect: update(spec), updateStatus, update(guard annotations) + require.Len(t, actions, 3) + // First action: spec update + ua0 := actions[0].(ktesting.UpdateAction) + require.Equal(t, "", ua0.GetSubresource()) + // Second action: status subresource + ua1 := actions[1].(ktesting.UpdateAction) + require.Equal(t, "status", ua1.GetSubresource()) + // Third action: metadata guard update + ua2 := actions[2].(ktesting.UpdateAction) + require.Equal(t, "", ua2.GetSubresource()) +} + func (n *fakeCSVNamespaceLister) Get(name string) (*metav1.PartialObjectMetadata, error) { for _, item := range n.items { if item != nil && item.Namespace == n.namespace && item.Name == name { From 42a1f10f75fef418b3573b2cbfe58a2bcd18a3e7 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Thu, 8 May 2025 13:28:46 -0400 Subject: [PATCH 7/8] Persist observed annotations on all status updates Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index c6a59989e0..1e82c80d51 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -820,17 +820,25 @@ 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, - Namespace: created.Namespace, - UID: created.UID, - }, - }, nil + prototype.Annotations[statusCopyHashAnnotation] = status + // persist status-hash annotation + updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) + updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updatedCreated.Name, + Namespace: updatedCreated.Namespace, + UID: updatedCreated.UID, + }, + }, nil } else if err != nil { return nil, err } @@ -896,11 +904,18 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns } // 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) - } + // persist status-hash annotation + prototype.Annotations[statusCopyHashAnnotation] = status + updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) + } } else { // Even if they're the same, ensure the returned prototype is annotated. prototype.Annotations[statusCopyHashAnnotation] = status From 61eeca54d37956d596b8caf75ebea85854f4e661 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 19 May 2025 16:14:18 -0400 Subject: [PATCH 8/8] GCI the file Signed-off-by: Brett Tofel --- pkg/controller/operators/olm/operatorgroup.go | 62 +++++++++---------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/pkg/controller/operators/olm/operatorgroup.go b/pkg/controller/operators/olm/operatorgroup.go index 1e82c80d51..b3e216bbb7 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -820,25 +820,25 @@ 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 - // persist status-hash annotation - updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) - updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion - if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) - } - return &v1alpha1.ClusterServiceVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: updatedCreated.Name, - Namespace: updatedCreated.Namespace, - UID: updatedCreated.UID, - }, - }, nil + prototype.Annotations[statusCopyHashAnnotation] = status + // persist status-hash annotation + updatedCreated, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update annotations after updating status: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updatedCreated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updatedCreated.GetGeneration()) + updatedCreated.Annotations[observedResourceVersionAnnotation] = updatedCreated.ResourceVersion + if _, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updatedCreated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after creation: %w", err) + } + return &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: updatedCreated.Name, + Namespace: updatedCreated.Namespace, + UID: updatedCreated.UID, + }, + }, nil } else if err != nil { return nil, err } @@ -904,18 +904,18 @@ func (a *Operator) copyToNamespace(prototype *v1alpha1.ClusterServiceVersion, ns } // 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. - // persist status-hash annotation - prototype.Annotations[statusCopyHashAnnotation] = status - updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) - if err != nil { - return nil, fmt.Errorf("failed to update: %w", err) - } - // record observed generation and resourceVersion for metadata-drift guard - updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) - updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion - if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { - return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) - } + // persist status-hash annotation + prototype.Annotations[statusCopyHashAnnotation] = status + updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), prototype, metav1.UpdateOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to update: %w", err) + } + // record observed generation and resourceVersion for metadata-drift guard + updated.Annotations[observedGenerationAnnotation] = fmt.Sprint(updated.GetGeneration()) + updated.Annotations[observedResourceVersionAnnotation] = updated.ResourceVersion + if updated, err = a.client.OperatorsV1alpha1().ClusterServiceVersions(nsTo).Update(context.TODO(), updated, metav1.UpdateOptions{}); err != nil { + return nil, fmt.Errorf("failed to update metadata guard annotations after status update: %w", err) + } } else { // Even if they're the same, ensure the returned prototype is annotated. prototype.Annotations[statusCopyHashAnnotation] = status