fix(openshift): use env var instead of clusterversion status (#2817)

Get the current OpenShift release version from the RELEASE_VERSION
environment variable since the behavior of the original source --
the ClusterVersion desired release status field -- has changed.

Signed-off-by: Tyler Slaton <tyslaton@redhat.com>
Co-authored-by: Nick Hale <njohnhale@gmail.com>

Author: tylerslaton

pkg/controller/operators/openshift/clusteroperator_controller_test.go

@@ -2,6 +2,7 @@ package openshift
 
 import (
 	"fmt"
+	"os"
 
 	semver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo/v2"
@@ -19,7 +20,6 @@ import (
 var _ = Describe("ClusterOperator controller", func() {
 	var (
 		clusterOperatorName types.NamespacedName
-		cv                  *configv1.ClusterVersion
 		csv                 *operatorsv1alpha1.ClusterServiceVersion
 	)
 
@@ -55,33 +55,11 @@ var _ = Describe("ClusterOperator controller", func() {
 	})
 
 	BeforeEach(func() {
-		// "version" singleton is available in OpenShift by default
-		cv = &configv1.ClusterVersion{}
-		cv.SetName("version")
-
-		Eventually(func() error {
-			return k8sClient.Create(ctx, cv)
-		}).Should(Succeed())
-
-		cv.Status = configv1.ClusterVersionStatus{
-			Desired: configv1.Release{
-				Version: clusterVersion,
-			},
-		}
-
-		Eventually(func() error {
-			return k8sClient.Status().Update(ctx, cv)
-		}).Should(Succeed())
+		os.Setenv(releaseEnvVar, clusterVersion)
 	})
 
 	AfterEach(func() {
-		Eventually(func() error {
-			err := k8sClient.Delete(ctx, cv)
-			if err != nil && apierrors.IsNotFound(err) {
-				err = nil
-			}
-			return err
-		}).Should(Succeed())
+		resetCurrentReleaseTo(clusterVersion)
 	})
 
 	It("should ensure the ClusterOperator always exists", func() {
@@ -143,10 +121,14 @@ var _ = Describe("ClusterOperator controller", func() {
 		}))
 
 		By("setting upgradeable=false when there's an error determining compatibility")
-		cv.Status = configv1.ClusterVersionStatus{}
-
+		// Reset the ClusterOperator with an invalid version set
+		Expect(resetCurrentReleaseTo("")).To(Succeed())
 		Eventually(func() error {
-			return k8sClient.Status().Update(ctx, cv)
+			err := k8sClient.Delete(ctx, co)
+			if err != nil && !apierrors.IsNotFound(err) {
+				return err
+			}
+			return nil
 		}).Should(Succeed())
 
 		Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
@@ -156,18 +138,18 @@ var _ = Describe("ClusterOperator controller", func() {
 			Type:               configv1.OperatorUpgradeable,
 			Status:             configv1.ConditionFalse,
 			Reason:             ErrorCheckingOperatorCompatibility,
-			Message:            "Encountered errors while checking compatibility with the next minor version of OpenShift: desired release version missing from ClusterVersion",
+			Message:            fmt.Sprintf("Encountered errors while checking compatibility with the next minor version of OpenShift: desired release version missing from %v env variable", releaseEnvVar),
 			LastTransitionTime: fixedNow(),
 		}))
 
-		cv.Status = configv1.ClusterVersionStatus{
-			Desired: configv1.Release{
-				Version: clusterVersion,
-			},
-		}
-
+		// Reset the ClusterOperator with a valid version set
+		Expect(resetCurrentReleaseTo(clusterVersion)).To(Succeed())
 		Eventually(func() error {
-			return k8sClient.Status().Update(ctx, cv)
+			err := k8sClient.Delete(ctx, co)
+			if err != nil && !apierrors.IsNotFound(err) {
+				return err
+			}
+			return nil
 		}).Should(Succeed())
 
 		By("setting upgradeable=false when incompatible operators exist")
@@ -327,3 +309,14 @@ var _ = Describe("ClusterOperator controller", func() {
 		}))
 	})
 })
+
+// resetCurrentRelease thread safely updates the currentRelease.version and then sets the openshift release
+// env var to the desired version. WARNING: This function should only be used for testing purposes as it
+// goes around the desired logic of only setting the version of the cluster for this operator once.
+func resetCurrentReleaseTo(version string) error {
+	currentRelease.mu.Lock()
+	defer currentRelease.mu.Unlock()
+
+	currentRelease.version = nil
+	return os.Setenv(releaseEnvVar, version)
+}

pkg/controller/operators/openshift/helpers.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
 	"strings"
+	"sync"
 
 	semver "github.com/blang/semver/v4"
 	configv1 "github.com/openshift/api/config/v1"
@@ -119,17 +121,17 @@ func transientErrors(err error) error {
 }
 
 func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) {
-	desired, err := desiredRelease(ctx, cli)
+	current, err := getCurrentRelease()
 	if err != nil {
 		return nil, err
 	}
 
-	if desired == nil {
+	if current == nil {
 		// Note: This shouldn't happen
 		return nil, fmt.Errorf("failed to determine current OpenShift Y-stream release")
 	}
 
-	next, err := nextY(*desired)
+	next, err := nextY(*current)
 	if err != nil {
 		return nil, err
 	}
@@ -168,24 +170,58 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 	return incompatible, nil
 }
 
-func desiredRelease(ctx context.Context, cli client.Client) (*semver.Version, error) {
-	cv := configv1.ClusterVersion{}
-	if err := cli.Get(ctx, client.ObjectKey{Name: "version"}, &cv); err != nil { // "version" is the name of OpenShift's ClusterVersion singleton
-		return nil, &transientError{fmt.Errorf("failed to get ClusterVersion: %w", err)}
+type openshiftRelease struct {
+	version *semver.Version
+	mu      sync.Mutex
+}
+
+var (
+	currentRelease = &openshiftRelease{}
+)
+
+const (
+	releaseEnvVar = "RELEASE_VERSION" // OpenShift's env variable for defining the current release
+)
+
+// getCurrentRelease thread safely retrieves the current version of OCP at the time of this operator starting.
+// This is defined by an environment variable that our release manifests define (and get dynamically updated)
+// by OCP. For the purposes of this package, that environment variable is a constant under the name of releaseEnvVar.
+//
+// Note: currentRelease is designed to be a singleton that only gets updated the first time that this function
+// is called. As a result, all calls to this will return the same value even if the releaseEnvVar gets
+// changed during runtime somehow.
+func getCurrentRelease() (*semver.Version, error) {
+	currentRelease.mu.Lock()
+	defer currentRelease.mu.Unlock()
+
+	if currentRelease.version != nil {
+		/*
+			If the version is already set, we don't want to set it again as the currentRelease
+			is designed to be a singleton. If a new version is set, we are making an assumption
+			that this controller will be restarted and thus pull in the new version from the
+			environment into memory.
+
+			Note: sync.Once is not used here as it was difficult to reliably test without hitting
+			race conditions.
+		*/
+		return currentRelease.version, nil
 	}
 
-	v := cv.Status.Desired.Version
-	if v == "" {
-		// The release version hasn't been set yet
-		return nil, fmt.Errorf("desired release version missing from ClusterVersion")
+	// Get the raw version from the releaseEnvVar environment variable
+	raw, ok := os.LookupEnv(releaseEnvVar)
+	if !ok || raw == "" {
+		// No env var set, try again later
+		return nil, fmt.Errorf("desired release version missing from %v env variable", releaseEnvVar)
 	}
 
-	desired, err := semver.ParseTolerant(v)
+	release, err := semver.ParseTolerant(raw)
 	if err != nil {
 		return nil, fmt.Errorf("cluster version has invalid desired release version: %w", err)
 	}
 
-	return &desired, nil
+	currentRelease.version = &release
+
+	return currentRelease.version, nil
 }
 
 func nextY(v semver.Version) (semver.Version, error) {

pkg/controller/operators/openshift/helpers_test.go

@@ -221,22 +221,13 @@ func TestIncompatibleOperators(t *testing.T) {
 	}
 	for _, tt := range []struct {
 		description string
-		cv          configv1.ClusterVersion
+		version     string
 		in          skews
 		expect      expect
 	}{
 		{
 			description: "Compatible",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.0.0",
-					},
-				},
-			},
+			version:     "1.0.0",
 			in: skews{
 				{
 					name:                "almond",
@@ -261,16 +252,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "Incompatible",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.0.0",
-					},
-				},
-			},
+			version:     "1.0.0",
 			in: skews{
 				{
 					name:                "almond",
@@ -331,16 +313,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "Mixed",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.0.0",
-					},
-				},
-			},
+			version:     "1.0.0",
 			in: skews{
 				{
 					name:                "almond",
@@ -376,16 +349,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "Mixed/BadVersion",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.0.0",
-					},
-				},
-			},
+			version:     "1.0.0",
 			in: skews{
 				{
 					name:                "almond",
@@ -424,16 +388,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "EmptyVersion",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "", // This should result in an transient error
-					},
-				},
-			},
+			version:     "", // This should result in an transient error
 			in: skews{
 				{
 					name:                "almond",
@@ -453,16 +408,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "ClusterZ",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.0.1", // Next Y-stream is 1.1.0, NOT 1.1.1
-					},
-				},
-			},
+			version:     "1.0.1", // Next Y-stream is 1.1.0, NOT 1.1.1
 			in: skews{
 				{
 					name:                "beech",
@@ -477,16 +423,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "ClusterPre",
-			cv: configv1.ClusterVersion{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "version",
-				},
-				Status: configv1.ClusterVersionStatus{
-					Desired: configv1.Release{
-						Version: "1.1.0-pre", // Next Y-stream is 1.1.0, NOT 1.2.0
-					},
-				},
-			},
+			version:     "1.1.0-pre", // Next Y-stream is 1.1.0, NOT 1.2.0
 			in: skews{
 				{
 					name:                "almond",
@@ -501,7 +438,9 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 	} {
 		t.Run(tt.description, func(t *testing.T) {
-			objs := []client.Object{tt.cv.DeepCopy()}
+			objs := []client.Object{}
+
+			resetCurrentReleaseTo(tt.version)
 
 			for _, s := range tt.in {
 				csv := &operatorsv1alpha1.ClusterServiceVersion{}

pkg/controller/operators/openshift/options.go

@@ -163,7 +163,6 @@ func WithOLMOperator() ReconcilerOption {
 
 		enqueue := handler.EnqueueRequestsFromMapFunc(config.mapClusterOperator)
 
-		name := "version"
 		originalCSV := predicate.NewPredicateFuncs(func(obj client.Object) bool {
 			csv, ok := obj.(*operatorsv1alpha1.ClusterServiceVersion)
 			if !ok {
@@ -174,8 +173,7 @@ func WithOLMOperator() ReconcilerOption {
 			return !csv.IsCopied() // Keep original CSVs only
 		})
 		config.TweakBuilder = func(bldr *builder.Builder) *builder.Builder {
-			return bldr.Watches(&source.Kind{Type: &operatorsv1alpha1.ClusterServiceVersion{}}, enqueue, builder.WithPredicates(originalCSV)).
-				Watches(&source.Kind{Type: &configv1.ClusterVersion{}}, enqueue, builder.WithPredicates(watchName(&name)))
+			return bldr.Watches(&source.Kind{Type: &operatorsv1alpha1.ClusterServiceVersion{}}, enqueue, builder.WithPredicates(originalCSV))
 		}
 	}
 }