Merge pull request #5542 from ephesused/issue5540

k8s-ci-robot · web-flow · commit 8fef99fa35eb · 2024-03-27T10:32:54.000-07:00
fix: improve accumulation failure message
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://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)
 }
 

Original file line number	Diff line number	Diff line change
`@@ -15,11 +15,11 @@ type errTimeOut struct {`
`15`	`15`	`cmd string`
`16`	`16`	`}`
`17`	`17`
`18`		`-func NewErrTimeOut(d time.Duration, c string) errTimeOut {`
`19`		`- return errTimeOut{duration: d, cmd: c}`
	`18`	`+func NewErrTimeOut(d time.Duration, c string) *errTimeOut {`
	`19`	`+ return &errTimeOut{duration: d, cmd: c}`
`20`	`20`	`}`
`21`	`21`
`22`		`-func (e errTimeOut) Error() string {`
	`22`	`+func (e *errTimeOut) Error() string {`
`23`	`23`	`return fmt.Sprintf("hit %s timeout running '%s'", e.duration, e.cmd)`
`24`	`24`	`}`
`25`	`25`