Bug 1803941: close helm request on error

pkg/helm/actions/config.go

@@ -14,8 +14,6 @@ import (
 var settings = initSettings()
 var plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm/actions")
 
-
-
 type configFlagsWithTransport struct {
 	*genericclioptions.ConfigFlags
 	Transport *http.RoundTripper

pkg/helm/handlers/handler_test.go

@@ -0,0 +1,189 @@
+package handlers
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/release"
+
+	"github.com/openshift/console/pkg/auth"
+)
+
+var fakeReleaseList = []*release.Release{
+	{
+		Name: "Test",
+	},
+}
+
+var fakeRelease = release.Release{
+	Name: "Test",
+}
+
+var fakeReleaseManifest = "manifest-data"
+
+func fakeHelmHandler() helmHandlers {
+	return helmHandlers{
+		getActionConfigurations: getFakeActionConfigurations,
+	}
+}
+
+func fakeInstallChart(mockedRelease *release.Release, err error) func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration) (*release.Release, error) {
+	return func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration) (r *release.Release, er error) {
+		return mockedRelease, err
+	}
+}
+
+func fakeListReleases(mockedReleases []*release.Release, err error) func(conf *action.Configuration) ([]*release.Release, error) {
+	return func(conf *action.Configuration) (releases []*release.Release, er error) {
+		return mockedReleases, err
+	}
+}
+
+func fakeGetManifest(mockedManifest string, err error) func(name string, url string, values map[string]interface{}, conf *action.Configuration) (string, error) {
+	return func(name string, url string, values map[string]interface{}, conf *action.Configuration) (r string, er error) {
+		return mockedManifest, err
+	}
+}
+
+func getFakeActionConfigurations(string, string, string, *http.RoundTripper) *action.Configuration {
+	return &action.Configuration{}
+}
+
+func TestHelmHandlers_HandleHelmList(t *testing.T) {
+	tests := []struct {
+		name             string
+		expectedResponse string
+		releaseList      []*release.Release
+		error
+		httpStatusCode int
+	}{
+		{
+			name:             "Error occurred at listing releases",
+			error:            errors.New("unknown error occurred"),
+			httpStatusCode:   http.StatusBadGateway,
+			expectedResponse: `{"error":"Failed to list helm releases: unknown error occurred"}`,
+		},
+		{
+			name:             "Return releases serialized in JSON format",
+			expectedResponse: `[{"name":"Test"}]`,
+			releaseList:      fakeReleaseList,
+			httpStatusCode:   http.StatusOK,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			handlers := fakeHelmHandler()
+			handlers.listReleases = fakeListReleases(tt.releaseList, tt.error)
+
+			request := httptest.NewRequest("", "/foo", strings.NewReader("{}"))
+			response := httptest.NewRecorder()
+
+			handlers.HandleHelmList(&auth.User{}, response, request)
+			if response.Code != tt.httpStatusCode {
+				t.Errorf("response code should be %v but got %v", tt.httpStatusCode, response.Code)
+			}
+			if response.Header().Get("Content-Type") != "application/json" {
+				t.Errorf("content type should be application/json but got %s", response.Header().Get("Content-Type"))
+			}
+			if response.Body.String() != tt.expectedResponse {
+				t.Errorf("response body not matching expected is %s and received is %s", tt.expectedResponse, response.Body.String())
+			}
+
+		})
+	}
+}
+
+func TestHelmHandlers_HandleHelmInstall(t *testing.T) {
+	tests := []struct {
+		name             string
+		expectedResponse string
+		installedRelease release.Release
+		error
+		httpStatusCode int
+	}{
+		{
+			name:             "Error occurred",
+			expectedResponse: `{"error":"Failed to install helm chart: Chart path is invalid"}`,
+			error:            errors.New("Chart path is invalid"),
+			httpStatusCode:   http.StatusBadGateway,
+		},
+		{
+			name:             "Successful install returns release info in JSON format",
+			installedRelease: fakeRelease,
+			httpStatusCode:   http.StatusOK,
+			expectedResponse: `{"name":"Test"}`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			handlers := fakeHelmHandler()
+			handlers.installChart = fakeInstallChart(&tt.installedRelease, tt.error)
+
+			request := httptest.NewRequest("", "/foo", strings.NewReader("{}"))
+			response := httptest.NewRecorder()
+
+			handlers.HandleHelmInstall(&auth.User{}, response, request)
+			if response.Code != tt.httpStatusCode {
+				t.Errorf("response code should be %v but got %v", tt.httpStatusCode, response.Code)
+			}
+			if response.Header().Get("Content-Type") != "application/json" {
+				t.Errorf("content type should be application/json but got %s", response.Header().Get("Content-Type"))
+			}
+			if response.Body.String() != tt.expectedResponse {
+				t.Errorf("response body not matching expected is %s and received is %s", tt.expectedResponse, response.Body.String())
+			}
+		})
+	}
+}
+
+func TestHelmHandlers_HandleHelmRenderManifest(t *testing.T) {
+	tests := []struct {
+		name                string
+		expectedResponse    string
+		expectedManifest    string
+		expectedContentType string
+		error
+		httpStatusCode int
+	}{
+		{
+			name:                "Error occurred",
+			error:               errors.New("Chart path is invalid"),
+			expectedResponse:    `{"error":"Failed to render manifests: Chart path is invalid"}`,
+			httpStatusCode:      http.StatusBadGateway,
+			expectedContentType: "application/json",
+		},
+		{
+			name:                "Return manifest in yaml format",
+			expectedResponse:    fakeReleaseManifest,
+			expectedManifest:    fakeReleaseManifest,
+			httpStatusCode:      http.StatusOK,
+			expectedContentType: "text/yaml",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			handlers := fakeHelmHandler()
+			handlers.renderManifests = fakeGetManifest(tt.expectedManifest, tt.error)
+
+			request := httptest.NewRequest("", "/foo", strings.NewReader("{}"))
+			response := httptest.NewRecorder()
+
+			handlers.HandleHelmRenderManifests(&auth.User{}, response, request)
+
+			if response.Code != tt.httpStatusCode {
+				t.Errorf("response code should be %v but got %v", tt.httpStatusCode, response.Code)
+			}
+			if response.Header().Get("Content-Type") != tt.expectedContentType {
+				t.Errorf("content type should be %s but got %s", tt.expectedContentType, response.Header().Get("Content-Type"))
+			}
+			if response.Body.String() != tt.expectedResponse {
+				t.Errorf("response body not matching expected is %s and received is %s", tt.expectedResponse, response.Body.String())
+			}
+
+		})
+	}
+}

pkg/helm/handlers/handlers.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 
 	"github.com/coreos/pkg/capnslog"
+	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/release"
 
 	"github.com/openshift/console/pkg/auth"
 	"github.com/openshift/console/pkg/helm/actions"
@@ -16,13 +18,32 @@ var (
 	plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm")
 )
 
-// HelmHandlers provides handlers to handle helm related requests
-type HelmHandlers struct {
+func New(apiUrl string, transport http.RoundTripper) *helmHandlers {
+	return &helmHandlers{
+		ApiServerHost:           apiUrl,
+		Transport:               transport,
+		getActionConfigurations: actions.GetActionConfigurations,
+		renderManifests:         actions.RenderManifests,
+		installChart:            actions.InstallChart,
+		listReleases:            actions.ListReleases,
+	}
+}
+
+// helmHandlers provides handlers to handle helm related requests
+type helmHandlers struct {
 	ApiServerHost string
 	Transport     http.RoundTripper
+
+	// helm action configurator
+	getActionConfigurations func(string, string, string, *http.RoundTripper) *action.Configuration
+
+	// helm actions
+	renderManifests func(string, string, map[string]interface{}, *action.Configuration) (string, error)
+	installChart    func(string, string, string, map[string]interface{}, *action.Configuration) (*release.Release, error)
+	listReleases    func(*action.Configuration) ([]*release.Release, error)
 }
 
-func (h *HelmHandlers) HandleHelmRenderManifests(user *auth.User, w http.ResponseWriter, r *http.Request) {
+func (h *helmHandlers) HandleHelmRenderManifests(user *auth.User, w http.ResponseWriter, r *http.Request) {
 	var req HelmRequest
 
 	err := json.NewDecoder(r.Body).Decode(&req)
@@ -31,44 +52,47 @@ func (h *HelmHandlers) HandleHelmRenderManifests(user *auth.User, w http.Respons
 		return
 	}
 
-	conf := actions.GetActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
-	resp, err := actions.RenderManifests(req.Name, req.ChartUrl, req.Values, conf)
+	conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
+	resp, err := h.renderManifests(req.Name, req.ChartUrl, req.Values, conf)
 	if err != nil {
-		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{fmt.Sprintf("Failed to render manifests: %v", err)})
+		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to render manifests: %v", err)})
+		return
 	}
 
 	w.Header().Set("Content-Type", "text/yaml")
 	w.Write([]byte(resp))
 }
 
-func (h *HelmHandlers) HandleHelmInstall(user *auth.User, w http.ResponseWriter, r *http.Request) {
+func (h *helmHandlers) HandleHelmInstall(user *auth.User, w http.ResponseWriter, r *http.Request) {
 	var req HelmRequest
 
 	err := json.NewDecoder(r.Body).Decode(&req)
 	if err != nil {
-		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{fmt.Sprintf("Failed to parse request: %v", err)})
+		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to parse request: %v", err)})
 		return
 	}
 
-	conf := actions.GetActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
-	resp, err := actions.InstallChart(req.Namespace, req.Name, req.ChartUrl, req.Values, conf)
+	conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
+	resp, err := h.installChart(req.Namespace, req.Name, req.ChartUrl, req.Values, conf)
 	if err != nil {
-		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{fmt.Sprintf("Failed to install helm chart: %v", err)})
+		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to install helm chart: %v", err)})
+		return
 	}
 
 	w.Header().Set("Content-Type", "application/json")
 	res, _ := json.Marshal(resp)
 	w.Write(res)
 }
 
-func (h *HelmHandlers) HandleHelmList(user *auth.User, w http.ResponseWriter, r *http.Request) {
+func (h *helmHandlers) HandleHelmList(user *auth.User, w http.ResponseWriter, r *http.Request) {
 	params := r.URL.Query()
 	ns := params.Get("ns")
 
-	conf := actions.GetActionConfigurations(h.ApiServerHost, ns, user.Token, &h.Transport)
-	resp, err := actions.ListReleases(conf)
+	conf := h.getActionConfigurations(h.ApiServerHost, ns, user.Token, &h.Transport)
+	resp, err := h.listReleases(conf)
 	if err != nil {
-		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{fmt.Sprintf("Failed to list helm releases: %v", err)})
+		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to list helm releases: %v", err)})
+		return
 	}
 
 	w.Header().Set("Content-Type", "application/json")

pkg/server/server.go

@@ -14,7 +14,7 @@ import (
 	"github.com/coreos/pkg/health"
 
 	"github.com/openshift/console/pkg/auth"
-	"github.com/openshift/console/pkg/helm/handlers"
+	helmhandlerspkg "github.com/openshift/console/pkg/helm/handlers"
 	"github.com/openshift/console/pkg/proxy"
 	"github.com/openshift/console/pkg/serverutils"
 	"github.com/openshift/console/pkg/version"
@@ -301,13 +301,10 @@ func (s *Server) HTTPHandler() http.Handler {
 	handle("/api/console/version", authHandler(s.versionHandler))
 
 	// Helm Endpoints
-	helmConfig := &handlers.HelmHandlers{
-		ApiServerHost: s.KubeAPIServerURL,
-		Transport:     s.K8sClient.Transport,
-	}
-	handle("/api/helm/template", authHandlerWithUser(helmConfig.HandleHelmRenderManifests))
-	handle("/api/helm/releases", authHandlerWithUser(helmConfig.HandleHelmList))
-	handle("/api/helm/release", authHandlerWithUser(helmConfig.HandleHelmInstall))
+	helmHandlers := helmhandlerspkg.New(s.KubeAPIServerURL, s.K8sClient.Transport)
+	handle("/api/helm/template", authHandlerWithUser(helmHandlers.HandleHelmRenderManifests))
+	handle("/api/helm/releases", authHandlerWithUser(helmHandlers.HandleHelmList))
+	handle("/api/helm/release", authHandlerWithUser(helmHandlers.HandleHelmInstall))
 
 	helmChartRepoProxy := proxy.NewProxy(s.HelmChartRepoProxyConfig)
 

test-backend.sh

@@ -20,7 +20,7 @@ export GOFLAGS="-mod=vendor"
 # Invoke ./cover for HTML output
 COVER=${COVER:-"-cover"}
 
-TESTABLE="pkg/auth pkg/proxy pkg/server"
+TESTABLE="pkg/auth pkg/proxy pkg/server pkg/helm/actions pkg/helm/handlers"
 FORMATTABLE="${TESTABLE} cmd/bridge pkg/version"
 
 # user has not provided PKG override