Skip to content

Commit 9eaf62f

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #2270 from njhale/fix/block-openshift-upgrades
fix(openshift): handle unquoted short max versions
2 parents 00402eb + 1ebebdb commit 9eaf62f

File tree

4 files changed

+97
-29
lines changed

4 files changed

+97
-29
lines changed

‎pkg/controller/operators/openshift/clusteroperator_controller_test.go

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package openshift
22

33
import (
4+
"fmt"
5+
46
semver "github.com/blang/semver/v4"
57
. "github.com/onsi/ginkgo"
68
. "github.com/onsi/gomega"
@@ -161,7 +163,7 @@ var _ = Describe("ClusterOperator controller", func() {
161163
withMax := func(version string) map[string]string {
162164
maxProperty := &api.Property{
163165
Type: MaxOpenShiftVersionProperty,
164-
Value: `"` + version + `"`, // Wrap in quotes so we don't break property marshaling
166+
Value: version,
165167
}
166168
value, err := projection.PropertiesAnnotationFromPropertyList([]*api.Property{maxProperty})
167169
Expect(err).ToNot(HaveOccurred())
@@ -170,7 +172,7 @@ var _ = Describe("ClusterOperator controller", func() {
170172
projection.PropertiesAnnotationKey: value,
171173
}
172174
}
173-
incompatible.SetAnnotations(withMax(clusterVersion))
175+
incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, clusterVersion))) // Wrap in quotes so we don't break property marshaling
174176

175177
Eventually(func() error {
176178
return k8sClient.Create(ctx, incompatible)
@@ -202,7 +204,7 @@ var _ = Describe("ClusterOperator controller", func() {
202204
// Set compatibility to the next minor version
203205
next := semver.MustParse(clusterVersion)
204206
Expect(next.IncrementMinor()).To(Succeed())
205-
incompatible.SetAnnotations(withMax(next.String()))
207+
incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, next.String())))
206208

207209
Eventually(func() error {
208210
return k8sClient.Update(ctx, incompatible)
@@ -216,5 +218,32 @@ var _ = Describe("ClusterOperator controller", func() {
216218
Status: configv1.ConditionTrue,
217219
LastTransitionTime: fixedNow(),
218220
}))
221+
222+
By("understanding unquoted short max versions; e.g. X.Y")
223+
// Mimic common pipeline shorthand
224+
v := semver.MustParse(clusterVersion)
225+
short := fmt.Sprintf("%d.%d", v.Major, v.Minor)
226+
incompatible.SetAnnotations(withMax(short))
227+
228+
Eventually(func() error {
229+
return k8sClient.Update(ctx, incompatible)
230+
}).Should(Succeed())
231+
232+
Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
233+
err := k8sClient.Get(ctx, clusterOperatorName, co)
234+
return co.Status.Conditions, err
235+
}, timeout).Should(ContainElement(configv1.ClusterOperatorStatusCondition{
236+
Type: configv1.OperatorUpgradeable,
237+
Status: configv1.ConditionFalse,
238+
Reason: IncompatibleOperatorsInstalled,
239+
Message: skews{
240+
{
241+
namespace: ns.GetName(),
242+
name: incompatible.GetName(),
243+
maxOpenShiftVersion: short + ".0",
244+
},
245+
}.String(),
246+
LastTransitionTime: fixedNow(),
247+
}))
219248
})
220249
})

‎pkg/controller/operators/openshift/helpers.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package openshift
22

33
import (
44
"context"
5-
"encoding/json"
65
"fmt"
76
"strings"
87

@@ -190,19 +189,15 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.
190189
continue
191190
}
192191

193-
var value string
194-
if err := json.Unmarshal([]byte(property.Value), &value); err != nil {
195-
errs = append(errs, err)
196-
continue
197-
}
198-
192+
value := strings.Trim(property.Value, "\"")
199193
if value == "" {
200194
continue
201195
}
202196

203197
version, err := semver.ParseTolerant(value)
204198
if err != nil {
205199
errs = append(errs, err)
200+
continue
206201
}
207202

208203
if max == nil {

‎pkg/controller/operators/openshift/helpers_test.go

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package openshift
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
67

78
semver "github.com/blang/semver/v4"
@@ -341,6 +342,11 @@ func TestIncompatibleOperators(t *testing.T) {
341342
namespace: "default",
342343
maxOpenShiftVersion: "1.0.0",
343344
},
345+
{
346+
name: "chestnut",
347+
namespace: "default",
348+
maxOpenShiftVersion: "1.0",
349+
},
344350
},
345351
expect: expect{
346352
err: false,
@@ -350,6 +356,11 @@ func TestIncompatibleOperators(t *testing.T) {
350356
namespace: "default",
351357
maxOpenShiftVersion: "1.0.0",
352358
},
359+
{
360+
name: "chestnut",
361+
namespace: "default",
362+
maxOpenShiftVersion: "1.0.0",
363+
},
353364
},
354365
},
355366
},
@@ -463,7 +474,10 @@ func TestIncompatibleOperators(t *testing.T) {
463474

464475
func TestMaxOpenShiftVersion(t *testing.T) {
465476
mustParse := func(s string) *semver.Version {
466-
version := semver.MustParse(s)
477+
version, err := semver.ParseTolerant(s)
478+
if err != nil {
479+
panic(fmt.Sprintf("bad version given for test case: %s", err))
480+
}
467481
return &version
468482
}
469483

@@ -485,7 +499,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
485499
},
486500
{
487501
description: "Nothing",
488-
in: []string{""},
502+
in: []string{`""`},
489503
expect: expect{
490504
err: false,
491505
max: nil,
@@ -494,8 +508,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
494508
{
495509
description: "Nothing/Mixed",
496510
in: []string{
497-
"",
498-
"1.0.0",
511+
`""`,
512+
`"1.0.0"`,
499513
},
500514
expect: expect{
501515
err: false,
@@ -504,7 +518,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
504518
},
505519
{
506520
description: "Garbage",
507-
in: []string{"bad_version"},
521+
in: []string{`"bad_version"`},
508522
expect: expect{
509523
err: true,
510524
max: nil,
@@ -513,8 +527,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
513527
{
514528
description: "Garbage/Mixed",
515529
in: []string{
516-
"bad_version",
517-
"1.0.0",
530+
`"bad_version"`,
531+
`"1.0.0"`,
518532
},
519533
expect: expect{
520534
err: true,
@@ -523,7 +537,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
523537
},
524538
{
525539
description: "Single",
526-
in: []string{"1.0.0"},
540+
in: []string{`"1.0.0"`},
527541
expect: expect{
528542
err: false,
529543
max: mustParse("1.0.0"),
@@ -532,8 +546,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
532546
{
533547
description: "Multiple",
534548
in: []string{
535-
"1.0.0",
536-
"2.0.0",
549+
`"1.0.0"`,
550+
`"2.0.0"`,
537551
},
538552
expect: expect{
539553
err: false,
@@ -543,8 +557,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
543557
{
544558
description: "Duplicates",
545559
in: []string{
546-
"1.0.0",
547-
"1.0.0",
560+
`"1.0.0"`,
561+
`"1.0.0"`,
548562
},
549563
expect: expect{
550564
err: false,
@@ -554,9 +568,9 @@ func TestMaxOpenShiftVersion(t *testing.T) {
554568
{
555569
description: "Duplicates/NonMax",
556570
in: []string{
557-
"1.0.0",
558-
"1.0.0",
559-
"2.0.0",
571+
`"1.0.0"`,
572+
`"1.0.0"`,
573+
`"2.0.0"`,
560574
},
561575
expect: expect{
562576
err: false,
@@ -566,21 +580,30 @@ func TestMaxOpenShiftVersion(t *testing.T) {
566580
{
567581
description: "Ambiguous",
568582
in: []string{
569-
"1.0.0",
570-
"1.0.0+1",
583+
`"1.0.0"`,
584+
`"1.0.0+1"`,
571585
},
572586
expect: expect{
573587
err: true,
574588
max: nil,
575589
},
576590
},
591+
{
592+
// Ensure unquoted short strings are accepted; e.g. X.Y
593+
description: "Unquoted/Short",
594+
in: []string{"4.8"},
595+
expect: expect{
596+
err: false,
597+
max: mustParse("4.8"),
598+
},
599+
},
577600
} {
578601
t.Run(tt.description, func(t *testing.T) {
579602
var properties []*api.Property
580603
for _, max := range tt.in {
581604
properties = append(properties, &api.Property{
582605
Type: MaxOpenShiftVersionProperty,
583-
Value: `"` + max + `"`, // Wrap in quotes so we don't break property marshaling
606+
Value: max,
584607
})
585608
}
586609

‎pkg/controller/registry/resolver/projection/properties_test.go

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,16 @@ func TestPropertiesAnnotationFromPropertyList(t *testing.T) {
5454
},
5555
expected: `{"properties":[{"type":"string","value":"hello"},{"type":"number","value":5},{"type":"array","value":[1,"two",3,"four"]},{"type":"object","value":{"hello":{"worl":"d"}}}]}`,
5656
},
57+
{
58+
name: "unquoted string",
59+
properties: []*api.Property{
60+
{
61+
Type: "version",
62+
Value: "4.8",
63+
},
64+
},
65+
expected: `{"properties":[{"type":"version","value":4.8}]}`,
66+
},
5767
} {
5868
t.Run(tc.name, func(t *testing.T) {
5969
actual, err := projection.PropertiesAnnotationFromPropertyList(tc.properties)
@@ -120,12 +130,23 @@ func TestPropertyListFromPropertiesAnnotation(t *testing.T) {
120130
{
121131
Type: "array",
122132
Value: `[1,"two",3,"four"]`,
123-
}, {
133+
},
134+
{
124135
Type: "object",
125136
Value: `{"hello":{"worl":"d"}}`,
126137
},
127138
},
128139
},
140+
{
141+
name: "unquoted string values",
142+
annotation: `{"properties":[{"type": "version","value": 4.8}]}`,
143+
expected: []*api.Property{
144+
{
145+
Type: "version",
146+
Value: "4.8",
147+
},
148+
},
149+
},
129150
} {
130151
t.Run(tc.name, func(t *testing.T) {
131152
actual, err := projection.PropertyListFromPropertiesAnnotation(tc.annotation)

0 commit comments

Comments
 (0)