Ignore broken symlinks and outside path, in commit

controllers/git_test.go

@@ -0,0 +1,107 @@
+package controllers
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/go-git/go-billy/v5/memfs"
+	"github.com/go-git/go-git/v5"
+	"github.com/go-git/go-git/v5/plumbing/object"
+	"github.com/go-git/go-git/v5/storage/memory"
+)
+
+func populateRepoFromFixture(repo *git.Repository, fixture string) error {
+	working, err := repo.Worktree()
+	if err != nil {
+		return err
+	}
+	fs := working.Filesystem
+
+	if err = filepath.Walk(fixture, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if info.IsDir() {
+			return fs.MkdirAll(fs.Join(path[len(fixture):]), info.Mode())
+		}
+		// copy symlinks as-is, so I can test what happens with broken symlinks
+		if info.Mode()&os.ModeSymlink > 0 {
+			target, err := os.Readlink(path)
+			if err != nil {
+				return err
+			}
+			return fs.Symlink(target, path[len(fixture):])
+		}
+
+		fileBytes, err := ioutil.ReadFile(path)
+		if err != nil {
+			return err
+		}
+
+		ff, err := fs.Create(path[len(fixture):])
+		if err != nil {
+			return err
+		}
+		defer ff.Close()
+
+		_, err = ff.Write(fileBytes)
+		return err
+	}); err != nil {
+		return err
+	}
+
+	_, err = working.Add(".")
+	if err != nil {
+		return err
+	}
+
+	if _, err = working.Commit("Initial revision from "+fixture, &git.CommitOptions{
+		Author: &object.Signature{
+			Name:  "Testbot",
+			Email: "[email protected]",
+			When:  time.Now(),
+		},
+	}); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func TestRepoForFixture(t *testing.T) {
+	repo, err := git.Init(memory.NewStorage(), memfs.New())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = populateRepoFromFixture(repo, "testdata/pathconfig")
+	if err != nil {
+		t.Error(err)
+	}
+}
+
+func TestIgnoreBrokenSymlink(t *testing.T) {
+	// init a git repo in the filesystem so we can operate on files there
+	tmp, err := ioutil.TempDir("", "flux-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmp)
+
+	repo, err := git.PlainInit(tmp, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = populateRepoFromFixture(repo, "testdata/brokenlink")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = commitChangedManifests(repo, tmp, nil, nil, "unused")
+	if err != errNoChanges {
+		t.Fatalf("expected no changes but got: %v", err)
+	}
+}

controllers/imageupdateautomation_controller.go

@@ -25,6 +25,7 @@ import (
 	"io/ioutil"
 	"math"
 	"os"
+	"path/filepath"
 	"strings"
 	"text/template"
 	"time"
@@ -200,6 +201,15 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 
 	log.V(debug).Info("cloned git repository", "gitrepository", originName, "branch", auto.Spec.Checkout.Branch, "working", tmp)
 
+	manifestsPath := tmp
+	if auto.Spec.Update.Path != "" {
+		if p, err := securejoin.SecureJoin(tmp, auto.Spec.Update.Path); err != nil {
+			return failWithError(err)
+		} else {
+			manifestsPath = p
+		}
+	}
+
 	switch {
 	case auto.Spec.Update != nil && auto.Spec.Update.Strategy == imagev1.UpdateStrategySetters:
 		// For setters we first want to compile a list of _all_ the
@@ -210,15 +220,6 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 			return failWithError(err)
 		}
 
-		manifestsPath := tmp
-		if auto.Spec.Update.Path != "" {
-			if p, err := securejoin.SecureJoin(tmp, auto.Spec.Update.Path); err != nil {
-				return failWithError(err)
-			} else {
-				manifestsPath = p
-			}
-		}
-
 		if result, err := updateAccordingToSetters(ctx, manifestsPath, policies.Items); err != nil {
 			return failWithError(err)
 		} else {
@@ -241,10 +242,29 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 		signingEntity, err = r.getSigningEntity(ctx, auto)
 	}
 
+	// construct the commit message from template and values
+	msgTmpl := auto.Spec.Commit.MessageTemplate
+	if msgTmpl == "" {
+		msgTmpl = defaultMessageTemplate
+	}
+	tmpl, err := template.New("commit message").Parse(msgTmpl)
+	if err != nil {
+		return failWithError(fmt.Errorf("unable to create commit message template from spec: %w", err))
+	}
+	messageBuf := &strings.Builder{}
+	if err := tmpl.Execute(messageBuf, templateValues); err != nil {
+		return failWithError(fmt.Errorf("failed to run template from spec: %w", err))
+	}
+
 	// The status message depends on what happens next. Since there's
 	// more than one way to succeed, there's some if..else below, and
 	// early returns only on failure.
-	if rev, err := commitAll(repo, &auto.Spec.Commit, templateValues, signingEntity); err != nil {
+	author := &object.Signature{
+		Name:  auto.Spec.Commit.AuthorName,
+		Email: auto.Spec.Commit.AuthorEmail,
+		When:  time.Now(),
+	}
+	if rev, err := commitChangedManifests(repo, tmp, signingEntity, author, messageBuf.String()); err != nil {
 		if err == errNoChanges {
 			r.event(ctx, auto, events.EventSeverityInfo, "no updates made")
 			log.V(debug).Info("no changes made in working directory; no commit")
@@ -476,7 +496,7 @@ func switchBranch(repo *gogit.Repository, pushBranch string) error {
 
 var errNoChanges error = errors.New("no changes made to working directory")
 
-func commitAll(repo *gogit.Repository, commit *imagev1.CommitSpec, values TemplateData, ent *openpgp.Entity) (string, error) {
+func commitChangedManifests(repo *gogit.Repository, absRepoPath string, ent *openpgp.Entity, author *object.Signature, message string) (string, error) {
 	working, err := repo.Worktree()
 	if err != nil {
 		return "", err
@@ -485,31 +505,38 @@ func commitAll(repo *gogit.Repository, commit *imagev1.CommitSpec, values Templa
 	status, err := working.Status()
 	if err != nil {
 		return "", err
-	} else if status.IsClean() {
-		return "", errNoChanges
 	}
 
-	msgTmpl := commit.MessageTemplate
-	if msgTmpl == "" {
-		msgTmpl = defaultMessageTemplate
-	}
-	tmpl, err := template.New("commit message").Parse(msgTmpl)
-	if err != nil {
-		return "", err
+	// go-git has [a bug](https://github.com/go-git/go-git/issues/253)
+	// whereby it thinks broken symlinks to absolute paths are
+	// modified. There's no circumstance in which we want to commit a
+	// change to a broken symlink: so, detect and skip those.
+	var changed bool
+	for file, _ := range status {
+		abspath := filepath.Join(absRepoPath, file)
+		info, err := os.Lstat(abspath)
+		if err != nil {
+			return "", fmt.Errorf("checking if %s is a symlink: %w", file, err)
+		}
+		if info.Mode()&os.ModeSymlink > 0 {
+			// symlinks are OK; broken symlinks are probably a result
+			// of the bug mentioned above, but not of interest in any
+			// case.
+			if _, err := os.Stat(abspath); os.IsNotExist(err) {
+				continue
+			}
+		}
+		working.Add(file)
+		changed = true
 	}
-	buf := &strings.Builder{}
-	if err := tmpl.Execute(buf, values); err != nil {
-		return "", err
+
+	if !changed {
+		return "", errNoChanges
 	}
 
 	var rev plumbing.Hash
-	if rev, err = working.Commit(buf.String(), &gogit.CommitOptions{
-		All: true,
-		Author: &object.Signature{
-			Name:  commit.AuthorName,
-			Email: commit.AuthorEmail,
-			When:  time.Now(),
-		},
+	if rev, err = working.Commit(message, &gogit.CommitOptions{
+		Author:  author,
 		SignKey: ent,
 	}); err != nil {
 		return "", err

controllers/testdata/brokenlink/bar.yaml

@@ -0,0 +1 @@
+/surely/does/not/exist

controllers/update_test.go

@@ -214,7 +214,7 @@ Images:
 			// Insert a setter reference into the deployment file,
 			// before creating the automation object itself.
 			commitInRepo(repoURL, branch, "Install setter marker", func(tmp string) {
-				replaceMarker(tmp, policyKey)
+				Expect(replaceMarker(tmp, policyKey)).To(Succeed())
 			})
 
 			// pull the head commit we just pushed, so it's not
@@ -333,10 +333,10 @@ Images:
 			// Insert a setter reference into the deployment file,
 			// before creating the automation object itself.
 			commitInRepo(repoURL, branch, "Install setter marker", func(tmp string) {
-				replaceMarker(path.Join(tmp, "yes"), policyKey)
+				Expect(replaceMarker(path.Join(tmp, "yes"), policyKey)).To(Succeed())
 			})
 			commitInRepo(repoURL, branch, "Install setter marker", func(tmp string) {
-				replaceMarker(path.Join(tmp, "no"), policyKey)
+				Expect(replaceMarker(path.Join(tmp, "no"), policyKey)).To(Succeed())
 			})
 
 			// pull the head commit we just pushed, so it's not
@@ -454,7 +454,7 @@ Images:
 			// Insert a setter reference into the deployment file,
 			// before creating the automation object itself.
 			commitInRepo(repoURL, branch, "Install setter marker", func(tmp string) {
-				replaceMarker(tmp, policyKey)
+				Expect(replaceMarker(tmp, policyKey)).To(Succeed())
 			})
 
 			// pull the head commit we just pushed, so it's not
@@ -684,7 +684,7 @@ Images:
 
 				BeforeEach(func() {
 					commitInRepo(cloneLocalRepoURL, branch, "Install setter marker", func(tmp string) {
-						replaceMarker(tmp, policyKey)
+						Expect(replaceMarker(tmp, policyKey)).To(Succeed())
 					})
 					waitForNewHead(localRepo, branch)
 
@@ -758,7 +758,7 @@ Images:
 					// Insert a setter reference into the deployment file,
 					// before creating the automation object itself.
 					commitInRepo(cloneLocalRepoURL, branch, "Install setter marker", func(tmp string) {
-						replaceMarker(tmp, policyKey)
+						Expect(replaceMarker(tmp, policyKey)).To(Succeed())
 					})
 
 					// pull the head commit we just pushed, so it's not
@@ -815,7 +815,7 @@ Images:
 					Expect(newObj.Status.LastPushTime).ToNot(BeNil())
 
 					compareRepoWithExpected(cloneLocalRepoURL, branch, "testdata/appconfig-setters-expected", func(tmp string) {
-						replaceMarker(tmp, policyKey)
+						Expect(replaceMarker(tmp, policyKey)).To(Succeed())
 					})
 				})
 
@@ -928,14 +928,18 @@ func expectCommittedAndPushed(conditions []metav1.Condition) {
 	Expect(rc.Message).To(ContainSubstring("committed and pushed"))
 }
 
-func replaceMarker(path string, policyKey types.NamespacedName) {
-	// NB this requires knowledge of what's in the git
-	// repo, so a little brittle
+func replaceMarker(path string, policyKey types.NamespacedName) error {
+	// NB this requires knowledge of what's in the git repo, so a little brittle
 	deployment := filepath.Join(path, "deploy.yaml")
 	filebytes, err := ioutil.ReadFile(deployment)
-	Expect(err).NotTo(HaveOccurred())
+	if err != nil {
+		return err
+	}
 	newfilebytes := bytes.ReplaceAll(filebytes, []byte("SETTER_SITE"), []byte(setterRef(policyKey)))
-	Expect(ioutil.WriteFile(deployment, newfilebytes, os.FileMode(0666))).To(Succeed())
+	if err = ioutil.WriteFile(deployment, newfilebytes, os.FileMode(0666)); err != nil {
+		return err
+	}
+	return nil
 }
 
 func setterRef(name types.NamespacedName) string {
@@ -1040,51 +1044,15 @@ func initGitRepo(gitServer *gittestserver.GitServer, fixture, branch, repository
 		return err
 	}
 
-	if err = filepath.Walk(fixture, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
-		if info.IsDir() {
-			return fs.MkdirAll(fs.Join(path[len(fixture):]), info.Mode())
-		}
-
-		fileBytes, err := ioutil.ReadFile(path)
-		if err != nil {
-			return err
-		}
-
-		ff, err := fs.Create(path[len(fixture):])
-		if err != nil {
-			return err
-		}
-		defer ff.Close()
-
-		_, err = ff.Write(fileBytes)
-		return err
-	}); err != nil {
-		return err
-	}
-
-	working, err := repo.Worktree()
+	err = populateRepoFromFixture(repo, fixture)
 	if err != nil {
 		return err
 	}
 
-	_, err = working.Add(".")
+	working, err := repo.Worktree()
 	if err != nil {
 		return err
 	}
-
-	if _, err = working.Commit("Initial revision from "+fixture, &git.CommitOptions{
-		Author: &object.Signature{
-			Name:  "Testbot",
-			Email: "[email protected]",
-			When:  time.Now(),
-		},
-	}); err != nil {
-		return err
-	}
-
 	if err = working.Checkout(&git.CheckoutOptions{
 		Branch: plumbing.NewBranchReferenceName(branch),
 		Create: true,