Skip to content

Commit

Permalink
add orderKappsValidateErr in crdupgradesafety preflight
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
azych committed Jan 20, 2025
1 parent 594cba3 commit 0845f38
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 1 deletion.
79 changes: 79 additions & 0 deletions internal/rukpak/preflights/crdupgradesafety/checks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package crdupgradesafety

import (
"errors"
"fmt"
"testing"

kappcus "carvel.dev/kapp/pkg/kapp/crdupgradesafety"
Expand Down Expand Up @@ -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())
})
}
}
65 changes: 64 additions & 1 deletion internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package crdupgradesafety

import (
"cmp"
"context"
"errors"
"fmt"
"slices"
"strings"

kappcus "carvel.dev/kapp/pkg/kapp/crdupgradesafety"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -112,9 +114,70 @@ 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 unforseen issues which likely mean that the internals of validator.Validate

Check failure on line 135 in internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

View workflow job for this annotation

GitHub Actions / lint

`unforseen` is a misspelling of `unforeseen` (misspell)
// 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
}

var errs []error

Check failure on line 149 in internal/rukpak/preflights/crdupgradesafety/crdupgradesafety.go

View workflow job for this annotation

GitHub Actions / lint

Consider pre-allocating `errs` (prealloc)
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...)
}

0 comments on commit 0845f38

Please sign in to comment.