diff --git a/go.mod b/go.mod index eafa8594f..6a7418039 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( github.com/klauspost/compress v1.18.0 github.com/opencontainers/go-digest v1.0.0 github.com/opencontainers/image-spec v1.1.1 - github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11 github.com/operator-framework/api v0.32.0 github.com/operator-framework/helm-operator-plugins v0.8.0 github.com/operator-framework/operator-registry v1.55.0 @@ -44,6 +43,7 @@ require ( k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 sigs.k8s.io/controller-runtime v0.21.0 sigs.k8s.io/controller-tools v0.18.0 + sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58 sigs.k8s.io/yaml v1.5.0 ) diff --git a/go.sum b/go.sum index f0cf82998..5174656a7 100644 --- a/go.sum +++ b/go.sum @@ -389,8 +389,6 @@ github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJw github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M= github.com/opencontainers/runtime-spec v1.2.1 h1:S4k4ryNgEpxW1dzyqffOmhI1BHYcjzU8lpJfSlR0xww= github.com/opencontainers/runtime-spec v1.2.1/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= -github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11 h1:eTNDkNRNV5lZvUbVM9Nop0lBcljSnA8rZX6yQPZ0ZnU= -github.com/openshift/crd-schema-checker v0.0.0-20240404194209-35a9033b1d11/go.mod h1:EmVJt97N+pfWFsli/ipXTBZqSG5F5KGQhm3c3IsGq1o= github.com/operator-framework/api v0.32.0 h1:LZSZr7at3NrjsjwQVNsYD+04o5wMq75jrR0dMYiIIH8= github.com/operator-framework/api v0.32.0/go.mod h1:OGJo6HUYxoQwpGaLr0lPJzSek51RiXajJSSa8Jzjvp8= github.com/operator-framework/helm-operator-plugins v0.8.0 h1:0f6HOQC5likkf0b/OvGvw7nhDb6h8Cj5twdCNjwNzMc= @@ -788,6 +786,8 @@ sigs.k8s.io/controller-runtime v0.21.0 h1:CYfjpEuicjUecRk+KAeyYh+ouUBn4llGyDYytI sigs.k8s.io/controller-runtime v0.21.0/go.mod h1:OSg14+F65eWqIu4DceX7k/+QRAbTTvxeQSNSOQpukWM= sigs.k8s.io/controller-tools v0.18.0 h1:rGxGZCZTV2wJreeRgqVoWab/mfcumTMmSwKzoM9xrsE= sigs.k8s.io/controller-tools v0.18.0/go.mod h1:gLKoiGBriyNh+x1rWtUQnakUYEujErjXs9pf+x/8n1U= +sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58 h1:VTvhbqgZMVoDpHHPuZLaOgzjjsJBhO8+vDKA1COuLCY= +sigs.k8s.io/crdify v0.4.1-0.20250613143457-398e4483fb58/go.mod h1:ZIFxaYNgKYmFtZCLPysncXQ8oqwnNlHQbRUfxJHZwzU= sigs.k8s.io/gateway-api v1.1.0 h1:DsLDXCi6jR+Xz8/xd0Z1PYl2Pn0TyaFMOPPZIj4inDM= sigs.k8s.io/gateway-api v1.1.0/go.mod h1:ZH4lHrL2sDi0FHZ9jjneb8kKnGzFWyrTya35sWUTrRs= sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 h1:gBQPwqORJ8d8/YNZWEjoZs7npUVDpVXUUOFfW6CgAqE= diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go deleted file mode 100644 index 4678b2de0..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go +++ /dev/null @@ -1,171 +0,0 @@ -// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e -// Attribution: -// Copyright 2024 The Carvel Authors. -// SPDX-License-Identifier: Apache-2.0 - -package crdupgradesafety - -import ( - "errors" - "fmt" - "maps" - "reflect" - "slices" - - "github.com/openshift/crd-schema-checker/pkg/manifestcomparators" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/util/validation/field" -) - -// ChangeValidation is a function that accepts a FieldDiff -// as a parameter and should return: -// - a boolean representation of whether or not the change -// - an error if the change would be unsafe -// has been fully handled (i.e no additional changes exist) -type ChangeValidation func(diff FieldDiff) (bool, error) - -// ChangeValidator is a Validation implementation focused on -// handling updates to existing fields in a CRD -type ChangeValidator struct { - // Validations is a slice of ChangeValidations - // to run against each changed field - Validations []ChangeValidation -} - -func (cv *ChangeValidator) Name() string { - return "ChangeValidator" -} - -// Validate will compare each version in the provided existing and new CRDs. -// Since the ChangeValidator is tailored to handling updates to existing fields in -// each version of a CRD. As such the following is assumed: -// - Validating the removal of versions during an update is handled outside of this -// validator. If a version in the existing version of the CRD does not exist in the new -// version that version of the CRD is skipped in this validator. -// - Removal of existing fields is unsafe. Regardless of whether or not this is handled -// by a validator outside this one, if a field is present in a version provided by the existing CRD -// but not present in the same version provided by the new CRD this validation will fail. -// -// Additionally, any changes that are not validated and handled by the known ChangeValidations -// are deemed as unsafe and returns an error. -func (cv *ChangeValidator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error { - errs := []error{} - for _, version := range old.Spec.Versions { - newVersion := manifestcomparators.GetVersionByName(&new, version.Name) - if newVersion == nil { - // if the new version doesn't exist skip this version - continue - } - flatOld := FlattenSchema(version.Schema.OpenAPIV3Schema) - flatNew := FlattenSchema(newVersion.Schema.OpenAPIV3Schema) - - diffs, err := CalculateFlatSchemaDiff(flatOld, flatNew) - if err != nil { - errs = append(errs, fmt.Errorf("calculating schema diff for CRD version %q", version.Name)) - continue - } - - for _, field := range slices.Sorted(maps.Keys(diffs)) { - diff := diffs[field] - - handled := false - for _, validation := range cv.Validations { - ok, err := validation(diff) - if err != nil { - errs = append(errs, fmt.Errorf("version %q, field %q: %w", version.Name, field, err)) - } - if ok { - handled = true - break - } - } - - if !handled { - errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", version.Name, field)) - } - } - } - - if len(errs) > 0 { - return errors.Join(errs...) - } - return nil -} - -type FieldDiff struct { - Old *apiextensionsv1.JSONSchemaProps - New *apiextensionsv1.JSONSchemaProps -} - -// FlatSchema is a flat representation of a CRD schema. -type FlatSchema map[string]*apiextensionsv1.JSONSchemaProps - -// FlattenSchema takes in a CRD version OpenAPIV3Schema and returns -// a flattened representation of it. For example, a CRD with a schema of: -// ```yaml -// -// ... -// spec: -// type: object -// properties: -// foo: -// type: string -// bar: -// type: string -// ... -// -// ``` -// would be represented as: -// -// map[string]*apiextensionsv1.JSONSchemaProps{ -// "^": {}, -// "^.spec": {}, -// "^.spec.foo": {}, -// "^.spec.bar": {}, -// } -// -// where "^" represents the "root" schema -func FlattenSchema(schema *apiextensionsv1.JSONSchemaProps) FlatSchema { - fieldMap := map[string]*apiextensionsv1.JSONSchemaProps{} - - manifestcomparators.SchemaHas(schema, - field.NewPath("^"), - field.NewPath("^"), - nil, - func(s *apiextensionsv1.JSONSchemaProps, _, simpleLocation *field.Path, _ []*apiextensionsv1.JSONSchemaProps) bool { - fieldMap[simpleLocation.String()] = s.DeepCopy() - return false - }) - - return fieldMap -} - -// CalculateFlatSchemaDiff finds fields in a FlatSchema that are different -// and returns a mapping of field --> old and new field schemas. If a field -// exists in the old FlatSchema but not the new an empty diff mapping and an error is returned. -func CalculateFlatSchemaDiff(o, n FlatSchema) (map[string]FieldDiff, error) { - diffMap := map[string]FieldDiff{} - for field, schema := range o { - if _, ok := n[field]; !ok { - return diffMap, fmt.Errorf("field %q in existing not found in new", field) - } - newSchema := n[field] - - // Copy the schemas and remove any child properties for comparison. - // In theory this will focus in on detecting changes for only the - // field we are looking at and ignore changes in the children fields. - // Since we are iterating through the map that should have all fields - // we should still detect changes in the children fields. - oldCopy := schema.DeepCopy() - newCopy := newSchema.DeepCopy() - oldCopy.Properties, oldCopy.Items = nil, nil - newCopy.Properties, newCopy.Items = nil, nil - if !reflect.DeepEqual(oldCopy, newCopy) { - diffMap[field] = FieldDiff{ - Old: oldCopy, - New: newCopy, - } - } - } - return diffMap, nil -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go deleted file mode 100644 index cc12bc5c1..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go +++ /dev/null @@ -1,338 +0,0 @@ -// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e -// Attribution: -// Copyright 2024 The Carvel Authors. -// SPDX-License-Identifier: Apache-2.0 - -package crdupgradesafety_test - -import ( - "errors" - "fmt" - "testing" - - "github.com/stretchr/testify/assert" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety" -) - -func TestCalculateFlatSchemaDiff(t *testing.T) { - for _, tc := range []struct { - name string - old crdupgradesafety.FlatSchema - new crdupgradesafety.FlatSchema - expectedDiff map[string]crdupgradesafety.FieldDiff - shouldError bool - }{ - { - name: "no diff in schemas, empty diff, no error", - old: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{}, - }, - new: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{}, - }, - expectedDiff: map[string]crdupgradesafety.FieldDiff{}, - }, - { - name: "diff in schemas, diff returned, no error", - old: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{}, - }, - new: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - expectedDiff: map[string]crdupgradesafety.FieldDiff{ - "foo": { - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ID: "bar"}, - }, - }, - }, - { - name: "diff in child properties only, no diff returned, no error", - old: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{ - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "bar": {ID: "bar"}, - }, - }, - }, - new: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{ - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "bar": {ID: "baz"}, - }, - }, - }, - expectedDiff: map[string]crdupgradesafety.FieldDiff{}, - }, - { - name: "diff in child items only, no diff returned, no error", - old: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{ - Items: &apiextensionsv1.JSONSchemaPropsOrArray{Schema: &apiextensionsv1.JSONSchemaProps{ID: "bar"}}, - }, - }, - new: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{ - Items: &apiextensionsv1.JSONSchemaPropsOrArray{Schema: &apiextensionsv1.JSONSchemaProps{ID: "baz"}}, - }, - }, - expectedDiff: map[string]crdupgradesafety.FieldDiff{}, - }, - { - name: "field exists in old but not new, no diff returned, error", - old: crdupgradesafety.FlatSchema{ - "foo": &apiextensionsv1.JSONSchemaProps{}, - }, - new: crdupgradesafety.FlatSchema{ - "bar": &apiextensionsv1.JSONSchemaProps{}, - }, - expectedDiff: map[string]crdupgradesafety.FieldDiff{}, - shouldError: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - diff, err := crdupgradesafety.CalculateFlatSchemaDiff(tc.old, tc.new) - assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError) - assert.Equal(t, tc.expectedDiff, diff) - }) - } -} - -func TestFlattenSchema(t *testing.T) { - schema := &apiextensionsv1.JSONSchemaProps{ - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "foo": { - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "bar": {}, - }, - }, - "baz": {}, - }, - } - - foo := schema.Properties["foo"] - foobar := schema.Properties["foo"].Properties["bar"] - baz := schema.Properties["baz"] - expected := crdupgradesafety.FlatSchema{ - "^": schema, - "^.foo": &foo, - "^.foo.bar": &foobar, - "^.baz": &baz, - } - - actual := crdupgradesafety.FlattenSchema(schema) - - assert.Equal(t, expected, actual) -} - -func TestChangeValidator(t *testing.T) { - validationErr1 := errors.New(`version "v1alpha1", field "^" has unknown change, refusing to determine that change is safe`) - validationErr2 := errors.New(`version "v1alpha1", field "^": fail`) - - for _, tc := range []struct { - name string - changeValidator *crdupgradesafety.ChangeValidator - old apiextensionsv1.CustomResourceDefinition - new apiextensionsv1.CustomResourceDefinition - expectedError error - }{ - { - name: "no changes, no error", - changeValidator: &crdupgradesafety.ChangeValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("should not run") - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - }, - { - name: "changes, validation successful, change is fully handled, no error", - changeValidator: &crdupgradesafety.ChangeValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return true, nil - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - }, - { - name: "changes, validation successful, change not fully handled, error", - changeValidator: &crdupgradesafety.ChangeValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, nil - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: validationErr1, - }, - { - name: "changes, validation failed, change fully handled, error", - changeValidator: &crdupgradesafety.ChangeValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return true, errors.New("fail") - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: validationErr2, - }, - { - name: "changes, validation failed, change not fully handled, ordered error", - changeValidator: &crdupgradesafety.ChangeValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("fail") - }, - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("error") - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version "v1alpha1", field "^": error`, validationErr1), - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := tc.changeValidator.Validate(tc.old, tc.new) - if tc.expectedError != nil { - assert.EqualError(t, err, tc.expectedError.Error()) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go deleted file mode 100644 index 61d8b55c3..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go +++ /dev/null @@ -1,254 +0,0 @@ -package crdupgradesafety - -import ( - "bytes" - "cmp" - "fmt" - "reflect" - - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/util/sets" -) - -type resetFunc func(diff FieldDiff) FieldDiff - -func isHandled(diff FieldDiff, reset resetFunc) bool { - diff = reset(diff) - return reflect.DeepEqual(diff.Old, diff.New) -} - -func Enum(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Enum = []apiextensionsv1.JSON{} - diff.New.Enum = []apiextensionsv1.JSON{} - return diff - } - - oldEnums := sets.New[string]() - for _, json := range diff.Old.Enum { - oldEnums.Insert(string(json.Raw)) - } - - newEnums := sets.New[string]() - for _, json := range diff.New.Enum { - newEnums.Insert(string(json.Raw)) - } - diffEnums := oldEnums.Difference(newEnums) - var err error - - switch { - case oldEnums.Len() == 0 && newEnums.Len() > 0: - err = fmt.Errorf("enum constraints %v added when there were no restrictions previously", newEnums.UnsortedList()) - case diffEnums.Len() > 0: - err = fmt.Errorf("enums %v removed from the set of previously allowed values", diffEnums.UnsortedList()) - } - - return isHandled(diff, reset), err -} - -func Required(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Required = []string{} - diff.New.Required = []string{} - return diff - } - - oldRequired := sets.New(diff.Old.Required...) - newRequired := sets.New(diff.New.Required...) - diffRequired := newRequired.Difference(oldRequired) - var err error - - if diffRequired.Len() > 0 { - err = fmt.Errorf("new required fields %v added", diffRequired.UnsortedList()) - } - - return isHandled(diff, reset), err -} - -func maxVerification[T cmp.Ordered](older *T, newer *T) error { - var err error - switch { - case older == nil && newer != nil: - err = fmt.Errorf("constraint %v added when there were no restrictions previously", *newer) - case older != nil && newer != nil && *newer < *older: - err = fmt.Errorf("constraint decreased from %v to %v", *older, *newer) - } - return err -} - -func Maximum(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Maximum = nil - diff.New.Maximum = nil - return diff - } - - err := maxVerification(diff.Old.Maximum, diff.New.Maximum) - if err != nil { - err = fmt.Errorf("maximum: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MaxItems(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MaxItems = nil - diff.New.MaxItems = nil - return diff - } - - err := maxVerification(diff.Old.MaxItems, diff.New.MaxItems) - if err != nil { - err = fmt.Errorf("maxItems: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MaxLength(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MaxLength = nil - diff.New.MaxLength = nil - return diff - } - - err := maxVerification(diff.Old.MaxLength, diff.New.MaxLength) - if err != nil { - err = fmt.Errorf("maxLength: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MaxProperties(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MaxProperties = nil - diff.New.MaxProperties = nil - return diff - } - - err := maxVerification(diff.Old.MaxProperties, diff.New.MaxProperties) - if err != nil { - err = fmt.Errorf("maxProperties: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func minVerification[T cmp.Ordered](older *T, newer *T) error { - var err error - switch { - case older == nil && newer != nil: - err = fmt.Errorf("constraint %v added when there were no restrictions previously", *newer) - case older != nil && newer != nil && *newer > *older: - err = fmt.Errorf("constraint increased from %v to %v", *older, *newer) - } - return err -} - -func Minimum(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Minimum = nil - diff.New.Minimum = nil - return diff - } - - err := minVerification(diff.Old.Minimum, diff.New.Minimum) - if err != nil { - err = fmt.Errorf("minimum: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MinItems(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MinItems = nil - diff.New.MinItems = nil - return diff - } - - err := minVerification(diff.Old.MinItems, diff.New.MinItems) - if err != nil { - err = fmt.Errorf("minItems: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MinLength(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MinLength = nil - diff.New.MinLength = nil - return diff - } - - err := minVerification(diff.Old.MinLength, diff.New.MinLength) - if err != nil { - err = fmt.Errorf("minLength: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func MinProperties(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.MinProperties = nil - diff.New.MinProperties = nil - return diff - } - - err := minVerification(diff.Old.MinProperties, diff.New.MinProperties) - if err != nil { - err = fmt.Errorf("minProperties: %s", err.Error()) - } - - return isHandled(diff, reset), err -} - -func Default(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Default = nil - diff.New.Default = nil - return diff - } - - var err error - - switch { - case diff.Old.Default == nil && diff.New.Default != nil: - err = fmt.Errorf("default value %q added when there was no default previously", string(diff.New.Default.Raw)) - case diff.Old.Default != nil && diff.New.Default == nil: - err = fmt.Errorf("default value %q removed", string(diff.Old.Default.Raw)) - case diff.Old.Default != nil && diff.New.Default != nil && !bytes.Equal(diff.Old.Default.Raw, diff.New.Default.Raw): - err = fmt.Errorf("default value changed from %q to %q", string(diff.Old.Default.Raw), string(diff.New.Default.Raw)) - } - - return isHandled(diff, reset), err -} - -func Type(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Type = "" - diff.New.Type = "" - return diff - } - - var err error - if diff.Old.Type != diff.New.Type { - err = fmt.Errorf("type changed from %q to %q", diff.Old.Type, diff.New.Type) - } - - return isHandled(diff, reset), err -} - -// Description changes are considered safe and non-breaking. -func Description(diff FieldDiff) (bool, error) { - reset := func(diff FieldDiff) FieldDiff { - diff.Old.Description = "" - diff.New.Description = "" - return diff - } - return isHandled(diff, reset), nil -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go deleted file mode 100644 index ebceed8b4..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ /dev/null @@ -1,1008 +0,0 @@ -package crdupgradesafety - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/require" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/utils/ptr" -) - -type testcase struct { - name string - diff FieldDiff - err error - handled bool -} - -func TestEnum(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - }, - }, - }, - err: nil, - handled: true, - }, - { - name: "new enum constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{}, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - }, - }, - }, - err: errors.New("enum constraints [foo] added when there were no restrictions previously"), - handled: true, - }, - { - name: "remove enum value, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - { - Raw: []byte("bar"), - }, - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("bar"), - }, - }, - }, - }, - err: errors.New("enums [foo] removed from the set of previously allowed values"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - { - name: "different field changed with enum, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - Enum: []apiextensionsv1.JSON{ - { - Raw: []byte("foo"), - }, - }, - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Enum(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestRequired(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Required: []string{ - "foo", - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Required: []string{ - "foo", - }, - }, - }, - err: nil, - handled: true, - }, - { - name: "new required field, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - Required: []string{ - "foo", - }, - }, - }, - err: errors.New("new required fields [foo] added"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Required(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMaximum(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(10.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(10.0), - }, - }, - err: nil, - handled: true, - }, - { - name: "new maximum constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(10.0), - }, - }, - err: errors.New("maximum: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "maximum constraint decreased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(20.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(10.0), - }, - }, - err: errors.New("maximum: constraint decreased from 20 to 10"), - handled: true, - }, - { - name: "maximum constraint increased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(20.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Maximum: ptr.To(30.0), - }, - }, - err: nil, - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Maximum(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMaxItems(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new maxItems constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(10)), - }, - }, - err: errors.New("maxItems: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "maxItems constraint decreased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(10)), - }, - }, - err: errors.New("maxItems: constraint decreased from 20 to 10"), - handled: true, - }, - { - name: "maxitems constraint increased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxItems: ptr.To(int64(20)), - }, - }, - err: nil, - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MaxItems(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMaxLength(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new maxLength constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(10)), - }, - }, - err: errors.New("maxLength: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "maxLength constraint decreased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(10)), - }, - }, - err: errors.New("maxLength: constraint decreased from 20 to 10"), - handled: true, - }, - { - name: "maxLength constraint increased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxLength: ptr.To(int64(20)), - }, - }, - err: nil, - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MaxLength(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMaxProperties(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new maxProperties constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(10)), - }, - }, - err: errors.New("maxProperties: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "maxProperties constraint decreased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(10)), - }, - }, - err: errors.New("maxProperties: constraint decreased from 20 to 10"), - handled: true, - }, - { - name: "maxProperties constraint increased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MaxProperties: ptr.To(int64(20)), - }, - }, - err: nil, - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MaxProperties(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMinItems(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new minItems constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(10)), - }, - }, - err: errors.New("minItems: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "minItems constraint decreased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "minItems constraint increased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinItems: ptr.To(int64(20)), - }, - }, - err: errors.New("minItems: constraint increased from 10 to 20"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MinItems(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMinimum(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(10.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(10.0), - }, - }, - err: nil, - handled: true, - }, - { - name: "new minimum constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(10.0), - }, - }, - err: errors.New("minimum: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "minLength constraint decreased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(20.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(10.0), - }, - }, - err: nil, - handled: true, - }, - { - name: "minLength constraint increased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(10.0), - }, - New: &apiextensionsv1.JSONSchemaProps{ - Minimum: ptr.To(20.0), - }, - }, - err: errors.New("minimum: constraint increased from 10 to 20"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Minimum(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMinLength(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new minLength constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(10)), - }, - }, - err: errors.New("minLength: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "minLength constraint decreased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "minLength constraint increased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinLength: ptr.To(int64(20)), - }, - }, - err: errors.New("minLength: constraint increased from 10 to 20"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MinLength(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestMinProperties(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "new minProperties constraint, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(10)), - }, - }, - err: errors.New("minProperties: constraint 10 added when there were no restrictions previously"), - handled: true, - }, - { - name: "minProperties constraint decreased, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(20)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(10)), - }, - }, - err: nil, - handled: true, - }, - { - name: "minProperties constraint increased, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(10)), - }, - New: &apiextensionsv1.JSONSchemaProps{ - MinProperties: ptr.To(int64(20)), - }, - }, - err: errors.New("minProperties: constraint increased from 10 to 20"), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := MinProperties(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestDefault(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("foo"), - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("foo"), - }, - }, - }, - err: nil, - handled: true, - }, - { - name: "new default value, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("foo"), - }, - }, - }, - err: errors.New("default value \"foo\" added when there was no default previously"), - handled: true, - }, - { - name: "default value removed, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("foo"), - }, - }, - New: &apiextensionsv1.JSONSchemaProps{}, - }, - err: errors.New("default value \"foo\" removed"), - handled: true, - }, - { - name: "default value changed, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("foo"), - }, - }, - New: &apiextensionsv1.JSONSchemaProps{ - Default: &apiextensionsv1.JSON{ - Raw: []byte("bar"), - }, - }, - }, - err: errors.New("default value changed from \"foo\" to \"bar\""), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Default(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestType(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Type: "string", - }, - New: &apiextensionsv1.JSONSchemaProps{ - Type: "string", - }, - }, - err: nil, - handled: true, - }, - { - name: "type changed, error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Type: "string", - }, - New: &apiextensionsv1.JSONSchemaProps{ - Type: "integer", - }, - }, - err: errors.New("type changed from \"string\" to \"integer\""), - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Type(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} - -func TestDescription(t *testing.T) { - for _, tc := range []testcase{ - { - name: "no diff, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Description: "some field", - }, - New: &apiextensionsv1.JSONSchemaProps{ - Description: "some field", - }, - }, - err: nil, - handled: true, - }, - { - name: "description changed, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Description: "old description", - }, - New: &apiextensionsv1.JSONSchemaProps{ - Description: "new description", - }, - }, - err: nil, - handled: true, - }, - { - name: "description added, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{}, - New: &apiextensionsv1.JSONSchemaProps{ - Description: "a new description was added", - }, - }, - err: nil, - handled: true, - }, - { - name: "description removed, no error, handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - Description: "this description will be removed", - }, - New: &apiextensionsv1.JSONSchemaProps{}, - }, - err: nil, - handled: true, - }, - { - name: "different field changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - }, - }, - err: nil, - handled: false, - }, - { - name: "different field changed with description, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - Description: "description", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - Description: "description", - }, - }, - err: nil, - handled: false, - }, - { - name: "description and ID changed, no error, not handled", - diff: FieldDiff{ - Old: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - Description: "old description", - }, - New: &apiextensionsv1.JSONSchemaProps{ - ID: "bar", - Description: "new description", - }, - }, - err: nil, - handled: false, - }, - } { - t.Run(tc.name, func(t *testing.T) { - handled, err := Description(tc.diff) - require.Equal(t, tc.err, err) - require.Equal(t, tc.handled, handled) - }) - } -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 0904bf4d4..fadc85873 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -12,51 +12,38 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/crdify/pkg/config" + "sigs.k8s.io/crdify/pkg/runner" + "sigs.k8s.io/crdify/pkg/validations" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util" ) type Option func(p *Preflight) -func WithValidator(v *Validator) Option { +func WithConfig(cfg *config.Config) Option { return func(p *Preflight) { - p.validator = v + p.config = cfg + } +} + +func WithRegistry(reg validations.Registry) Option { + return func(p *Preflight) { + p.registry = reg } } type Preflight struct { crdClient apiextensionsv1client.CustomResourceDefinitionInterface - validator *Validator + config *config.Config + registry validations.Registry } func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface, opts ...Option) *Preflight { - changeValidations := []ChangeValidation{ - Description, - Enum, - Required, - Maximum, - MaxItems, - MaxLength, - MaxProperties, - Minimum, - MinItems, - MinLength, - MinProperties, - Default, - Type, - } p := &Preflight{ crdClient: crdCli, - // create a default validator. Can be overridden via the options - validator: &Validator{ - Validations: []Validation{ - NewValidationFunc("NoScopeChange", NoScopeChange), - NewValidationFunc("NoStoredVersionRemoved", NoStoredVersionRemoved), - NewValidationFunc("NoExistingFieldRemoved", NoExistingFieldRemoved), - &ServedVersionValidator{Validations: changeValidations}, - &ChangeValidator{Validations: changeValidations}, - }, - }, + config: defaultConfig(), + registry: defaultRegistry(), } for _, o := range opts { @@ -84,6 +71,11 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro return fmt.Errorf("parsing release %q objects: %w", rel.Name, err) } + runner, err := runner.New(p.config, p.registry) + if err != nil { + return fmt.Errorf("creating CRD validation runner: %w", err) + } + validateErrors := make([]error, 0, len(relObjects)) for _, obj := range relObjects { if obj.GetObjectKind().GroupVersionKind() != apiextensionsv1.SchemeGroupVersion.WithKind("CustomResourceDefinition") { @@ -110,11 +102,91 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro return fmt.Errorf("getting existing resource for CRD %q: %w", newCrd.Name, err) } - err = p.validator.Validate(*oldCrd, *newCrd) - if err != nil { - validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q failed: %w", newCrd.Name, err)) + results := runner.Run(oldCrd, newCrd) + if results.HasFailures() { + resultErrs := crdWideErrors(results) + resultErrs = append(resultErrs, sameVersionErrors(results)...) + resultErrs = append(resultErrs, servedVersionErrors(results)...) + + validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q: %w", newCrd.Name, errors.Join(resultErrs...))) } } return errors.Join(validateErrors...) } + +func defaultConfig() *config.Config { + return &config.Config{ + // Ignore served version validations if conversion policy is set. + Conversion: config.ConversionPolicyIgnore, + // Fail-closed by default + UnhandledEnforcement: config.EnforcementPolicyError, + // Use the default validation configurations as they are + // the strictest possible. + Validations: []config.ValidationConfig{ + // Do not enforce the description validation + // because OLM should not block on field description changes. + { + Name: "description", + Enforcement: config.EnforcementPolicyNone, + }, + }, + } +} + +func defaultRegistry() validations.Registry { + return runner.DefaultRegistry() +} + +func crdWideErrors(results *runner.Results) []error { + if results == nil { + return nil + } + + errs := []error{} + for _, result := range results.CRDValidation { + for _, err := range result.Errors { + errs = append(errs, fmt.Errorf("%s: %s", result.Name, err)) + } + } + + return errs +} + +func sameVersionErrors(results *runner.Results) []error { + if results == nil { + return nil + } + + errs := []error{} + for version, propertyResults := range results.SameVersionValidation { + for property, comparisonResults := range propertyResults { + for _, result := range comparisonResults { + for _, err := range result.Errors { + errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err)) + } + } + } + } + + return errs +} + +func servedVersionErrors(results *runner.Results) []error { + if results == nil { + return nil + } + + errs := []error{} + for version, propertyResults := range results.ServedVersionValidation { + for property, comparisonResults := range propertyResults { + for _, result := range comparisonResults { + for _, err := range result.Errors { + errs = append(errs, fmt.Errorf("%s: %s: %s: %s", version, property, result.Name, err)) + } + } + } + } + + return errs +} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index 12241bd7f..43613d899 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go @@ -30,11 +30,8 @@ func (c *MockCRDGetter) Get(ctx context.Context, name string, options metav1.Get return c.oldCrd, c.getErr } -func newMockPreflight(crd *apiextensionsv1.CustomResourceDefinition, err error, customValidator *crdupgradesafety.Validator) *crdupgradesafety.Preflight { +func newMockPreflight(crd *apiextensionsv1.CustomResourceDefinition, err error) *crdupgradesafety.Preflight { var preflightOpts []crdupgradesafety.Option - if customValidator != nil { - preflightOpts = append(preflightOpts, crdupgradesafety.WithValidator(customValidator)) - } return crdupgradesafety.NewPreflight(&MockCRDGetter{ oldCrd: crd, getErr: err, @@ -75,7 +72,6 @@ func TestInstall(t *testing.T) { tests := []struct { name string oldCrdPath string - validator *crdupgradesafety.Validator release *release.Release wantErrMsgs []string wantCrdGetErr error @@ -129,22 +125,6 @@ func TestInstall(t *testing.T) { }, wantErrMsgs: []string{"json: cannot unmarshal"}, }, - { - name: "custom validator", - oldCrdPath: "old-crd.json", - release: &release.Release{ - Name: "test-release", - Manifest: getManifestString(t, "old-crd.json"), - }, - validator: &crdupgradesafety.Validator{ - Validations: []crdupgradesafety.Validation{ - crdupgradesafety.NewValidationFunc("test", func(old, new apiextensionsv1.CustomResourceDefinition) error { - return fmt.Errorf("custom validation error!!") - }), - }, - }, - wantErrMsgs: []string{"custom validation error!!"}, - }, { name: "valid upgrade", oldCrdPath: "old-crd.json", @@ -163,19 +143,19 @@ func TestInstall(t *testing.T) { Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, wantErrMsgs: []string{ - `"NoScopeChange"`, - `"NoStoredVersionRemoved"`, - `enum constraints`, - `new required fields`, - `maximum: constraint`, - `maxItems: constraint`, - `maxLength: constraint`, - `maxProperties: constraint`, - `minimum: constraint`, - `minItems: constraint`, - `minLength: constraint`, - `minProperties: constraint`, - `default value`, + `scope:`, + `storedVersionRemoval:`, + `enum:`, + `required:`, + `maximum:`, + `maxItems:`, + `maxLength:`, + `maxProperties:`, + `minimum:`, + `minItems:`, + `minLength:`, + `minProperties:`, + `default:`, }, }, { @@ -188,14 +168,24 @@ func TestInstall(t *testing.T) { Manifest: getManifestString(t, "crd-field-removed.json"), }, wantErrMsgs: []string{ - `"NoExistingFieldRemoved"`, + `existingFieldRemoval:`, + }, + }, + { + name: "new crd validation should not fail on description changes", + // Separate test from above as this error will cause the validator to + // return early and skip some of the above validations. + oldCrdPath: "old-crd.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-description-changed.json"), }, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr, tc.validator) + preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr) err := preflight.Install(context.Background(), tc.release) if len(tc.wantErrMsgs) != 0 { for _, expectedErrMsg := range tc.wantErrMsgs { @@ -212,7 +202,6 @@ func TestUpgrade(t *testing.T) { tests := []struct { name string oldCrdPath string - validator *crdupgradesafety.Validator release *release.Release wantErrMsgs []string wantCrdGetErr error @@ -266,22 +255,6 @@ func TestUpgrade(t *testing.T) { }, wantErrMsgs: []string{"json: cannot unmarshal"}, }, - { - name: "custom validator", - oldCrdPath: "old-crd.json", - release: &release.Release{ - Name: "test-release", - Manifest: getManifestString(t, "old-crd.json"), - }, - validator: &crdupgradesafety.Validator{ - Validations: []crdupgradesafety.Validation{ - crdupgradesafety.NewValidationFunc("test", func(old, new apiextensionsv1.CustomResourceDefinition) error { - return fmt.Errorf("custom validation error!!") - }), - }, - }, - wantErrMsgs: []string{"custom validation error!!"}, - }, { name: "valid upgrade", oldCrdPath: "old-crd.json", @@ -300,19 +273,19 @@ func TestUpgrade(t *testing.T) { Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, wantErrMsgs: []string{ - `"NoScopeChange"`, - `"NoStoredVersionRemoved"`, - `enum constraints`, - `new required fields`, - `maximum: constraint`, - `maxItems: constraint`, - `maxLength: constraint`, - `maxProperties: constraint`, - `minimum: constraint`, - `minItems: constraint`, - `minLength: constraint`, - `minProperties: constraint`, - `default value`, + `scope:`, + `storedVersionRemoval:`, + `enum:`, + `required:`, + `maximum:`, + `maxItems:`, + `maxLength:`, + `maxProperties:`, + `minimum:`, + `minItems:`, + `minLength:`, + `minProperties:`, + `default:`, }, }, { @@ -325,7 +298,7 @@ func TestUpgrade(t *testing.T) { Manifest: getManifestString(t, "crd-field-removed.json"), }, wantErrMsgs: []string{ - `"NoExistingFieldRemoved"`, + `existingFieldRemoval:`, }, }, { @@ -344,14 +317,24 @@ func TestUpgrade(t *testing.T) { Manifest: getManifestString(t, "crd-conversion-no-webhook.json"), }, wantErrMsgs: []string{ - `"ServedVersionValidator" validation failed: version upgrade "v1" to "v2", field "^.spec.foobarbaz": enums`, + `validating upgrade for CRD "crontabs.stable.example.com": v1 <-> v2: ^.spec.foobarbaz: enum: allowed enum values removed`, + }, + }, + { + name: "new crd validation should not fail on description changes", + // Separate test from above as this error will cause the validator to + // return early and skip some of the above validations. + oldCrdPath: "old-crd.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-description-changed.json"), }, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr, tc.validator) + preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr) err := preflight.Upgrade(context.Background(), tc.release) if len(tc.wantErrMsgs) != 0 { for _, expectedErrMsg := range tc.wantErrMsgs { diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go deleted file mode 100644 index d66f1ed9c..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go +++ /dev/null @@ -1,74 +0,0 @@ -package crdupgradesafety - -import ( - "errors" - "fmt" - "maps" - "slices" - - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - versionhelper "k8s.io/apimachinery/pkg/version" -) - -type ServedVersionValidator struct { - Validations []ChangeValidation -} - -func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error { - // If conversion webhook is specified, pass check - if new.Spec.Conversion != nil && new.Spec.Conversion.Strategy == apiextensionsv1.WebhookConverter { - return nil - } - - errs := []error{} - servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{} - for _, version := range new.Spec.Versions { - if version.Served { - servedVersions = append(servedVersions, version) - } - } - - slices.SortFunc(servedVersions, func(a, b apiextensionsv1.CustomResourceDefinitionVersion) int { - return versionhelper.CompareKubeAwareVersionStrings(a.Name, b.Name) - }) - - for i, oldVersion := range servedVersions[:len(servedVersions)-1] { - for _, newVersion := range servedVersions[i+1:] { - flatOld := FlattenSchema(oldVersion.Schema.OpenAPIV3Schema) - flatNew := FlattenSchema(newVersion.Schema.OpenAPIV3Schema) - diffs, err := CalculateFlatSchemaDiff(flatOld, flatNew) - if err != nil { - errs = append(errs, fmt.Errorf("calculating schema diff between CRD versions %q and %q", oldVersion.Name, newVersion.Name)) - continue - } - - for _, field := range slices.Sorted(maps.Keys(diffs)) { - diff := diffs[field] - - handled := false - for _, validation := range c.Validations { - ok, err := validation(diff) - if err != nil { - errs = append(errs, fmt.Errorf("version upgrade %q to %q, field %q: %w", oldVersion.Name, newVersion.Name, field, err)) - } - if ok { - handled = true - break - } - } - - if !handled { - errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", oldVersion.Name, field)) - } - } - } - } - if len(errs) > 0 { - return errors.Join(errs...) - } - return nil -} - -func (c *ServedVersionValidator) Name() string { - return "ServedVersionValidator" -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go deleted file mode 100644 index 67b0c6205..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go +++ /dev/null @@ -1,191 +0,0 @@ -package crdupgradesafety_test - -import ( - "errors" - "fmt" - "testing" - - "github.com/stretchr/testify/assert" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety" -) - -func TestServedVersionValidator(t *testing.T) { - validationErr1 := errors.New(`version "v1alpha1", field "^" has unknown change, refusing to determine that change is safe`) - validationErr2 := errors.New(`version upgrade "v1alpha1" to "v1alpha2", field "^": fail`) - - for _, tc := range []struct { - name string - servedVersionValidator *crdupgradesafety.ServedVersionValidator - new apiextensionsv1.CustomResourceDefinition - expectedError error - }{ - { - name: "no changes, no error", - servedVersionValidator: &crdupgradesafety.ServedVersionValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("should not run") - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - }, - }, - }, - }, - { - name: "changes, validation successful, change is fully handled, no error", - servedVersionValidator: &crdupgradesafety.ServedVersionValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return true, nil - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - { - Name: "v1alpha2", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - }, - { - name: "changes, validation successful, change not fully handled, error", - servedVersionValidator: &crdupgradesafety.ServedVersionValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, nil - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - { - Name: "v1alpha2", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: validationErr1, - }, - { - name: "changes, validation failed, change fully handled, error", - servedVersionValidator: &crdupgradesafety.ServedVersionValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return true, errors.New("fail") - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - { - Name: "v1alpha2", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: validationErr2, - }, - { - name: "changes, validation failed, change not fully handled, ordered error", - servedVersionValidator: &crdupgradesafety.ServedVersionValidator{ - Validations: []crdupgradesafety.ChangeValidation{ - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("fail") - }, - func(_ crdupgradesafety.FieldDiff) (bool, error) { - return false, errors.New("error") - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{}, - }, - }, - { - Name: "v1alpha2", - Served: true, - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - ID: "foo", - }, - }, - }, - }, - }, - }, - expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version upgrade "v1alpha1" to "v1alpha2", field "^": error`, validationErr1), - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := tc.servedVersionValidator.Validate(apiextensionsv1.CustomResourceDefinition{}, tc.new) - if tc.expectedError != nil { - assert.EqualError(t, err, tc.expectedError.Error()) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator.go deleted file mode 100644 index 6fec6cbe5..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator.go +++ /dev/null @@ -1,123 +0,0 @@ -// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e -// Attribution: -// Copyright 2024 The Carvel Authors. -// SPDX-License-Identifier: Apache-2.0 - -package crdupgradesafety - -import ( - "errors" - "fmt" - "strings" - - "github.com/openshift/crd-schema-checker/pkg/manifestcomparators" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/util/sets" -) - -// Validation is a representation of a validation to run -// against a CRD being upgraded -type Validation interface { - // Validate contains the actual validation logic. An error being - // returned means validation has failed - Validate(old, new apiextensionsv1.CustomResourceDefinition) error - // Name returns a human-readable name for the validation - Name() string -} - -// ValidateFunc is a function to validate a CustomResourceDefinition -// for safe upgrades. It accepts the old and new CRDs and returns an -// error if performing an upgrade from old -> new is unsafe. -type ValidateFunc func(old, new apiextensionsv1.CustomResourceDefinition) error - -// ValidationFunc is a helper to wrap a ValidateFunc -// as an implementation of the Validation interface -type ValidationFunc struct { - name string - validateFunc ValidateFunc -} - -func NewValidationFunc(name string, vfunc ValidateFunc) Validation { - return &ValidationFunc{ - name: name, - validateFunc: vfunc, - } -} - -func (vf *ValidationFunc) Name() string { - return vf.name -} - -func (vf *ValidationFunc) Validate(old, new apiextensionsv1.CustomResourceDefinition) error { - return vf.validateFunc(old, new) -} - -type Validator struct { - Validations []Validation -} - -func (v *Validator) Validate(old, new apiextensionsv1.CustomResourceDefinition) error { - validateErrs := []error{} - for _, validation := range v.Validations { - if err := validation.Validate(old, new); err != nil { - formattedErr := fmt.Errorf("CustomResourceDefinition %s failed upgrade safety validation. %q validation failed: %w", - new.Name, validation.Name(), err) - - validateErrs = append(validateErrs, formattedErr) - } - } - if len(validateErrs) > 0 { - return errors.Join(validateErrs...) - } - return nil -} - -func NoScopeChange(old, new apiextensionsv1.CustomResourceDefinition) error { - if old.Spec.Scope != new.Spec.Scope { - return fmt.Errorf("scope changed from %q to %q", old.Spec.Scope, new.Spec.Scope) - } - return nil -} - -func NoStoredVersionRemoved(old, new apiextensionsv1.CustomResourceDefinition) error { - newVersions := sets.New[string]() - for _, version := range new.Spec.Versions { - if !newVersions.Has(version.Name) { - newVersions.Insert(version.Name) - } - } - - for _, storedVersion := range old.Status.StoredVersions { - if !newVersions.Has(storedVersion) { - return fmt.Errorf("stored version %q removed", storedVersion) - } - } - - return nil -} - -func NoExistingFieldRemoved(old, new apiextensionsv1.CustomResourceDefinition) error { - reg := manifestcomparators.NewRegistry() - err := reg.AddComparator(manifestcomparators.NoFieldRemoval()) - if err != nil { - return err - } - - results, errs := reg.Compare(&old, &new) - if len(errs) > 0 { - return errors.Join(errs...) - } - - errSet := []error{} - - for _, result := range results { - if len(result.Errors) > 0 { - errSet = append(errSet, errors.New(strings.Join(result.Errors, "\n"))) - } - } - if len(errSet) > 0 { - return errors.Join(errSet...) - } - - return nil -} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator_test.go deleted file mode 100644 index e13ac9487..000000000 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/validator_test.go +++ /dev/null @@ -1,340 +0,0 @@ -// Originally copied from https://github.com/carvel-dev/kapp/tree/d7fc2e15439331aa3a379485bb124e91a0829d2e -// Attribution: -// Copyright 2024 The Carvel Authors. -// SPDX-License-Identifier: Apache-2.0 - -package crdupgradesafety - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" -) - -func TestValidator(t *testing.T) { - for _, tc := range []struct { - name string - validations []Validation - shouldErr bool - }{ - { - name: "no validators, no error", - validations: []Validation{}, - }, - { - name: "passing validator, no error", - validations: []Validation{ - NewValidationFunc("pass", func(_, _ apiextensionsv1.CustomResourceDefinition) error { - return nil - }), - }, - }, - { - name: "failing validator, error", - validations: []Validation{ - NewValidationFunc("fail", func(_, _ apiextensionsv1.CustomResourceDefinition) error { - return errors.New("boom") - }), - }, - shouldErr: true, - }, - { - name: "passing+failing validator, error", - validations: []Validation{ - NewValidationFunc("pass", func(_, _ apiextensionsv1.CustomResourceDefinition) error { - return nil - }), - NewValidationFunc("fail", func(_, _ apiextensionsv1.CustomResourceDefinition) error { - return errors.New("boom") - }), - }, - shouldErr: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - v := Validator{ - Validations: tc.validations, - } - var o, n apiextensionsv1.CustomResourceDefinition - - err := v.Validate(o, n) - require.Equal(t, tc.shouldErr, err != nil) - }) - } -} - -func TestNoScopeChange(t *testing.T) { - for _, tc := range []struct { - name string - old apiextensionsv1.CustomResourceDefinition - new apiextensionsv1.CustomResourceDefinition - shouldError bool - }{ - { - name: "no scope change, no error", - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Scope: apiextensionsv1.ClusterScoped, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Scope: apiextensionsv1.ClusterScoped, - }, - }, - }, - { - name: "scope change, error", - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Scope: apiextensionsv1.ClusterScoped, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Scope: apiextensionsv1.NamespaceScoped, - }, - }, - shouldError: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := NoScopeChange(tc.old, tc.new) - require.Equal(t, tc.shouldError, err != nil) - }) - } -} - -func TestNoStoredVersionRemoved(t *testing.T) { - for _, tc := range []struct { - name string - old apiextensionsv1.CustomResourceDefinition - new apiextensionsv1.CustomResourceDefinition - shouldError bool - }{ - { - name: "no stored versions, no error", - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - }, - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{}, - }, - { - name: "stored versions, no stored version removed, no error", - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - }, - { - Name: "v1alpha2", - }, - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Status: apiextensionsv1.CustomResourceDefinitionStatus{ - StoredVersions: []string{ - "v1alpha1", - }, - }, - }, - }, - { - name: "stored versions, stored version removed, error", - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha2", - }, - }, - }, - }, - old: apiextensionsv1.CustomResourceDefinition{ - Status: apiextensionsv1.CustomResourceDefinitionStatus{ - StoredVersions: []string{ - "v1alpha1", - }, - }, - }, - shouldError: true, - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := NoStoredVersionRemoved(tc.old, tc.new) - require.Equal(t, tc.shouldError, err != nil) - }) - } -} - -func TestNoExistingFieldRemoved(t *testing.T) { - for _, tc := range []struct { - name string - new apiextensionsv1.CustomResourceDefinition - old apiextensionsv1.CustomResourceDefinition - shouldError bool - }{ - { - name: "no existing field removed, no error", - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - }, - { - name: "existing field removed, error", - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - "fieldTwo": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - shouldError: true, - }, - { - name: "new version is added with the field removed, no error", - old: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - "fieldTwo": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - new: apiextensionsv1.CustomResourceDefinition{ - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1alpha1", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - "fieldTwo": { - Type: "string", - }, - }, - }, - }, - }, - { - Name: "v1alpha2", - Schema: &apiextensionsv1.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "fieldOne": { - Type: "string", - }, - }, - }, - }, - }, - }, - }, - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := NoExistingFieldRemoved(tc.old, tc.new) - assert.Equal(t, tc.shouldError, err != nil) - }) - } -} diff --git a/testdata/manifests/crd-description-changed.json b/testdata/manifests/crd-description-changed.json new file mode 100644 index 000000000..ae30459e3 --- /dev/null +++ b/testdata/manifests/crd-description-changed.json @@ -0,0 +1,122 @@ +{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": { + "name": "crontabs.stable.example.com" + }, + "spec": { + "group": "stable.example.com", + "versions": [ + { + "name": "v1", + "served": true, + "storage": false, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "description": "description two", + "type": "object", + "properties": { + "removedField": { + "type":"integer" + }, + "enum": { + "type":"integer" + }, + "minMaxValue": { + "type":"integer" + }, + "required": { + "type":"integer" + }, + "minMaxItems": { + "type":"array", + "items": { + "type":"string" + } + }, + "minMaxLength": { + "type":"string" + }, + "defaultVal": { + "type": "string" + }, + "requiredVal": { + "type": "string" + } + } + } + }, + "required": [ + "requiredVal" + ] + } + } + }, + { + "name": "v2", + "served": true, + "storage": true, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "removedField": { + "type":"integer" + }, + "enum": { + "type":"integer" + }, + "minMaxValue": { + "type":"integer" + }, + "required": { + "type":"integer" + }, + "minMaxItems": { + "type":"array", + "items": { + "type":"string" + } + }, + "minMaxLength": { + "type":"string" + }, + "defaultVal": { + "type": "string" + }, + "requiredVal": { + "type": "string" + } + } + } + }, + "required": [ + "requiredVal" + ] + } + } + } + ], + "scope": "Cluster", + "names": { + "plural": "crontabs", + "singular": "crontab", + "kind": "CronTab", + "shortNames": [ + "ct" + ] + } + }, + "status": { + "storedVersions": [ + "v1", + "v2" + ] + } +} diff --git a/testdata/manifests/old-crd.json b/testdata/manifests/old-crd.json index 08e763451..1f3ff5a4b 100644 --- a/testdata/manifests/old-crd.json +++ b/testdata/manifests/old-crd.json @@ -16,6 +16,7 @@ "type": "object", "properties": { "spec": { + "description": "description one", "type": "object", "properties": { "removedField": {