Skip to content

Commit afe8c54

Browse files
joelanfordjianzhangbjz
authored andcommitted
UPSTREAM: <carry>: 🐛 CRD upgrade safety fixes and ratcheting (#2123)
* move crd upgrade safety testdata into crdupgradesafety package Signed-off-by: Joe Lanford <joe.lanford@gmail.com> * 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 <joe.lanford@gmail.com>
1 parent 44a11fe commit afe8c54

17 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
)
@@ -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)