fix(openshift): update openshiftRelease to be protected by a mutex

Signed-off-by: Tyler Slaton <[email protected]>

Author: Tyler Slaton

pkg/controller/operators/openshift/clusteroperator_controller_test.go

@@ -3,7 +3,6 @@ package openshift
 import (
 	"fmt"
 	"os"
-	"sync"
 
 	semver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo/v2"
@@ -60,7 +59,7 @@ var _ = Describe("ClusterOperator controller", func() {
 	})
 
 	AfterEach(func() {
-		openshiftReleaseOnce = sync.Once{}
+		resetCurrentReleaseTo(clusterVersion)
 	})
 
 	It("should ensure the ClusterOperator always exists", func() {
@@ -124,7 +123,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		By("setting upgradeable=false when there's an error determining compatibility")
 
 		By("changing the openshift environment variable to be invalid and restarting the ClusterOperator")
-		resetOpenshiftReleaseOnceTo("")
+		resetCurrentReleaseTo("")
 		Eventually(func() error {
 			return k8sClient.Delete(ctx, co)
 		}).Should(Succeed())
@@ -141,7 +140,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		}))
 
 		By("setting an appropriate variable for the cluster version and restarting the ClusterOperator")
-		resetOpenshiftReleaseOnceTo(clusterVersion)
+		resetCurrentReleaseTo(clusterVersion)
 		Eventually(func() error {
 			return k8sClient.Delete(ctx, co)
 		}).Should(Succeed())
@@ -304,15 +303,13 @@ var _ = Describe("ClusterOperator controller", func() {
 	})
 })
 
-// resetOpenshiftReleaseOnce creates a new sync.Once for the openshift release and then sets the openshift
+// 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.
-var resetMutex sync.Mutex
+func resetCurrentReleaseTo(version string) {
+	currentRelease.mu.Lock()
+	defer currentRelease.mu.Unlock()
 
-func resetOpenshiftReleaseOnceTo(version string) {
-	resetMutex.Lock()
-	defer resetMutex.Unlock()
-
-	openshiftReleaseOnce = *new(sync.Once)
+	currentRelease.version = nil
 	os.Setenv(releaseEnvVar, version)
 }

pkg/controller/operators/openshift/helpers.go

@@ -121,7 +121,7 @@ func transientErrors(err error) error {
 }
 
 func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) {
-	current, err := getCurrentRelease(ctx)
+	current, err := getCurrentRelease()
 	if err != nil {
 		return nil, err
 	}
@@ -170,39 +170,51 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 	return incompatible, nil
 }
 
+type openshiftRelease struct {
+	version *semver.Version
+	mu      sync.Mutex
+}
+
 var (
-	openshiftReleaseOnce = sync.Once{}
-	openshiftRelease     *semver.Version
+	currentRelease = &openshiftRelease{}
 )
 
 const (
 	releaseEnvVar = "OPENSHIFT_RELEASE" // OpenShift's env variable for defining the current release
 )
 
-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
-		}
+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
+	}
 
-		openshiftRelease = &release
-	})
+	// Get the raw version from the OPENSHIFT_RELEASE 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 OPENSHIFT_RELEASE env variable")
+	}
 
-	if doError != nil {
-		return nil, doError
+	release, err := semver.ParseTolerant(raw)
+	if err != nil {
+		return nil, fmt.Errorf("cluster version has invalid desired release version: %w", err)
 	}
 
-	return openshiftRelease, nil
+	currentRelease.version = &release
+
+	return currentRelease.version, nil
 }
 
 func nextY(v semver.Version) (semver.Version, error) {

pkg/controller/operators/openshift/helpers_test.go

@@ -440,7 +440,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		t.Run(tt.description, func(t *testing.T) {
 			objs := []client.Object{}
 
-			resetOpenshiftReleaseOnceTo(tt.version)
+			resetCurrentReleaseTo(tt.version)
 
 			for _, s := range tt.in {
 				csv := &operatorsv1alpha1.ClusterServiceVersion{}