Skip to content

Commit 0e728b3

Browse files
trgeigertrgeiger
authored
Add ServedVersionValidator preflight check (#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 #1091 Signed-off-by: Tayler Geiger <[email protected]>
1 parent 04ee036 commit 0e728b3

File tree

9 files changed

+412
-18
lines changed

9 files changed

+412
-18
lines changed

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

Lines changed: 72 additions & 0 deletions
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

Lines changed: 15 additions & 15 deletions
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

Lines changed: 19 additions & 0 deletions
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

Lines changed: 70 additions & 0 deletions
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

Lines changed: 46 additions & 0 deletions
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

Lines changed: 82 additions & 0 deletions
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

Lines changed: 36 additions & 1 deletion
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)