Skip to content

Commit dd4850f

Browse files
awgreeneawgreene
committed
Package Server Manager should enforce expected csv values
**Problem:** The package-server-manifest (PSM) is a downstream specific component responsible for reconciling the package-server csv, which is defined in code as a yaml file. When the PSM reconciles the package-server csv, it: - Attempts to generate the base of the expected CSV from the yaml mentioned above. - Passes the reconcileCSV function as the mutateFn parameter and the expectedCSV as the `obj` parameter into into controller-runtime's createOrUpdate function. Controller runtime's createOrUpdate function will apply the mutateFn function against: - The `obj` you passed in if it does not already exist. OR - The existing on cluster object. In the case where the mutateFn is applied to the applied to the existing object, changes made to the package-server csv yaml are ignored because the mutateFn (ie the reconcileCSV function) doesn't consider changes to the manifest. **Solution:** Update the reconcileCSV function to: - Ensure that expected annotations and labels are present on the csv. - Ensure that the expected spec is present on the csv.
1 parent 11644a5 commit dd4850f

File tree

4 files changed

+147
-74
lines changed

4 files changed

+147
-74
lines changed

‎go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ module github.com/openshift/operator-framework-olm
33
go 1.18
44

55
require (
6+
github.com/blang/semver/v4 v4.0.0
67
github.com/go-bindata/go-bindata/v3 v3.1.3
78
github.com/go-logr/logr v1.2.2
89
github.com/golang/mock v1.6.0
@@ -72,7 +73,6 @@ require (
7273
github.com/asaskevich/govalidator v0.0.0-20200428143746-21a406dcc535 // indirect
7374
github.com/beorn7/perks v1.0.1 // indirect
7475
github.com/blang/semver v3.5.1+incompatible // indirect
75-
github.com/blang/semver/v4 v4.0.0 // indirect
7676
github.com/cespare/xxhash/v2 v2.1.2 // indirect
7777
github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5 // indirect
7878
github.com/containerd/cgroups v1.0.3 // indirect

‎pkg/package-server-manager/config.go

Lines changed: 50 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import (
1111
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1212
"k8s.io/apimachinery/pkg/util/intstr"
1313
"k8s.io/utils/pointer"
14+
15+
"github.com/openshift/operator-framework-olm/pkg/manifests"
1416
)
1517

1618
func getReplicas(ha bool) int32 {
@@ -61,61 +63,85 @@ func getTopologyModeFromInfra(infra *configv1.Infrastructure) bool {
6163
}
6264

6365
// ensureCSV is responsible for ensuring the state of the @csv ClusterServiceVersion custom
66+
// resource matches that of the codified defaults and high availability configurations, where
67+
// codified defaults are defined by the csv returned by the manifests.NewPackageServerCSV
68+
// function.
69+
func ensureCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) (bool, error) {
70+
expectedCSV, err := manifests.NewPackageServerCSV(
71+
manifests.WithName(csv.Name),
72+
manifests.WithNamespace(csv.Namespace),
73+
manifests.WithImage(image),
74+
)
75+
if err != nil {
76+
return false, err
77+
}
78+
79+
ensureCSVHighAvailability(image, expectedCSV, highlyAvailableMode)
80+
81+
var modified bool
82+
83+
for k, v := range expectedCSV.GetLabels() {
84+
if vv, ok := csv.GetLabels()[k]; !ok || vv != v {
85+
log.Info("setting expected label", "key", k, "value", v)
86+
csv.ObjectMeta.Labels[k] = v
87+
modified = true
88+
}
89+
}
90+
91+
for k, v := range expectedCSV.GetAnnotations() {
92+
if vv, ok := csv.GetAnnotations()[k]; !ok || vv != v {
93+
log.Info("setting expected annotation", "key", k, "value", v)
94+
csv.ObjectMeta.Annotations[k] = v
95+
modified = true
96+
}
97+
}
98+
99+
if !reflect.DeepEqual(expectedCSV.Spec, csv.Spec) {
100+
log.Info("updating csv spec")
101+
csv.Spec = expectedCSV.Spec
102+
modified = true
103+
}
104+
105+
if modified {
106+
log.V(3).Info("csv has been modified")
107+
}
108+
109+
return modified, err
110+
}
111+
112+
// ensureCSVHighAvailability is responsible for ensuring the state of the @csv ClusterServiceVersion custom
64113
// resource matches the expected state based on any high availability expectations being exposed.
65-
func ensureCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) bool {
114+
func ensureCSVHighAvailability(image string, csv *olmv1alpha1.ClusterServiceVersion, highlyAvailableMode bool) {
66115
var modified bool
67116

68117
deploymentSpecs := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs
69118
deployment := &deploymentSpecs[0].Spec
70119

71120
currentImage := deployment.Template.Spec.Containers[0].Image
72121
if currentImage != image {
73-
log.Info("updating the image", "old", currentImage, "new", image)
74122
deployment.Template.Spec.Containers[0].Image = image
75123
modified = true
76124
}
77125

78126
expectedReplicas := getReplicas(highlyAvailableMode)
79127
if *deployment.Replicas != expectedReplicas {
80-
log.Info("updating the replica count", "old", deployment.Replicas, "new", expectedReplicas)
81128
deployment.Replicas = pointer.Int32Ptr(expectedReplicas)
82129
modified = true
83130
}
84131

85132
expectedRolloutConfiguration := getRolloutStrategy(highlyAvailableMode)
86133
if !reflect.DeepEqual(deployment.Strategy.RollingUpdate, expectedRolloutConfiguration) {
87-
log.Info("updating the rollout strategy")
88134
deployment.Strategy.RollingUpdate = expectedRolloutConfiguration
89135
modified = true
90136
}
91137

92138
expectedAffinityConfiguration := getAntiAffinityConfig(highlyAvailableMode)
93139
if !reflect.DeepEqual(deployment.Template.Spec.Affinity, expectedAffinityConfiguration) {
94-
log.Info("updating the pod anti-affinity configuration")
95140
deployment.Template.Spec.Affinity = expectedAffinityConfiguration
96141
modified = true
97142
}
98143

99144
if modified {
100-
log.V(3).Info("csv has been modified")
101145
csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec = *deployment
102146
}
103-
104-
return modified
105-
}
106-
107-
func validateCSV(log logr.Logger, csv *olmv1alpha1.ClusterServiceVersion) bool {
108-
deploymentSpecs := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs
109-
if len(deploymentSpecs) != 1 {
110-
log.Info("csv contains more than one or zero nested deployment specs")
111-
return false
112-
}
113-
114-
deployment := &deploymentSpecs[0].Spec
115-
if len(deployment.Template.Spec.Containers) != 1 {
116-
log.Info("csv contains more than one container")
117-
return false
118-
}
119-
120-
return true
121147
}

‎pkg/package-server-manager/controller.go

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package controllers
1818

1919
import (
2020
"context"
21+
"fmt"
2122
"sync"
2223

2324
"github.com/go-logr/logr"
@@ -85,11 +86,12 @@ func (r *PackageServerCSVReconciler) Reconcile(ctx context.Context, req ctrl.Req
8586
res, err := controllerutil.CreateOrUpdate(ctx, r.Client, required, func() error {
8687
return reconcileCSV(r.Log, r.Image, required, highAvailabilityMode)
8788
})
89+
90+
log.Info("reconciliation result", "res", res)
8891
if err != nil {
8992
log.Error(err, "failed to create or update the packageserver csv")
9093
return ctrl.Result{}, nil
9194
}
92-
log.Info("reconciliation result", "res", res)
9395

9496
return ctrl.Result{}, nil
9597
}
@@ -98,19 +100,13 @@ func reconcileCSV(log logr.Logger, image string, csv *olmv1alpha1.ClusterService
98100
if csv.ObjectMeta.CreationTimestamp.IsZero() {
99101
log.Info("attempting to create the packageserver csv")
100102
}
101-
if !validateCSV(log, csv) {
102-
log.Info("updating invalid csv to use the default configuration")
103-
tmp, err := manifests.NewPackageServerCSV(
104-
manifests.WithName(csv.Name),
105-
manifests.WithNamespace(csv.Namespace),
106-
manifests.WithImage(image),
107-
)
108-
if err != nil {
109-
return err
110-
}
111-
csv.Spec = tmp.Spec
103+
104+
modified, err := ensureCSV(log, image, csv, highAvailabilityMode)
105+
if err != nil {
106+
return fmt.Errorf("error ensuring CSV: %v", err)
112107
}
113-
if !ensureCSV(log, image, csv, highAvailabilityMode) {
108+
109+
if modified {
114110
log.V(3).Info("no further updates are necessary to the packageserver csv")
115111
}
116112

‎pkg/package-server-manager/controller_test.go

Lines changed: 87 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@ package controllers
33
import (
44
"testing"
55

6+
semver "github.com/blang/semver/v4"
67
configv1 "github.com/openshift/api/config/v1"
78
"github.com/openshift/operator-framework-olm/pkg/manifests"
9+
"github.com/operator-framework/api/pkg/lib/version"
810
olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
911
"github.com/stretchr/testify/require"
1012
appsv1 "k8s.io/api/apps/v1"
@@ -72,10 +74,39 @@ func intOrStr(val int) *intstr.IntOrString {
7274
return &tmp
7375
}
7476

77+
type testCSVOption func(*olmv1alpha1.ClusterServiceVersion)
78+
79+
func withVersion(v semver.Version) func(*olmv1alpha1.ClusterServiceVersion) {
80+
return func(csv *olmv1alpha1.ClusterServiceVersion) {
81+
csv.Spec.Version = version.OperatorVersion{v}
82+
}
83+
}
84+
85+
func withDescription(description string) func(*olmv1alpha1.ClusterServiceVersion) {
86+
return func(csv *olmv1alpha1.ClusterServiceVersion) {
87+
csv.Spec.Description = description
88+
}
89+
}
90+
91+
func withAffinity(affinity *corev1.Affinity) func(*olmv1alpha1.ClusterServiceVersion) {
92+
return func(csv *olmv1alpha1.ClusterServiceVersion) {
93+
csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Affinity = affinity
94+
}
95+
}
96+
func withRolloutStrategy(strategy *appsv1.RollingUpdateDeployment) func(*olmv1alpha1.ClusterServiceVersion) {
97+
return func(csv *olmv1alpha1.ClusterServiceVersion) {
98+
csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Strategy.RollingUpdate = strategy
99+
}
100+
}
101+
102+
func withReplicas(replicas *int32) func(*olmv1alpha1.ClusterServiceVersion) {
103+
return func(csv *olmv1alpha1.ClusterServiceVersion) {
104+
csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Replicas = replicas
105+
}
106+
}
107+
75108
func newTestCSV(
76-
replicas *int32,
77-
strategy *appsv1.RollingUpdateDeployment,
78-
affinity *corev1.Affinity,
109+
options ...testCSVOption,
79110
) *olmv1alpha1.ClusterServiceVersion {
80111
csv, err := manifests.NewPackageServerCSV(
81112
manifests.WithName(name),
@@ -84,11 +115,10 @@ func newTestCSV(
84115
if err != nil {
85116
return nil
86117
}
87-
deployment := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec
88-
deployment.Template.Spec.Affinity = affinity
89-
deployment.Replicas = replicas
90-
deployment.Strategy.RollingUpdate = strategy
91-
csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec = deployment
118+
119+
for _, o := range options {
120+
o(csv)
121+
}
92122

93123
return csv
94124
}
@@ -133,83 +163,104 @@ func TestEnsureCSV(t *testing.T) {
133163
singleReplicas := pointer.Int32(singleReplicaCount)
134164
image := getImageFromManifest()
135165

166+
type wanted struct {
167+
expectedBool bool
168+
expectedErr error
169+
}
170+
136171
tt := []struct {
137172
name string
138173
inputCSV *olmv1alpha1.ClusterServiceVersion
139174
expectedCSV *olmv1alpha1.ClusterServiceVersion
140175
highlyAvailable bool
141-
want bool
176+
want wanted
142177
}{
143178
{
144179
name: "Modified/HighlyAvailable/CorrectReplicasIncorrectRolling",
145-
want: true,
180+
want: wanted{true, nil},
146181
highlyAvailable: true,
147-
inputCSV: newTestCSV(defaultReplicas, emptyRollout, defaultAffinity),
148-
expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity),
182+
inputCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(emptyRollout), withAffinity(defaultAffinity)),
183+
expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
149184
},
150185
{
151186
name: "Modified/HighlyAvailable/IncorrectReplicasCorrectRolling",
152-
want: true,
187+
want: wanted{true, nil},
153188
highlyAvailable: true,
154-
inputCSV: newTestCSV(singleReplicas, defaultRollout, defaultAffinity),
155-
expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity),
189+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
190+
expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
156191
},
157192
{
158193
name: "Modified/HighlyAvailable/IncorrectPodAntiAffinity",
159-
want: true,
194+
want: wanted{true, nil},
160195
highlyAvailable: true,
161-
inputCSV: newTestCSV(singleReplicas, defaultRollout, newPodAffinity(&corev1.PodAntiAffinity{
196+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(defaultRollout), withAffinity(newPodAffinity(&corev1.PodAntiAffinity{
162197
PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{
163198
{
164199
Weight: 1,
165200
},
166201
},
167-
})),
168-
expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity),
202+
}))),
203+
expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
169204
},
170205
{
171206
name: "NotModified/HighlyAvailable",
172-
want: false,
207+
want: wanted{false, nil},
173208
highlyAvailable: true,
174-
inputCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity),
175-
expectedCSV: newTestCSV(defaultReplicas, defaultRollout, defaultAffinity),
209+
inputCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
210+
expectedCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(defaultRollout), withAffinity(defaultAffinity)),
176211
},
212+
177213
{
178214
name: "Modified/SingleReplica/CorrectReplicasIncorrectRolling",
179-
want: true,
215+
want: wanted{true, nil},
180216
highlyAvailable: false,
181-
inputCSV: newTestCSV(singleReplicas, defaultRollout, &corev1.Affinity{}),
182-
expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}),
217+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(defaultRollout), withAffinity(&corev1.Affinity{})),
218+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
183219
},
184220
{
185221
name: "Modified/SingleReplica/IncorrectReplicasCorrectRolling",
186-
want: true,
222+
want: wanted{true, nil},
187223
highlyAvailable: false,
188-
inputCSV: newTestCSV(defaultReplicas, emptyRollout, &corev1.Affinity{}),
189-
expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}),
224+
inputCSV: newTestCSV(withReplicas(defaultReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
225+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
190226
},
191227
{
192228
name: "Modified/SingleReplica/IncorrectPodAntiAffinity",
193-
want: true,
229+
want: wanted{true, nil},
230+
highlyAvailable: false,
231+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(defaultAffinity)),
232+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
233+
},
234+
{
235+
name: "Modified/SingleReplica/IncorrectVersion",
236+
want: wanted{true, nil},
237+
highlyAvailable: false,
238+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{}), withVersion(semver.Version{Major: 0, Minor: 0, Patch: 0})),
239+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
240+
},
241+
{
242+
name: "Modified/SingleReplica/IncorrectDescription",
243+
want: wanted{true, nil},
194244
highlyAvailable: false,
195-
inputCSV: newTestCSV(singleReplicas, emptyRollout, defaultAffinity),
196-
expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}),
245+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{}), withDescription("foo")),
246+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
197247
},
198248
{
199249
name: "NotModified/SingleReplica",
200-
want: false,
250+
want: wanted{false, nil},
201251
highlyAvailable: false,
202-
inputCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}),
203-
expectedCSV: newTestCSV(singleReplicas, emptyRollout, &corev1.Affinity{}),
252+
inputCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
253+
expectedCSV: newTestCSV(withReplicas(singleReplicas), withRolloutStrategy(emptyRollout), withAffinity(&corev1.Affinity{})),
204254
},
205255
}
206256

207257
for _, tc := range tt {
208258
tc := tc
209259

210260
t.Run(tc.name, func(t *testing.T) {
211-
got := ensureCSV(logger, image, tc.inputCSV, tc.highlyAvailable)
212-
require.EqualValues(t, tc.want, got)
261+
gotBool, gotErr := ensureCSV(logger, image, tc.inputCSV, tc.highlyAvailable)
262+
require.EqualValues(t, tc.want.expectedBool, gotBool)
263+
require.EqualValues(t, tc.want.expectedErr, gotErr)
213264
require.EqualValues(t, tc.inputCSV.Spec, tc.expectedCSV.Spec)
214265
})
215266
}

0 commit comments

Comments
 (0)