From 717420db34636508995bedf8423045c6769682c4 Mon Sep 17 00:00:00 2001 From: Karuppiah Natarajan Date: Sun, 21 Jul 2019 14:53:52 +0530 Subject: [PATCH] fix repo url being decoded Signed-off-by: Karuppiah Natarajan --- cmd/helm/init.go | 3 +- cmd/helm/repo_add.go | 3 +- cmd/helm/repo_update.go | 3 +- glide.lock | 6 ++++ glide.yaml | 11 +++---- pkg/downloader/manager.go | 3 +- pkg/getter/mocks/getter.go | 67 ++++++++++++++++++++++++++++++++++++++ pkg/repo/chartrepo.go | 15 +++++---- pkg/repo/chartrepo_test.go | 50 ++++++++++++++++++++++++++++ pkg/repo/index_test.go | 3 +- 10 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 pkg/getter/mocks/getter.go diff --git a/cmd/helm/init.go b/cmd/helm/init.go index b628dc008..964a77498 100644 --- a/cmd/helm/init.go +++ b/cmd/helm/init.go @@ -21,6 +21,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/spf13/afero" "io" "os" "time" @@ -411,7 +412,7 @@ func initStableRepo(cacheFile string, out io.Writer, skipRefresh bool, home helm // In this case, the cacheFile is always absolute. So passing empty string // is safe. - if err := r.DownloadIndexFile(""); err != nil { + if err := r.DownloadIndexFile("", afero.NewOsFs()); err != nil { return nil, fmt.Errorf("Looks like %q is not a valid chart repository or cannot be reached: %s", stableRepositoryURL, err.Error()) } diff --git a/cmd/helm/repo_add.go b/cmd/helm/repo_add.go index bfb3f0174..fdb82e2fd 100644 --- a/cmd/helm/repo_add.go +++ b/cmd/helm/repo_add.go @@ -18,6 +18,7 @@ package main import ( "fmt" + "github.com/spf13/afero" "io" "github.com/spf13/cobra" @@ -127,7 +128,7 @@ func addRepository(name, url, username, password string, home helmpath.Home, cer return err } - if err := r.DownloadIndexFile(home.Cache()); err != nil { + if err := r.DownloadIndexFile(home.Cache(), afero.NewOsFs()); err != nil { return fmt.Errorf("Looks like %q is not a valid chart repository or cannot be reached: %s", url, err.Error()) } diff --git a/cmd/helm/repo_update.go b/cmd/helm/repo_update.go index 291b21b72..edc4bfaa6 100644 --- a/cmd/helm/repo_update.go +++ b/cmd/helm/repo_update.go @@ -19,6 +19,7 @@ package main import ( "errors" "fmt" + "github.com/spf13/afero" "io" "sync" @@ -96,7 +97,7 @@ func updateCharts(repos []*repo.ChartRepository, out io.Writer, home helmpath.Ho fmt.Fprintf(out, "...Skip %s chart repository\n", re.Config.Name) return } - err := re.DownloadIndexFile(home.Cache()) + err := re.DownloadIndexFile(home.Cache(), afero.NewOsFs()) if err != nil { fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", re.Config.Name, re.Config.URL, err) } else { diff --git a/glide.lock b/glide.lock index 298de2612..937737cc0 100644 --- a/glide.lock +++ b/glide.lock @@ -251,6 +251,10 @@ imports: version: 10ef21a441db47d8b13ebcc5fd2310f636973c77 - name: github.com/sirupsen/logrus version: 89742aefa4b206dcf400792f3bd35b542998eb3b +- name: github.com/spf13/afero + version: 588a75ec4f32903aa5e39a2619ba6a4631e28424 + subpackages: + - mem - name: github.com/spf13/cobra version: c439c4fa093711d42e1b01acb1235b52004753c1 subpackages: @@ -778,6 +782,8 @@ imports: subpackages: - sortorder testImports: +- name: github.com/golang/mock + version: 9fa652df1129bef0e734c9cf9bf6dbae9ef3b9fa - name: github.com/pmezard/go-difflib version: d8ed2627bdf02c080bf22230dbb337003b7aba2d subpackages: diff --git a/glide.yaml b/glide.yaml index 716fc56ef..f8ca9675f 100644 --- a/glide.yaml +++ b/glide.yaml @@ -61,9 +61,8 @@ import: version: kubernetes-1.11.1 - package: github.com/cyphar/filepath-securejoin version: ^0.2.1 - -testImports: -- package: github.com/stretchr/testify - version: ^1.1.4 - subpackages: - - assert +- package: github.com/spf13/afero + version: ^1.2.2 +testImport: +- package: github.com/golang/mock + version: ^1.3.1 diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 67f9dc7bf..d1d156673 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -18,6 +18,7 @@ package downloader import ( "errors" "fmt" + "github.com/spf13/afero" "io" "io/ioutil" "net/url" @@ -453,7 +454,7 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { } wg.Add(1) go func(r *repo.ChartRepository) { - if err := r.DownloadIndexFile(m.HelmHome.Cache()); err != nil { + if err := r.DownloadIndexFile(m.HelmHome.Cache(), afero.NewOsFs()); err != nil { fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", r.Config.Name, r.Config.URL, err) } else { fmt.Fprintf(out, "...Successfully got an update from the %q chart repository\n", r.Config.Name) diff --git a/pkg/getter/mocks/getter.go b/pkg/getter/mocks/getter.go new file mode 100644 index 000000000..12fd7a5ac --- /dev/null +++ b/pkg/getter/mocks/getter.go @@ -0,0 +1,67 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This is a mock for the Getter interface + +// Code generated by MockGen. DO NOT EDIT. +// Source: pkg/getter/getter.go +// Command To Generate: mockgen -source pkg/getter/getter.go -package mocks Getter > pkg/getter/mocks/getter.go + +// Package mocks is a generated GoMock package. +package mocks + +import ( + bytes "bytes" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockGetter is a mock of Getter interface +type MockGetter struct { + ctrl *gomock.Controller + recorder *MockGetterMockRecorder +} + +// MockGetterMockRecorder is the mock recorder for MockGetter +type MockGetterMockRecorder struct { + mock *MockGetter +} + +// NewMockGetter creates a new mock instance +func NewMockGetter(ctrl *gomock.Controller) *MockGetter { + mock := &MockGetter{ctrl: ctrl} + mock.recorder = &MockGetterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockGetter) EXPECT() *MockGetterMockRecorder { + return m.recorder +} + +// Get mocks base method +func (m *MockGetter) Get(url string) (*bytes.Buffer, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", url) + ret0, _ := ret[0].(*bytes.Buffer) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Get indicates an expected call of Get +func (mr *MockGetterMockRecorder) Get(url interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockGetter)(nil).Get), url) +} diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index cd9d6c547..3175af731 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -18,9 +18,11 @@ package repo // import "k8s.io/helm/pkg/repo" import ( "fmt" + "github.com/spf13/afero" "io/ioutil" "net/url" "os" + "path" "path/filepath" "strings" @@ -110,15 +112,14 @@ func (r *ChartRepository) Load() error { // // cachePath is prepended to any index that does not have an absolute path. This // is for pre-2.2.0 repo files. -func (r *ChartRepository) DownloadIndexFile(cachePath string) error { - var indexURL string +func (r *ChartRepository) DownloadIndexFile(cachePath string, fs afero.Fs) error { parsedURL, err := url.Parse(r.Config.URL) if err != nil { return err } - parsedURL.Path = strings.TrimSuffix(parsedURL.Path, "/") + "/index.yaml" - - indexURL = parsedURL.String() + parsedURL.RawPath = path.Join(parsedURL.RawPath, "index.yaml") + parsedURL.Path = path.Join(parsedURL.Path, "index.yaml") + indexURL := parsedURL.String() r.setCredentials() resp, err := r.Client.Get(indexURL) @@ -146,7 +147,7 @@ func (r *ChartRepository) DownloadIndexFile(cachePath string) error { cp = filepath.Join(cachePath, cp) } - return ioutil.WriteFile(cp, index, 0644) + return afero.WriteFile(fs, cp, index, 0644) } // If HttpGetter is used, this method sets the configured repository credentials on the HttpGetter. @@ -224,7 +225,7 @@ func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion if err != nil { return "", err } - if err := r.DownloadIndexFile(tempIndexFile.Name()); err != nil { + if err := r.DownloadIndexFile(tempIndexFile.Name(), afero.NewOsFs()); err != nil { return "", fmt.Errorf("Looks like %q is not a valid chart repository or cannot be reached: %s", repoURL, err) } diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index 19071872d..44796b12d 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -17,7 +17,12 @@ limitations under the License. package repo import ( + "bytes" + "github.com/golang/mock/gomock" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" "io/ioutil" + "k8s.io/helm/pkg/getter/mocks" "net/http" "net/http/httptest" "os" @@ -311,3 +316,48 @@ func TestResolveReferenceURL(t *testing.T) { t.Errorf("%s contains query string from base URL when it shouldn't", chartURL) } } + +func TestChartRepository_DownloadIndexFile(t *testing.T) { + t.Run("should not decode the path in the repo url", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + r, err := NewChartRepository(&Entry{ + Name: "testrepo", + URL: "http://example-charts.com/some%2Fpath/test", + Cache: "/repository/cache/testrepo-index.yaml", + }, getter.All(environment.EnvSettings{})) + if err != nil { + t.Errorf("Problem creating chart repository from %s: %v", "testrepo", err) + } + + fs := afero.NewMemMapFs() + err = fs.MkdirAll("/repository/cache/", 0644) + assert.NoError(t, err, "problem creating test repository cache") + + mockGetter := mocks.NewMockGetter(ctrl) + r.Client = mockGetter + indexYamlContent := `apiVersion: v1 +entries: + testchart: + - apiVersion: v1 + appVersion: 1.2.3 + created: 2019-07-06T17:56:26.384768233Z + description: test + digest: 93f924d4498d588bcdda88c7401e27c6fa0f50ff0601e78885eca13eb683c1e2 + home: https://github.com/test/test + icon: https://github.com/test/test/test.png + name: testchart + sources: + - https://github.com/test/test + urls: + - https://test.com/test-1.0.0.tgz + version: 1.0.0` + + mockGetter.EXPECT().Get("http://example-charts.com/some%2Fpath/test/index.yaml"). + Return(bytes.NewBufferString(indexYamlContent), nil) + + err = r.DownloadIndexFile("", fs) + assert.NoError(t, err, "problem downloading index file") + }) +} diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index 2ce817ce3..df6ae63b0 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -17,6 +17,7 @@ limitations under the License. package repo import ( + "github.com/spf13/afero" "io/ioutil" "os" "path/filepath" @@ -151,7 +152,7 @@ func TestDownloadIndexFile(t *testing.T) { t.Errorf("Problem creating chart repository from %s: %v", testRepo, err) } - if err := r.DownloadIndexFile(""); err != nil { + if err := r.DownloadIndexFile("", afero.NewOsFs()); err != nil { t.Errorf("%#v", err) }