🐛 add orderKappsValidateErr in crdupgradesafety preflight

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())
+		})
+	}
+}

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...)
+}