diff --git a/pkg/api/install.go b/pkg/api/install.go index 5d465383a..c5e7802ef 100644 --- a/pkg/api/install.go +++ b/pkg/api/install.go @@ -16,7 +16,7 @@ type InstallRequest struct { type InstallResponse struct { Error string `json:"error,omitempty"` - Status string `json:"status"` + Status string `json:"status,omitempty"` } // RODO: we could use interface as well if everything's in same package @@ -34,9 +34,7 @@ func Install(svc Service) http.Handler { cfg := InstallConfig{ChartName: req.Chart, Name: req.Name, Namespace: req.Namespace} res, err := svc.Install(r.Context(), cfg, req.Values) if err != nil { - response.Error = err.Error() - logger.Errorf("[Install] error while installing chart: %v", err) - w.WriteHeader(http.StatusInternalServerError) + respondError(w, "error while installing chart: %v", err) return } response.Status = res.status @@ -47,3 +45,13 @@ func Install(svc Service) http.Handler { } }) } + +func respondError(w http.ResponseWriter, logprefix string, err error) { + response := InstallResponse{Error: err.Error()} + w.WriteHeader(http.StatusInternalServerError) + if err := json.NewEncoder(w).Encode(&response); err != nil { + logger.Errorf("[Install] %s %v", logprefix, err) + w.WriteHeader(http.StatusInternalServerError) + return + } +} diff --git a/pkg/api/install_api_test.go b/pkg/api/install_api_test.go index 72961e8c2..5c4190a68 100644 --- a/pkg/api/install_api_test.go +++ b/pkg/api/install_api_test.go @@ -1,6 +1,7 @@ package api_test import ( + "errors" "fmt" "io/ioutil" "net/http" @@ -62,7 +63,7 @@ func (s *InstallerTestSuite) TestShouldReturnDeployedStatusOnSuccessfulInstall() resp, err := http.DefaultClient.Do(req) - assert.Equal(s.T(), 200, resp.StatusCode) + assert.Equal(s.T(), http.StatusOK, resp.StatusCode) expectedResponse := `{"status":"deployed"}` + "\n" respBody, _ := ioutil.ReadAll(resp.Body) assert.Equal(s.T(), expectedResponse, string(respBody)) @@ -71,6 +72,26 @@ func (s *InstallerTestSuite) TestShouldReturnDeployedStatusOnSuccessfulInstall() s.mockChartLoader.AssertExpectations(s.T()) } +func (s *InstallerTestSuite) TestShouldReturnInternalServerErrorOnFailure() { + chartName := "stable/redis-ha" + body := fmt.Sprintf(`{ + "chart":"%s", + "name": "redis-v5", + "namespace": "something"}`, chartName) + req, _ := http.NewRequest("POST", fmt.Sprintf("%s/install", s.server.URL), strings.NewReader(body)) + s.mockChartLoader.On("LocateChart", chartName, s.appConfig).Return("./testdata/albatross", errors.New("Invalid chart")) + + resp, err := http.DefaultClient.Do(req) + + assert.Equal(s.T(), http.StatusInternalServerError, resp.StatusCode) + expectedResponse := `{"error":"error in locating chart: Invalid chart"}` + "\n" + respBody, _ := ioutil.ReadAll(resp.Body) + assert.Equal(s.T(), expectedResponse, string(respBody)) + require.NoError(s.T(), err) + s.mockInstaller.AssertExpectations(s.T()) + s.mockChartLoader.AssertExpectations(s.T()) +} + func (s *InstallerTestSuite) TearDownTest() { s.server.Close() } diff --git a/pkg/api/service.go b/pkg/api/service.go index 3b58b6cd8..5857f4c49 100644 --- a/pkg/api/service.go +++ b/pkg/api/service.go @@ -2,19 +2,20 @@ package api import ( "context" + "errors" "fmt" + "strings" "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/cli" - "helm.sh/helm/v3/pkg/release" _ "k8s.io/client-go/plugin/pkg/client/auth" ) type installer interface { SetConfig(InstallConfig) - Run(*chart.Chart, map[string]interface{}) (*release.Release, error) + runner } type chartloader interface { @@ -35,7 +36,7 @@ type InstallConfig struct { type chartValues map[string]interface{} -type installResult struct { +type InstallResult struct { status string } @@ -47,7 +48,10 @@ func (s Service) getValues(vals chartValues) (chartValues, error) { return vals, nil } -func (s Service) Install(ctx context.Context, cfg InstallConfig, values chartValues) (*installResult, error) { +func (s Service) Install(ctx context.Context, cfg InstallConfig, values chartValues) (*InstallResult, error) { + if err := s.validate(cfg, values); err != nil { + return nil, fmt.Errorf("error request validation: %v", err) + } chart, err := s.loadChart(cfg.ChartName) if err != nil { return nil, err @@ -72,19 +76,27 @@ func (s Service) loadChart(chartName string) (*chart.Chart, error) { return requestedChart, nil } -func (s Service) installChart(icfg InstallConfig, ch *chart.Chart, vals chartValues) (*installResult, error) { +func (s Service) installChart(icfg InstallConfig, ch *chart.Chart, vals chartValues) (*InstallResult, error) { s.installer.SetConfig(icfg) release, err := s.installer.Run(ch, vals) if err != nil { return nil, fmt.Errorf("error in installing chart: %v", err) } - result := new(installResult) + result := new(InstallResult) if release.Info != nil { result.status = release.Info.Status.String() } return result, nil } +func (s Service) validate(icfg InstallConfig, values chartValues) error { + if strings.HasPrefix(icfg.ChartName, ".") || + strings.HasPrefix(icfg.ChartName, "/") { + return errors.New("cannot refer local chart") + } + return nil +} + func NewService(settings *cli.EnvSettings, cl chartloader, i installer) Service { return Service{ settings: settings, diff --git a/pkg/api/service_test.go b/pkg/api/service_test.go index 728f94287..7094815a6 100644 --- a/pkg/api/service_test.go +++ b/pkg/api/service_test.go @@ -53,6 +53,25 @@ func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnInvalidChart() { s.installer.AssertNotCalled(t, "Run") } +func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnLocalChartReference() { + chartName := "./some/local-chart" + cfg := InstallConfig{ + Name: "some-component", + Namespace: "hermes", + ChartName: chartName, + } + var vals chartValues + + res, err := s.svc.Install(s.ctx, cfg, vals) + + t := s.T() + assert.Nil(t, res) + assert.EqualError(t, err, "error request validation: cannot refer local chart") + s.chartloader.AssertNotCalled(t, "LocateChart") + s.installer.AssertNotCalled(t, "SetConfig") + s.installer.AssertNotCalled(t, "Run") +} + func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnFailedIntallRun() { chartName := "stable/valid-chart" cfg := InstallConfig{ diff --git a/pkg/api/upgrade.go b/pkg/api/upgrade.go index fba5639f7..e50dcd41f 100644 --- a/pkg/api/upgrade.go +++ b/pkg/api/upgrade.go @@ -88,7 +88,7 @@ func (h UpgradeHandler) UpgradeRelease(releaseName, releaseNamespace, chartPath if _, err := history.Run(releaseName); err == driver.ErrReleaseNotFound { fmt.Printf("Release %q does not exist. Installing it now.\n", releaseName) - //TODO: yet to accomodate namespace and releasename, just refactoring + //TODO: yet to accommodate namespace and releasename, just refactoring icfg := InstallConfig{ Namespace: releaseNamespace, Name: releaseName, diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go index 3cf584cd0..e6ad71767 100644 --- a/pkg/cli/values/options.go +++ b/pkg/cli/values/options.go @@ -17,7 +17,6 @@ limitations under the License. package values import ( - "fmt" "io/ioutil" "net/url" "os" @@ -40,7 +39,6 @@ type Options struct { // MergeValues merges values from files specified via -f/--values and directly // via --set, --set-string, or --set-file, marshaling them to YAML func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, error) { - fmt.Println("%+v", opts) base := map[string]interface{}{} // User specified a values files via -f/--values