diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index aa4a4c4bcd..ae51aaeb71 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/Makefile b/Makefile index db91104e2d..1410d0891e 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 # @@ -211,7 +211,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 18c8b19008..e304984201 100644 --- a/pkg/controller/operators/olm/operatorgroup.go +++ b/pkg/controller/operators/olm/operatorgroup.go @@ -361,7 +361,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{ @@ -791,6 +791,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) { @@ -804,6 +809,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) @@ -812,6 +818,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, @@ -826,11 +836,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) } @@ -844,6 +857,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{ @@ -940,7 +960,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..d2519a74f4 100644 --- a/pkg/controller/operators/olm/operatorgroup_test.go +++ b/pkg/controller/operators/olm/operatorgroup_test.go @@ -8,14 +8,14 @@ import ( "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" ) @@ -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", + 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{ @@ -96,7 +109,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,8 +126,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", }, }, }, @@ -149,7 +163,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,8 +180,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", }, }, }, @@ -202,7 +217,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,8 +234,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", }, }, }, @@ -269,7 +285,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 +295,8 @@ func TestCopyToNamespace(t *testing.T) { Namespace: "to", UID: "uid", Annotations: map[string]string{ - "$copyhash-nonstatus": "hn", - "$copyhash-status": "hs", + nonStatusCopyHashAnnotation: "hn", + statusCopyHashAnnotation: "hs", }, }, }, @@ -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()) }