Skip to content

Commit b94bfc4

Browse files
committed
🐛 fix crdupgradesafety diff when items schema differ (#1863)
* crdupgradesafety: original copies from kapp Signed-off-by: Joe Lanford <[email protected]> * crdupgradesafety: drop kapp dependency Signed-off-by: Joe Lanford <[email protected]> * crdupgradesafety: remove unused code Signed-off-by: Joe Lanford <[email protected]> * crdupgradesafety: fixup apiextensionsv1 import aliases Signed-off-by: Joe Lanford <[email protected]> * crdupgradesafety: fix to ignore diffs in child items Signed-off-by: Joe Lanford <[email protected]> --------- Signed-off-by: Joe Lanford <[email protected]>
1 parent 481e275 commit b94bfc4

File tree

10 files changed

+1063
-397
lines changed

10 files changed

+1063
-397
lines changed

go.mod

+1-11
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ module github.com/operator-framework/operator-controller
33
go 1.22.5
44

55
require (
6-
carvel.dev/kapp v0.63.3
76
github.com/BurntSushi/toml v1.4.0
87
github.com/Masterminds/semver/v3 v3.3.0
98
github.com/blang/semver/v4 v4.0.0
@@ -16,6 +15,7 @@ require (
1615
github.com/onsi/ginkgo/v2 v2.21.0
1716
github.com/onsi/gomega v1.35.1
1817
github.com/opencontainers/go-digest v1.0.0
18+
github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11
1919
github.com/operator-framework/api v0.27.0
2020
github.com/operator-framework/catalogd v1.0.0-rc1
2121
github.com/operator-framework/helm-operator-plugins v0.7.0
@@ -38,7 +38,6 @@ require (
3838
)
3939

4040
require (
41-
carvel.dev/vendir v0.40.0 // indirect
4241
dario.cat/mergo v1.0.1 // indirect
4342
github.com/AdaLogics/go-fuzz-headers v0.0.0-20230811130428-ced1acdcaa24 // indirect
4443
github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 // indirect
@@ -69,9 +68,6 @@ require (
6968
github.com/containers/libtrust v0.0.0-20230121012942-c1716e8a8d01 // indirect
7069
github.com/containers/ocicrypt v1.2.0 // indirect
7170
github.com/containers/storage v1.55.0 // indirect
72-
github.com/cppforlife/cobrautil v0.0.0-20221130162803-acdfead391ef // indirect
73-
github.com/cppforlife/color v1.9.1-0.20200716202919-6706ac40b835 // indirect
74-
github.com/cppforlife/go-cli-ui v0.0.0-20220425131040-94f26b16bc14 // indirect
7571
github.com/cyberphone/json-canonicalization v0.0.0-20231217050601-ba74d44ecf5f // indirect
7672
github.com/cyphar/filepath-securejoin v0.3.6 // indirect
7773
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
@@ -128,7 +124,6 @@ require (
128124
github.com/h2non/go-is-svg v0.0.0-20160927212452-35e8c4b0612c // indirect
129125
github.com/hashicorp/errwrap v1.1.0 // indirect
130126
github.com/hashicorp/go-multierror v1.1.1 // indirect
131-
github.com/hashicorp/go-version v1.6.0 // indirect
132127
github.com/huandu/xstrings v1.5.0 // indirect
133128
github.com/imdario/mergo v0.3.16 // indirect
134129
github.com/inconshreveable/mousetrap v1.1.0 // indirect
@@ -137,8 +132,6 @@ require (
137132
github.com/joelanford/ignore v0.1.1 // indirect
138133
github.com/josharian/intern v1.0.0 // indirect
139134
github.com/json-iterator/go v1.1.12 // indirect
140-
github.com/k14s/starlark-go v0.0.0-20200720175618-3a5c849cc368 // indirect
141-
github.com/k14s/ytt v0.36.0 // indirect
142135
github.com/klauspost/compress v1.17.11 // indirect
143136
github.com/klauspost/pgzip v1.2.6 // indirect
144137
github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 // indirect
@@ -172,7 +165,6 @@ require (
172165
github.com/oklog/ulid v1.3.1 // indirect
173166
github.com/opencontainers/image-spec v1.1.0 // indirect
174167
github.com/opencontainers/runtime-spec v1.2.0 // indirect
175-
github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11 // indirect
176168
github.com/operator-framework/operator-lib v0.15.0 // indirect
177169
github.com/otiai10/copy v1.14.0 // indirect
178170
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
@@ -202,8 +194,6 @@ require (
202194
github.com/ulikunitz/xz v0.5.12 // indirect
203195
github.com/vbatts/tar-split v0.11.5 // indirect
204196
github.com/vbauerster/mpb/v8 v8.7.5 // indirect
205-
github.com/vito/go-interact v1.0.1 // indirect
206-
github.com/vmware-tanzu/carvel-kapp-controller v0.51.0 // indirect
207197
github.com/x448/float16 v0.8.4 // indirect
208198
github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
209199
github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect

go.sum

-277
Large diffs are not rendered by default.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e
2+
// Attribution:
3+
// Copyright 2024 The Carvel Authors.
4+
// SPDX-License-Identifier: Apache-2.0
5+
6+
package crdupgradesafety
7+
8+
import (
9+
"errors"
10+
"fmt"
11+
"reflect"
12+
13+
"github.com/openshift/crd-schema-checker/pkg/manifestcomparators"
14+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
15+
"k8s.io/apimachinery/pkg/util/validation/field"
16+
)
17+
18+
// ChangeValidation is a function that accepts a FieldDiff
19+
// as a parameter and should return:
20+
// - a boolean representation of whether or not the change
21+
// - an error if the change would be unsafe
22+
// has been fully handled (i.e no additional changes exist)
23+
type ChangeValidation func(diff FieldDiff) (bool, error)
24+
25+
// ChangeValidator is a Validation implementation focused on
26+
// handling updates to existing fields in a CRD
27+
type ChangeValidator struct {
28+
// Validations is a slice of ChangeValidations
29+
// to run against each changed field
30+
Validations []ChangeValidation
31+
}
32+
33+
func (cv *ChangeValidator) Name() string {
34+
return "ChangeValidator"
35+
}
36+
37+
// Validate will compare each version in the provided existing and new CRDs.
38+
// Since the ChangeValidator is tailored to handling updates to existing fields in
39+
// each version of a CRD. As such the following is assumed:
40+
// - Validating the removal of versions during an update is handled outside of this
41+
// validator. If a version in the existing version of the CRD does not exist in the new
42+
// version that version of the CRD is skipped in this validator.
43+
// - Removal of existing fields is unsafe. Regardless of whether or not this is handled
44+
// by a validator outside this one, if a field is present in a version provided by the existing CRD
45+
// but not present in the same version provided by the new CRD this validation will fail.
46+
//
47+
// Additionally, any changes that are not validated and handled by the known ChangeValidations
48+
// are deemed as unsafe and returns an error.
49+
func (cv *ChangeValidator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error {
50+
errs := []error{}
51+
for _, version := range old.Spec.Versions {
52+
newVersion := manifestcomparators.GetVersionByName(&new, version.Name)
53+
if newVersion == nil {
54+
// if the new version doesn't exist skip this version
55+
continue
56+
}
57+
flatOld := FlattenSchema(version.Schema.OpenAPIV3Schema)
58+
flatNew := FlattenSchema(newVersion.Schema.OpenAPIV3Schema)
59+
60+
diffs, err := CalculateFlatSchemaDiff(flatOld, flatNew)
61+
if err != nil {
62+
errs = append(errs, fmt.Errorf("calculating schema diff for CRD version %q", version.Name))
63+
continue
64+
}
65+
66+
for field, diff := range diffs {
67+
handled := false
68+
for _, validation := range cv.Validations {
69+
ok, err := validation(diff)
70+
if err != nil {
71+
errs = append(errs, fmt.Errorf("version %q, field %q: %w", version.Name, field, err))
72+
}
73+
if ok {
74+
handled = true
75+
break
76+
}
77+
}
78+
79+
if !handled {
80+
errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", version.Name, field))
81+
}
82+
}
83+
}
84+
85+
if len(errs) > 0 {
86+
return errors.Join(errs...)
87+
}
88+
return nil
89+
}
90+
91+
type FieldDiff struct {
92+
Old *apiextensionsv1.JSONSchemaProps
93+
New *apiextensionsv1.JSONSchemaProps
94+
}
95+
96+
// FlatSchema is a flat representation of a CRD schema.
97+
type FlatSchema map[string]*apiextensionsv1.JSONSchemaProps
98+
99+
// FlattenSchema takes in a CRD version OpenAPIV3Schema and returns
100+
// a flattened representation of it. For example, a CRD with a schema of:
101+
// ```yaml
102+
//
103+
// ...
104+
// spec:
105+
// type: object
106+
// properties:
107+
// foo:
108+
// type: string
109+
// bar:
110+
// type: string
111+
// ...
112+
//
113+
// ```
114+
// would be represented as:
115+
//
116+
// map[string]*apiextensionsv1.JSONSchemaProps{
117+
// "^": {},
118+
// "^.spec": {},
119+
// "^.spec.foo": {},
120+
// "^.spec.bar": {},
121+
// }
122+
//
123+
// where "^" represents the "root" schema
124+
func FlattenSchema(schema *apiextensionsv1.JSONSchemaProps) FlatSchema {
125+
fieldMap := map[string]*apiextensionsv1.JSONSchemaProps{}
126+
127+
manifestcomparators.SchemaHas(schema,
128+
field.NewPath("^"),
129+
field.NewPath("^"),
130+
nil,
131+
func(s *apiextensionsv1.JSONSchemaProps, _, simpleLocation *field.Path, _ []*apiextensionsv1.JSONSchemaProps) bool {
132+
fieldMap[simpleLocation.String()] = s.DeepCopy()
133+
return false
134+
})
135+
136+
return fieldMap
137+
}
138+
139+
// CalculateFlatSchemaDiff finds fields in a FlatSchema that are different
140+
// and returns a mapping of field --> old and new field schemas. If a field
141+
// exists in the old FlatSchema but not the new an empty diff mapping and an error is returned.
142+
func CalculateFlatSchemaDiff(o, n FlatSchema) (map[string]FieldDiff, error) {
143+
diffMap := map[string]FieldDiff{}
144+
for field, schema := range o {
145+
if _, ok := n[field]; !ok {
146+
return diffMap, fmt.Errorf("field %q in existing not found in new", field)
147+
}
148+
newSchema := n[field]
149+
150+
// Copy the schemas and remove any child properties for comparison.
151+
// In theory this will focus in on detecting changes for only the
152+
// field we are looking at and ignore changes in the children fields.
153+
// Since we are iterating through the map that should have all fields
154+
// we should still detect changes in the children fields.
155+
oldCopy := schema.DeepCopy()
156+
newCopy := newSchema.DeepCopy()
157+
oldCopy.Properties, oldCopy.Items = nil, nil
158+
newCopy.Properties, newCopy.Items = nil, nil
159+
if !reflect.DeepEqual(oldCopy, newCopy) {
160+
diffMap[field] = FieldDiff{
161+
Old: oldCopy,
162+
New: newCopy,
163+
}
164+
}
165+
}
166+
return diffMap, nil
167+
}

0 commit comments

Comments
 (0)