Skip to content

Commit 3e2401f

Browse files
Merge pull request #527 from jianzhangbjz/bug-58462
OCPBUGS-61890: 🐛 CRD upgrade safety fixes and ratcheting (#2123)
2 parents 44a11fe + e1be79b commit 3e2401f

File tree

20 files changed

+286
-58
lines changed

20 files changed

+286
-58
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ require (
4444
k8s.io/utils v0.0.0-20250604170112-4c0f3b243397
4545
sigs.k8s.io/controller-runtime v0.21.0
4646
sigs.k8s.io/controller-tools v0.18.0
47-
sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58
47+
sigs.k8s.io/crdify v0.4.1-0.20250825182107-69e65223aee0
4848
sigs.k8s.io/yaml v1.6.0
4949
)
5050

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -765,8 +765,8 @@ sigs.k8s.io/controller-runtime v0.21.0 h1:CYfjpEuicjUecRk+KAeyYh+ouUBn4llGyDYytI
765765
sigs.k8s.io/controller-runtime v0.21.0/go.mod h1:OSg14+F65eWqIu4DceX7k/+QRAbTTvxeQSNSOQpukWM=
766766
sigs.k8s.io/controller-tools v0.18.0 h1:rGxGZCZTV2wJreeRgqVoWab/mfcumTMmSwKzoM9xrsE=
767767
sigs.k8s.io/controller-tools v0.18.0/go.mod h1:gLKoiGBriyNh+x1rWtUQnakUYEujErjXs9pf+x/8n1U=
768-
sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58 h1:VTvhbqgZMVoDpHHPuZLaOgzjjsJBhO8+vDKA1COuLCY=
769-
sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58/go.mod h1:ZIFxaYNgKYmFtZCLPysncXQ8oqwnNlHQbRUfxJHZwzU=
768+
sigs.k8s.io/crdify v0.4.1-0.20250825182107-69e65223aee0 h1:jfBjW0kwwx2ULnzRrs+Jnepn345JbpAVqzekHBeIGgY=
769+
sigs.k8s.io/crdify v0.4.1-0.20250825182107-69e65223aee0/go.mod h1:ZIFxaYNgKYmFtZCLPysncXQ8oqwnNlHQbRUfxJHZwzU=
770770
sigs.k8s.io/gateway-api v1.1.0 h1:DsLDXCi6jR+Xz8/xd0Z1PYl2Pn0TyaFMOPPZIj4inDM=
771771
sigs.k8s.io/gateway-api v1.1.0/go.mod h1:ZH4lHrL2sDi0FHZ9jjneb8kKnGzFWyrTya35sWUTrRs=
772772
sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 h1:gBQPwqORJ8d8/YNZWEjoZs7npUVDpVXUUOFfW6CgAqE=

internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"sigs.k8s.io/crdify/pkg/config"
1616
"sigs.k8s.io/crdify/pkg/runner"
1717
"sigs.k8s.io/crdify/pkg/validations"
18+
"sigs.k8s.io/crdify/pkg/validations/property"
1819

1920
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
2021
)
@@ -131,6 +132,13 @@ func defaultConfig() *config.Config {
131132
Name: "description",
132133
Enforcement: config.EnforcementPolicyNone,
133134
},
135+
{
136+
Name: "enum",
137+
Enforcement: config.EnforcementPolicyError,
138+
Configuration: map[string]interface{}{
139+
"additionPolicy": property.AdditionPolicyAllow,
140+
},
141+
},
134142
},
135143
}
136144
}

internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go

Lines changed: 61 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func newMockPreflight(crd *apiextensionsv1.CustomResourceDefinition, err error)
3939
}, preflightOpts...)
4040
}
4141

42-
const crdFolder string = "../../../../../testdata/manifests"
42+
const crdFolder string = "testdata/manifests"
4343

4444
func getCrdFromManifestFile(t *testing.T, oldCrdFile string) *apiextensionsv1.CustomResourceDefinition {
4545
if oldCrdFile == "" {
@@ -67,14 +67,22 @@ func getManifestString(t *testing.T, crdFile string) string {
6767
return string(buff)
6868
}
6969

70+
func wantErrorMsgs(wantMsgs []string) require.ErrorAssertionFunc {
71+
return func(t require.TestingT, haveErr error, _ ...interface{}) {
72+
for _, wantMsg := range wantMsgs {
73+
require.ErrorContains(t, haveErr, wantMsg)
74+
}
75+
}
76+
}
77+
7078
// TestInstall exists only for completeness as Install() is currently a no-op. It can be used as
7179
// a template for real tests in the future if the func is implemented.
7280
func TestInstall(t *testing.T) {
7381
tests := []struct {
7482
name string
7583
oldCrdPath string
7684
release *release.Release
77-
wantErrMsgs []string
85+
requireErr require.ErrorAssertionFunc
7886
wantCrdGetErr error
7987
}{
8088
{
@@ -92,7 +100,7 @@ func TestInstall(t *testing.T) {
92100
Name: "test-release",
93101
Manifest: "abcd",
94102
},
95-
wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
103+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
96104
},
97105
{
98106
name: "release with no CRD objects",
@@ -108,7 +116,7 @@ func TestInstall(t *testing.T) {
108116
Manifest: getManifestString(t, "crd-valid-upgrade.json"),
109117
},
110118
wantCrdGetErr: fmt.Errorf("error!"),
111-
wantErrMsgs: []string{"error!"},
119+
requireErr: wantErrorMsgs([]string{"error!"}),
112120
},
113121
{
114122
name: "fail to get old crd, not found error",
@@ -124,7 +132,7 @@ func TestInstall(t *testing.T) {
124132
Name: "test-release",
125133
Manifest: getManifestString(t, "crd-invalid"),
126134
},
127-
wantErrMsgs: []string{"json: cannot unmarshal"},
135+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
128136
},
129137
{
130138
name: "valid upgrade",
@@ -143,7 +151,7 @@ func TestInstall(t *testing.T) {
143151
Name: "test-release",
144152
Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
145153
},
146-
wantErrMsgs: []string{
154+
requireErr: wantErrorMsgs([]string{
147155
`scope:`,
148156
`storedVersionRemoval:`,
149157
`enum:`,
@@ -157,7 +165,7 @@ func TestInstall(t *testing.T) {
157165
`minLength:`,
158166
`minProperties:`,
159167
`default:`,
160-
},
168+
}),
161169
},
162170
{
163171
name: "new crd validation failure for existing field removal",
@@ -168,9 +176,9 @@ func TestInstall(t *testing.T) {
168176
Name: "test-release",
169177
Manifest: getManifestString(t, "crd-field-removed.json"),
170178
},
171-
wantErrMsgs: []string{
179+
requireErr: wantErrorMsgs([]string{
172180
`existingFieldRemoval:`,
173-
},
181+
}),
174182
},
175183
{
176184
name: "new crd validation should not fail on description changes",
@@ -188,10 +196,8 @@ func TestInstall(t *testing.T) {
188196
t.Run(tc.name, func(t *testing.T) {
189197
preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
190198
err := preflight.Install(context.Background(), tc.release)
191-
if len(tc.wantErrMsgs) != 0 {
192-
for _, expectedErrMsg := range tc.wantErrMsgs {
193-
require.ErrorContains(t, err, expectedErrMsg)
194-
}
199+
if tc.requireErr != nil {
200+
tc.requireErr(t, err)
195201
} else {
196202
require.NoError(t, err)
197203
}
@@ -204,7 +210,7 @@ func TestUpgrade(t *testing.T) {
204210
name string
205211
oldCrdPath string
206212
release *release.Release
207-
wantErrMsgs []string
213+
requireErr require.ErrorAssertionFunc
208214
wantCrdGetErr error
209215
}{
210216
{
@@ -222,7 +228,7 @@ func TestUpgrade(t *testing.T) {
222228
Name: "test-release",
223229
Manifest: "abcd",
224230
},
225-
wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
231+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
226232
},
227233
{
228234
name: "release with no CRD objects",
@@ -238,7 +244,7 @@ func TestUpgrade(t *testing.T) {
238244
Manifest: getManifestString(t, "crd-valid-upgrade.json"),
239245
},
240246
wantCrdGetErr: fmt.Errorf("error!"),
241-
wantErrMsgs: []string{"error!"},
247+
requireErr: wantErrorMsgs([]string{"error!"}),
242248
},
243249
{
244250
name: "fail to get old crd, not found error",
@@ -254,7 +260,7 @@ func TestUpgrade(t *testing.T) {
254260
Name: "test-release",
255261
Manifest: getManifestString(t, "crd-invalid"),
256262
},
257-
wantErrMsgs: []string{"json: cannot unmarshal"},
263+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
258264
},
259265
{
260266
name: "valid upgrade",
@@ -273,7 +279,7 @@ func TestUpgrade(t *testing.T) {
273279
Name: "test-release",
274280
Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
275281
},
276-
wantErrMsgs: []string{
282+
requireErr: wantErrorMsgs([]string{
277283
`scope:`,
278284
`storedVersionRemoval:`,
279285
`enum:`,
@@ -287,7 +293,7 @@ func TestUpgrade(t *testing.T) {
287293
`minLength:`,
288294
`minProperties:`,
289295
`default:`,
290-
},
296+
}),
291297
},
292298
{
293299
name: "new crd validation failure for existing field removal",
@@ -298,9 +304,9 @@ func TestUpgrade(t *testing.T) {
298304
Name: "test-release",
299305
Manifest: getManifestString(t, "crd-field-removed.json"),
300306
},
301-
wantErrMsgs: []string{
307+
requireErr: wantErrorMsgs([]string{
302308
`existingFieldRemoval:`,
303-
},
309+
}),
304310
},
305311
{
306312
name: "webhook conversion strategy exists",
@@ -317,9 +323,9 @@ func TestUpgrade(t *testing.T) {
317323
Name: "test-release",
318324
Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
319325
},
320-
wantErrMsgs: []string{
321-
`validating upgrade for CRD "crontabs.stable.example.com": v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
322-
},
326+
requireErr: wantErrorMsgs([]string{
327+
`validating upgrade for CRD "crontabs.stable.example.com": v1 -> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
328+
}),
323329
},
324330
{
325331
name: "new crd validation should not fail on description changes",
@@ -331,16 +337,43 @@ func TestUpgrade(t *testing.T) {
331337
Manifest: getManifestString(t, "crd-description-changed.json"),
332338
},
333339
},
340+
{
341+
name: "success when old crd and new crd contain the exact same validation issues",
342+
oldCrdPath: "crd-conversion-no-webhook.json",
343+
release: &release.Release{
344+
Name: "test-release",
345+
Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
346+
},
347+
},
348+
{
349+
name: "failure when old crd and new crd contain the exact same validation issues, but new crd introduces another validation issue",
350+
oldCrdPath: "crd-conversion-no-webhook.json",
351+
release: &release.Release{
352+
Name: "test-release",
353+
Manifest: getManifestString(t, "crd-conversion-no-webhook-extra-issue.json"),
354+
},
355+
requireErr: func(t require.TestingT, err error, _ ...interface{}) {
356+
require.ErrorContains(t, err,
357+
`validating upgrade for CRD "crontabs.stable.example.com":`,
358+
)
359+
// The newly introduced issue is reported
360+
require.Contains(t, err.Error(),
361+
`v1 -> v2: ^.spec.extraField: type: type changed : "boolean" -> "string"`,
362+
)
363+
// The existing issue is not reported
364+
require.NotContains(t, err.Error(),
365+
`v1 -> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
366+
)
367+
},
368+
},
334369
}
335370

336371
for _, tc := range tests {
337372
t.Run(tc.name, func(t *testing.T) {
338373
preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
339374
err := preflight.Upgrade(context.Background(), tc.release)
340-
if len(tc.wantErrMsgs) != 0 {
341-
for _, expectedErrMsg := range tc.wantErrMsgs {
342-
require.ErrorContains(t, err, expectedErrMsg)
343-
}
375+
if tc.requireErr != nil {
376+
tc.requireErr(t, err)
344377
} else {
345378
require.NoError(t, err)
346379
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
{
2+
"apiVersion": "apiextensions.k8s.io/v1",
3+
"kind": "CustomResourceDefinition",
4+
"metadata": {
5+
"name": "crontabs.stable.example.com"
6+
},
7+
"spec": {
8+
"group": "stable.example.com",
9+
"versions": [
10+
{
11+
"name": "v2",
12+
"served": true,
13+
"storage": false,
14+
"schema": {
15+
"openAPIV3Schema": {
16+
"type": "object",
17+
"properties": {
18+
"spec": {
19+
"type": "object",
20+
"properties": {
21+
"foobarbaz": {
22+
"type":"string",
23+
"enum":[
24+
"bark",
25+
"woof"
26+
]
27+
},
28+
"extraField": {
29+
"type":"string"
30+
}
31+
}
32+
}
33+
}
34+
}
35+
}
36+
},
37+
{
38+
"name": "v1",
39+
"served": true,
40+
"storage": false,
41+
"schema": {
42+
"openAPIV3Schema": {
43+
"type": "object",
44+
"properties": {
45+
"spec": {
46+
"type": "object",
47+
"properties": {
48+
"foobarbaz": {
49+
"type":"string",
50+
"enum":[
51+
"foo",
52+
"bar",
53+
"baz"
54+
]
55+
},
56+
"extraField": {
57+
"type":"boolean"
58+
}
59+
}
60+
}
61+
}
62+
}
63+
}
64+
}
65+
],
66+
"scope": "Cluster",
67+
"names": {
68+
"plural": "crontabs",
69+
"singular": "crontab",
70+
"kind": "CronTab",
71+
"shortNames": [
72+
"ct"
73+
]
74+
}
75+
}
76+
}

testdata/manifests/crd-description-changed.json renamed to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@
2323
"type":"integer"
2424
},
2525
"enum": {
26-
"type":"integer"
26+
"type": "string",
27+
"enum": ["a", "b", "c"]
2728
},
2829
"minMaxValue": {
2930
"type":"integer"
@@ -70,7 +71,8 @@
7071
"type":"integer"
7172
},
7273
"enum": {
73-
"type":"integer"
74+
"type": "string",
75+
"enum": ["a", "b", "c"]
7476
},
7577
"minMaxValue": {
7678
"type":"integer"

testdata/manifests/crd-field-removed.json renamed to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
"type":"integer"
2323
},
2424
"enum": {
25-
"type":"integer"
25+
"type": "string",
26+
"enum": ["a", "b", "c"]
2627
},
2728
"minMaxValue": {
2829
"type":"integer"
@@ -66,7 +67,8 @@
6667
"type": "object",
6768
"properties": {
6869
"enum": {
69-
"type":"integer"
70+
"type": "string",
71+
"enum": ["a", "b", "c"]
7072
},
7173
"minMaxValue": {
7274
"type":"integer"

0 commit comments

Comments
 (0)