From 9f572ba147ceb057ebb3ebbd834e368e25890411 Mon Sep 17 00:00:00 2001 From: Artur Zych <5843875+azych@users.noreply.github.com> Date: Thu, 13 Mar 2025 11:09:32 +0100 Subject: [PATCH 1/3] fix: ensure order in multi-line errors returned by crdupgradesafety validators --- .../crdupgradesafety/change_validator.go | 6 +- .../crdupgradesafety/change_validator_test.go | 23 ++- .../preflights/crdupgradesafety/checks.go | 5 +- .../crdupgradesafety/checks_test.go | 180 ++++++++++++++++++ .../crdupgradesafety/crdupgradesafety.go | 1 - 5 files changed, 206 insertions(+), 9 deletions(-) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go index 3bc661777..4678b2de0 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator.go @@ -8,7 +8,9 @@ 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" @@ -63,7 +65,9 @@ func (cv *ChangeValidator) Validate(old, new apiextensionsv1.CustomResourceDefin continue } - for field, diff := range diffs { + for _, field := range slices.Sorted(maps.Keys(diffs)) { + diff := diffs[field] + handled := false for _, validation := range cv.Validations { ok, err := validation(diff) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go index baf5d4f4b..cc12bc5c1 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/change_validator_test.go @@ -7,6 +7,7 @@ package crdupgradesafety_test import ( "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -130,12 +131,15 @@ func TestFlattenSchema(t *testing.T) { } 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 - shouldError bool + expectedError error }{ { name: "no changes, no error", @@ -242,7 +246,7 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + expectedError: validationErr1, }, { name: "changes, validation failed, change fully handled, error", @@ -279,15 +283,18 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + expectedError: validationErr2, }, { - name: "changes, validation failed, change not fully handled, error", + 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{ @@ -316,12 +323,16 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + 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) - assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError) + 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 index 8db45df22..2c2d77ed6 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go @@ -5,6 +5,7 @@ import ( "cmp" "errors" "fmt" + "maps" "reflect" "slices" @@ -45,7 +46,9 @@ func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourc continue } - for field, diff := range diffs { + for _, field := range slices.Sorted(maps.Keys(diffs)) { + diff := diffs[field] + handled := false for _, validation := range c.Validations { ok, err := validation(diff) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go index a47c27aa8..c71da9101 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/utils/ptr" @@ -983,3 +984,182 @@ func TestOrderKappsValidateErr(t *testing.T) { }) } } + +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 *ServedVersionValidator + new apiextensionsv1.CustomResourceDefinition + expectedError error + }{ + { + name: "no changes, no error", + servedVersionValidator: &ServedVersionValidator{ + Validations: []ChangeValidation{ + func(_ 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: &ServedVersionValidator{ + Validations: []ChangeValidation{ + func(_ 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: &ServedVersionValidator{ + Validations: []ChangeValidation{ + func(_ 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: &ServedVersionValidator{ + Validations: []ChangeValidation{ + func(_ 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: &ServedVersionValidator{ + Validations: []ChangeValidation{ + func(_ FieldDiff) (bool, error) { + return false, errors.New("fail") + }, + func(_ 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/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 72a1c46c5..e46bc0cc2 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -113,7 +113,6 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro err = p.validator.Validate(*oldCrd, *newCrd) if err != nil { - err = orderKappsValidateErr(err) validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q failed: %w", newCrd.Name, err)) } } From 46c0846e249c0eac472b9a2de3df50717de1d9db Mon Sep 17 00:00:00 2001 From: Artur Zych <5843875+azych@users.noreply.github.com> Date: Thu, 13 Mar 2025 11:11:57 +0100 Subject: [PATCH 2/3] chore: remove redundant orderKappsValidateErr --- .../crdupgradesafety/checks_test.go | 78 ------------------- .../crdupgradesafety/crdupgradesafety.go | 63 --------------- 2 files changed, 141 deletions(-) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go index c71da9101..2fbe04543 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go @@ -907,84 +907,6 @@ func TestType(t *testing.T) { } } -func TestOrderKappsValidateErr(t *testing.T) { - testErr1 := errors.New("fallback1") - testErr2 := errors.New("fallback2") - - generateErrors := func(n int, base string) []error { - var result []error - for i := n; i >= 0; i-- { - result = append(result, fmt.Errorf("%s%d", base, i)) - } - return result - } - - joinedAndNested := func(format string, errs ...error) error { - return fmt.Errorf(format, errors.Join(errs...)) - } - - testCases := []struct { - name string - inpuError error - expectedError error - }{ - { - name: "fallback: initial error was not error.Join'ed", - inpuError: testErr1, - expectedError: testErr1, - }, - { - name: "fallback: nested error was not wrapped", - inpuError: errors.Join(testErr1), - expectedError: testErr1, - }, - { - name: "fallback: multiple nested errors, one was not wrapped", - inpuError: errors.Join(testErr2, fmt.Errorf("%w", testErr1)), - expectedError: errors.Join(testErr2, fmt.Errorf("%w", testErr1)), - }, - { - name: "fallback: nested error did not contain \":\"", - inpuError: errors.Join(fmt.Errorf("%w", testErr1)), - expectedError: testErr1, - }, - { - name: "fallback: multiple nested errors, one did not contain \":\"", - inpuError: errors.Join(joinedAndNested("fail: %w", testErr2), joinedAndNested("%w", testErr1)), - expectedError: errors.Join(fmt.Errorf("fail: %w", testErr2), testErr1), - }, - { - name: "fallback: nested error was not error.Join'ed", - inpuError: errors.Join(fmt.Errorf("fail: %w", testErr1)), - expectedError: fmt.Errorf("fail: %w", testErr1), - }, - { - name: "fallback: multiple nested errors, one was not error.Join'ed", - inpuError: errors.Join(joinedAndNested("fail: %w", testErr2), fmt.Errorf("fail: %w", testErr1)), - expectedError: fmt.Errorf("fail: %w\nfail: %w", testErr2, testErr1), - }, - { - name: "ensures order for a single group of multiple deeply nested errors", - inpuError: errors.Join(joinedAndNested("fail: %w", testErr2, testErr1)), - expectedError: fmt.Errorf("fail: %w\n%w", testErr1, testErr2), - }, - { - name: "ensures order for multiple groups of deeply nested errors", - inpuError: errors.Join( - joinedAndNested("fail: %w", testErr2, testErr1), - joinedAndNested("validation: %w", generateErrors(5, "err")...), - ), - expectedError: fmt.Errorf("fail: %w\n%w\nvalidation: err0\nerr1\nerr2\nerr3\nerr4\nerr5", testErr1, testErr2), - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - err := orderKappsValidateErr(tc.inpuError) - require.EqualError(t, err, tc.expectedError.Error()) - }) - } -} - 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`) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index e46bc0cc2..6bc177cd1 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -1,11 +1,9 @@ package crdupgradesafety import ( - "cmp" "context" "errors" "fmt" - "slices" "strings" "helm.sh/helm/v3/pkg/release" @@ -119,64 +117,3 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro return errors.Join(validateErrors...) } - -// orderKappsValidateErr is meant as a temporary solution to the problem -// of randomly ordered multi-line validation error returned by kapp's validator.Validate() -// -// The problem is that kapp's field validations are performed in map iteration order, which is not fixed. -// Errors from those validations are then error.Join'ed, fmt.Errorf'ed and error.Join'ed again, -// which means original messages are available at 3rd level of nesting, and this is where we need to -// sort them to ensure we do not enter into constant reconciliation loop because of random order of -// failure message we ultimately set in ClusterExtension's status conditions. -// -// This helper attempts to do that and falls back to original unchanged error message -// in case of any unforeseen issues which likely mean that the internals of validator.Validate -// have changed. -// -// For full context see: -// github.com/operator-framework/operator-controller/issues/1456 (original issue and comments) -// github.com/carvel-dev/kapp/pull/1047 (PR to ensure order in upstream) -// -// TODO: remove this once ordering has been handled by the upstream. -func orderKappsValidateErr(err error) error { - joinedValidationErrs, ok := err.(interface{ Unwrap() []error }) - if !ok { - return err - } - - // nolint: prealloc - var errs []error - for _, validationErr := range joinedValidationErrs.Unwrap() { - unwrappedValidationErr := errors.Unwrap(validationErr) - // validator.Validate did not error.Join'ed validation errors - // kapp's internals changed - fallback to original error - if unwrappedValidationErr == nil { - return err - } - - prefix, _, ok := strings.Cut(validationErr.Error(), ":") - // kapp's internal error format changed - fallback to original error - if !ok { - return err - } - - // attempt to unwrap and sort field errors - joinedFieldErrs, ok := unwrappedValidationErr.(interface{ Unwrap() []error }) - // ChangeValidator did not error.Join'ed field validation errors - // kapp's internals changed - fallback to original error - if !ok { - return err - } - - // ensure order of the field validation errors - unwrappedFieldErrs := joinedFieldErrs.Unwrap() - slices.SortFunc(unwrappedFieldErrs, func(a, b error) int { - return cmp.Compare(a.Error(), b.Error()) - }) - - // re-join the sorted field errors keeping the original error prefix from kapp - errs = append(errs, fmt.Errorf("%s: %w", prefix, errors.Join(unwrappedFieldErrs...))) - } - - return errors.Join(errs...) -} From a82e95bd10e604cd9e0e4b821be4b8fac0984ee7 Mon Sep 17 00:00:00 2001 From: Artur Zych <5843875+azych@users.noreply.github.com> Date: Thu, 13 Mar 2025 11:20:48 +0100 Subject: [PATCH 3/3] refactor: extract ServedVersionValidator to its own file --- .../preflights/crdupgradesafety/checks.go | 67 ------ .../crdupgradesafety/checks_test.go | 181 ----------------- .../shared_version_validator.go | 74 +++++++ .../shared_version_validator_test.go | 191 ++++++++++++++++++ 4 files changed, 265 insertions(+), 248 deletions(-) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go index 2c2d77ed6..669f65e57 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go @@ -3,80 +3,13 @@ package crdupgradesafety import ( "bytes" "cmp" - "errors" "fmt" - "maps" "reflect" - "slices" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/sets" - 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" -} - type resetFunc func(diff FieldDiff) FieldDiff func isHandled(diff FieldDiff, reset resetFunc) bool { diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go index 2fbe04543..36618b584 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go @@ -2,10 +2,8 @@ package crdupgradesafety import ( "errors" - "fmt" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/utils/ptr" @@ -906,182 +904,3 @@ func TestType(t *testing.T) { }) } } - -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 *ServedVersionValidator - new apiextensionsv1.CustomResourceDefinition - expectedError error - }{ - { - name: "no changes, no error", - servedVersionValidator: &ServedVersionValidator{ - Validations: []ChangeValidation{ - func(_ 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: &ServedVersionValidator{ - Validations: []ChangeValidation{ - func(_ 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: &ServedVersionValidator{ - Validations: []ChangeValidation{ - func(_ 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: &ServedVersionValidator{ - Validations: []ChangeValidation{ - func(_ 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: &ServedVersionValidator{ - Validations: []ChangeValidation{ - func(_ FieldDiff) (bool, error) { - return false, errors.New("fail") - }, - func(_ 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/shared_version_validator.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go new file mode 100644 index 000000000..d66f1ed9c --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go @@ -0,0 +1,74 @@ +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 new file mode 100644 index 000000000..67b0c6205 --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go @@ -0,0 +1,191 @@ +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) + } + }) + } +}