From 01458d24d8305a680d2197020cf0eb69ec3b0e24 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Mon, 20 Oct 2025 12:43:11 +0100 Subject: [PATCH] UPSTREAM: : Backport the fix without add the crddif dependency Backport CRD upgrade-safety filtering and message tweaks from openshift/operator-controller#527 while keeping the existing in-repo validator stack (no crdify bump). --- .../preflights/crdupgradesafety/checks.go | 21 ++- .../crdupgradesafety/checks_test.go | 14 +- .../crdupgradesafety/crdupgradesafety.go | 113 +++++++++++++- .../crdupgradesafety/crdupgradesafety_test.go | 141 ++++++++++++++---- .../shared_version_validator.go | 50 +++++-- .../shared_version_validator_test.go | 6 +- ...crd-conversion-no-webhook-extra-issue.json | 76 ++++++++++ .../manifests/crd-conversion-no-webhook.json | 0 .../manifests/crd-conversion-webhook-old.json | 0 .../manifests/crd-conversion-webhook.json | 0 .../manifests/crd-description-changed.json | 124 +++++++++++++++ .../manifests/crd-field-removed.json | 6 +- .../testdata}/manifests/crd-invalid | 0 .../manifests/crd-invalid-upgrade.json | 7 +- .../testdata/manifests/crd-unhandled-new.json | 40 +++++ .../testdata/manifests/crd-unhandled-old.json | 39 +++++ .../manifests/crd-valid-upgrade.json | 9 +- .../testdata}/manifests/no-crds.json | 0 .../testdata}/manifests/old-crd.json | 6 +- 19 files changed, 574 insertions(+), 78 deletions(-) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-no-webhook.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-webhook-old.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-conversion-webhook.json (100%) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-field-removed.json (96%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-invalid (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-invalid-upgrade.json (92%) create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json create mode 100644 internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/crd-valid-upgrade.json (93%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/no-crds.json (100%) rename {testdata => internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata}/manifests/old-crd.json (94%) diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go index 61d8b55c3..d353339b9 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" "fmt" "reflect" + "slices" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -38,9 +39,13 @@ func Enum(diff FieldDiff) (bool, error) { switch { case oldEnums.Len() == 0 && newEnums.Len() > 0: - err = fmt.Errorf("enum constraints %v added when there were no restrictions previously", newEnums.UnsortedList()) + newEnumList := newEnums.UnsortedList() + slices.Sort(newEnumList) + err = fmt.Errorf("enum: constraints %v added when there were no restrictions previously", newEnumList) case diffEnums.Len() > 0: - err = fmt.Errorf("enums %v removed from the set of previously allowed values", diffEnums.UnsortedList()) + diffEnumList := diffEnums.UnsortedList() + slices.Sort(diffEnumList) + err = fmt.Errorf("enum: allowed enum values removed: %v", diffEnumList) } return isHandled(diff, reset), err @@ -59,7 +64,9 @@ func Required(diff FieldDiff) (bool, error) { var err error if diffRequired.Len() > 0 { - err = fmt.Errorf("new required fields %v added", diffRequired.UnsortedList()) + diffRequiredList := diffRequired.UnsortedList() + slices.Sort(diffRequiredList) + err = fmt.Errorf("required: new required fields %v added", diffRequiredList) } return isHandled(diff, reset), err @@ -218,11 +225,11 @@ func Default(diff FieldDiff) (bool, 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)) + 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)) + 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)) + 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 @@ -237,7 +244,7 @@ func Type(diff FieldDiff) (bool, error) { 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) + err = fmt.Errorf("type: type changed : %q -> %q", diff.Old.Type, diff.New.Type) } return isHandled(diff, reset), err diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go index ebceed8b4..aa27ec18b 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go @@ -53,7 +53,7 @@ func TestEnum(t *testing.T) { }, }, }, - err: errors.New("enum constraints [foo] added when there were no restrictions previously"), + err: errors.New("enum: constraints [foo] added when there were no restrictions previously"), handled: true, }, { @@ -77,7 +77,7 @@ func TestEnum(t *testing.T) { }, }, }, - err: errors.New("enums [foo] removed from the set of previously allowed values"), + err: errors.New("enum: allowed enum values removed: [foo]"), handled: true, }, { @@ -154,7 +154,7 @@ func TestRequired(t *testing.T) { }, }, }, - err: errors.New("new required fields [foo] added"), + err: errors.New("required: new required fields [foo] added"), handled: true, }, { @@ -800,7 +800,7 @@ func TestDefault(t *testing.T) { }, }, }, - err: errors.New("default value \"foo\" added when there was no default previously"), + err: errors.New("default: value \"foo\" added when there was no default previously"), handled: true, }, { @@ -813,7 +813,7 @@ func TestDefault(t *testing.T) { }, New: &apiextensionsv1.JSONSchemaProps{}, }, - err: errors.New("default value \"foo\" removed"), + err: errors.New("default: value \"foo\" removed"), handled: true, }, { @@ -830,7 +830,7 @@ func TestDefault(t *testing.T) { }, }, }, - err: errors.New("default value changed from \"foo\" to \"bar\""), + err: errors.New("default: value changed from \"foo\" to \"bar\""), handled: true, }, { @@ -880,7 +880,7 @@ func TestType(t *testing.T) { Type: "integer", }, }, - err: errors.New("type changed from \"string\" to \"integer\""), + err: errors.New("type: type changed : \"string\" -> \"integer\""), handled: true, }, { diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 0904bf4d4..b156e414c 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -47,7 +47,6 @@ func NewPreflight(crdCli apiextensionsv1client.CustomResourceDefinitionInterface } p := &Preflight{ crdClient: crdCli, - // create a default validator. Can be overridden via the options validator: &Validator{ Validations: []Validation{ NewValidationFunc("NoScopeChange", NoScopeChange), @@ -95,26 +94,126 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro if err != nil { return fmt.Errorf("converting object %q to unstructured: %w", obj.GetName(), err) } - err = runtime.DefaultUnstructuredConverter.FromUnstructured(uMap, newCrd) - if err != nil { + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(uMap, newCrd); err != nil { return fmt.Errorf("converting unstructured to CRD object: %w", err) } oldCrd, err := p.crdClient.Get(ctx, newCrd.Name, metav1.GetOptions{}) if err != nil { - // if there is no existing CRD, there is nothing to break - // so it is immediately successful. if apierrors.IsNotFound(err) { continue } return fmt.Errorf("getting existing resource for CRD %q: %w", newCrd.Name, err) } - err = p.validator.Validate(*oldCrd, *newCrd) - if err != nil { + if err = p.validator.Validate(*oldCrd, *newCrd); err != nil { validateErrors = append(validateErrors, fmt.Errorf("validating upgrade for CRD %q failed: %w", newCrd.Name, err)) } } return errors.Join(validateErrors...) } + +const unhandledSummaryPrefix = "unhandled changes found" + +func conciseUnhandledMessage(raw string) string { + if !strings.Contains(raw, unhandledSummaryPrefix) { + return raw + } + + details := extractUnhandledDetails(raw) + if len(details) == 0 { + return unhandledSummaryPrefix + } + + return fmt.Sprintf("%s (%s)", unhandledSummaryPrefix, strings.Join(details, "; ")) +} + +func extractUnhandledDetails(raw string) []string { + type diffEntry struct { + before string + after string + beforeRaw string + afterRaw string + } + + entries := map[string]*diffEntry{} + order := []string{} + + for _, line := range strings.Split(raw, "\n") { + trimmed := strings.TrimSpace(line) + if len(trimmed) < 2 { + continue + } + + sign := trimmed[0] + if sign != '-' && sign != '+' { + continue + } + + field, value, rawValue := parseUnhandledDiffValue(trimmed[1:]) + if field == "" { + continue + } + + entry, ok := entries[field] + if !ok { + entry = &diffEntry{} + entries[field] = entry + order = append(order, field) + } + + if sign == '-' { + entry.before = value + entry.beforeRaw = rawValue + } else { + entry.after = value + entry.afterRaw = rawValue + } + } + + details := []string{} + for _, field := range order { + entry := entries[field] + if entry.before == "" && entry.after == "" { + continue + } + if entry.before == entry.after && entry.beforeRaw == entry.afterRaw { + continue + } + + before := entry.before + if before == "" { + before = "" + } + after := entry.after + if after == "" { + after = "" + } + if entry.before == entry.after && entry.beforeRaw != entry.afterRaw { + after = after + " (changed)" + } + + details = append(details, fmt.Sprintf("%s %s -> %s", field, before, after)) + } + + return details +} + +func parseUnhandledDiffValue(fragment string) (string, string, string) { + cleaned := strings.TrimSpace(fragment) + cleaned = strings.TrimPrefix(cleaned, "\t") + cleaned = strings.TrimSpace(cleaned) + cleaned = strings.TrimSuffix(cleaned, ",") + + parts := strings.SplitN(cleaned, ":", 2) + if len(parts) != 2 { + return "", "", "" + } + + field := strings.TrimSpace(parts[0]) + value := strings.TrimSpace(parts[1]) + + stripped := strings.Trim(value, `"`) + return field, stripped, value +} diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go index 12241bd7f..686048f0b 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/crdupgradesafety_test.go @@ -41,7 +41,7 @@ func newMockPreflight(crd *apiextensionsv1.CustomResourceDefinition, err error, }, preflightOpts...) } -const crdFolder string = "../../../../../testdata/manifests" +const crdFolder string = "testdata/manifests" func getCrdFromManifestFile(t *testing.T, oldCrdFile string) *apiextensionsv1.CustomResourceDefinition { if oldCrdFile == "" { @@ -69,6 +69,14 @@ func getManifestString(t *testing.T, crdFile string) string { return string(buff) } +func wantErrorMsgs(wantMsgs []string) require.ErrorAssertionFunc { + return func(t require.TestingT, haveErr error, _ ...interface{}) { + for _, wantMsg := range wantMsgs { + require.ErrorContains(t, haveErr, wantMsg) + } + } +} + // TestInstall exists only for completeness as Install() is currently a no-op. It can be used as // a template for real tests in the future if the func is implemented. func TestInstall(t *testing.T) { @@ -77,7 +85,8 @@ func TestInstall(t *testing.T) { oldCrdPath string validator *crdupgradesafety.Validator release *release.Release - wantErrMsgs []string + requireErr require.ErrorAssertionFunc + validate func(*testing.T, error) wantCrdGetErr error }{ { @@ -95,7 +104,7 @@ func TestInstall(t *testing.T) { Name: "test-release", Manifest: "abcd", }, - wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"}, + requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}), }, { name: "release with no CRD objects", @@ -111,7 +120,7 @@ func TestInstall(t *testing.T) { Manifest: getManifestString(t, "crd-valid-upgrade.json"), }, wantCrdGetErr: fmt.Errorf("error!"), - wantErrMsgs: []string{"error!"}, + requireErr: wantErrorMsgs([]string{"error!"}), }, { name: "fail to get old crd, not found error", @@ -127,7 +136,7 @@ func TestInstall(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid"), }, - wantErrMsgs: []string{"json: cannot unmarshal"}, + requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}), }, { name: "custom validator", @@ -143,7 +152,7 @@ func TestInstall(t *testing.T) { }), }, }, - wantErrMsgs: []string{"custom validation error!!"}, + requireErr: wantErrorMsgs([]string{"custom validation error!!"}), }, { name: "valid upgrade", @@ -162,11 +171,11 @@ func TestInstall(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `"NoScopeChange"`, `"NoStoredVersionRemoved"`, - `enum constraints`, - `new required fields`, + `enum:`, + `required:`, `maximum: constraint`, `maxItems: constraint`, `maxLength: constraint`, @@ -175,8 +184,8 @@ func TestInstall(t *testing.T) { `minItems: constraint`, `minLength: constraint`, `minProperties: constraint`, - `default value`, - }, + `default:`, + }), }, { name: "new crd validation failure for existing field removal", @@ -187,8 +196,18 @@ func TestInstall(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-field-removed.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `"NoExistingFieldRemoved"`, + }), + }, + { + 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"), }, }, } @@ -197,10 +216,12 @@ func TestInstall(t *testing.T) { t.Run(tc.name, func(t *testing.T) { preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr, tc.validator) err := preflight.Install(context.Background(), tc.release) - if len(tc.wantErrMsgs) != 0 { - for _, expectedErrMsg := range tc.wantErrMsgs { - require.ErrorContainsf(t, err, expectedErrMsg, "") - } + if tc.validate != nil { + tc.validate(t, err) + return + } + if tc.requireErr != nil { + tc.requireErr(t, err) } else { require.NoError(t, err) } @@ -214,7 +235,8 @@ func TestUpgrade(t *testing.T) { oldCrdPath string validator *crdupgradesafety.Validator release *release.Release - wantErrMsgs []string + requireErr require.ErrorAssertionFunc + validate func(*testing.T, error) wantCrdGetErr error }{ { @@ -232,7 +254,7 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: "abcd", }, - wantErrMsgs: []string{"json: cannot unmarshal string into Go value of type unstructured.detector"}, + requireErr: wantErrorMsgs([]string{"json: cannot unmarshal string into Go value of type unstructured.detector"}), }, { name: "release with no CRD objects", @@ -248,7 +270,7 @@ func TestUpgrade(t *testing.T) { Manifest: getManifestString(t, "crd-valid-upgrade.json"), }, wantCrdGetErr: fmt.Errorf("error!"), - wantErrMsgs: []string{"error!"}, + requireErr: wantErrorMsgs([]string{"error!"}), }, { name: "fail to get old crd, not found error", @@ -264,7 +286,7 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid"), }, - wantErrMsgs: []string{"json: cannot unmarshal"}, + requireErr: wantErrorMsgs([]string{"json: cannot unmarshal"}), }, { name: "custom validator", @@ -280,7 +302,7 @@ func TestUpgrade(t *testing.T) { }), }, }, - wantErrMsgs: []string{"custom validation error!!"}, + requireErr: wantErrorMsgs([]string{"custom validation error!!"}), }, { name: "valid upgrade", @@ -299,11 +321,11 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-invalid-upgrade.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `"NoScopeChange"`, `"NoStoredVersionRemoved"`, - `enum constraints`, - `new required fields`, + `enum:`, + `required:`, `maximum: constraint`, `maxItems: constraint`, `maxLength: constraint`, @@ -312,8 +334,8 @@ func TestUpgrade(t *testing.T) { `minItems: constraint`, `minLength: constraint`, `minProperties: constraint`, - `default value`, - }, + `default:`, + }), }, { name: "new crd validation failure for existing field removal", @@ -324,9 +346,9 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-field-removed.json"), }, - wantErrMsgs: []string{ + requireErr: wantErrorMsgs([]string{ `"NoExistingFieldRemoved"`, - }, + }), }, { name: "webhook conversion strategy exists", @@ -343,8 +365,56 @@ func TestUpgrade(t *testing.T) { Name: "test-release", Manifest: getManifestString(t, "crd-conversion-no-webhook.json"), }, - wantErrMsgs: []string{ - `"ServedVersionValidator" validation failed: version upgrade "v1" to "v2", field "^.spec.foobarbaz": enums`, + requireErr: wantErrorMsgs([]string{ + `"ServedVersionValidator" validation failed: v1 -> v2: ^.spec.foobarbaz: enum:`, + }), + }, + { + name: "existing served version issues preserved between upgrades", + oldCrdPath: "crd-conversion-no-webhook.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-conversion-no-webhook.json"), + }, + }, + { + name: "existing served version issues ignored but new issues reported", + oldCrdPath: "crd-conversion-no-webhook.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-conversion-no-webhook-extra-issue.json"), + }, + validate: func(t *testing.T, err error) { + require.Error(t, err) + require.Contains(t, err.Error(), `v1 -> v2: ^.spec.extraField: type: type changed : "boolean" -> "string"`) + require.NotContains(t, err.Error(), `^.spec.foobarbaz`) + }, + }, + { + name: "success when old crd and new crd contain the exact same validation issues", + oldCrdPath: "crd-conversion-no-webhook.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-conversion-no-webhook.json"), + }, + }, + { + name: "failure when old crd and new crd contain the exact same validation issues, but new crd introduces another validation issue", + oldCrdPath: "crd-conversion-no-webhook.json", + release: &release.Release{ + Name: "test-release", + Manifest: getManifestString(t, "crd-conversion-no-webhook-extra-issue.json"), + }, + requireErr: func(t require.TestingT, err error, _ ...interface{}) { + require.Error(t, err) + // The newly introduced issue is reported + require.Contains(t, err.Error(), + `v1 -> v2: ^.spec.extraField: type: type changed : "boolean" -> "string"`, + ) + // The existing issue is not reported + require.NotContains(t, err.Error(), + `v1 -> v2: ^.spec.foobarbaz: enum: allowed enum values removed`, + ) }, }, } @@ -353,10 +423,15 @@ func TestUpgrade(t *testing.T) { t.Run(tc.name, func(t *testing.T) { preflight := newMockPreflight(getCrdFromManifestFile(t, tc.oldCrdPath), tc.wantCrdGetErr, tc.validator) err := preflight.Upgrade(context.Background(), tc.release) - if len(tc.wantErrMsgs) != 0 { - for _, expectedErrMsg := range tc.wantErrMsgs { - require.ErrorContainsf(t, err, expectedErrMsg, "") + if tc.validate != nil { + if tc.requireErr != nil { + panic("test case sets both validate and requireErr") } + tc.validate(t, err) + return + } + if tc.requireErr != nil { + tc.requireErr(t, err) } else { require.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 index d66f1ed9c..e8c481ca7 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator.go @@ -20,20 +20,55 @@ func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourc return nil } + newErrs := c.collectServedVersionErrors(new) + if len(newErrs) == 0 { + return nil + } + + existingErrCounts := map[string]int{} + if len(old.Spec.Versions) > 0 { + for _, err := range c.collectServedVersionErrors(old) { + existingErrCounts[err.Error()]++ + } + } + + filteredErrs := make([]error, 0, len(newErrs)) + for _, err := range newErrs { + msg := err.Error() + if count, ok := existingErrCounts[msg]; ok && count > 0 { + existingErrCounts[msg]-- + continue + } + filteredErrs = append(filteredErrs, err) + } + + if len(filteredErrs) > 0 { + return errors.Join(filteredErrs...) + } + return nil +} + +func (c *ServedVersionValidator) collectServedVersionErrors(crd apiextensionsv1.CustomResourceDefinition) []error { errs := []error{} servedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{} - for _, version := range new.Spec.Versions { + for _, version := range crd.Spec.Versions { if version.Served { servedVersions = append(servedVersions, version) } } + if len(servedVersions) < 2 { + return nil + } + 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:] { + for i := 0; i < len(servedVersions)-1; i++ { + oldVersion := servedVersions[i] + for j := i + 1; j < len(servedVersions); j++ { + newVersion := servedVersions[j] flatOld := FlattenSchema(oldVersion.Schema.OpenAPIV3Schema) flatNew := FlattenSchema(newVersion.Schema.OpenAPIV3Schema) diffs, err := CalculateFlatSchemaDiff(flatOld, flatNew) @@ -49,7 +84,7 @@ func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourc 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)) + errs = append(errs, fmt.Errorf("%s -> %s: %s: %w", oldVersion.Name, newVersion.Name, field, err)) } if ok { handled = true @@ -58,15 +93,12 @@ func (c *ServedVersionValidator) Validate(old, new apiextensionsv1.CustomResourc } if !handled { - errs = append(errs, fmt.Errorf("version %q, field %q has unknown change, refusing to determine that change is safe", oldVersion.Name, field)) + errs = append(errs, fmt.Errorf("%s -> %s: %s: has unknown change, refusing to determine that change is safe", oldVersion.Name, newVersion.Name, field)) } } } } - if len(errs) > 0 { - return errors.Join(errs...) - } - return nil + return errs } func (c *ServedVersionValidator) Name() string { 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 index 67b0c6205..5562d4348 100644 --- a/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/shared_version_validator_test.go @@ -12,8 +12,8 @@ import ( ) 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`) + validationErr1 := errors.New(`v1alpha1 -> v1alpha2: ^: has unknown change, refusing to determine that change is safe`) + validationErr2 := errors.New(`v1alpha1 -> v1alpha2: ^: fail`) for _, tc := range []struct { name string @@ -176,7 +176,7 @@ func TestServedVersionValidator(t *testing.T) { }, }, }, - expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version upgrade "v1alpha1" to "v1alpha2", field "^": error`, validationErr1), + expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `v1alpha1 -> v1alpha2: ^: error`, validationErr1), }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json new file mode 100644 index 000000000..0bfd13384 --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook-extra-issue.json @@ -0,0 +1,76 @@ +{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": { + "name": "crontabs.stable.example.com" + }, + "spec": { + "group": "stable.example.com", + "versions": [ + { + "name": "v2", + "served": true, + "storage": false, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "foobarbaz": { + "type":"string", + "enum":[ + "bark", + "woof" + ] + }, + "extraField": { + "type":"string" + } + } + } + } + } + } + }, + { + "name": "v1", + "served": true, + "storage": false, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "foobarbaz": { + "type":"string", + "enum":[ + "foo", + "bar", + "baz" + ] + }, + "extraField": { + "type":"boolean" + } + } + } + } + } + } + } + ], + "scope": "Cluster", + "names": { + "plural": "crontabs", + "singular": "crontab", + "kind": "CronTab", + "shortNames": [ + "ct" + ] + } + } +} diff --git a/testdata/manifests/crd-conversion-no-webhook.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook.json similarity index 100% rename from testdata/manifests/crd-conversion-no-webhook.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-no-webhook.json diff --git a/testdata/manifests/crd-conversion-webhook-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook-old.json similarity index 100% rename from testdata/manifests/crd-conversion-webhook-old.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook-old.json diff --git a/testdata/manifests/crd-conversion-webhook.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook.json similarity index 100% rename from testdata/manifests/crd-conversion-webhook.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-conversion-webhook.json diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json new file mode 100644 index 000000000..0e7f9a600 --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-description-changed.json @@ -0,0 +1,124 @@ +{ + "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": "string", + "enum": ["a", "b", "c"] + }, + "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": "string", + "enum": ["a", "b", "c"] + }, + "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/crd-field-removed.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json similarity index 96% rename from testdata/manifests/crd-field-removed.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json index 86ba06e40..650b13fd4 100644 --- a/testdata/manifests/crd-field-removed.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-field-removed.json @@ -22,7 +22,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" @@ -66,7 +67,8 @@ "type": "object", "properties": { "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" diff --git a/testdata/manifests/crd-invalid b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid similarity index 100% rename from testdata/manifests/crd-invalid rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid diff --git a/testdata/manifests/crd-invalid-upgrade.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json similarity index 92% rename from testdata/manifests/crd-invalid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json index 4131a68fb..3c95ccb25 100644 --- a/testdata/manifests/crd-invalid-upgrade.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-invalid-upgrade.json @@ -24,11 +24,8 @@ "type":"integer" }, "enum": { - "type":"integer", - "enum":[ - 1, - 2 - ] + "type": "string", + "enum": ["a", "b"] }, "minMaxValue": { "type":"integer", diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json new file mode 100644 index 000000000..6fed77fc1 --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-new.json @@ -0,0 +1,40 @@ +{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": { + "name": "widgets.example.com" + }, + "spec": { + "group": "example.com", + "versions": [ + { + "name": "v1alpha1", + "served": true, + "storage": true, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "field": { + "type": "string", + "format": "email" + } + } + } + } + } + } + } + ], + "scope": "Namespaced", + "names": { + "plural": "widgets", + "singular": "widget", + "kind": "Widget" + } + } +} + diff --git a/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json new file mode 100644 index 000000000..a87fbd505 --- /dev/null +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-unhandled-old.json @@ -0,0 +1,39 @@ +{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": { + "name": "widgets.example.com" + }, + "spec": { + "group": "example.com", + "versions": [ + { + "name": "v1alpha1", + "served": true, + "storage": true, + "schema": { + "openAPIV3Schema": { + "type": "object", + "properties": { + "spec": { + "type": "object", + "properties": { + "field": { + "type": "string" + } + } + } + } + } + } + } + ], + "scope": "Namespaced", + "names": { + "plural": "widgets", + "singular": "widget", + "kind": "Widget" + } + } +} + diff --git a/testdata/manifests/crd-valid-upgrade.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json similarity index 93% rename from testdata/manifests/crd-valid-upgrade.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json index 52380dc92..cbc2e3ec1 100644 --- a/testdata/manifests/crd-valid-upgrade.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/crd-valid-upgrade.json @@ -22,7 +22,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" @@ -69,7 +70,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" @@ -116,7 +118,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c", "adding-enum-is-allowed"] }, "minMaxValue": { "type":"integer" diff --git a/testdata/manifests/no-crds.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/no-crds.json similarity index 100% rename from testdata/manifests/no-crds.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/no-crds.json diff --git a/testdata/manifests/old-crd.json b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json similarity index 94% rename from testdata/manifests/old-crd.json rename to internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json index 08e763451..6e7bdf83f 100644 --- a/testdata/manifests/old-crd.json +++ b/internal/operator-controller/rukpak/preflights/crdupgradesafety/testdata/manifests/old-crd.json @@ -22,7 +22,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer" @@ -69,7 +70,8 @@ "type":"integer" }, "enum": { - "type":"integer" + "type": "string", + "enum": ["a", "b", "c"] }, "minMaxValue": { "type":"integer"