From f80650e8ce55212d9e576e4838a799bade41cd14 Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Fri, 16 Feb 2024 12:10:55 -0500 Subject: [PATCH 1/3] fix: improve accumulation failure message For accumulation errors when the file load fails due to malformed YAML and the base load fails due to a timeout, report both errors. Previously only the malformed YAML error was returned, masking the git repo timeout. --- api/internal/target/kusttarget.go | 9 ++++++++- api/internal/utils/errtimeout.go | 6 +++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/api/internal/target/kusttarget.go b/api/internal/target/kusttarget.go index eefa1ac63..44c82bc59 100644 --- a/api/internal/target/kusttarget.go +++ b/api/internal/target/kusttarget.go @@ -425,7 +425,14 @@ func (kt *KustTarget) accumulateResources( } ldr, err := kt.ldr.New(path) if err != nil { - if kusterr.IsMalformedYAMLError(errF) { // Some error occurred while tyring to decode YAML file + // If accumulateFile found malformed YAML and there was a failure + // loading the resource as a base, then the resource is likely a + // file. The loader failure message is unnecessary, and could be + // confusing. Report only the file load error. + // + // However, a loader timeout implies there is a git repo at the + // path. In that case, both errors could be important. + if kusterr.IsMalformedYAMLError(errF) && !utils.IsErrTimeout(err) { return nil, errF } return nil, errors.WrapPrefixf( diff --git a/api/internal/utils/errtimeout.go b/api/internal/utils/errtimeout.go index c1761d2c6..a0d861c7b 100644 --- a/api/internal/utils/errtimeout.go +++ b/api/internal/utils/errtimeout.go @@ -15,11 +15,11 @@ type errTimeOut struct { cmd string } -func NewErrTimeOut(d time.Duration, c string) errTimeOut { - return errTimeOut{duration: d, cmd: c} +func NewErrTimeOut(d time.Duration, c string) *errTimeOut { + return &errTimeOut{duration: d, cmd: c} } -func (e errTimeOut) Error() string { +func (e *errTimeOut) Error() string { return fmt.Sprintf("hit %s timeout running '%s'", e.duration, e.cmd) } From 62eca858f32c8863f5e01e4be52767b108ca3795 Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Fri, 8 Mar 2024 14:33:09 -0500 Subject: [PATCH 2/3] test: add test for issue 5440 --- api/internal/target/kusttarget_test.go | 79 ++++++++++++++++++++++++++ api/internal/target/maker_test.go | 36 ++++++++++-- 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/api/internal/target/kusttarget_test.go b/api/internal/target/kusttarget_test.go index a2d426249..501130aa7 100644 --- a/api/internal/target/kusttarget_test.go +++ b/api/internal/target/kusttarget_test.go @@ -8,11 +8,13 @@ import ( "fmt" "reflect" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/api/ifc" . "sigs.k8s.io/kustomize/api/internal/target" + "sigs.k8s.io/kustomize/api/internal/utils" "sigs.k8s.io/kustomize/api/pkg/loader" "sigs.k8s.io/kustomize/api/provider" "sigs.k8s.io/kustomize/api/resmap" @@ -455,3 +457,80 @@ func TestDuplicateExternalTransformersForbidden(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "may not add resource with an already registered id: ValueAnnotator.v1.transformers.example.co/notImportantHere") } + +func TestErrorMessageForMalformedYAML(t *testing.T) { + // These testcases verify behavior for the scenario described in + // https://github.com/kubernetes-sigs/kustomize/issues/5540 . + + testcases := map[string]struct { + loaderNewReturnsError error + shouldShowLoadError bool + }{ + "shouldShowLoadError": { + loaderNewReturnsError: utils.NewErrTimeOut(time.Second, "git init"), + shouldShowLoadError: true, + }, + "shouldNotShowLoadError": { + loaderNewReturnsError: NewErrMissingKustomization("/should-fail/resources.yaml"), + shouldShowLoadError: false, + }, + } + + th := kusttest_test.MakeHarness(t) + th.WriteF("/should-fail/kustomization.yaml", `resources: +- resources.yaml +`) + th.WriteF("/should-fail/resources.yaml", ` + + + +`) + + for name, tc := range testcases { + t.Run(name, func(subT *testing.T) { + ldrWrapper := func(baseLoader ifc.Loader) ifc.Loader { + return loaderNewThrowsError{ + baseLoader: baseLoader, + newReturnsError: tc.loaderNewReturnsError, + } + } + _, err := makeAndLoadKustTargetWithLoaderOverride(t, th.GetFSys(), "/should-fail", ldrWrapper).AccumulateTarget() + require.Error(t, err) + errString := err.Error() + assert.Contains(t, errString, "accumulating resources from 'resources.yaml'") + assert.Contains(t, errString, "MalformedYAMLError: yaml: line 3: mapping values are not allowed in this context") + if tc.shouldShowLoadError { + assert.Regexp(t, `hit \w+ timeout running '`, errString) + } else { + assert.NotRegexp(t, `hit \w+ timeout running '`, errString) + } + }) + } +} + +// loaderNewReturnsError duplicates baseLoader's behavior except +// that New() returns the specified error. +type loaderNewThrowsError struct { + baseLoader ifc.Loader + newReturnsError error +} + +func (l loaderNewThrowsError) Repo() string { + return l.baseLoader.Repo() +} + +func (l loaderNewThrowsError) Root() string { + return l.baseLoader.Root() +} + +func (l loaderNewThrowsError) New(_ string) (ifc.Loader, error) { + return nil, l.newReturnsError +} + +func (l loaderNewThrowsError) Load(location string) ([]byte, error) { + return l.baseLoader.Load(location) +} + +func (l loaderNewThrowsError) Cleanup() error { + return l.baseLoader.Cleanup() +} diff --git a/api/internal/target/maker_test.go b/api/internal/target/maker_test.go index 319195652..95b1095a1 100644 --- a/api/internal/target/maker_test.go +++ b/api/internal/target/maker_test.go @@ -6,6 +6,7 @@ package target_test import ( "testing" + "sigs.k8s.io/kustomize/api/ifc" fLdr "sigs.k8s.io/kustomize/api/internal/loader" pLdr "sigs.k8s.io/kustomize/api/internal/plugins/loader" "sigs.k8s.io/kustomize/api/internal/target" @@ -21,11 +22,7 @@ func makeAndLoadKustTarget( fSys filesys.FileSystem, root string) *target.KustTarget { t.Helper() - kt := makeKustTargetWithRf(t, fSys, root, provider.NewDefaultDepProvider()) - if err := kt.Load(); err != nil { - t.Fatalf("Unexpected load error %v", err) - } - return kt + return makeAndLoadKustTargetWithLoaderOverride(t, fSys, root, nil) } func makeKustTargetWithRf( @@ -34,10 +31,37 @@ func makeKustTargetWithRf( root string, pvd *provider.DepProvider) *target.KustTarget { t.Helper() - ldr, err := fLdr.NewLoader(fLdr.RestrictionRootOnly, root, fSys) + return makeKustTargetWithRfAndLoaderOverride(t, fSys, root, pvd, nil) +} + +func makeAndLoadKustTargetWithLoaderOverride( + t *testing.T, + fSys filesys.FileSystem, + root string, + ldrWrapperFn func(ifc.Loader) ifc.Loader) *target.KustTarget { + t.Helper() + kt := makeKustTargetWithRfAndLoaderOverride(t, fSys, root, provider.NewDefaultDepProvider(), ldrWrapperFn) + if err := kt.Load(); err != nil { + t.Fatalf("Unexpected load error %v", err) + } + return kt +} + +func makeKustTargetWithRfAndLoaderOverride( + t *testing.T, + fSys filesys.FileSystem, + root string, + pvd *provider.DepProvider, + ldrWrapperFn func(ifc.Loader) ifc.Loader) *target.KustTarget { + t.Helper() + baseLoader, err := fLdr.NewLoader(fLdr.RestrictionRootOnly, root, fSys) if err != nil { t.Fatal(err) } + ldr := baseLoader + if ldrWrapperFn != nil { + ldr = ldrWrapperFn(baseLoader) + } rf := resmap.NewFactory(pvd.GetResourceFactory()) pc := types.DisabledPluginConfig() return target.NewKustTarget( From 14a9a9849f2998f2b6007b854eae26d94dfb740b Mon Sep 17 00:00:00 2001 From: Ed Overton Date: Mon, 11 Mar 2024 15:16:11 -0400 Subject: [PATCH 3/3] test: correct lint issues --- api/internal/target/kusttarget_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/internal/target/kusttarget_test.go b/api/internal/target/kusttarget_test.go index df0904dce..8e6ef0f9a 100644 --- a/api/internal/target/kusttarget_test.go +++ b/api/internal/target/kusttarget_test.go @@ -530,9 +530,9 @@ func (l loaderNewThrowsError) New(_ string) (ifc.Loader, error) { } func (l loaderNewThrowsError) Load(location string) ([]byte, error) { - return l.baseLoader.Load(location) + return l.baseLoader.Load(location) //nolint:wrapcheck // baseLoader's error is sufficient } func (l loaderNewThrowsError) Cleanup() error { - return l.baseLoader.Cleanup() + return l.baseLoader.Cleanup() //nolint:wrapcheck // baseLoader's error is sufficient }