Merge pull request #3330 from monopole/loaderTest

Provide timeouts for all remote loads.

api/internal/git/cloner.go

@@ -4,10 +4,6 @@
 package git
 
 import (
-	"log"
-	"os/exec"
-
-	"github.com/pkg/errors"
 	"sigs.k8s.io/kustomize/api/filesys"
 )
 
@@ -18,84 +14,29 @@ type Cloner func(repoSpec *RepoSpec) error
 // to say, some remote API, to obtain a local clone of
 // a remote repo.
 func ClonerUsingGitExec(repoSpec *RepoSpec) error {
-	gitProgram, err := exec.LookPath("git")
-	if err != nil {
-		return errors.Wrap(err, "no 'git' program on path")
-	}
-	repoSpec.Dir, err = filesys.NewTmpConfirmedDir()
+	r, err := newCmdRunner()
 	if err != nil {
 		return err
 	}
-
-	cmd := exec.Command(
-		gitProgram,
-		"init",
-		repoSpec.Dir.String())
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		log.Printf("Error initializing git repo: %s", out)
-		return errors.Wrapf(
-			err,
-			"trouble initializing git repo in %s",
-			repoSpec.Dir.String())
+	repoSpec.Dir = r.dir
+	if err = r.run("init"); err != nil {
+		return err
 	}
-
-	cmd = exec.Command(
-		gitProgram,
-		"remote",
-		"add",
-		"origin",
-		repoSpec.CloneSpec())
-	cmd.Dir = repoSpec.Dir.String()
-	out, err = cmd.CombinedOutput()
-	if err != nil {
-		log.Printf("Error adding remote: %s", out)
-		return errors.Wrapf(err, "trouble adding remote %s", repoSpec.CloneSpec())
+	if err = r.run(
+		"remote", "add", "origin", repoSpec.CloneSpec()); err != nil {
+		return err
 	}
-
 	ref := "HEAD"
 	if repoSpec.Ref != "" {
 		ref = repoSpec.Ref
 	}
-
-	cmd = exec.Command(
-		gitProgram,
-		"fetch",
-		"--depth=1",
-		"origin",
-		ref)
-	cmd.Dir = repoSpec.Dir.String()
-	out, err = cmd.CombinedOutput()
-	if err != nil {
-		log.Printf("Error fetching ref: %s", out)
-		return errors.Wrapf(err, "trouble fetching %s", ref)
+	if err = r.run("fetch", "--depth=1", "origin", ref); err != nil {
+		return err
 	}
-
-	cmd = exec.Command(
-		gitProgram,
-		"checkout",
-		"FETCH_HEAD")
-	cmd.Dir = repoSpec.Dir.String()
-	out, err = cmd.CombinedOutput()
-	if err != nil {
-		log.Printf("Error checking out ref: %s", out)
-		return errors.Wrapf(err, "trouble checking out %s", ref)
+	if err = r.run("checkout", "FETCH_HEAD"); err != nil {
+		return err
 	}
-
-	cmd = exec.Command(
-		gitProgram,
-		"submodule",
-		"update",
-		"--init",
-		"--recursive")
-	cmd.Dir = repoSpec.Dir.String()
-	out, err = cmd.CombinedOutput()
-	if err != nil {
-		log.Printf("Error fetching submodules: %s", out)
-		return errors.Wrapf(err, "trouble fetching submodules for %s", repoSpec.CloneSpec())
-	}
-
-	return nil
+	return r.run("submodule", "update", "--init", "--recursive")
 }
 
 // DoNothingCloner returns a cloner that only sets

api/internal/git/gitrunner.go

@@ -0,0 +1,65 @@
+// Copyright 2020 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package git
+
+import (
+	"os/exec"
+	"time"
+
+	"github.com/pkg/errors"
+	"sigs.k8s.io/kustomize/api/filesys"
+	"sigs.k8s.io/kustomize/api/types"
+)
+
+// Arbitrary, but non-infinite, timeout for running commands.
+const defaultDuration = 27 * time.Second
+
+// gitRunner runs the external git binary.
+type gitRunner struct {
+	gitProgram string
+	duration   time.Duration
+	dir        filesys.ConfirmedDir
+}
+
+// newCmdRunner returns a gitRunner if it can find the binary.
+// It also creats a temp directory for cloning repos.
+func newCmdRunner() (*gitRunner, error) {
+	gitProgram, err := exec.LookPath("git")
+	if err != nil {
+		return nil, errors.Wrap(err, "no 'git' program on path")
+	}
+	dir, err := filesys.NewTmpConfirmedDir()
+	if err != nil {
+		return nil, err
+	}
+	return &gitRunner{
+		gitProgram: gitProgram,
+		duration:   defaultDuration,
+		dir:        dir,
+	}, nil
+}
+
+// run a command with a timeout.
+func (r gitRunner) run(args ...string) (err error) {
+	ch := make(chan bool, 1)
+	defer close(ch)
+	//nolint: gosec
+	cmd := exec.Command(r.gitProgram, args...)
+	cmd.Dir = r.dir.String()
+	timer := time.NewTimer(r.duration)
+	defer timer.Stop()
+	go func() {
+		_, err = cmd.CombinedOutput()
+		ch <- true
+	}()
+	select {
+	case <-ch:
+		if err != nil {
+			return errors.Wrapf(err, "git cmd = '%s'", cmd.String())
+		}
+		return nil
+	case <-timer.C:
+		return types.NewErrTimeOut(r.duration, cmd.String())
+	}
+}

api/krusty/remoteload_test.go

@@ -0,0 +1,40 @@
+// Copyright 2019 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package krusty_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/kustomize/api/filesys"
+	"sigs.k8s.io/kustomize/api/krusty"
+	"sigs.k8s.io/kustomize/api/types"
+)
+
+func TestRemoteLoad(t *testing.T) {
+	fSys := filesys.MakeFsOnDisk()
+	b := krusty.MakeKustomizer(fSys, krusty.MakeDefaultOptions())
+	m, err := b.Run(
+		"github.com/kubernetes-sigs/kustomize/examples/multibases/dev/?ref=v1.0.6")
+	if types.IsErrTimeout(err) {
+		// Don't fail on timeouts.
+		t.SkipNow()
+	}
+	if !assert.NoError(t, err) {
+		t.FailNow()
+	}
+	yml, err := m.AsYaml()
+	assert.NoError(t, err)
+	assert.Equal(t, `apiVersion: v1
+kind: Pod
+metadata:
+  labels:
+    app: myapp
+  name: dev-myapp-pod
+spec:
+  containers:
+  - image: nginx:1.7.9
+    name: nginx
+`, string(yml))
+}

api/loader/getter.go

@@ -7,11 +7,13 @@ import (
 	"context"
 	"log"
 	"os"
+	"time"
 
 	"github.com/yujunz/go-getter"
 	"sigs.k8s.io/kustomize/api/filesys"
 	"sigs.k8s.io/kustomize/api/ifc"
 	"sigs.k8s.io/kustomize/api/internal/git"
+	"sigs.k8s.io/kustomize/api/types"
 )
 
 type remoteTargetSpec struct {
@@ -28,7 +30,12 @@ type remoteTargetSpec struct {
 // Getter is a function that can gets resource
 type remoteTargetGetter func(rs *remoteTargetSpec) error
 
-func newLoaderAtGetter(raw string, fSys filesys.FileSystem, referrer *fileLoader, cloner git.Cloner, getter remoteTargetGetter) (ifc.Loader, error) {
+func newLoaderAtGetter(
+	raw string,
+	fSys filesys.FileSystem,
+	referrer *fileLoader,
+	cloner git.Cloner,
+	getter remoteTargetGetter) (ifc.Loader, error) {
 	rs := &remoteTargetSpec{
 		Raw: raw,
 	}
@@ -86,7 +93,22 @@ func getRemoteTarget(rs *remoteTargetSpec) error {
 		},
 		Options: opts,
 	}
-	return client.Get()
+
+	ch := make(chan bool, 1)
+	defer close(ch)
+	d := 21 * time.Second // arbitrary
+	timer := time.NewTimer(d)
+	defer timer.Stop()
+	go func() {
+		err = client.Get()
+		ch <- true
+	}()
+	select {
+	case <-ch:
+	case <-timer.C:
+		err = types.NewErrTimeOut(d, "go-getter client.Get")
+	}
+	return err
 }
 
 func getNothing(rs *remoteTargetSpec) error {

api/types/errtimeout.go

@@ -0,0 +1,36 @@
+// Copyright 2020 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package types
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/pkg/errors"
+)
+
+type errTimeOut struct {
+	duration time.Duration
+	cmd      string
+}
+
+func NewErrTimeOut(d time.Duration, c string) errTimeOut {
+	return errTimeOut{duration: d, cmd: c}
+}
+
+func (e errTimeOut) Error() string {
+	return fmt.Sprintf("hit %s timeout running '%s'", e.duration, e.cmd)
+}
+
+func IsErrTimeout(err error) bool {
+	if err == nil {
+		return false
+	}
+	_, ok := err.(errTimeOut)
+	if ok {
+		return true
+	}
+	_, ok = errors.Cause(err).(errTimeOut)
+	return ok
+}