Skip to content

Commit 236319b

Browse files
joelanfordjoelanford
authored
🐛 CRD upgrade safety fixes and ratcheting (#2123)
* move crd upgrade safety testdata into crdupgradesafety package Signed-off-by: Joe Lanford <[email protected]> * fix: bump crdify to fix bugs and regressions regression fixes: 1. correctly handle processing of properties that are OpenAPI items 2. allow enums to have values added. bug fix: crdify's served version validator was updated to actually compare the old CRD with the new CRD so that any issues identified in the old CRD do not continue to be reported when performing comparisons between served versions of the new CRD. This effectively allows issues in the served version validations to be acknowledged once when they are introduced, but then those issues are essentially grandfathered in such that they do not have to be acknowledged again in the future. This issue was actually identified in a case where an operator upgrade was stopped by the CRD upgrade check despite there being no changes whatsoever to the CRD. The "old" and "new" CRDs contained the exact same issues, but since crdify was looking exclusively at the "new" CRD, it found those issues and reported them. --------- Signed-off-by: Joe Lanford <[email protected]>
1 parent 1ec8871 commit 236319b

15 files changed

+168
-45
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
)
@@ -130,6 +131,13 @@ func defaultConfig() *config.Config {
130131
Name: "description",
131132
Enforcement: config.EnforcementPolicyNone,
132133
},
134+
{
135+
Name: "enum",
136+
Enforcement: config.EnforcementPolicyError,
137+
Configuration: map[string]interface{}{
138+
"additionPolicy": property.AdditionPolicyAllow,
139+
},
140+
},
133141
},
134142
}
135143
}

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

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

41-
const crdFolder string = "../../../../../testdata/manifests"
41+
const crdFolder string = "testdata/manifests"
4242

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

69+
func wantErrorMsgs(wantMsgs []string) require.ErrorAssertionFunc {
70+
return func(t require.TestingT, haveErr error, _ ...interface{}) {
71+
for _, wantMsg := range wantMsgs {
72+
require.ErrorContains(t, haveErr, wantMsg)
73+
}
74+
}
75+
}
76+
6977
// TestInstall exists only for completeness as Install() is currently a no-op. It can be used as
7078
// a template for real tests in the future if the func is implemented.
7179
func TestInstall(t *testing.T) {
7280
tests := []struct {
7381
name string
7482
oldCrdPath string
7583
release *release.Release
76-
wantErrMsgs []string
84+
requireErr require.ErrorAssertionFunc
7785
wantCrdGetErr error
7886
}{
7987
{
@@ -91,7 +99,7 @@ func TestInstall(t *testing.T) {
9199
Name: "test-release",
92100
Manifest: "abcd",
93101
},
94-
wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
102+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
95103
},
96104
{
97105
name: "release with no CRD objects",
@@ -107,7 +115,7 @@ func TestInstall(t *testing.T) {
107115
Manifest: getManifestString(t, "crd-valid-upgrade.json"),
108116
},
109117
wantCrdGetErr: fmt.Errorf("error!"),
110-
wantErrMsgs: []string{"error!"},
118+
requireErr: wantErrorMsgs([]string{"error!"}),
111119
},
112120
{
113121
name: "fail to get old crd, not found error",
@@ -123,7 +131,7 @@ func TestInstall(t *testing.T) {
123131
Name: "test-release",
124132
Manifest: getManifestString(t, "crd-invalid"),
125133
},
126-
wantErrMsgs: []string{"json: cannot unmarshal"},
134+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
127135
},
128136
{
129137
name: "valid upgrade",
@@ -142,7 +150,7 @@ func TestInstall(t *testing.T) {
142150
Name: "test-release",
143151
Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
144152
},
145-
wantErrMsgs: []string{
153+
requireErr: wantErrorMsgs([]string{
146154
`scope:`,
147155
`storedVersionRemoval:`,
148156
`enum:`,
@@ -156,7 +164,7 @@ func TestInstall(t *testing.T) {
156164
`minLength:`,
157165
`minProperties:`,
158166
`default:`,
159-
},
167+
}),
160168
},
161169
{
162170
name: "new crd validation failure for existing field removal",
@@ -167,9 +175,9 @@ func TestInstall(t *testing.T) {
167175
Name: "test-release",
168176
Manifest: getManifestString(t, "crd-field-removed.json"),
169177
},
170-
wantErrMsgs: []string{
178+
requireErr: wantErrorMsgs([]string{
171179
`existingFieldRemoval:`,
172-
},
180+
}),
173181
},
174182
{
175183
name: "new crd validation should not fail on description changes",
@@ -187,10 +195,8 @@ func TestInstall(t *testing.T) {
187195
t.Run(tc.name, func(t *testing.T) {
188196
preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
189197
err := preflight.Install(context.Background(), tc.release)
190-
if len(tc.wantErrMsgs) != 0 {
191-
for _, expectedErrMsg := range tc.wantErrMsgs {
192-
require.ErrorContains(t, err, expectedErrMsg)
193-
}
198+
if tc.requireErr != nil {
199+
tc.requireErr(t, err)
194200
} else {
195201
require.NoError(t, err)
196202
}
@@ -203,7 +209,7 @@ func TestUpgrade(t *testing.T) {
203209
name string
204210
oldCrdPath string
205211
release *release.Release
206-
wantErrMsgs []string
212+
requireErr require.ErrorAssertionFunc
207213
wantCrdGetErr error
208214
}{
209215
{
@@ -221,7 +227,7 @@ func TestUpgrade(t *testing.T) {
221227
Name: "test-release",
222228
Manifest: "abcd",
223229
},
224-
wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"},
230+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}),
225231
},
226232
{
227233
name: "release with no CRD objects",
@@ -237,7 +243,7 @@ func TestUpgrade(t *testing.T) {
237243
Manifest: getManifestString(t, "crd-valid-upgrade.json"),
238244
},
239245
wantCrdGetErr: fmt.Errorf("error!"),
240-
wantErrMsgs: []string{"error!"},
246+
requireErr: wantErrorMsgs([]string{"error!"}),
241247
},
242248
{
243249
name: "fail to get old crd, not found error",
@@ -253,7 +259,7 @@ func TestUpgrade(t *testing.T) {
253259
Name: "test-release",
254260
Manifest: getManifestString(t, "crd-invalid"),
255261
},
256-
wantErrMsgs: []string{"json: cannot unmarshal"},
262+
requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}),
257263
},
258264
{
259265
name: "valid upgrade",
@@ -272,7 +278,7 @@ func TestUpgrade(t *testing.T) {
272278
Name: "test-release",
273279
Manifest: getManifestString(t, "crd-invalid-upgrade.json"),
274280
},
275-
wantErrMsgs: []string{
281+
requireErr: wantErrorMsgs([]string{
276282
`scope:`,
277283
`storedVersionRemoval:`,
278284
`enum:`,
@@ -286,7 +292,7 @@ func TestUpgrade(t *testing.T) {
286292
`minLength:`,
287293
`minProperties:`,
288294
`default:`,
289-
},
295+
}),
290296
},
291297
{
292298
name: "new crd validation failure for existing field removal",
@@ -297,9 +303,9 @@ func TestUpgrade(t *testing.T) {
297303
Name: "test-release",
298304
Manifest: getManifestString(t, "crd-field-removed.json"),
299305
},
300-
wantErrMsgs: []string{
306+
requireErr: wantErrorMsgs([]string{
301307
`existingFieldRemoval:`,
302-
},
308+
}),
303309
},
304310
{
305311
name: "webhook conversion strategy exists",
@@ -316,9 +322,9 @@ func TestUpgrade(t *testing.T) {
316322
Name: "test-release",
317323
Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
318324
},
319-
wantErrMsgs: []string{
320-
`validating upgrade for CRD "crontabs.stable.example.com": v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
321-
},
325+
requireErr: wantErrorMsgs([]string{
326+
`validating upgrade for CRD "crontabs.stable.example.com": v1 -> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
327+
}),
322328
},
323329
{
324330
name: "new crd validation should not fail on description changes",
@@ -330,16 +336,43 @@ func TestUpgrade(t *testing.T) {
330336
Manifest: getManifestString(t, "crd-description-changed.json"),
331337
},
332338
},
339+
{
340+
name: "success when old crd and new crd contain the exact same validation issues",
341+
oldCrdPath: "crd-conversion-no-webhook.json",
342+
release: &release.Release{
343+
Name: "test-release",
344+
Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
345+
},
346+
},
347+
{
348+
name: "failure when old crd and new crd contain the exact same validation issues, but new crd introduces another validation issue",
349+
oldCrdPath: "crd-conversion-no-webhook.json",
350+
release: &release.Release{
351+
Name: "test-release",
352+
Manifest: getManifestString(t, "crd-conversion-no-webhook-extra-issue.json"),
353+
},
354+
requireErr: func(t require.TestingT, err error, _ ...interface{}) {
355+
require.ErrorContains(t, err,
356+
`validating upgrade for CRD "crontabs.stable.example.com":`,
357+
)
358+
// The newly introduced issue is reported
359+
require.Contains(t, err.Error(),
360+
`v1 -> v2: ^.spec.extraField: type: type changed : "boolean" -> "string"`,
361+
)
362+
// The existing issue is not reported
363+
require.NotContains(t, err.Error(),
364+
`v1 -> v2: ^.spec.foobarbaz: enum: allowed enum values removed`,
365+
)
366+
},
367+
},
333368
}
334369

335370
for _, tc := range tests {
336371
t.Run(tc.name, func(t *testing.T) {
337372
preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr)
338373
err := preflight.Upgrade(context.Background(), tc.release)
339-
if len(tc.wantErrMsgs) != 0 {
340-
for _, expectedErrMsg := range tc.wantErrMsgs {
341-
require.ErrorContains(t, err, expectedErrMsg)
342-
}
374+
if tc.requireErr != nil {
375+
tc.requireErr(t, err)
343376
} else {
344377
require.NoError(t, err)
345378
}

‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json‎

Lines changed: 76 additions & 0 deletions
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-conversion-no-webhook.json‎ renamed to ‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook.json‎

File renamed without changes.

‎testdata/manifests/crd-conversion-webhook-old.json‎ renamed to ‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook-old.json‎

File renamed without changes.

‎testdata/manifests/crd-conversion-webhook.json‎ renamed to ‎internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook.json‎

File renamed without changes.

‎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)