[release-4.10] OCPBUGS-1525: Package Server Manager should enforce expected csv values

go.mod

@@ -3,6 +3,7 @@ module github.com/openshift/operator-framework-olm
 go 1.17
 
 require (
+	github.com/blang/semver/v4 v4.0.0
 	github.com/go-bindata/go-bindata/v3 v3.1.3
 	github.com/go-logr/logr v0.4.0
 	github.com/golang/mock v1.6.0
@@ -57,8 +58,7 @@ require (
 	github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/blang/semver v3.5.1+incompatible // indirect
-	github.com/blang/semver/v4 v4.0.0 // indirect
-	github.com/cespare/xxhash/v2 v2.1.1 // indirect
+	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/containerd/cgroups v1.0.3 // indirect
 	github.com/containerd/containerd v1.5.13 // indirect
 	github.com/containerd/continuity v0.1.0 // indirect

go.sum

@@ -205,8 +205,9 @@ github.com/certifi/gocertifi v0.0.0-20191021191039-0944d244cd40/go.mod h1:sGbDF6
 github.com/certifi/gocertifi v0.0.0-20200922220541-2c3bb06c6054/go.mod h1:sGbDF6GwGcLpkNXPUTkMRoywsNa/ol15pxFe6ERfguA=
 github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
-github.com/cespare/xxhash/v2 v2.1.1 h1:6MnRN8NT7+YBpUIWxHtefFZOKTAPgGjpQSxqLNn0+qY=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
+github.com/cespare/xxhash/v2 v2.1.2 h1:YRXhKfTDauu4ajMg1TPgFO5jnlC2HCbmLXMcTG5cbYE=
+github.com/cespare/xxhash/v2 v2.1.2/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5/go.mod h1:/iP1qXHoty45bqomnu2LM+VVyAEdWN+vtSHGlQgyxbw=
 github.com/checkpoint-restore/go-criu/v4 v4.1.0/go.mod h1:xUQBLp4RLc5zJtWY++yjOoMoB5lihDt7fai+75m+rGw=
 github.com/checkpoint-restore/go-criu/v5 v5.0.0/go.mod h1:cfwC0EG7HMUenopBsUf9d89JlCLQIfgVcNsNN0t6T2M=

pkg/package-server-manager/config.go

@@ -11,6 +11,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/pointer"
+
+	"github.com/openshift/operator-framework-olm/pkg/manifests"
 )
 
 func getReplicas(ha bool) int32 {
@@ -61,61 +63,91 @@ func getTopologyModeFromInfra(infra *configv1.Infrastructure) bool {
 }
 
 // ensureCSV is responsible for ensuring the state of the @csv ClusterServiceVersion custom
+// resource matches that of the codified defaults and high availability configurations, where
+// codified defaults are defined by the csv returned by the manifests.NewPackageServerCSV
+// function.
+func ensureCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) (bool, error) {
+	expectedCSV, err := manifests.NewPackageServerCSV(
+		manifests.WithName(csv.Name),
+		manifests.WithNamespace(csv.Namespace),
+		manifests.WithImage(image),
+	)
+	if err != nil {
+		return false, err
+	}
+
+	ensureCSVHighAvailability(image, expectedCSV, highlyAvailableMode)
+
+	var modified bool
+
+	for k, v := range expectedCSV.GetLabels() {
+		if csv.GetLabels() == nil {
+			csv.SetLabels(make(map[string]string))
+		}
+		if vv, ok := csv.GetLabels()[k]; !ok || vv != v {
+			log.Info("setting expected label", "key", k, "value", v)
+			csv.ObjectMeta.Labels[k] = v
+			modified = true
+		}
+	}
+
+	for k, v := range expectedCSV.GetAnnotations() {
+		if csv.GetAnnotations() == nil {
+			csv.SetAnnotations(make(map[string]string))
+		}
+		if vv, ok := csv.GetAnnotations()[k]; !ok || vv != v {
+			log.Info("setting expected annotation", "key", k, "value", v)
+			csv.ObjectMeta.Annotations[k] = v
+			modified = true
+		}
+	}
+
+	if !reflect.DeepEqual(expectedCSV.Spec, csv.Spec) {
+		log.Info("updating csv spec")
+		csv.Spec = expectedCSV.Spec
+		modified = true
+	}
+
+	if modified {
+		log.V(3).Info("csv has been modified")
+	}
+
+	return modified, err
+}
+
+// ensureCSVHighAvailability is responsible for ensuring the state of the @csv ClusterServiceVersion custom
 // resource matches the expected state based on any high availability expectations being exposed.
-func ensureCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) bool {
+func ensureCSVHighAvailability(image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) {
 	var modified bool
 
 	deploymentSpecs := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs
 	deployment := &deploymentSpecs[0].Spec
 
 	currentImage := deployment.Template.Spec.Containers[0].Image
 	if currentImage != image {
-		log.Info("updating the image", "old", currentImage, "new", image)
 		deployment.Template.Spec.Containers[0].Image = image
 		modified = true
 	}
 
 	expectedReplicas := getReplicas(highlyAvailableMode)
 	if *deployment.Replicas != expectedReplicas {
-		log.Info("updating the replica count", "old", deployment.Replicas, "new", expectedReplicas)
 		deployment.Replicas = pointer.Int32Ptr(expectedReplicas)
 		modified = true
 	}
 
 	expectedRolloutConfiguration := getRolloutStrategy(highlyAvailableMode)
 	if !reflect.DeepEqual(deployment.Strategy.RollingUpdate, expectedRolloutConfiguration) {
-		log.Info("updating the rollout strategy")
 		deployment.Strategy.RollingUpdate = expectedRolloutConfiguration
 		modified = true
 	}
 
 	expectedAffinityConfiguration := getAntiAffinityConfig(highlyAvailableMode)
 	if !reflect.DeepEqual(deployment.Template.Spec.Affinity, expectedAffinityConfiguration) {
-		log.Info("updating the pod anti-affinity configuration")
 		deployment.Template.Spec.Affinity = expectedAffinityConfiguration
 		modified = true
 	}
 
 	if modified {
-		log.V(3).Info("csv has been modified")
 		csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec = *deployment
 	}
-
-	return modified
-}
-
-func validateCSV(log logr.Logger, csv *olmv1alpha1.ClusterServiceVersion) bool {
-	deploymentSpecs := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs
-	if len(deploymentSpecs) != 1 {
-		log.Info("csv contains more than one or zero nested deployment specs")
-		return false
-	}
-
-	deployment := &deploymentSpecs[0].Spec
-	if len(deployment.Template.Spec.Containers) != 1 {
-		log.Info("csv contains more than one container")
-		return false
-	}
-
-	return true
 }

pkg/package-server-manager/controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 	"sync"
 
 	"github.com/go-logr/logr"
@@ -85,11 +86,12 @@ func (r *PackageServerCSVReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	res, err := controllerutil.CreateOrUpdate(ctx, r.Client, required, func() error {
 		return reconcileCSV(r.Log, r.Image, required, highAvailabilityMode)
 	})
+
+	log.Info("reconciliation result", "res", res)
 	if err != nil {
 		log.Error(err, "failed to create or update the packageserver csv")
 		return ctrl.Result{}, nil
 	}
-	log.Info("reconciliation result", "res", res)
 
 	return ctrl.Result{}, nil
 }
@@ -98,19 +100,13 @@ func reconcileCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterService
 	if csv.ObjectMeta.CreationTimestamp.IsZero() {
 		log.Info("attempting to create the packageserver csv")
 	}
-	if !validateCSV(log, csv) {
-		log.Info("updating invalid csv to use the default configuration")
-		tmp, err := manifests.NewPackageServerCSV(
-			manifests.WithName(csv.Name),
-			manifests.WithNamespace(csv.Namespace),
-			manifests.WithImage(image),
-		)
-		if err != nil {
-			return err
-		}
-		csv.Spec = tmp.Spec
+
+	modified, err := ensureCSV(log, image, csv, highAvailabilityMode)
+	if err != nil {
+		return fmt.Errorf("error ensuring CSV: %v", err)
 	}
-	if !ensureCSV(log, image, csv, highAvailabilityMode) {
+
+	if !modified {
 		log.V(3).Info("no further updates are necessary to the packageserver csv")
 	}