Merge branch 'main' into mc/olm-tests

Author: Moritz Clasmeier

cmd/deploy.go

@@ -111,7 +111,7 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 			return errors.New("cannot use both --olm and --konflux flags together (not currently implemented)")
 		}
 		clusterType := env.GetCurrentClusterType()
-		if clusterType != env.InfraOpenShift4 {
+		if !clusterType.IsOpenShift() {
 			return fmt.Errorf("--konflux flag is only supported on OpenShift 4 clusters (current cluster type: %s)", clusterType.String())
 		}
 	}

internal/deployer/deploy_via_operator.go

@@ -55,7 +55,10 @@ func (d *Deployer) ensureOperatorDeployed(ctx context.Context) error {
 	}
 
 	// Detect current operator deployment mode
-	operatorExists, currentMode := d.detectOperatorDeploymentMode(ctx)
+	operatorExists, currentMode, err := d.detectOperatorDeploymentMode(ctx)
+	if err != nil {
+		return fmt.Errorf("detecting operator deployment mode: %w", err)
+	}
 	needsDeployment := false
 	needsTeardown := false
 

internal/deployer/deployer.go

@@ -100,6 +100,10 @@ var (
 	}
 )
 
+const (
+	injectedCABundleConfigMap = "injected-cabundle-stackrox-central-services"
+)
+
 // Deployer is the base deployer for ACS
 type Deployer struct {
 	logger                    *logger.Logger
@@ -222,6 +226,9 @@ func (d *Deployer) deleteCentralResources(ctx context.Context, wait bool) error
 		{Name: "central-db-backup", Kind: "pvc", OwnerName: centralCrName},
 		{Name: "admin-password", Kind: "secret"},
 		{Name: "scanner-db-password", Kind: "secret", OwnerName: centralCrName},
+		// In case the Cluster Network Operator has succeeded in re-creating the injectedCABundleConfigMap
+		// after our operator has already deleted it.
+		{Name: injectedCABundleConfigMap, Kind: "configmap"},
 	} {
 		d.logger.Dimf("Attempting to delete %s/%s", resource.Kind, resource.Name)
 		if resource.OwnerName != "" {
@@ -261,11 +268,7 @@ func (d *Deployer) preventOtherControllersFromReconciling(ctx context.Context) e
 }
 
 func (d *Deployer) preventCABundleInjection(ctx context.Context) error {
-	configMapName := "injected-cabundle-stackrox-central-services"
-
-	if !d.doesResourceExist(ctx, "configmap", configMapName, d.centralNamespace) {
-		return nil
-	}
+	configMapName := injectedCABundleConfigMap
 
 	d.logger.Info("Removing CNO label from injected-cabundle ConfigMap to prevent CNO from injecting the CA bundle during cleanup")
 	_, err := d.runKubectl(ctx, k8s.KubectlOptions{

internal/deployer/operator.go

@@ -91,7 +91,8 @@ func (d *Deployer) downloadAndExtractOperatorBundle(ctx context.Context, bundleI
 	return bundleDir, nil
 }
 
-// identifyCRDFileNames identifies CRD files in the bundle directory
+// identifyCRDFileNames identifies CRD files in the bundle directory.
+// Returns list of CRD files found in the bundle.
 func (d *Deployer) identifyCRDFileNames(bundleDir string) ([]string, error) {
 	var crdFiles []string
 
@@ -108,24 +109,22 @@ func (d *Deployer) identifyCRDFileNames(bundleDir string) ([]string, error) {
 			return nil
 		}
 
-		// TODO(ROX-34499): The following detection logic does not seem particularly robust. We should
-		// probably parse the YAML and check api group and kind fields.
-		name := strings.ToLower(info.Name())
-		if strings.Contains(name, "customresourcedefinition") ||
-			strings.Contains(name, "platform.stackrox.io") ||
-			strings.Contains(name, "config.stackrox.io") {
-			if strings.Contains(name, "clusterserviceversion") {
-				return nil
-			}
+		content, err := os.ReadFile(path)
+		if err != nil {
+			d.logger.Warningf("Failed to read file %q from extracted bundle: %v", path, err)
+			return nil
+		}
 
-			content, err := os.ReadFile(path)
-			if err != nil {
-				return nil
-			}
+		var meta struct {
+			Kind string `yaml:"kind"`
+		}
+		if err := yaml.Unmarshal(content, &meta); err != nil {
+			d.logger.Warningf("Failed to unmarshal file %q from extracted bundle: %v", path, err)
+			return nil
+		}
 
-			if strings.Contains(string(content), "kind: CustomResourceDefinition") {
-				crdFiles = append(crdFiles, path)
-			}
+		if meta.Kind == "CustomResourceDefinition" {
+			crdFiles = append(crdFiles, path)
 		}
 
 		return nil
@@ -202,7 +201,7 @@ func (d *Deployer) getOperatorBundleImage() string {
 
 // ensureKonfluxImageRewriting configures image rewriting for Konflux images
 func (d *Deployer) ensureKonfluxImageRewriting(ctx context.Context) error {
-	if env.GetCurrentClusterType() != env.InfraOpenShift4 {
+	if !env.GetCurrentClusterType().IsOpenShift() {
 		return errors.New("image rewriting for Konflux is only supported on OpenShift4 clusters")
 	}
 
@@ -290,7 +289,7 @@ func (d *Deployer) applyImageContentSourcePolicy(ctx context.Context) error {
 
 // removeKonfluxImageRewriting removes the ImageContentSourcePolicy for Konflux images if it exists
 func (d *Deployer) removeKonfluxImageRewriting(ctx context.Context) error {
-	if env.GetCurrentClusterType() != env.InfraOpenShift4 {
+	if !env.GetCurrentClusterType().IsOpenShift() {
 		return nil
 	}
 
@@ -643,7 +642,10 @@ func (d *Deployer) teardownOperatorNonOLM(ctx context.Context) error {
 
 // teardownOperator removes the operator if it exists, detecting the deployment mode automatically.
 func (d *Deployer) teardownOperator(ctx context.Context) error {
-	operatorExists, operatorMode := d.detectOperatorDeploymentMode(ctx)
+	operatorExists, operatorMode, err := d.detectOperatorDeploymentMode(ctx)
+	if err != nil {
+		return fmt.Errorf("detecting operator deployment mode: %w", err)
+	}
 	if !operatorExists {
 		d.logger.Dim("No operator deployment found, skipping operator teardown")
 		return nil

internal/deployer/operator_olm.go

@@ -72,26 +72,46 @@ func (d *Deployer) deployOperatorViaOLM(ctx context.Context) error {
 	return nil
 }
 
-// checkOLMInstalled checks if OLM is installed in the cluster.
+// checkOLMInstalled checks if OLM is installed in the cluster by verifying
+// the API server is ready to serve the required OLM resource types.
 func (d *Deployer) checkOLMInstalled(ctx context.Context) error {
-	// Check for OLM CRDs
-	requiredCRDs := []string{
+	requiredResources := []string{
 		"catalogsources.operators.coreos.com",
 		"subscriptions.operators.coreos.com",
 		"installplans.operators.coreos.com",
 		"clusterserviceversions.operators.coreos.com",
 	}
 
-	for _, crd := range requiredCRDs {
-		// TODO(ROX-34499): actually this is not the right way to check whether it's safe to create a resource of a given kind.
-		// A CRD can be present, but still being loaded or end up not accepted by the API server.
-		// Instead we should use the `kubectl api-resources` subcommand which exposes the status we're looking for.
-		_, err := d.runKubectl(ctx, k8s.KubectlOptions{
-			Args: []string{"get", "crd", crd},
-		})
-		if err != nil {
-			return fmt.Errorf("OLM not installed: CRD %s not found. Please install OLM first", crd)
+	result, err := d.runKubectl(ctx, k8s.KubectlOptions{
+		Args: []string{"api-resources", "--api-group=operators.coreos.com", "-o", "name"},
+	})
+	if err != nil {
+		if result.Stderr != "" {
+			d.logger.Error("kubectl stderr:")
+			for stderrLine := range strings.SplitSeq(result.Stderr, "\n") {
+				d.logger.Errorf("stderr: %s", stderrLine)
+			}
+		}
+		return fmt.Errorf("failed to query api-group operators.coreos.com: %w", err)
+	}
+
+	available := make(map[string]bool)
+	for line := range strings.SplitSeq(strings.TrimSpace(result.Stdout), "\n") {
+		name := strings.TrimSpace(line)
+		available[name] = true
+	}
+
+	var missingResources []string
+	for _, resource := range requiredResources {
+		if !available[resource] {
+			missingResources = append(missingResources, resource)
+		}
+	}
+	if len(missingResources) > 0 {
+		for _, resource := range missingResources {
+			d.logger.Errorf("OLM resource not served by the API server: %s", resource)
 		}
+		return fmt.Errorf("OLM is not properly installed, %d required resource(s) missing", len(missingResources))
 	}
 
 	d.logger.Success("✓ OLM detected in cluster")
@@ -335,35 +355,34 @@ func (d *Deployer) waitForCSVSuccess(ctx context.Context) error {
 
 // detectOperatorDeploymentMode detects how the operator is currently deployed.
 // Returns (operatorExists bool, isOLM OperatorDeploymentMode)
-func (d *Deployer) detectOperatorDeploymentMode(ctx context.Context) (bool, OperatorDeploymentMode) {
+func (d *Deployer) detectOperatorDeploymentMode(ctx context.Context) (bool, OperatorDeploymentMode, error) {
+	const olmOwnerLabel = "olm.owner"
+
 	// First, check if a Subscription exists (OLM-specific resource)
 	_, err := d.runKubectl(ctx, k8s.KubectlOptions{
 		Args: []string{"get", "subscription", subscriptionName, "-n", operatorNamespace},
 	})
 	if err == nil {
-		return true, OperatorModeOLM
+		return true, OperatorModeOLM, nil
 	}
 
-	// If no subscription, check if operator deployment exists.
-	_, err = d.runKubectl(ctx, k8s.KubectlOptions{
-		Args: []string{"get", "deployment", operatorDeploymentName, "-n", operatorNamespace},
-	})
-	if err == nil {
-		// Deployment exists - check if it has OLM owner labels.
-		result, err := d.runKubectl(ctx, k8s.KubectlOptions{
-			Args: []string{"get", "deployment", operatorDeploymentName, "-n", operatorNamespace, "-o", "jsonpath={.metadata.labels}"},
-		})
-		// TODO(ROX-34499): This is not very robust. Better retrieve a specific label in the `get`
-		// command?
-		if err == nil && strings.Contains(result.Stdout, "olm.owner") {
-			return true, OperatorModeOLM
-		}
-		// Deployment exists without OLM labels = non-OLM deployment.
-		return true, OperatorModeNonOLM
+	// If no subscription, check if operator deployment exists/if it has the expected OLM label.
+	labelValue, err := k8s.RetrieveClusterResourceLabel(ctx, d.logger, operatorNamespace, "deployment", operatorDeploymentName, olmOwnerLabel)
+	if k8s.IsResourceNotFound(err) {
+		// No operator deployment found.
+		return false, OperatorModeNonOLM, nil
+	}
+	if err != nil {
+		return false, OperatorModeNonOLM, err
+	}
+
+	if labelValue == "" {
+		// Deployment exists without OLM labels -> non-OLM deployment.
+		return true, OperatorModeNonOLM, nil
 	}
 
-	// No operator found.
-	return false, OperatorModeNonOLM
+	// Label set -> OLM deployment.
+	return true, OperatorModeOLM, nil
 }
 
 // teardownOperatorOLM removes the operator when installed via OLM.

internal/env/env.go

@@ -33,6 +33,8 @@ const (
 	InfraGKE
 	// InfraOpenShift4 represents an OpenShift 4 cluster
 	InfraOpenShift4
+	// Generic OpenShift4 cluster (e.g. for prow CI)
+	OpenShift4
 	// LocalKind represents a Kind (Kubernetes in Docker) cluster
 	LocalKind
 )
@@ -115,6 +117,8 @@ func (ct ClusterType) String() string {
 	case InfraGKE:
 		return "GKE"
 	case InfraOpenShift4:
+		return "OpenShift4 (infra)"
+	case OpenShift4:
 		return "OpenShift4"
 	case LocalKind:
 		return "Kind"
@@ -123,6 +127,10 @@ func (ct ClusterType) String() string {
 	}
 }
 
+func (ct ClusterType) IsOpenShift() bool {
+	return ct == InfraOpenShift4 || ct == OpenShift4
+}
+
 // KubeConfig represents a simplified kubectl configuration
 type KubeConfig struct {
 	CurrentContext string
@@ -183,17 +191,12 @@ func detectClusterType(config KubeConfig, apiResources []string) ClusterType {
 		return InfraGKE
 	}
 
-	// Check for OpenShift 4 clusters by examining the server hostname
-	if serverURL := getServerURL(config); serverURL != "" {
-		if parsedURL, err := url.Parse(serverURL); err == nil {
-			hostname := parsedURL.Hostname()
-			if strings.HasSuffix(hostname, ".ocp.infra.rox.systems") {
-				// Further verify it's OpenShift 4 by checking the API resources
-				if isOpenShift4(apiResources) {
-					return InfraOpenShift4
-				}
-			}
+	// Check for OpenShift 4 clusters.
+	if isOpenShift4(apiResources) {
+		if isInfraOpenShift4(config) {
+			return InfraOpenShift4
 		}
+		return OpenShift4
 	}
 
 	// Check for Kind clusters
@@ -205,6 +208,19 @@ func detectClusterType(config KubeConfig, apiResources []string) ClusterType {
 	return ClusterTypeUnknown
 }
 
+func isInfraOpenShift4(config KubeConfig) bool {
+	serverURL := getServerURL(config)
+	if serverURL == "" {
+		return false
+	}
+	parsedURL, err := url.Parse(serverURL)
+	if err != nil {
+		return false
+	}
+	hostname := parsedURL.Hostname()
+	return strings.HasSuffix(hostname, ".ocp.infra.rox.systems")
+}
+
 // getServerURL retrieves the server URL from the KubeConfig
 func getServerURL(config KubeConfig) string {
 	if len(config.Clusters) == 0 {

internal/env/env_integration_test.go

@@ -19,7 +19,7 @@ func TestDetectClusterType_Integration(t *testing.T) {
 	t.Logf("Detected cluster type: %s", clusterType)
 
 	// The cluster type should never be invalid (even if Unknown)
-	validTypes := []ClusterType{ClusterTypeUnknown, InfraGKE, InfraOpenShift4, LocalKind}
+	validTypes := []ClusterType{ClusterTypeUnknown, InfraGKE, InfraOpenShift4, OpenShift4, LocalKind}
 	found := false
 	for _, valid := range validTypes {
 		if clusterType == valid {

internal/env/env_test.go

@@ -2,6 +2,8 @@ package env
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestDetectClusterType_GKE(t *testing.T) {
@@ -40,7 +42,7 @@ func TestDetectClusterType_GKE_ExactMatch(t *testing.T) {
 	}
 }
 
-func TestDetectClusterType_OpenShift4(t *testing.T) {
+func TestDetectClusterType_InfraOpenShift4(t *testing.T) {
 	config := KubeConfig{
 		CurrentContext: "admin",
 		Clusters: []KubeCluster{
@@ -58,31 +60,28 @@ func TestDetectClusterType_OpenShift4(t *testing.T) {
 	}
 
 	result := detectClusterType(config, apiResources)
-	if result != InfraOpenShift4 {
-		t.Errorf("detectClusterType() = %v (%s), want %v (%s)", result, result.String(), InfraOpenShift4, InfraOpenShift4.String())
-	}
+	assert.Equal(t, InfraOpenShift4, result)
 }
 
-func TestDetectClusterType_OpenShift4_WrongHostname(t *testing.T) {
+func TestDetectClusterType_OpenShift4(t *testing.T) {
 	config := KubeConfig{
-		CurrentContext: "admin",
+		CurrentContext: "some-context-name",
 		Clusters: []KubeCluster{
 			{
-				Name:   "openshift-cluster",
-				Server: "https://api.my-cluster.example.com:6443",
+				Name:   "some-other-name",
+				Server: "https://my-cluster.example.com:6443",
 			},
 		},
 	}
 	apiResources := []string{
 		"pods",
 		"services",
 		"clusterversions.config.openshift.io",
+		"clusteroperators.config.openshift.io",
 	}
 
 	result := detectClusterType(config, apiResources)
-	if result != ClusterTypeUnknown {
-		t.Errorf("detectClusterType() = %v (%s), want %v (%s)", result, result.String(), ClusterTypeUnknown, ClusterTypeUnknown.String())
-	}
+	assert.Equal(t, OpenShift4, result)
 }
 
 func TestDetectClusterType_OpenShift4_NoAPIResources(t *testing.T) {
@@ -295,6 +294,11 @@ func TestClusterTypeString(t *testing.T) {
 		{
 			name:        "InfraOpenShift4",
 			clusterType: InfraOpenShift4,
+			want:        "OpenShift4 (infra)",
+		},
+		{
+			name:        "OpenShift4",
+			clusterType: OpenShift4,
 			want:        "OpenShift4",
 		},
 		{