From ed16ae995aa8b15c5eda3edff5fd566ebcde6c0b Mon Sep 17 00:00:00 2001 From: Artur Zych <5843875+azych@users.noreply.github.com> Date: Mon, 20 Jan 2025 16:29:03 +0100 Subject: [PATCH] add orderKappsValidateErr in crdupgradesafety preflight orderKappsValidateErr() is meant as a temporary solution to an external (ie. dependency) problem. carvel.dev/kapp/pkg/kapp/crdupgradesafety Validate() can return a multi-line error message which comes in random order. Until that is changed upstream, we need to fix this on our side to avoid falling into cycle of constantly trying to reconcile ClusterExtension's status due to random error message we set in its conditions. --- .../crdupgradesafety/checks_test.go | 79 +++++++++++++++++++ .../crdupgradesafety/crdupgradesafety.go | 66 +++++++++++++++- 2 files changed, 144 insertions(+), 1 deletion(-) 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...) +}