From baf5b76a957dc52a2fca84fa1328628cc78cc307 Mon Sep 17 00:00:00 2001
From: Matt Farina <matt@mattfarina.com>
Date: Mon, 21 Sep 2020 10:42:47 -0400
Subject: [PATCH] Fixing issue with idempotent repo add

A security issue fixed in 3.3.2 caught repos with the same name
being added a second time and produced an error. This caused an
issue for tools, such as helmfile, that will add the same name
with the same configuration multiple times.

This fix checks that the configuration on the existing and new
repo are the same. If there is no change it notes it and exists
with a 0 exit code. If there is a change the existing error is
returned (for reverse compat). If --force-update is given the
user opts in to changing the config for the name.

Closes #8771

Signed-off-by: Matt Farina <matt@mattfarina.com>
---
 cmd/helm/repo_add.go                   | 22 +++++++++++++----
 cmd/helm/repo_add_test.go              | 34 ++++++++++++++++++++++----
 cmd/helm/testdata/output/repo-add2.txt |  1 +
 3 files changed, 47 insertions(+), 10 deletions(-)
 create mode 100644 cmd/helm/testdata/output/repo-add2.txt

diff --git a/cmd/helm/repo_add.go b/cmd/helm/repo_add.go
index 1c2162bfa..f79c213c0 100644
--- a/cmd/helm/repo_add.go
+++ b/cmd/helm/repo_add.go
@@ -116,11 +116,6 @@ func (o *repoAddOptions) run(out io.Writer) error {
 		return err
 	}
 
-	// If the repo exists and --force-update was not specified, error out.
-	if !o.forceUpdate && f.Has(o.name) {
-		return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name)
-	}
-
 	if o.username != "" && o.password == "" {
 		fd := int(os.Stdin.Fd())
 		fmt.Fprint(out, "Password: ")
@@ -143,6 +138,23 @@ func (o *repoAddOptions) run(out io.Writer) error {
 		InsecureSkipTLSverify: o.insecureSkipTLSverify,
 	}
 
+	// If the repo exists do one of two things:
+	// 1. If the configuration for the name is the same continue without error
+	// 2. When the config is different require --force-update
+	if !o.forceUpdate && f.Has(o.name) {
+		existing := f.Get(o.name)
+		if c != *existing {
+
+			// The input coming in for the name is different from what is already
+			// configured. Return an error.
+			return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name)
+		}
+
+		// The add is idempotent so do nothing
+		fmt.Fprintf(out, "%q already exists with the same configuration, skipping\n", o.name)
+		return nil
+	}
+
 	r, err := repo.NewChartRepository(&c, getter.All(settings))
 	if err != nil {
 		return err
diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go
index d358ad970..f3bc54985 100644
--- a/cmd/helm/repo_add_test.go
+++ b/cmd/helm/repo_add_test.go
@@ -40,14 +40,38 @@ func TestRepoAddCmd(t *testing.T) {
 	}
 	defer srv.Stop()
 
+	// A second test server is setup to verify URL changing
+	srv2, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer srv2.Stop()
+
 	tmpdir := ensure.TempDir(t)
 	repoFile := filepath.Join(tmpdir, "repositories.yaml")
 
-	tests := []cmdTestCase{{
-		name:   "add a repository",
-		cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
-		golden: "output/repo-add.txt",
-	}}
+	tests := []cmdTestCase{
+		{
+			name:   "add a repository",
+			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
+			golden: "output/repo-add.txt",
+		},
+		{
+			name:   "add repository second time",
+			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
+			golden: "output/repo-add2.txt",
+		},
+		{
+			name:      "add repository different url",
+			cmd:       fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv2.URL(), repoFile, tmpdir),
+			wantError: true,
+		},
+		{
+			name:   "add repository second time",
+			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s --force-update", srv2.URL(), repoFile, tmpdir),
+			golden: "output/repo-add.txt",
+		},
+	}
 
 	runTestCmd(t, tests)
 }
diff --git a/cmd/helm/testdata/output/repo-add2.txt b/cmd/helm/testdata/output/repo-add2.txt
new file mode 100644
index 000000000..263ffa9e4
--- /dev/null
+++ b/cmd/helm/testdata/output/repo-add2.txt
@@ -0,0 +1 @@
+"test-name" already exists with the same configuration, skipping