diff --git a/go.mod b/go.mod index 97e9c9f63a..3a0f668cb2 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/openshift/operator-framework-olm go 1.18 require ( + github.com/blang/semver/v4 v4.0.0 github.com/go-bindata/go-bindata/v3 v3.1.3 github.com/go-logr/logr v1.2.2 github.com/golang/mock v1.6.0 @@ -72,7 +73,6 @@ 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.2 // indirect github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5 // indirect github.com/containerd/cgroups v1.0.3 // indirect diff --git a/pkg/package-server-manager/config.go b/pkg/package-server-manager/config.go index 1e8a2c79c5..5ef355020d 100644 --- a/pkg/package-server-manager/config.go +++ b/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,8 +63,61 @@ 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 @@ -70,52 +125,29 @@ func ensureCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterServiceVer 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 } diff --git a/pkg/package-server-manager/controller.go b/pkg/package-server-manager/controller.go index 9930fac50f..90499bb063 100644 --- a/pkg/package-server-manager/controller.go +++ b/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") } diff --git a/pkg/package-server-manager/controller_test.go b/pkg/package-server-manager/controller_test.go index 4ca1e4d340..f91a5f2281 100644 --- a/pkg/package-server-manager/controller_test.go +++ b/pkg/package-server-manager/controller_test.go @@ -3,8 +3,10 @@ package controllers import ( "testing" + semver "github.com/blang/semver/v4" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/operator-framework-olm/pkg/manifests" + "github.com/operator-framework/api/pkg/lib/version" olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" @@ -72,10 +74,39 @@ func intOrStr(val int) *intstr.IntOrString { return &tmp } +type testCSVOption func(*olmv1alpha1.ClusterServiceVersion) + +func withVersion(v semver.Version) func(*olmv1alpha1.ClusterServiceVersion) { + return func(csv *olmv1alpha1.ClusterServiceVersion) { + csv.Spec.Version = version.OperatorVersion{v} + } +} + +func withDescription(description string) func(*olmv1alpha1.ClusterServiceVersion) { + return func(csv *olmv1alpha1.ClusterServiceVersion) { + csv.Spec.Description = description + } +} + +func withAffinity(affinity *corev1.Affinity) func(*olmv1alpha1.ClusterServiceVersion) { + return func(csv *olmv1alpha1.ClusterServiceVersion) { + csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Affinity = affinity + } +} +func withRollingUpdateStrategy(strategy *appsv1.RollingUpdateDeployment) func(*olmv1alpha1.ClusterServiceVersion) { + return func(csv *olmv1alpha1.ClusterServiceVersion) { + csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Strategy.RollingUpdate = strategy + } +} + +func withReplicas(replicas *int32) func(*olmv1alpha1.ClusterServiceVersion) { + return func(csv *olmv1alpha1.ClusterServiceVersion) { + csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Replicas = replicas + } +} + func newTestCSV( - replicas *int32, - strategy *appsv1.RollingUpdateDeployment, - affinity *corev1.Affinity, + options ...testCSVOption, ) *olmv1alpha1.ClusterServiceVersion { csv, err := manifests.NewPackageServerCSV( manifests.WithName(name), @@ -84,11 +115,10 @@ func newTestCSV( if err != nil { return nil } - deployment := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec - deployment.Template.Spec.Affinity = affinity - deployment.Replicas = replicas - deployment.Strategy.RollingUpdate = strategy - csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec = deployment + + for _, o := range options { + o(csv) + } return csv } @@ -133,74 +163,94 @@ func TestEnsureCSV(t *testing.T) { singleReplicas := pointer.Int32(singleReplicaCount) image := getImageFromManifest() + type wanted struct { + expectedBool bool + expectedErr error + } + tt := []struct { name string inputCSV *olmv1alpha1.ClusterServiceVersion expectedCSV *olmv1alpha1.ClusterServiceVersion highlyAvailable bool - want bool + want wanted }{ { name: "Modified/HighlyAvailable/CorrectReplicasIncorrectRolling", - want: true, + want: wanted{true, nil}, highlyAvailable: true, - inputCSV: newTestCSV(defaultReplicas, emptyRollout, defaultAffinity), - expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity), + inputCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(defaultAffinity)), + expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), }, { name: "Modified/HighlyAvailable/IncorrectReplicasCorrectRolling", - want: true, + want: wanted{true, nil}, highlyAvailable: true, - inputCSV: newTestCSV(singleReplicas, defaultRollout, defaultAffinity), - expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity), + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), + expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), }, { name: "Modified/HighlyAvailable/IncorrectPodAntiAffinity", - want: true, + want: wanted{true, nil}, highlyAvailable: true, - inputCSV: newTestCSV(singleReplicas, defaultRollout, newPodAffinity(&corev1.PodAntiAffinity{ + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(newPodAffinity(&corev1.PodAntiAffinity{ PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ { Weight: 1, }, }, - })), - expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity), + }))), + expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), }, { name: "NotModified/HighlyAvailable", - want: false, + want: wanted{false, nil}, highlyAvailable: true, - inputCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity), - expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity), + inputCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), + expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(defaultAffinity)), }, + { name: "Modified/SingleReplica/CorrectReplicasIncorrectRolling", - want: true, + want: wanted{true, nil}, highlyAvailable: false, - inputCSV: newTestCSV(singleReplicas, defaultRollout, &corev1.Affinity{}), - expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}), + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(defaultRollout), withAffinity(&corev1.Affinity{})), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), }, { name: "Modified/SingleReplica/IncorrectReplicasCorrectRolling", - want: true, + want: wanted{true, nil}, highlyAvailable: false, - inputCSV: newTestCSV(defaultReplicas, emptyRollout, &corev1.Affinity{}), - expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}), + inputCSV: newTestCSV(withReplicas(defaultReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), }, { name: "Modified/SingleReplica/IncorrectPodAntiAffinity", - want: true, + want: wanted{true, nil}, + highlyAvailable: false, + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(defaultAffinity)), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), + }, + { + name: "Modified/SingleReplica/IncorrectVersion", + want: wanted{true, nil}, + highlyAvailable: false, + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{}), withVersion(semver.Version{Major: 0, Minor: 0, Patch: 0})), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), + }, + { + name: "Modified/SingleReplica/IncorrectDescription", + want: wanted{true, nil}, highlyAvailable: false, - inputCSV: newTestCSV(singleReplicas, emptyRollout, defaultAffinity), - expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}), + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{}), withDescription("foo")), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), }, { name: "NotModified/SingleReplica", - want: false, + want: wanted{false, nil}, highlyAvailable: false, - inputCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}), - expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}), + inputCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), + expectedCSV: newTestCSV(withReplicas(singleReplicas), withRollingUpdateStrategy(emptyRollout), withAffinity(&corev1.Affinity{})), }, } @@ -208,8 +258,9 @@ func TestEnsureCSV(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { - got := ensureCSV(logger, image, tc.inputCSV, tc.highlyAvailable) - require.EqualValues(t, tc.want, got) + gotBool, gotErr := ensureCSV(logger, image, tc.inputCSV, tc.highlyAvailable) + require.EqualValues(t, tc.want.expectedBool, gotBool) + require.EqualValues(t, tc.want.expectedErr, gotErr) require.EqualValues(t, tc.inputCSV.Spec, tc.expectedCSV.Spec) }) }