Repository: helm/helm
Pull request #8493 (2020-07-21 06:21:20)

fix insecure-skip-tls-verify flag does't work on helm install
Signed-off-by: yxxhero <aiopsclub@163.com>

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/master/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:
    fix insecure-skip-tls-verify flag does't work on helm install, Keep FindChartInRepoURL and FindChartInAuthRepoURL functions signatures intact.
issue: https://github.com/helm/helm/issues/8492
**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


bacongobbler: Can you please also make sure to write some unit tests to cover this case moving forward. Thanks.

Patch:
diff --git a/cmd/helm/dependency_build.go b/cmd/helm/dependency_build.go
index 4e87684ce..478b49479 100644
--- a/cmd/helm/dependency_build.go
+++ b/cmd/helm/dependency_build.go
@@ -16,7 +16,6 @@ limitations under the License.
 package main
 
 import (
-	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -66,11 +65,7 @@ func newDependencyBuildCmd(out io.Writer) *cobra.Command {
 			if client.Verify {
 				man.Verify = downloader.VerifyIfPossible
 			}
-			err := man.Build()
-			if e, ok := err.(downloader.ErrRepoNotFound); ok {
-				return fmt.Errorf("%s. Please add the missing repos via 'helm repo add'", e.Error())
-			}
-			return err
+			return man.Build()
 		},
 	}
 
diff --git a/pkg/action/install.go b/pkg/action/install.go
index 48a3aeeca..383dd2c86 100644
--- a/pkg/action/install.go
+++ b/pkg/action/install.go
@@ -654,8 +654,8 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) (
 		dl.Verify = downloader.VerifyAlways
 	}
 	if c.RepoURL != "" {
-		chartURL, err := repo.FindChartInAuthRepoURL(c.RepoURL, c.Username, c.Password, name, version,
-			c.CertFile, c.KeyFile, c.CaFile, getter.All(settings))
+		chartURL, err := repo.FindChartInAuthAndTLSRepoURL(c.RepoURL, c.Username, c.Password, name, version,
+			c.CertFile, c.KeyFile, c.CaFile, c.InsecureSkipTLSverify, getter.All(settings))
 		if err != nil {
 			return "", err
 		}
diff --git a/pkg/action/pull.go b/pkg/action/pull.go
index a46e98bae..220ca11b2 100644
--- a/pkg/action/pull.go
+++ b/pkg/action/pull.go
@@ -89,7 +89,7 @@ func (p *Pull) Run(chartRef string) (string, error) {
 	}
 
 	if p.RepoURL != "" {
-		chartURL, err := repo.FindChartInAuthRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, getter.All(p.Settings))
+		chartURL, err := repo.FindChartInAuthAndTLSRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, p.InsecureSkipTLSverify, getter.All(p.Settings))
 		if err != nil {
 			return out.String(), err
 		}
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index 2d6918739..00198de0c 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -42,17 +42,6 @@ import (
 	"helm.sh/helm/v3/pkg/repo"
 )
 
-// ErrRepoNotFound indicates that chart repositories can't be found in local repo cache.
-// The value of Repos is missing repos.
-type ErrRepoNotFound struct {
-	Repos []string
-}
-
-// Error implements the error interface.
-func (e ErrRepoNotFound) Error() string {
-	return fmt.Sprintf("no repository definition for %s", strings.Join(e.Repos, ", "))
-}
-
 // Manager handles the lifecycle of fetching, resolving, and storing dependencies.
 type Manager struct {
 	// Out is used to print warnings and notifications.
@@ -422,7 +411,7 @@ Loop:
 		missing = append(missing, dd.Repository)
 	}
 	if len(missing) > 0 {
-		return ErrRepoNotFound{missing}
+		return errors.Errorf("no repository definition for %s. Please add the missing repos via 'helm repo add'", strings.Join(missing, ", "))
 	}
 	return nil
 }
diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go
index c2c366a1e..edb86eaeb 100644
--- a/pkg/repo/chartrepo.go
+++ b/pkg/repo/chartrepo.go
@@ -205,6 +205,14 @@ func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caF
 // without adding repo to repositories, like FindChartInRepoURL,
 // but it also receives credentials for the chart repository.
 func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) {
+	return FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile, false, getters)
+}
+
+// FindChartInAuthRepoURL finds chart in chart repository pointed by repoURL
+// without adding repo to repositories, like FindChartInRepoURL,
+// but it also receives credentials and TLS verify flag for the chart repository.
+// TODO Helm 4, FindChartInAuthAndTLSRepoURL should be integrated into FindChartInAuthRepoURL.
+func FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, insecureSkipTLSverify bool, getters getter.Providers) (string, error) {
 
 	// Download and write the index file to a temporary location
 	buf := make([]byte, 20)
@@ -212,13 +220,14 @@ func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion
 	name := strings.ReplaceAll(base64.StdEncoding.EncodeToString(buf), "/", "-")
 
 	c := Entry{
-		URL:      repoURL,
-		Username: username,
-		Password: password,
-		CertFile: certFile,
-		KeyFile:  keyFile,
-		CAFile:   caFile,
-		Name:     name,
+		URL:                   repoURL,
+		Username:              username,
+		Password:              password,
+		CertFile:              certFile,
+		KeyFile:               keyFile,
+		CAFile:                caFile,
+		Name:                  name,
+		InsecureSkipTLSverify: insecureSkipTLSverify,
 	}
 	r, err := NewChartRepository(&c, getters)
 	if err != nil {


Test patch:
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index dd3cbbd7b..ea235c13f 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -360,32 +360,3 @@ func TestBuild_WithRepositoryAlias(t *testing.T) {
 		Repository: "@test",
 	})
 }
-
-func TestErrRepoNotFound_Error(t *testing.T) {
-	type fields struct {
-		Repos []string
-	}
-	tests := []struct {
-		name   string
-		fields fields
-		want   string
-	}{
-		{
-			name: "OK",
-			fields: fields{
-				Repos: []string{"https://charts1.example.com", "https://charts2.example.com"},
-			},
-			want: "no repository definition for https://charts1.example.com, https://charts2.example.com",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			e := ErrRepoNotFound{
-				Repos: tt.fields.Repos,
-			}
-			if got := e.Error(); got != tt.want {
-				t.Errorf("Error() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go
index f50d6a2b6..5317bcbc0 100644
--- a/pkg/repo/chartrepo_test.go
+++ b/pkg/repo/chartrepo_test.go
@@ -276,6 +276,44 @@ func startLocalServerForTests(handler http.Handler) (*httptest.Server, error) {
 	return httptest.NewServer(handler), nil
 }
 
+// startLocalTLSServerForTests Start the local helm server with TLS
+func startLocalTLSServerForTests(handler http.Handler) (*httptest.Server, error) {
+	if handler == nil {
+		fileBytes, err := ioutil.ReadFile("testdata/local-index.yaml")
+		if err != nil {
+			return nil, err
+		}
+		handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write(fileBytes)
+		})
+	}
+
+	return httptest.NewTLSServer(handler), nil
+}
+
+func TestFindChartInAuthAndTLSRepoURL(t *testing.T) {
+	srv, err := startLocalTLSServerForTests(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer srv.Close()
+
+	chartURL, err := FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "", "", "", "", true, getter.All(&cli.EnvSettings{}))
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
+	if chartURL != "https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz" {
+		t.Errorf("%s is not the valid URL", chartURL)
+	}
+
+	// If the insecureSkipTLsverify is false, it will return an error that contains "x509: certificate signed by unknown authority".
+	_, err = FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "0.1.0", "", "", "", false, getter.All(&cli.EnvSettings{}))
+
+	if !strings.Contains(err.Error(), "x509: certificate signed by unknown authority") {
+		t.Errorf("Expected TLS error for function  FindChartInAuthAndTLSRepoURL not found, but got a different error (%v)", err)
+	}
+}
+
 func TestFindChartInRepoURL(t *testing.T) {
 	srv, err := startLocalServerForTests(nil)
 	if err != nil {