🐛 Ensure fixed order in multi-line errors returned by crdupgradesafety validators (#1864)

* fix: ensure order in multi-line errors returned by crdupgradesafety validators

* chore: remove redundant orderKappsValidateErr

* refactor: extract ServedVersionValidator to its own file

---------

Co-authored-by: Artur Zych <[email protected]>

Author: azych

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)

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

internal/operator-controller/rukpak/preflights/crdupgradesafety/checks.go

@@ -3,77 +3,13 @@ package crdupgradesafety
 import (
 	"bytes"
 	"cmp"
-	"errors"
 	"fmt"
 	"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, diff := range diffs {
-				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 {

internal/operator-controller/rukpak/preflights/crdupgradesafety/checks_test.go

@@ -2,7 +2,6 @@ package crdupgradesafety
 
 import (
 	"errors"
-	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -905,81 +904,3 @@ 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/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"
@@ -113,71 +111,9 @@ 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...)
-}