Skip to content

[release-4.11] OCPBUGS-1104: Package Server Manager should enforce expected csv values #381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 13, 2022
Merged

[release-4.11] OCPBUGS-1104: Package Server Manager should enforce expected csv values #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
80 changes: 56 additions & 24 deletions pkg/package-server-manager/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
22 changes: 9 additions & 13 deletions pkg/package-server-manager/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controllers

import (
"context"
"fmt"
"sync"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -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
}
Expand All @@ -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")
}

Expand Down
123 changes: 87 additions & 36 deletions pkg/package-server-manager/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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),
Expand All @@ -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
}
Expand Down Expand Up @@ -133,83 +163,104 @@ 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{})),
},
}

for _, tc := range tt {
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)
})
}
Expand Down