Skip to content

Commit 0e728b3

Browse files
trgeigertrgeiger
authored
Add ServedVersionValidator preflight check (operator-framework#1191)
New preflight check that checks for either the presence of the "Webhook" conversion strategy to pass, or checks for any incompatible changes between served versions in the new CRD which, if found, causes failure.. Closes operator-framework#1091 Signed-off-by: Tayler Geiger <[email protected]>
1 parent 04ee036 commit 0e728b3

9 files changed

+412
-18
lines changed

‎internal/rukpak/preflights/crdupgradesafety/checks.go

+72
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package crdupgradesafety
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"slices"
7+
8+
kappcus "carvel.dev/kapp/pkg/kapp/crdupgradesafety"
9+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
10+
versionhelper "k8s.io/apimachinery/pkg/version"
11+
)
12+
13+
type ServedVersionValidator struct {
14+
Validations []kappcus.ChangeValidation
15+
}
16+
17+
func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error {
18+
// If conversion webhook is specified, pass check
19+
if new.Spec.Conversion != nil && new.Spec.Conversion.Strategy == apiextensionsv1.WebhookConverter {
20+
return nil
21+
}
22+
23+
errs := []error{}
24+
servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{}
25+
for _, version := range new.Spec.Versions {
26+
if version.Served {
27+
servedVersions = append(servedVersions, version)
28+
}
29+
}
30+
31+
slices.SortFunc(servedVersions, func(a, b apiextensionsv1.CustomResourceDefinitionVersion) int {
32+
return versionhelper.CompareKubeAwareVersionStrings(a.Name, b.Name)
33+
})
34+
35+
for i, oldVersion := range servedVersions[:len(servedVersions)-1] {
36+
for _, newVersion := range servedVersions[i+1:] {
37+
flatOld := kappcus.FlattenSchema(oldVersion.Schema.OpenAPIV3Schema)
38+
flatNew := kappcus.FlattenSchema(newVersion.Schema.OpenAPIV3Schema)
39+
diffs, err := kappcus.CalculateFlatSchemaDiff(flatOld, flatNew)
40+
if err != nil {
41+
errs = append(errs, fmt.Errorf("calculating schema diff between CRD versions %q and %q", oldVersion.Name, newVersion.Name))
42+
continue
43+
}
44+
45+
for field, diff := range diffs {
46+
handled := false
47+
for _, validation := range c.Validations {
48+
ok, err := validation(diff)
49+
if err != nil {
50+
errs = append(errs, fmt.Errorf("version upgrade %q to %q, field %q: %w", oldVersion.Name, newVersion.Name, field, err))
51+
}
52+
if ok {
53+
handled = true
54+
break
55+
}
56+
}
57+
58+
if !handled {
59+
errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", oldVersion.Name, field))
60+
}
61+
}
62+
}
63+
}
64+
if len(errs) > 0 {
65+
return errors.Join(errs...)
66+
}
67+
return nil
68+
}
69+
70+
func (c *ServedVersionValidator) Name() string {
71+
return "ServedVersionValidator"
72+
}

‎internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

+15-15
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,19 @@ type Preflight struct {
3131
}
3232

3333
func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface, opts ...Option) *Preflight {
34+
changeValidations := []kappcus.ChangeValidation{
35+
kappcus.EnumChangeValidation,
36+
kappcus.RequiredFieldChangeValidation,
37+
kappcus.MaximumChangeValidation,
38+
kappcus.MaximumItemsChangeValidation,
39+
kappcus.MaximumLengthChangeValidation,
40+
kappcus.MaximumPropertiesChangeValidation,
41+
kappcus.MinimumChangeValidation,
42+
kappcus.MinimumItemsChangeValidation,
43+
kappcus.MinimumLengthChangeValidation,
44+
kappcus.MinimumPropertiesChangeValidation,
45+
kappcus.DefaultValueChangeValidation,
46+
}
3447
p := &Preflight{
3548
crdClient: crdCli,
3649
// create a default validator. Can be overridden via the options
@@ -39,21 +52,8 @@ func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface
3952
kappcus.NewValidationFunc("NoScopeChange", kappcus.NoScopeChange),
4053
kappcus.NewValidationFunc("NoStoredVersionRemoved", kappcus.NoStoredVersionRemoved),
4154
kappcus.NewValidationFunc("NoExistingFieldRemoved", kappcus.NoExistingFieldRemoved),
42-
&kappcus.ChangeValidator{
43-
Validations: []kappcus.ChangeValidation{
44-
kappcus.EnumChangeValidation,
45-
kappcus.RequiredFieldChangeValidation,
46-
kappcus.MaximumChangeValidation,
47-
kappcus.MaximumItemsChangeValidation,
48-
kappcus.MaximumLengthChangeValidation,
49-
kappcus.MaximumPropertiesChangeValidation,
50-
kappcus.MinimumChangeValidation,
51-
kappcus.MinimumItemsChangeValidation,
52-
kappcus.MinimumLengthChangeValidation,
53-
kappcus.MinimumPropertiesChangeValidation,
54-
kappcus.DefaultValueChangeValidation,
55-
},
56-
},
55+
&ServedVersionValidator{Validations: changeValidations},
56+
&kappcus.ChangeValidator{Validations: changeValidations},
5757
},
5858
},
5959
}

‎internal/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go

+19
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,25 @@ func TestUpgrade(t *testing.T) {
329329
`"NoExistingFieldRemoved"`,
330330
},
331331
},
332+
{
333+
name: "webhook conversion strategy exists",
334+
oldCrdPath: "crd-conversion-webhook-old.json",
335+
release: &release.Release{
336+
Name: "test-release",
337+
Manifest: getManifestString(t, "crd-conversion-webhook.json"),
338+
},
339+
},
340+
{
341+
name: "new crd validation failure when missing conversion strategy and enum values removed",
342+
oldCrdPath: "crd-conversion-webhook-old.json",
343+
release: &release.Release{
344+
Name: "test-release",
345+
Manifest: getManifestString(t, "crd-conversion-no-webhook.json"),
346+
},
347+
wantErrMsgs: []string{
348+
`"ServedVersionValidator" validation failed: version upgrade "v1" to "v2", field "^.spec.foobarbaz": enum values removed`,
349+
},
350+
},
332351
}
333352

334353
for _, tc := range tests {

‎testdata/manifests/crd-conversion-no-webhook.json

+70
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
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+
}
29+
}
30+
}
31+
}
32+
}
33+
},
34+
{
35+
"name": "v1",
36+
"served": true,
37+
"storage": false,
38+
"schema": {
39+
"openAPIV3Schema": {
40+
"type": "object",
41+
"properties": {
42+
"spec": {
43+
"type": "object",
44+
"properties": {
45+
"foobarbaz": {
46+
"type":"string",
47+
"enum":[
48+
"foo",
49+
"bar",
50+
"baz"
51+
]
52+
}
53+
}
54+
}
55+
}
56+
}
57+
}
58+
}
59+
],
60+
"scope": "Cluster",
61+
"names": {
62+
"plural": "crontabs",
63+
"singular": "crontab",
64+
"kind": "CronTab",
65+
"shortNames": [
66+
"ct"
67+
]
68+
}
69+
}
70+
}

‎testdata/manifests/crd-conversion-webhook-old.json

+46
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
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": "v1",
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+
"foo",
25+
"bar",
26+
"baz"
27+
]
28+
}
29+
}
30+
}
31+
}
32+
}
33+
}
34+
}
35+
],
36+
"scope": "Cluster",
37+
"names": {
38+
"plural": "crontabs",
39+
"singular": "crontab",
40+
"kind": "CronTab",
41+
"shortNames": [
42+
"ct"
43+
]
44+
}
45+
}
46+
}

‎testdata/manifests/crd-conversion-webhook.json

+82
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
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+
}
29+
}
30+
}
31+
}
32+
}
33+
},
34+
{
35+
"name": "v1",
36+
"served": true,
37+
"storage": false,
38+
"schema": {
39+
"openAPIV3Schema": {
40+
"type": "object",
41+
"properties": {
42+
"spec": {
43+
"type": "object",
44+
"properties": {
45+
"foobarbaz": {
46+
"type":"string",
47+
"enum":[
48+
"foo",
49+
"bar",
50+
"baz"
51+
]
52+
}
53+
}
54+
}
55+
}
56+
}
57+
}
58+
}
59+
],
60+
"scope": "Cluster",
61+
"names": {
62+
"plural": "crontabs",
63+
"singular": "crontab",
64+
"kind": "CronTab",
65+
"shortNames": [
66+
"ct"
67+
]
68+
},
69+
"conversion": {
70+
"strategy": "Webhook",
71+
"webhook": {
72+
"clientConfig": {
73+
"service": {
74+
"namespace": "crd-conversion-webhook",
75+
"name": "crd-conversion-webhook",
76+
"path": "/crdconversion"
77+
}
78+
}
79+
}
80+
}
81+
}
82+
}

‎testdata/manifests/crd-field-removed.json

+36-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,42 @@
1515
"openAPIV3Schema": {
1616
"type": "object",
1717
"properties": {
18-
}
18+
"spec": {
19+
"type": "object",
20+
"properties": {
21+
"removedField": {
22+
"type":"integer"
23+
},
24+
"enum": {
25+
"type":"integer"
26+
},
27+
"minMaxValue": {
28+
"type":"integer"
29+
},
30+
"required": {
31+
"type":"integer"
32+
},
33+
"minMaxItems": {
34+
"type":"array",
35+
"items": {
36+
"type":"string"
37+
}
38+
},
39+
"minMaxLength": {
40+
"type":"string"
41+
},
42+
"defaultVal": {
43+
"type": "string"
44+
},
45+
"requiredVal": {
46+
"type": "string"
47+
}
48+
}
49+
}
50+
},
51+
"required": [
52+
"requiredVal"
53+
]
1954
}
2055
}
2156
},

0 commit comments

Comments
 (0)