Merge pull request #5542 from ephesused/issue5540

fix: improve accumulation failure message
kubernetes-sigs · Mar 27, 2024 · 8fef99f · 8fef99f
2 parents a6149b1 + 14a9a98
commit 8fef99f
Show file tree

Hide file tree

Showing 4 changed files with 118 additions and 8 deletions.
diff --git 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/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"
@@ -457,3 +459,80 @@ func TestDuplicateExternalTransformersForbidden(t *testing.T) {
  require.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", `<!DOCTYPE html>
+<html class="html-devise-layout ui-light-gray" lang="en">
+<head prefix="og: http:https://ogp.me/ns#">
+<meta charset="utf-8">
+`)
+
+ 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) //nolint:wrapcheck // baseLoader's error is sufficient
+}
+
+func (l loaderNewThrowsError) Cleanup() error {
+ return l.baseLoader.Cleanup() //nolint:wrapcheck // baseLoader's error is sufficient
+}
diff --git 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,23 +22,46 @@ func makeAndLoadKustTarget(
  fSys filesys.FileSystem,
  root string) *target.KustTarget {
  t.Helper()
- kt := makeKustTargetWithRf(t, fSys, root, provider.NewDefaultDepProvider())
+ return makeAndLoadKustTargetWithLoaderOverride(t, fSys, root, nil)
+}
+
+func makeKustTargetWithRf(
+ t *testing.T,
+ fSys filesys.FileSystem,
+ root string,
+ pvd *provider.DepProvider) *target.KustTarget {
+ t.Helper()
+ 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 makeKustTargetWithRf(
+func makeKustTargetWithRfAndLoaderOverride(
  t *testing.T,
  fSys filesys.FileSystem,
  root string,
- pvd *provider.DepProvider) *target.KustTarget {
+ pvd *provider.DepProvider,
+ ldrWrapperFn func(ifc.Loader) ifc.Loader) *target.KustTarget {
  t.Helper()
- ldr, err := fLdr.NewLoader(fLdr.RestrictionRootOnly, root, fSys)
+ 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(

diff --git 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)
 }