Merge pull request #1449 from richardmarshall/git_cycle_detection

Fix indirect cycle detection for git resources

pkg/git/cloner.go

@@ -18,6 +18,7 @@ package git
 
 import (
 	"bytes"
+	"log"
 	"os/exec"
 
 	"github.com/pkg/errors"
@@ -45,8 +46,10 @@ func ClonerUsingGitExec(repoSpec *RepoSpec) error {
 		repoSpec.Dir.String())
 	var out bytes.Buffer
 	cmd.Stdout = &out
+	cmd.Stderr = &out
 	err = cmd.Run()
 	if err != nil {
+		log.Printf("Error initializing empty git repo: %s", out.String())
 		return errors.Wrapf(
 			err,
 			"trouble initializing empty git repo in %s",
@@ -60,9 +63,11 @@ func ClonerUsingGitExec(repoSpec *RepoSpec) error {
 		"origin",
 		repoSpec.CloneSpec())
 	cmd.Stdout = &out
+	cmd.Stderr = &out
 	cmd.Dir = repoSpec.Dir.String()
 	err = cmd.Run()
 	if err != nil {
+		log.Printf("Error setting git remote: %s", out.String())
 		return errors.Wrapf(
 			err,
 			"trouble adding remote %s",
@@ -78,9 +83,11 @@ func ClonerUsingGitExec(repoSpec *RepoSpec) error {
 		"origin",
 		repoSpec.Ref)
 	cmd.Stdout = &out
+	cmd.Stderr = &out
 	cmd.Dir = repoSpec.Dir.String()
 	err = cmd.Run()
 	if err != nil {
+		log.Printf("Error performing git fetch: %s", out.String())
 		return errors.Wrapf(err, "trouble fetching %s", repoSpec.Ref)
 	}
 
@@ -90,9 +97,11 @@ func ClonerUsingGitExec(repoSpec *RepoSpec) error {
 		"--hard",
 		"FETCH_HEAD")
 	cmd.Stdout = &out
+	cmd.Stderr = &out
 	cmd.Dir = repoSpec.Dir.String()
 	err = cmd.Run()
 	if err != nil {
+		log.Printf("Error performing git reset: %s", out.String())
 		return errors.Wrapf(
 			err, "trouble hard resetting empty repository to %s", repoSpec.Ref)
 	}

pkg/loader/fileloader.go

@@ -179,7 +179,7 @@ func (fl *fileLoader) New(path string) (ifc.Loader, error) {
 			return nil, err
 		}
 		return newLoaderAtGitClone(
-			repoSpec, fl.validator, fl.fSys, fl.referrer, fl.cloner)
+			repoSpec, fl.validator, fl.fSys, fl, fl.cloner)
 	}
 	if filepath.IsAbs(path) {
 		return nil, fmt.Errorf("new root '%s' cannot be absolute", path)

pkg/loader/fileloader_test.go

@@ -530,3 +530,67 @@ func TestLocalLoaderReferencingGitBase(t *testing.T) {
 		t.Fatalf("unexpected root %s", l2.Root())
 	}
 }
+
+func TestRepoDirectCycleDetection(t *testing.T) {
+	topDir := "/cycles"
+	cloneRoot := topDir + "/someClone"
+	fSys := fs.MakeFakeFS()
+	fSys.MkdirAll(topDir)
+	fSys.MkdirAll(cloneRoot)
+
+	root, err := demandDirectoryRoot(fSys, topDir)
+	if err != nil {
+		t.Fatalf("unexpected err: %v\n", err)
+	}
+	l1 := newLoaderAtConfirmedDir(
+		RestrictionRootOnly, validators.MakeFakeValidator(), root, fSys, nil,
+		git.DoNothingCloner(fs.ConfirmedDir(cloneRoot)))
+	p1 := "github.com/someOrg/someRepo/foo"
+	rs1, err := git.NewRepoSpecFromUrl(p1)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	l1.repoSpec = rs1
+	_, err = l1.New(p1)
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+	if !strings.Contains(err.Error(), "cycle detected") {
+		t.Fatalf("unexpected err: %v", err)
+	}
+}
+
+func TestRepoIndirectCycleDetection(t *testing.T) {
+	topDir := "/cycles"
+	cloneRoot := topDir + "/someClone"
+	fSys := fs.MakeFakeFS()
+	fSys.MkdirAll(topDir)
+	fSys.MkdirAll(cloneRoot)
+
+	root, err := demandDirectoryRoot(fSys, topDir)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	l0 := newLoaderAtConfirmedDir(
+		RestrictionRootOnly, validators.MakeFakeValidator(), root, fSys, nil,
+		git.DoNothingCloner(fs.ConfirmedDir(cloneRoot)))
+
+	p1 := "github.com/someOrg/someRepo1"
+	p2 := "github.com/someOrg/someRepo2"
+
+	l1, err := l0.New(p1)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	l2, err := l1.New(p2)
+	if err != nil {
+		t.Fatalf("unexpected err: %v", err)
+	}
+	_, err = l2.New(p1)
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+	if !strings.Contains(err.Error(), "cycle detected") {
+		t.Fatalf("unexpected err: %v", err)
+	}
+}

pkg/target/kusttarget.go

@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"log"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -329,9 +330,11 @@ func (kt *KustTarget) accumulateResources(
 				return err
 			}
 		} else {
-			err = kt.accumulateFile(ra, path)
-			if err != nil {
-				return err
+			err2 := kt.accumulateFile(ra, path)
+			if err2 != nil {
+				// Log ldr.New() error to highlight git failures.
+				log.Print(err.Error())
+				return err2
 			}
 		}
 	}