From c115b6320678bf3d4b046aef6b3d73378c966a5f Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Thu, 1 Nov 2018 13:30:45 -0700 Subject: [PATCH 1/3] Remove namespace usage --- cmd/tiller-proxy/internal/handler/handler.go | 15 +++++---- cmd/tiller-proxy/main.go | 9 +++--- pkg/proxy/fake/proxy.go | 6 ++-- pkg/proxy/proxy.go | 24 +++++++-------- pkg/proxy/proxy_test.go | 32 +++++++++----------- 5 files changed, 41 insertions(+), 45 deletions(-) diff --git a/cmd/tiller-proxy/internal/handler/handler.go b/cmd/tiller-proxy/internal/handler/handler.go index 1615c847d2e..61679bd84e3 100644 --- a/cmd/tiller-proxy/internal/handler/handler.go +++ b/cmd/tiller-proxy/internal/handler/handler.go @@ -26,13 +26,12 @@ import ( "github.com/gorilla/mux" "github.com/kubeapps/common/response" - log "github.com/sirupsen/logrus" - "github.com/urfave/negroni" - "k8s.io/helm/pkg/proto/hapi/chart" - "github.com/kubeapps/kubeapps/pkg/auth" chartUtils "github.com/kubeapps/kubeapps/pkg/chart" proxy "github.com/kubeapps/kubeapps/pkg/proxy" + log "github.com/sirupsen/logrus" + "github.com/urfave/negroni" + "k8s.io/helm/pkg/proto/hapi/chart" ) // Context key type for request contexts @@ -221,7 +220,7 @@ func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, p return } } - rel, err := h.ProxyClient.UpdateRelease(params["releaseName"], params["namespace"], chartDetails.Values, ch) + rel, err := h.ProxyClient.UpdateRelease(params["releaseName"], chartDetails.Values, ch) if err != nil { response.NewErrorResponse(errorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) return @@ -253,7 +252,7 @@ func (h *TillerProxy) ListReleases(w http.ResponseWriter, req *http.Request, par // GetRelease returns the release info func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, params Params) { - rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) + rel, err := h.ProxyClient.GetRelease(params["releaseName"]) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return @@ -281,7 +280,7 @@ func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, param // DeleteRelease removes a release from a namespace func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, params Params) { if !h.DisableAuth { - rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) + rel, err := h.ProxyClient.GetRelease(params["releaseName"]) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return @@ -306,7 +305,7 @@ func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, pa if req.URL.Query().Get("purge") == "1" || req.URL.Query().Get("purge") == "true" { purge = true } - err := h.ProxyClient.DeleteRelease(params["releaseName"], params["namespace"], purge) + err := h.ProxyClient.DeleteRelease(params["releaseName"], purge) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return diff --git a/cmd/tiller-proxy/main.go b/cmd/tiller-proxy/main.go index 6aad3a4d533..e60bdf4b960 100644 --- a/cmd/tiller-proxy/main.go +++ b/cmd/tiller-proxy/main.go @@ -24,6 +24,9 @@ import ( "github.com/gorilla/mux" "github.com/heptiolabs/healthcheck" + "github.com/kubeapps/kubeapps/cmd/tiller-proxy/internal/handler" + chartUtils "github.com/kubeapps/kubeapps/pkg/chart" + tillerProxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" "github.com/spf13/pflag" "github.com/urfave/negroni" @@ -33,10 +36,6 @@ import ( "k8s.io/helm/pkg/helm" "k8s.io/helm/pkg/helm/environment" "k8s.io/helm/pkg/tlsutil" - - "github.com/kubeapps/kubeapps/cmd/tiller-proxy/internal/handler" - chartUtils "github.com/kubeapps/kubeapps/pkg/chart" - tillerProxy "github.com/kubeapps/kubeapps/pkg/proxy" ) const ( @@ -144,6 +143,8 @@ func main() { ProxyClient: proxy, } // Routes + // TODO Some of these routes do not require namespace anymore (i.e getRelease) + // it has been kept this way for compatibility reasons. Change. apiv1 := r.PathPrefix("/v1").Subrouter() apiv1.Methods("GET").Path("/releases").Handler(negroni.New( authGate, diff --git a/pkg/proxy/fake/proxy.go b/pkg/proxy/fake/proxy.go index a69476f39a0..b6f8fa69741 100644 --- a/pkg/proxy/fake/proxy.go +++ b/pkg/proxy/fake/proxy.go @@ -73,7 +73,7 @@ func (f *FakeProxy) CreateRelease(name, namespace, values string, ch *chart.Char return &r, nil } -func (f *FakeProxy) UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) { +func (f *FakeProxy) UpdateRelease(name string, values string, ch *chart.Chart) (*release.Release, error) { for _, r := range f.Releases { if r.Name == name { return &r, nil @@ -82,7 +82,7 @@ func (f *FakeProxy) UpdateRelease(name, namespace string, values string, ch *cha return nil, fmt.Errorf("Release %s not found", name) } -func (f *FakeProxy) GetRelease(name, namespace string) (*release.Release, error) { +func (f *FakeProxy) GetRelease(name string) (*release.Release, error) { for _, r := range f.Releases { if r.Name == name { return &r, nil @@ -91,7 +91,7 @@ func (f *FakeProxy) GetRelease(name, namespace string) (*release.Release, error) return nil, fmt.Errorf("Release %s not found", name) } -func (f *FakeProxy) DeleteRelease(name, namespace string, purge bool) error { +func (f *FakeProxy) DeleteRelease(name string, purge bool) error { for i, r := range f.Releases { if r.Name == name { if purge { diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 7a528c05c39..b9a6f47e8b2 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -78,9 +78,7 @@ type AppOverview struct { Status string `json:"status"` } -// TODO: Rename get for getRelease -// TODO: Remove namespace since release name is unique -func (p *Proxy) get(name, namespace string) (*release.Release, error) { +func (p *Proxy) getRelease(name string) (*release.Release, error) { release, err := p.helmClient.ReleaseContent(name) if err != nil { @@ -236,11 +234,11 @@ func (p *Proxy) CreateRelease(name, namespace, values string, ch *chart.Chart) ( } // UpdateRelease upgrades a tiller release -func (p *Proxy) UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) { +func (p *Proxy) UpdateRelease(name, values string, ch *chart.Chart) (*release.Release, error) { lock(name) defer unlock(name) // Check if the release already exists - _, err := p.get(name, namespace) + _, err := p.getRelease(name) if err != nil { return nil, err } @@ -258,18 +256,18 @@ func (p *Proxy) UpdateRelease(name, namespace string, values string, ch *chart.C } // GetRelease returns the info of a release -func (p *Proxy) GetRelease(name, namespace string) (*release.Release, error) { +func (p *Proxy) GetRelease(name string) (*release.Release, error) { lock(name) defer unlock(name) - return p.get(name, namespace) + return p.getRelease(name) } // DeleteRelease deletes a release -func (p *Proxy) DeleteRelease(name, namespace string, purge bool) error { +func (p *Proxy) DeleteRelease(name string, purge bool) error { lock(name) defer unlock(name) // Validate that the release actually belongs to the namespace - _, err := p.get(name, namespace) + _, err := p.getRelease(name) if err != nil { return err } @@ -297,11 +295,11 @@ func prettyError(err error) error { // TillerClient for exposed funcs type TillerClient interface { - GetReleaseStatus(relName string) (release.Status_Code, error) + GetReleaseStatus(name string) (release.Status_Code, error) ResolveManifest(namespace, values string, ch *chart.Chart) (string, error) ListReleases(namespace string, releaseListLimit int, status string) ([]AppOverview, error) CreateRelease(name, namespace, values string, ch *chart.Chart) (*release.Release, error) - UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) - GetRelease(name, namespace string) (*release.Release, error) - DeleteRelease(name, namespace string, purge bool) error + UpdateRelease(name string, values string, ch *chart.Chart) (*release.Release, error) + GetRelease(name string) (*release.Release, error) + DeleteRelease(name string, purge bool) error } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index eed8d8e173a..59b08e45a0d 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -250,7 +250,7 @@ func TestHelmReleaseUpdated(t *testing.T) { app := AppOverview{rs, version, ns, "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - result, err := proxy.UpdateRelease(rs, ns, "", ch) + result, err := proxy.UpdateRelease(rs, "", ch) if err != nil { t.Errorf("Unexpected error %v", err) } @@ -272,7 +272,6 @@ func TestHelmReleaseUpdated(t *testing.T) { } func TestUpdateMissingHelmRelease(t *testing.T) { - ns := "myns" rs := "foo" chartName := "bar" version := "v1.0.0" @@ -285,7 +284,7 @@ func TestUpdateMissingHelmRelease(t *testing.T) { app := AppOverview{rs2, version, ns2, "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - _, err := proxy.UpdateRelease(rs, ns, "", ch) + _, err := proxy.UpdateRelease(rs, "", ch) if err == nil { t.Error("Update should fail, there is not a release in the namespace specified") } @@ -298,21 +297,20 @@ func TestGetHelmRelease(t *testing.T) { app1 := AppOverview{"foo", "1.0.0", "my_ns", "icon.png", "DEPLOYED"} app2 := AppOverview{"bar", "1.0.0", "other_ns", "icon2.png", "DELETED"} type testStruct struct { - existingApps []AppOverview - shouldFail bool - targetApp string - tartegNamespace string - expectedResult string + existingApps []AppOverview + shouldFail bool + targetApp string + expectedResult string } tests := []testStruct{ - {[]AppOverview{app1, app2}, false, "foo", "my_ns", "foo"}, - {[]AppOverview{app1, app2}, false, "foo", "", "foo"}, + {[]AppOverview{app1, app2}, false, "foo", "foo"}, + {[]AppOverview{app1, app2}, false, "foo", "foo"}, } for _, test := range tests { proxy := newFakeProxy(test.existingApps) - res, err := proxy.GetRelease(test.targetApp, test.tartegNamespace) + res, err := proxy.GetRelease(test.targetApp) if test.shouldFail && err == nil { - t.Errorf("Get %s/%s should fail", test.tartegNamespace, test.targetApp) + t.Errorf("Get %s should fail", test.targetApp) } if !test.shouldFail { if err != nil { @@ -330,7 +328,7 @@ func TestHelmReleaseDeleted(t *testing.T) { proxy := newFakeProxy([]AppOverview{app}) // TODO: Add a test for a non-purged release when the fake helm cli supports it - err := proxy.DeleteRelease(app.ReleaseName, app.Namespace, true) + err := proxy.DeleteRelease(app.ReleaseName, true) if err != nil { t.Errorf("Unexpected error %v", err) } @@ -347,9 +345,9 @@ func TestDeleteMissingHelmRelease(t *testing.T) { app := AppOverview{"foo", "1.0.0", "my_ns", "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - err := proxy.DeleteRelease("not_foo", "other_ns", true) + err := proxy.DeleteRelease("not_foo", true) if err == nil { - t.Error("Delete should fail, there is not a release in the namespace specified") + t.Error("Delete should fail, there is not a release") } rels, err := proxy.helmClient.ListReleases() if err != nil { @@ -390,13 +388,13 @@ func TestEnsureThreadSafety(t *testing.T) { } }, func() { - _, err := proxy.UpdateRelease(rs, ns, "", ch) + _, err := proxy.UpdateRelease(rs, "", ch) if err != nil { t.Errorf("Unexpected error %v", err) } }, func() { - err := proxy.DeleteRelease(rs, ns, true) + err := proxy.DeleteRelease(rs, true) if err != nil { t.Errorf("Unexpected error %v", err) } From f6de93b868b04f08211d19a6f33c4fdcfa732c97 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Mon, 5 Nov 2018 14:56:11 -0800 Subject: [PATCH 2/3] Revert "Remove namespace usage" This reverts commit c115b6320678bf3d4b046aef6b3d73378c966a5f. --- cmd/tiller-proxy/internal/handler/handler.go | 15 ++++----- cmd/tiller-proxy/main.go | 9 +++--- pkg/proxy/fake/proxy.go | 6 ++-- pkg/proxy/proxy.go | 24 ++++++++------- pkg/proxy/proxy_test.go | 32 +++++++++++--------- 5 files changed, 45 insertions(+), 41 deletions(-) diff --git a/cmd/tiller-proxy/internal/handler/handler.go b/cmd/tiller-proxy/internal/handler/handler.go index 61679bd84e3..1615c847d2e 100644 --- a/cmd/tiller-proxy/internal/handler/handler.go +++ b/cmd/tiller-proxy/internal/handler/handler.go @@ -26,12 +26,13 @@ import ( "github.com/gorilla/mux" "github.com/kubeapps/common/response" - "github.com/kubeapps/kubeapps/pkg/auth" - chartUtils "github.com/kubeapps/kubeapps/pkg/chart" - proxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" "github.com/urfave/negroni" "k8s.io/helm/pkg/proto/hapi/chart" + + "github.com/kubeapps/kubeapps/pkg/auth" + chartUtils "github.com/kubeapps/kubeapps/pkg/chart" + proxy "github.com/kubeapps/kubeapps/pkg/proxy" ) // Context key type for request contexts @@ -220,7 +221,7 @@ func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, p return } } - rel, err := h.ProxyClient.UpdateRelease(params["releaseName"], chartDetails.Values, ch) + rel, err := h.ProxyClient.UpdateRelease(params["releaseName"], params["namespace"], chartDetails.Values, ch) if err != nil { response.NewErrorResponse(errorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) return @@ -252,7 +253,7 @@ func (h *TillerProxy) ListReleases(w http.ResponseWriter, req *http.Request, par // GetRelease returns the release info func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, params Params) { - rel, err := h.ProxyClient.GetRelease(params["releaseName"]) + rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return @@ -280,7 +281,7 @@ func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, param // DeleteRelease removes a release from a namespace func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, params Params) { if !h.DisableAuth { - rel, err := h.ProxyClient.GetRelease(params["releaseName"]) + rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return @@ -305,7 +306,7 @@ func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, pa if req.URL.Query().Get("purge") == "1" || req.URL.Query().Get("purge") == "true" { purge = true } - err := h.ProxyClient.DeleteRelease(params["releaseName"], purge) + err := h.ProxyClient.DeleteRelease(params["releaseName"], params["namespace"], purge) if err != nil { response.NewErrorResponse(errorCode(err), err.Error()).Write(w) return diff --git a/cmd/tiller-proxy/main.go b/cmd/tiller-proxy/main.go index e60bdf4b960..6aad3a4d533 100644 --- a/cmd/tiller-proxy/main.go +++ b/cmd/tiller-proxy/main.go @@ -24,9 +24,6 @@ import ( "github.com/gorilla/mux" "github.com/heptiolabs/healthcheck" - "github.com/kubeapps/kubeapps/cmd/tiller-proxy/internal/handler" - chartUtils "github.com/kubeapps/kubeapps/pkg/chart" - tillerProxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" "github.com/spf13/pflag" "github.com/urfave/negroni" @@ -36,6 +33,10 @@ import ( "k8s.io/helm/pkg/helm" "k8s.io/helm/pkg/helm/environment" "k8s.io/helm/pkg/tlsutil" + + "github.com/kubeapps/kubeapps/cmd/tiller-proxy/internal/handler" + chartUtils "github.com/kubeapps/kubeapps/pkg/chart" + tillerProxy "github.com/kubeapps/kubeapps/pkg/proxy" ) const ( @@ -143,8 +144,6 @@ func main() { ProxyClient: proxy, } // Routes - // TODO Some of these routes do not require namespace anymore (i.e getRelease) - // it has been kept this way for compatibility reasons. Change. apiv1 := r.PathPrefix("/v1").Subrouter() apiv1.Methods("GET").Path("/releases").Handler(negroni.New( authGate, diff --git a/pkg/proxy/fake/proxy.go b/pkg/proxy/fake/proxy.go index b6f8fa69741..a69476f39a0 100644 --- a/pkg/proxy/fake/proxy.go +++ b/pkg/proxy/fake/proxy.go @@ -73,7 +73,7 @@ func (f *FakeProxy) CreateRelease(name, namespace, values string, ch *chart.Char return &r, nil } -func (f *FakeProxy) UpdateRelease(name string, values string, ch *chart.Chart) (*release.Release, error) { +func (f *FakeProxy) UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) { for _, r := range f.Releases { if r.Name == name { return &r, nil @@ -82,7 +82,7 @@ func (f *FakeProxy) UpdateRelease(name string, values string, ch *chart.Chart) ( return nil, fmt.Errorf("Release %s not found", name) } -func (f *FakeProxy) GetRelease(name string) (*release.Release, error) { +func (f *FakeProxy) GetRelease(name, namespace string) (*release.Release, error) { for _, r := range f.Releases { if r.Name == name { return &r, nil @@ -91,7 +91,7 @@ func (f *FakeProxy) GetRelease(name string) (*release.Release, error) { return nil, fmt.Errorf("Release %s not found", name) } -func (f *FakeProxy) DeleteRelease(name string, purge bool) error { +func (f *FakeProxy) DeleteRelease(name, namespace string, purge bool) error { for i, r := range f.Releases { if r.Name == name { if purge { diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b9a6f47e8b2..7a528c05c39 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -78,7 +78,9 @@ type AppOverview struct { Status string `json:"status"` } -func (p *Proxy) getRelease(name string) (*release.Release, error) { +// TODO: Rename get for getRelease +// TODO: Remove namespace since release name is unique +func (p *Proxy) get(name, namespace string) (*release.Release, error) { release, err := p.helmClient.ReleaseContent(name) if err != nil { @@ -234,11 +236,11 @@ func (p *Proxy) CreateRelease(name, namespace, values string, ch *chart.Chart) ( } // UpdateRelease upgrades a tiller release -func (p *Proxy) UpdateRelease(name, values string, ch *chart.Chart) (*release.Release, error) { +func (p *Proxy) UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) { lock(name) defer unlock(name) // Check if the release already exists - _, err := p.getRelease(name) + _, err := p.get(name, namespace) if err != nil { return nil, err } @@ -256,18 +258,18 @@ func (p *Proxy) UpdateRelease(name, values string, ch *chart.Chart) (*release.Re } // GetRelease returns the info of a release -func (p *Proxy) GetRelease(name string) (*release.Release, error) { +func (p *Proxy) GetRelease(name, namespace string) (*release.Release, error) { lock(name) defer unlock(name) - return p.getRelease(name) + return p.get(name, namespace) } // DeleteRelease deletes a release -func (p *Proxy) DeleteRelease(name string, purge bool) error { +func (p *Proxy) DeleteRelease(name, namespace string, purge bool) error { lock(name) defer unlock(name) // Validate that the release actually belongs to the namespace - _, err := p.getRelease(name) + _, err := p.get(name, namespace) if err != nil { return err } @@ -295,11 +297,11 @@ func prettyError(err error) error { // TillerClient for exposed funcs type TillerClient interface { - GetReleaseStatus(name string) (release.Status_Code, error) + GetReleaseStatus(relName string) (release.Status_Code, error) ResolveManifest(namespace, values string, ch *chart.Chart) (string, error) ListReleases(namespace string, releaseListLimit int, status string) ([]AppOverview, error) CreateRelease(name, namespace, values string, ch *chart.Chart) (*release.Release, error) - UpdateRelease(name string, values string, ch *chart.Chart) (*release.Release, error) - GetRelease(name string) (*release.Release, error) - DeleteRelease(name string, purge bool) error + UpdateRelease(name, namespace string, values string, ch *chart.Chart) (*release.Release, error) + GetRelease(name, namespace string) (*release.Release, error) + DeleteRelease(name, namespace string, purge bool) error } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 59b08e45a0d..eed8d8e173a 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -250,7 +250,7 @@ func TestHelmReleaseUpdated(t *testing.T) { app := AppOverview{rs, version, ns, "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - result, err := proxy.UpdateRelease(rs, "", ch) + result, err := proxy.UpdateRelease(rs, ns, "", ch) if err != nil { t.Errorf("Unexpected error %v", err) } @@ -272,6 +272,7 @@ func TestHelmReleaseUpdated(t *testing.T) { } func TestUpdateMissingHelmRelease(t *testing.T) { + ns := "myns" rs := "foo" chartName := "bar" version := "v1.0.0" @@ -284,7 +285,7 @@ func TestUpdateMissingHelmRelease(t *testing.T) { app := AppOverview{rs2, version, ns2, "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - _, err := proxy.UpdateRelease(rs, "", ch) + _, err := proxy.UpdateRelease(rs, ns, "", ch) if err == nil { t.Error("Update should fail, there is not a release in the namespace specified") } @@ -297,20 +298,21 @@ func TestGetHelmRelease(t *testing.T) { app1 := AppOverview{"foo", "1.0.0", "my_ns", "icon.png", "DEPLOYED"} app2 := AppOverview{"bar", "1.0.0", "other_ns", "icon2.png", "DELETED"} type testStruct struct { - existingApps []AppOverview - shouldFail bool - targetApp string - expectedResult string + existingApps []AppOverview + shouldFail bool + targetApp string + tartegNamespace string + expectedResult string } tests := []testStruct{ - {[]AppOverview{app1, app2}, false, "foo", "foo"}, - {[]AppOverview{app1, app2}, false, "foo", "foo"}, + {[]AppOverview{app1, app2}, false, "foo", "my_ns", "foo"}, + {[]AppOverview{app1, app2}, false, "foo", "", "foo"}, } for _, test := range tests { proxy := newFakeProxy(test.existingApps) - res, err := proxy.GetRelease(test.targetApp) + res, err := proxy.GetRelease(test.targetApp, test.tartegNamespace) if test.shouldFail && err == nil { - t.Errorf("Get %s should fail", test.targetApp) + t.Errorf("Get %s/%s should fail", test.tartegNamespace, test.targetApp) } if !test.shouldFail { if err != nil { @@ -328,7 +330,7 @@ func TestHelmReleaseDeleted(t *testing.T) { proxy := newFakeProxy([]AppOverview{app}) // TODO: Add a test for a non-purged release when the fake helm cli supports it - err := proxy.DeleteRelease(app.ReleaseName, true) + err := proxy.DeleteRelease(app.ReleaseName, app.Namespace, true) if err != nil { t.Errorf("Unexpected error %v", err) } @@ -345,9 +347,9 @@ func TestDeleteMissingHelmRelease(t *testing.T) { app := AppOverview{"foo", "1.0.0", "my_ns", "icon.png", "DEPLOYED"} proxy := newFakeProxy([]AppOverview{app}) - err := proxy.DeleteRelease("not_foo", true) + err := proxy.DeleteRelease("not_foo", "other_ns", true) if err == nil { - t.Error("Delete should fail, there is not a release") + t.Error("Delete should fail, there is not a release in the namespace specified") } rels, err := proxy.helmClient.ListReleases() if err != nil { @@ -388,13 +390,13 @@ func TestEnsureThreadSafety(t *testing.T) { } }, func() { - _, err := proxy.UpdateRelease(rs, "", ch) + _, err := proxy.UpdateRelease(rs, ns, "", ch) if err != nil { t.Errorf("Unexpected error %v", err) } }, func() { - err := proxy.DeleteRelease(rs, true) + err := proxy.DeleteRelease(rs, ns, true) if err != nil { t.Errorf("Unexpected error %v", err) } From cd6ddf94359de0c3919be852977fa900ddec8bc2 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Date: Mon, 5 Nov 2018 17:03:56 -0800 Subject: [PATCH 3/3] Check namespace --- pkg/proxy/proxy.go | 18 ++++++++++++------ pkg/proxy/proxy_test.go | 8 +++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index 7a528c05c39..003dad9a695 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -78,15 +78,21 @@ type AppOverview struct { Status string `json:"status"` } -// TODO: Rename get for getRelease -// TODO: Remove namespace since release name is unique -func (p *Proxy) get(name, namespace string) (*release.Release, error) { +func (p *Proxy) getRelease(name, namespace string) (*release.Release, error) { release, err := p.helmClient.ReleaseContent(name) if err != nil { return nil, prettyError(err) } + // We check that the release found is from the provided namespace. + // If `namespace` is an empty string we do not do that check + // This check check is to prevent users of for example updating releases that might be + // in namespaces that they do not have access to. + if namespace != "" && release.Release.Namespace != namespace { + return nil, fmt.Errorf("Release %q not found in namespace %q", name, namespace) + } + return release.Release, nil } @@ -240,7 +246,7 @@ func (p *Proxy) UpdateRelease(name, namespace string, values string, ch *chart.C lock(name) defer unlock(name) // Check if the release already exists - _, err := p.get(name, namespace) + _, err := p.getRelease(name, namespace) if err != nil { return nil, err } @@ -261,7 +267,7 @@ func (p *Proxy) UpdateRelease(name, namespace string, values string, ch *chart.C func (p *Proxy) GetRelease(name, namespace string) (*release.Release, error) { lock(name) defer unlock(name) - return p.get(name, namespace) + return p.getRelease(name, namespace) } // DeleteRelease deletes a release @@ -269,7 +275,7 @@ func (p *Proxy) DeleteRelease(name, namespace string, purge bool) error { lock(name) defer unlock(name) // Validate that the release actually belongs to the namespace - _, err := p.get(name, namespace) + _, err := p.getRelease(name, namespace) if err != nil { return err } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index eed8d8e173a..2a69dc215cf 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -301,18 +301,20 @@ func TestGetHelmRelease(t *testing.T) { existingApps []AppOverview shouldFail bool targetApp string - tartegNamespace string + targetNamespace string expectedResult string } tests := []testStruct{ {[]AppOverview{app1, app2}, false, "foo", "my_ns", "foo"}, {[]AppOverview{app1, app2}, false, "foo", "", "foo"}, + // Can't retrieve release from another namespace + {[]AppOverview{app1, app2}, true, "foo", "other_ns", "foo"}, } for _, test := range tests { proxy := newFakeProxy(test.existingApps) - res, err := proxy.GetRelease(test.targetApp, test.tartegNamespace) + res, err := proxy.GetRelease(test.targetApp, test.targetNamespace) if test.shouldFail && err == nil { - t.Errorf("Get %s/%s should fail", test.tartegNamespace, test.targetApp) + t.Errorf("Get %s/%s should fail", test.targetNamespace, test.targetApp) } if !test.shouldFail { if err != nil {