diff --git a/internal/rukpak/preflights/crdupgradesafety/checks_test.go b/internal/rukpak/preflights/crdupgradesafety/checks_test.go index 6544006ce..5e1bee3fd 100644 --- a/internal/rukpak/preflights/crdupgradesafety/checks_test.go +++ b/internal/rukpak/preflights/crdupgradesafety/checks_test.go @@ -2,6 +2,7 @@ package crdupgradesafety import ( "errors" + "fmt" "testing" kappcus "carvel.dev/kapp/pkg/kapp/crdupgradesafety" @@ -905,3 +906,81 @@ 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()) + }) + } +} diff --git a/internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go b/internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go index 7cdd905f6..2599cedda 100644 --- a/internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go +++ b/internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go @@ -1,9 +1,11 @@ package crdupgradesafety import ( + "cmp" "context" "errors" "fmt" + "slices" "strings" kappcus "carvel.dev/kapp/pkg/kapp/crdupgradesafety" @@ -84,7 +86,7 @@ func (p *Preflight) runPreflight(ctx context.Context, rel *release.Release) erro return fmt.Errorf("parsing release %q objects: %w", rel.Name, err) } - validateErrors := []error{} + validateErrors := make([]error, 0, len(relObjects)) for _, obj := range relObjects { if obj.GetObjectKind().GroupVersionKind() != apiextensionsv1.SchemeGroupVersion.WithKind("CustomResourceDefinition") { continue @@ -112,9 +114,71 @@ 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)) } } 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...) +}