Merge pull request #1145 from jianzhangbjz/OCPBUGS-63436-4.19

OCPBUGS-64925: Fix TOCTOU race condition in ensureInstallPlan (#3682)

Author: openshift-merge-bot[bot]

Makefile

@@ -157,6 +157,17 @@ vendor:
 	go mod tidy
 	go mod vendor
 	go mod verify
+	# Copy required CRD manifests for tests
+	@OPENSHIFT_API_VERSION=$$(go list -m -f '{{.Version}}' github.com/openshift/api) && \
+	GOMODCACHE=$$(go env GOMODCACHE) && \
+	OPENSHIFT_API_PATH="$$GOMODCACHE/github.com/openshift/api@$$OPENSHIFT_API_VERSION" && \
+	if [ -d "$$OPENSHIFT_API_PATH/config/v1/zz_generated.crd-manifests" ]; then \
+		mkdir -p vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests && \
+		cp -f "$$OPENSHIFT_API_PATH/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusteroperators.crd.yaml" \
+			vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/ 2>/dev/null || true && \
+		cp -f "$$OPENSHIFT_API_PATH/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml" \
+			vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/ 2>/dev/null || true; \
+	fi
 
 .PHONY: manifests
 manifests: ## Generate manifests

go.mod

@@ -14,7 +14,7 @@ require (
 	github.com/maxbrunsfeld/counterfeiter/v6 v6.11.2
 	github.com/mikefarah/yq/v3 v3.0.0-20201202084205-8846255d1c37
 	github.com/onsi/ginkgo/v2 v2.23.3
-	github.com/openshift/api v3.9.0+incompatible
+	github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc
 	github.com/operator-framework/api v0.30.0
 	github.com/operator-framework/operator-lifecycle-manager v0.0.0-00010101000000-000000000000
 	github.com/operator-framework/operator-registry v1.51.0
@@ -227,7 +227,7 @@ require (
 
 replace (
 	// controller runtime
-	github.com/openshift/api => github.com/openshift/api v0.0.0-20210517065120-b325f58df679
+	github.com/openshift/api => github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc
 	github.com/openshift/client-go => github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0 // release-4.5
 
 	// use staged repositories

go.sum

@@ -22,7 +22,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/onsi/ginkgo/v2 v2.23.3
 	github.com/onsi/gomega v1.36.2
-	github.com/openshift/api v3.9.0+incompatible
+	github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc
 	github.com/openshift/client-go v0.0.0-20220525160904-9e1acff93e4a
 	github.com/operator-framework/api v0.30.0
 	github.com/operator-framework/operator-registry v1.51.0
@@ -191,7 +191,7 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.63.2
 
 replace (
 	// controller runtime
-	github.com/openshift/api => github.com/openshift/api v0.0.0-20221021112143-4226c2167e40 // release-4.12
+	github.com/openshift/api => github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc // release-4.19
 	github.com/openshift/client-go => github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c // release-4.12
 )
 

staging/operator-lifecycle-manager/go.mod

@@ -1823,8 +1823,8 @@ github.com/opencontainers/image-spec v1.1.0 h1:8SG7/vwALn54lVB/0yZ/MMwhFrPYtpEHQ
 github.com/opencontainers/image-spec v1.1.0/go.mod h1:W4s4sFTMaBeK1BQLXbG4AdM2szdn85PY75RI83NrTrM=
 github.com/opencontainers/runtime-spec v1.2.0 h1:z97+pHb3uELt/yiAWD691HNHQIF07bE7dzrbT927iTk=
 github.com/opencontainers/runtime-spec v1.2.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
-github.com/openshift/api v0.0.0-20221021112143-4226c2167e40 h1:PxjGCA72RtsdHWToZLkjjeWm7WXXx4cuv0u4gtvLbrk=
-github.com/openshift/api v0.0.0-20221021112143-4226c2167e40/go.mod h1:aQ6LDasvHMvHZXqLHnX2GRmnfTWCF/iIwz8EMTTIE9A=
+github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc h1:BGKjHtYzBweOSu1UwTnNqtPbJZ4VzOTqVFlUDpP+6U8=
+github.com/openshift/api v0.0.0-20250425163235-9b80d67473bc/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
 github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c h1:CV76yFOTXmq9VciBR3Bve5ZWzSxdft7gaMVB3kS0rwg=
 github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c/go.mod h1:lFMO8mLHXWFzSdYvGNo8ivF9SfF6zInA8ZGw4phRnUE=
 github.com/operator-framework/api v0.30.0 h1:44hCmGnEnZk/Miol5o44dhSldNH0EToQUG7vZTl29kk=

staging/operator-lifecycle-manager/go.sum

@@ -1635,12 +1635,6 @@ func (o *Operator) ensureInstallPlan(logger *logrus.Entry, namespace string, gen
 		return nil, nil
 	}
 
-	// Check if any existing installplans are creating the same resources
-	installPlans, err := o.listInstallPlans(namespace)
-	if err != nil {
-		return nil, err
-	}
-
 	// There are multiple(2) worker threads process the namespaceQueue.
 	// Both worker can work at the same time when 2 separate updates are made for the namespace.
 	// The following sequence causes 2 installplans are created for a subscription
@@ -1661,6 +1655,13 @@ func (o *Operator) ensureInstallPlan(logger *logrus.Entry, namespace string, gen
 	o.muInstallPlan.Lock()
 	defer o.muInstallPlan.Unlock()
 
+	// Check if any existing installplans are creating the same resources
+	// This must be done inside the lock to prevent TOCTOU race condition
+	installPlans, err := o.listInstallPlans(namespace)
+	if err != nil {
+		return nil, err
+	}
+
 	for _, installPlan := range installPlans {
 		if installPlan.Spec.Generation == gen {
 			return reference.GetReference(installPlan)

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"reflect"
 	"strings"
+	"sync"
 	"testing"
 	"testing/quick"
 	"time"
@@ -2462,3 +2463,107 @@ func hasExpectedCondition(ip *v1alpha1.InstallPlan, expectedCondition v1alpha1.I
 	}
 	return false
 }
+
+// TestEnsureInstallPlanConcurrency tests that concurrent calls to ensureInstallPlan
+// do not create duplicate InstallPlans for the same subscription.
+// This test verifies the fix for a TOCTOU race condition where multiple worker threads
+// could create duplicate InstallPlans if they both check for existing plans before either
+// has created one.
+func TestEnsureInstallPlanConcurrency(t *testing.T) {
+	namespace := "test-ns"
+	gen := 1
+	numGoroutines := 10
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	// Create a fake operator
+	op, err := NewFakeOperator(ctx, namespace, []string{namespace})
+	require.NoError(t, err)
+
+	// Create a test subscription
+	sub := &v1alpha1.Subscription{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-sub",
+			Namespace: namespace,
+			UID:       types.UID("test-uid"),
+		},
+		Spec: &v1alpha1.SubscriptionSpec{
+			Package: "test-package",
+		},
+	}
+
+	// Create test steps for the InstallPlan
+	steps := []*v1alpha1.Step{
+		{
+			Resolving: "test-csv",
+			Resource: v1alpha1.StepResource{
+				CatalogSource:          "test-catalog",
+				CatalogSourceNamespace: namespace,
+				Group:                  "operators.coreos.com",
+				Version:                "v1alpha1",
+				Kind:                   "ClusterServiceVersion",
+				Name:                   "test-csv",
+				Manifest:               toManifest(t, csv("test-csv", namespace, nil, nil)),
+			},
+			Status: v1alpha1.StepStatusUnknown,
+		},
+	}
+
+	// Use WaitGroup to synchronize goroutines
+	var wg sync.WaitGroup
+	// Use a channel to collect results
+	results := make(chan *corev1.ObjectReference, numGoroutines)
+	// Use a sync.Once-like mechanism to start all goroutines at roughly the same time
+	startBarrier := make(chan struct{})
+
+	logger := logrus.NewEntry(logrus.New())
+
+	// Launch multiple goroutines that will call ensureInstallPlan concurrently
+	for i := 0; i < numGoroutines; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			// Wait for the start signal
+			<-startBarrier
+
+			// Call ensureInstallPlan
+			ref, err := op.ensureInstallPlan(logger, namespace, gen, []*v1alpha1.Subscription{sub}, v1alpha1.ApprovalAutomatic, steps, nil)
+			require.NoError(t, err)
+			results <- ref
+		}()
+	}
+
+	// Start all goroutines
+	close(startBarrier)
+
+	// Wait for all goroutines to complete
+	wg.Wait()
+	close(results)
+
+	// Collect all results
+	var refs []*corev1.ObjectReference
+	for ref := range results {
+		refs = append(refs, ref)
+	}
+
+	// Verify we got the expected number of results
+	require.Len(t, refs, numGoroutines, "should have received results from all goroutines")
+
+	// Verify all refs point to the same InstallPlan
+	firstRef := refs[0]
+	for i, ref := range refs {
+		require.Equal(t, firstRef.Name, ref.Name, "goroutine %d returned different InstallPlan name", i)
+		require.Equal(t, firstRef.Namespace, ref.Namespace, "goroutine %d returned different InstallPlan namespace", i)
+		require.Equal(t, firstRef.UID, ref.UID, "goroutine %d returned different InstallPlan UID", i)
+	}
+
+	// Verify only one InstallPlan was created in the cluster
+	ipList, err := op.client.OperatorsV1alpha1().InstallPlans(namespace).List(ctx, metav1.ListOptions{})
+	require.NoError(t, err)
+	require.Len(t, ipList.Items, 1, "exactly one InstallPlan should have been created")
+
+	// Verify the created InstallPlan has the correct generation
+	createdIP := &ipList.Items[0]
+	require.Equal(t, gen, createdIP.Spec.Generation, "InstallPlan should have the correct generation")
+}

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go

@@ -5853,6 +5853,12 @@ func RequireObjectsInCache(t *testing.T, lister operatorlister.OperatorLister, n
 			fetched, err = lister.RbacV1().RoleBindingLister().RoleBindings(namespace).Get(o.GetName())
 		case *v1alpha1.ClusterServiceVersion:
 			fetched, err = lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(namespace).Get(o.GetName())
+			if err != nil {
+				if apierrors.IsNotFound(err) {
+					return err
+				}
+				return errors.Join(err, fmt.Errorf("namespace: %v, error: %v", namespace, err))
+			}
 			// We don't care about finalizers
 			object.(*v1alpha1.ClusterServiceVersion).Finalizers = nil
 			fetched.(*v1alpha1.ClusterServiceVersion).Finalizers = nil

staging/operator-lifecycle-manager/pkg/controller/operators/olm/operator_test.go

@@ -2,6 +2,7 @@ package openshift
 
 import (
 	"context"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -46,15 +47,25 @@ const (
 var _ = BeforeSuite(func() {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
-	base := filepath.Join("..", "..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")
+	// Try to find CRDs in vendor first, fall back to GOMODCACHE if not found
+	base := filepath.Join("..", "..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests")
+	if _, err := os.Stat(base); os.IsNotExist(err) {
+		// Fall back to GOMODCACHE
+		gomodcache := os.Getenv("GOMODCACHE")
+		if gomodcache == "" {
+			gomodcache = filepath.Join(os.Getenv("HOME"), "go", "pkg", "mod")
+		}
+		base = filepath.Join(gomodcache, "github.com", "openshift", "api@v0.0.0-20250425163235-9b80d67473bc", "config", "v1", "zz_generated.crd-manifests")
+	}
+
 	testEnv = &envtest.Environment{
 		ErrorIfCRDPathMissing: true,
 		CRDs: []*apiextensionsv1.CustomResourceDefinition{
 			crds.ClusterServiceVersion(),
 		},
 		CRDDirectoryPaths: []string{
-			filepath.Join(base, "0000_00_cluster-version-operator_01_clusteroperator.crd.yaml"),
-			filepath.Join(base, "0000_00_cluster-version-operator_01_clusterversion.crd.yaml"),
+			filepath.Join(base, "0000_00_cluster-version-operator_01_clusteroperators.crd.yaml"),
+			filepath.Join(base, "0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml"),
 		},
 	}