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

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

Signed-off-by: Tyler Slaton <[email protected]>
Co-authored-by: Nick Hale <[email protected]>

Author: Tyler Slaton

Diff for: pkg/controller/operators/openshift/clusteroperator_controller_test.go

@@ -2,6 +2,8 @@ package openshift
 
 import (
 	"fmt"
+	"os"
+	"sync"
 
 	semver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo/v2"
@@ -19,7 +21,6 @@ import (
 var _ = Describe("ClusterOperator controller", func() {
 	var (
 		clusterOperatorName types.NamespacedName
-		cv                  *configv1.ClusterVersion
 		csv                 *operatorsv1alpha1.ClusterServiceVersion
 	)
 
@@ -55,33 +56,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())
+		openshiftReleaseOnce = sync.Once{}
 	})
 
 	It("should ensure the ClusterOperator always exists", func() {
@@ -143,10 +122,11 @@ var _ = Describe("ClusterOperator controller", func() {
 		}))
 
 		By("setting upgradeable=false when there's an error determining compatibility")
-		cv.Status = configv1.ClusterVersionStatus{}
 
+		By("changing the openshift environment variable to be invalid and restarting the ClusterOperator")
+		resetOpenshiftReleaseOnceTo("")
 		Eventually(func() error {
-			return k8sClient.Status().Update(ctx, cv)
+			return k8sClient.Delete(ctx, co)
 		}).Should(Succeed())
 
 		Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
@@ -156,18 +136,14 @@ 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:            "Encountered errors while checking compatibility with the next minor version of OpenShift: desired release version missing from OPENSHIFT_RELEASE env variable",
 			LastTransitionTime: fixedNow(),
 		}))
 
-		cv.Status = configv1.ClusterVersionStatus{
-			Desired: configv1.Release{
-				Version: clusterVersion,
-			},
-		}
-
+		By("setting an appropriate variable for the cluster version and restarting the ClusterOperator")
+		resetOpenshiftReleaseOnceTo(clusterVersion)
 		Eventually(func() error {
-			return k8sClient.Status().Update(ctx, cv)
+			return k8sClient.Delete(ctx, co)
 		}).Should(Succeed())
 
 		By("setting upgradeable=false when incompatible operators exist")
@@ -327,3 +303,11 @@ var _ = Describe("ClusterOperator controller", func() {
 		}))
 	})
 })
+
+// resetOpenshiftReleaseOnce creates a new sync.Once for the openshift release and then sets the openshift
+// 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 resetOpenshiftReleaseOnceTo(version string) {
+	openshiftReleaseOnce = *new(sync.Once)
+	os.Setenv(releaseEnvVar, version)
+}

Diff for: 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(ctx)
 	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,39 @@ 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)}
-	}
+var (
+	openshiftReleaseOnce = sync.Once{}
+	openshiftRelease     *semver.Version
+)
 
-	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")
-	}
+const (
+	releaseEnvVar = "OPENSHIFT_RELEASE" // OpenShift's env variable for defining the current release
+)
 
-	desired, err := semver.ParseTolerant(v)
-	if err != nil {
-		return nil, fmt.Errorf("cluster version has invalid desired release version: %w", err)
+func getCurrentRelease(ctx context.Context) (*semver.Version, error) {
+	var doError error
+	openshiftReleaseOnce.Do(func() {
+		// Get the raw version from the OPENSHIFT_RELEASE environment variable
+		raw, ok := os.LookupEnv(releaseEnvVar)
+		if !ok || raw == "" {
+			doError = fmt.Errorf("desired release version missing from OPENSHIFT_RELEASE env variable")
+			return
+		}
+
+		release, err := semver.ParseTolerant(raw)
+		if err != nil {
+			doError = fmt.Errorf("cluster version has invalid desired release version: %w", err)
+			return
+		}
+
+		openshiftRelease = &release
+	})
+
+	if doError != nil {
+		return nil, doError
 	}
 
-	return &desired, nil
+	return openshiftRelease, nil
 }
 
 func nextY(v semver.Version) (semver.Version, error) {

Diff for: 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{}
+
+			resetOpenshiftReleaseOnceTo(tt.version)
 
 			for _, s := range tt.in {
 				csv := &operatorsv1alpha1.ClusterServiceVersion{}
@@ -525,7 +464,7 @@ func TestIncompatibleOperators(t *testing.T) {
 			scheme := runtime.NewScheme()
 			require.NoError(t, AddToScheme(scheme))
 
-			fcli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build()
+			fcli := fake.NewClientBuilder().WithObjects(objs...).WithScheme(scheme).Build()
 			incompatible, err := incompatibleOperators(context.Background(), fcli)
 			if tt.expect.err {
 				require.Error(t, err)

Diff for: pkg/controller/operators/openshift/options.go

@@ -175,7 +175,7 @@ func WithOLMOperator() ReconcilerOption {
 		})
 		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)))
+				Watches(&source.Kind{Type: &configv1.ClusterVersion{}}, enqueue, builder.WithPredicates(watchName(&name))) // Note: is this still needed?
 		}
 	}
 }