From db3fc03ba9830ab5b984884fa4b6d9b037ed09e9 Mon Sep 17 00:00:00 2001 From: Simon Alling Date: Tue, 26 Nov 2019 09:34:01 +0100 Subject: [PATCH] Move HTTP/handler utils to a separate package (#1311) * Move HTTP functions to pkg/handlerutil Moving forward, they will be needed to implement Helm 3 support (see #1309), and they were in an internal package, so they couldn't be imported outside the `cmd/tiller-proxy` directory. Because they were all pure (or, in the case of ServeHTTP, at least very simple), I felt confident moving them to another file and reusing them. * Move error code test to pkg/handlerutil https://github.com/kubeapps/kubeapps/pull/1311#discussion_r349671189 --- cmd/tiller-proxy/internal/handler/handler.go | 114 +++++------------- .../internal/handler/handler_test.go | 23 ---- cmd/tiller-proxy/main.go | 13 +- pkg/handlerutil/handlerutil.go | 62 ++++++++++ pkg/handlerutil/handlerutil_test.go | 29 +++++ 5 files changed, 128 insertions(+), 113 deletions(-) create mode 100644 pkg/handlerutil/handlerutil.go create mode 100644 pkg/handlerutil/handlerutil_test.go diff --git a/cmd/tiller-proxy/internal/handler/handler.go b/cmd/tiller-proxy/internal/handler/handler.go index 6b622401bb1..500d25ee35e 100644 --- a/cmd/tiller-proxy/internal/handler/handler.go +++ b/cmd/tiller-proxy/internal/handler/handler.go @@ -21,14 +21,13 @@ import ( "encoding/json" "io/ioutil" "net/http" - "regexp" "strconv" "strings" - "github.com/gorilla/mux" "github.com/kubeapps/common/response" "github.com/kubeapps/kubeapps/pkg/auth" chartUtils "github.com/kubeapps/kubeapps/pkg/chart" + "github.com/kubeapps/kubeapps/pkg/handlerutil" proxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" "github.com/urfave/negroni" @@ -64,59 +63,6 @@ func AuthGate() negroni.HandlerFunc { } } -// Params a key-value map of path params -type Params map[string]string - -// WithParams can be used to wrap handlers to take an extra arg for path params -type WithParams func(http.ResponseWriter, *http.Request, Params) - -func (h WithParams) ServeHTTP(w http.ResponseWriter, req *http.Request) { - vars := mux.Vars(req) - h(w, req, vars) -} - -// WithoutParams can be used to wrap handlers that doesn't take params -type WithoutParams func(http.ResponseWriter, *http.Request) - -func (h WithoutParams) ServeHTTP(w http.ResponseWriter, req *http.Request) { - h(w, req) -} - -func isNotFound(err error) bool { - return strings.Contains(err.Error(), "not found") -} - -func isAlreadyExists(err error) bool { - return strings.Contains(err.Error(), "is still in use") || strings.Contains(err.Error(), "already exists") -} - -func isForbidden(err error) bool { - return strings.Contains(err.Error(), "Unauthorized") -} - -func isUnprocessable(err error) bool { - re := regexp.MustCompile(`release.*failed`) - return re.MatchString(err.Error()) -} - -func errorCode(err error) int { - return errorCodeWithDefault(err, http.StatusInternalServerError) -} - -func errorCodeWithDefault(err error, defaultCode int) int { - errCode := defaultCode - if isAlreadyExists(err) { - errCode = http.StatusConflict - } else if isNotFound(err) { - errCode = http.StatusNotFound - } else if isForbidden(err) { - errCode = http.StatusForbidden - } else if isUnprocessable(err) { - errCode = http.StatusUnprocessableEntity - } - return errCode -} - func getChart(req *http.Request, cu chartUtils.Resolver) (*chartUtils.Details, *chart.Chart, error) { defer req.Body.Close() body, err := ioutil.ReadAll(req.Body) @@ -142,7 +88,7 @@ func returnForbiddenActions(forbiddenActions []auth.Action, w http.ResponseWrite w.Header().Set("Content-Type", "application/json") body, err := json.Marshal(forbiddenActions) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } response.NewErrorResponse(http.StatusForbidden, string(body)).Write(w) @@ -166,23 +112,23 @@ func (h *TillerProxy) logStatus(name string) { } // CreateRelease creates a new release in the namespace given as Param -func (h *TillerProxy) CreateRelease(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) CreateRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { log.Printf("Creating Helm Release") chartDetails, ch, err := getChart(req, h.ChartClient) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if !h.DisableAuth { manifest, err := h.ProxyClient.ResolveManifest(params["namespace"], chartDetails.Values, ch) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } userAuth := req.Context().Value(userKey).(auth.Checker) forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "create", manifest) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if len(forbiddenActions) > 0 { @@ -192,7 +138,7 @@ func (h *TillerProxy) CreateRelease(w http.ResponseWriter, req *http.Request, pa } rel, err := h.ProxyClient.CreateRelease(chartDetails.ReleaseName, params["namespace"], chartDetails.Values, ch) if err != nil { - response.NewErrorResponse(errorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) return } log.Printf("Installed release %s", rel.Name) @@ -201,7 +147,7 @@ func (h *TillerProxy) CreateRelease(w http.ResponseWriter, req *http.Request, pa } // OperateRelease decides which method to call depending in the "action" query param -func (h *TillerProxy) OperateRelease(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) OperateRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { switch req.FormValue("action") { case "upgrade": h.UpgradeRelease(w, req, params) @@ -214,7 +160,7 @@ func (h *TillerProxy) OperateRelease(w http.ResponseWriter, req *http.Request, p } // RollbackRelease performs an action over a release -func (h *TillerProxy) RollbackRelease(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) RollbackRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { log.Printf("Rolling back %s", params["releaseName"]) revision := req.FormValue("revision") if revision == "" { @@ -223,20 +169,20 @@ func (h *TillerProxy) RollbackRelease(w http.ResponseWriter, req *http.Request, } revisionInt, err := strconv.ParseInt(revision, 10, 64) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if !h.DisableAuth { manifest, err := h.ProxyClient.ResolveManifestFromRelease(params["releaseName"], int32(revisionInt)) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } userAuth := req.Context().Value(userKey).(auth.Checker) // Using "upgrade" action since the concept is the same forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "upgrade", manifest) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if len(forbiddenActions) > 0 { @@ -246,7 +192,7 @@ func (h *TillerProxy) RollbackRelease(w http.ResponseWriter, req *http.Request, } rel, err := h.ProxyClient.RollbackRelease(params["releaseName"], params["namespace"], int32(revisionInt)) if err != nil { - response.NewErrorResponse(errorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) return } log.Printf("Rollback release for %s to %d", rel.Name, revisionInt) @@ -255,23 +201,23 @@ func (h *TillerProxy) RollbackRelease(w http.ResponseWriter, req *http.Request, } // UpgradeRelease upgrades a release in the namespace given as Param -func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { log.Printf("Upgrading Helm Release") chartDetails, ch, err := getChart(req, h.ChartClient) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if !h.DisableAuth { manifest, err := h.ProxyClient.ResolveManifest(params["namespace"], chartDetails.Values, ch) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } userAuth := req.Context().Value(userKey).(auth.Checker) forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "upgrade", manifest) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if len(forbiddenActions) > 0 { @@ -281,7 +227,7 @@ func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, p } 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) + response.NewErrorResponse(handlerutil.ErrorCodeWithDefault(err, http.StatusUnprocessableEntity), err.Error()).Write(w) return } log.Printf("Upgraded release %s", rel.Name) @@ -293,39 +239,39 @@ func (h *TillerProxy) UpgradeRelease(w http.ResponseWriter, req *http.Request, p func (h *TillerProxy) ListAllReleases(w http.ResponseWriter, req *http.Request) { apps, err := h.ProxyClient.ListReleases("", h.ListLimit, req.URL.Query().Get("statuses")) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } response.NewDataResponse(apps).Write(w) } // ListReleases in the namespace given as Param -func (h *TillerProxy) ListReleases(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) ListReleases(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { apps, err := h.ProxyClient.ListReleases(params["namespace"], h.ListLimit, req.URL.Query().Get("statuses")) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } response.NewDataResponse(apps).Write(w) } // GetRelease returns the release info -func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, params Params) { +func (h *TillerProxy) GetRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if !h.DisableAuth { manifest, err := h.ProxyClient.ResolveManifest(params["namespace"], rel.Config.Raw, rel.Chart) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } userAuth := req.Context().Value(userKey).(auth.Checker) forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "get", manifest) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if len(forbiddenActions) > 0 { @@ -337,22 +283,22 @@ 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) { +func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, params handlerutil.Params) { if !h.DisableAuth { rel, err := h.ProxyClient.GetRelease(params["releaseName"], params["namespace"]) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } manifest, err := h.ProxyClient.ResolveManifest(params["namespace"], rel.Config.Raw, rel.Chart) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } userAuth := req.Context().Value(userKey).(auth.Checker) forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "delete", manifest) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } if len(forbiddenActions) > 0 { @@ -366,7 +312,7 @@ func (h *TillerProxy) DeleteRelease(w http.ResponseWriter, req *http.Request, pa } err := h.ProxyClient.DeleteRelease(params["releaseName"], params["namespace"], purge) if err != nil { - response.NewErrorResponse(errorCode(err), err.Error()).Write(w) + response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) return } w.Header().Set("Status-Code", "200") diff --git a/cmd/tiller-proxy/internal/handler/handler_test.go b/cmd/tiller-proxy/internal/handler/handler_test.go index 415fa91da90..1476a1c7b58 100644 --- a/cmd/tiller-proxy/internal/handler/handler_test.go +++ b/cmd/tiller-proxy/internal/handler/handler_test.go @@ -19,7 +19,6 @@ package handler import ( "context" "fmt" - "net/http" "net/http/httptest" "reflect" "strings" @@ -34,28 +33,6 @@ import ( proxyFake "github.com/kubeapps/kubeapps/pkg/proxy/fake" ) -func TestErrorCodeWithDefault(t *testing.T) { - type test struct { - err error - defaultCode int - expectedCode int - } - tests := []test{ - {fmt.Errorf("a release named foo already exists"), http.StatusInternalServerError, http.StatusConflict}, - {fmt.Errorf("release foo not found"), http.StatusInternalServerError, http.StatusNotFound}, - {fmt.Errorf("Unauthorized to get release foo"), http.StatusInternalServerError, http.StatusForbidden}, - {fmt.Errorf("release \"Foo \" failed"), http.StatusInternalServerError, http.StatusUnprocessableEntity}, - {fmt.Errorf("This is an unexpected error"), http.StatusInternalServerError, http.StatusInternalServerError}, - {fmt.Errorf("This is an unexpected error"), http.StatusUnprocessableEntity, http.StatusUnprocessableEntity}, - } - for _, s := range tests { - code := errorCodeWithDefault(s.err, s.defaultCode) - if code != s.expectedCode { - t.Errorf("Expected '%v' to return code %v got %v", s.err, s.expectedCode, code) - } - } -} - func TestActions(t *testing.T) { type testScenario struct { // Scenario params diff --git a/cmd/tiller-proxy/main.go b/cmd/tiller-proxy/main.go index fda54fc5a87..d38f078419c 100644 --- a/cmd/tiller-proxy/main.go +++ b/cmd/tiller-proxy/main.go @@ -32,6 +32,7 @@ import ( appRepo "github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/client/clientset/versioned" "github.com/kubeapps/kubeapps/cmd/tiller-proxy/internal/handler" chartUtils "github.com/kubeapps/kubeapps/pkg/chart" + "github.com/kubeapps/kubeapps/pkg/handlerutil" tillerProxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" "github.com/spf13/pflag" @@ -156,27 +157,27 @@ func main() { apiv1 := r.PathPrefix("/v1").Subrouter() apiv1.Methods("GET").Path("/releases").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithoutParams(h.ListAllReleases)), + negroni.Wrap(handlerutil.WithoutParams(h.ListAllReleases)), )) apiv1.Methods("GET").Path("/namespaces/{namespace}/releases").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithParams(h.ListReleases)), + negroni.Wrap(handlerutil.WithParams(h.ListReleases)), )) apiv1.Methods("POST").Path("/namespaces/{namespace}/releases").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithParams(h.CreateRelease)), + negroni.Wrap(handlerutil.WithParams(h.CreateRelease)), )) apiv1.Methods("GET").Path("/namespaces/{namespace}/releases/{releaseName}").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithParams(h.GetRelease)), + negroni.Wrap(handlerutil.WithParams(h.GetRelease)), )) apiv1.Methods("PUT").Path("/namespaces/{namespace}/releases/{releaseName}").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithParams(h.OperateRelease)), + negroni.Wrap(handlerutil.WithParams(h.OperateRelease)), )) apiv1.Methods("DELETE").Path("/namespaces/{namespace}/releases/{releaseName}").Handler(negroni.New( authGate, - negroni.Wrap(handler.WithParams(h.DeleteRelease)), + negroni.Wrap(handlerutil.WithParams(h.DeleteRelease)), )) // Chartsvc reverse proxy diff --git a/pkg/handlerutil/handlerutil.go b/pkg/handlerutil/handlerutil.go new file mode 100644 index 00000000000..579c69cab56 --- /dev/null +++ b/pkg/handlerutil/handlerutil.go @@ -0,0 +1,62 @@ +package handlerutil + +import ( + "net/http" + "regexp" + "strings" + + "github.com/gorilla/mux" +) + +// Params a key-value map of path params +type Params map[string]string + +// WithParams can be used to wrap handlers to take an extra arg for path params +type WithParams func(http.ResponseWriter, *http.Request, Params) + +func (h WithParams) ServeHTTP(w http.ResponseWriter, req *http.Request) { + vars := mux.Vars(req) + h(w, req, vars) +} + +// WithoutParams can be used to wrap handlers that doesn't take params +type WithoutParams func(http.ResponseWriter, *http.Request) + +func (h WithoutParams) ServeHTTP(w http.ResponseWriter, req *http.Request) { + h(w, req) +} + +func isNotFound(err error) bool { + return strings.Contains(err.Error(), "not found") +} + +func isAlreadyExists(err error) bool { + return strings.Contains(err.Error(), "is still in use") || strings.Contains(err.Error(), "already exists") +} + +func isForbidden(err error) bool { + return strings.Contains(err.Error(), "Unauthorized") +} + +func isUnprocessable(err error) bool { + re := regexp.MustCompile(`release.*failed`) + return re.MatchString(err.Error()) +} + +func ErrorCode(err error) int { + return ErrorCodeWithDefault(err, http.StatusInternalServerError) +} + +func ErrorCodeWithDefault(err error, defaultCode int) int { + errCode := defaultCode + if isAlreadyExists(err) { + errCode = http.StatusConflict + } else if isNotFound(err) { + errCode = http.StatusNotFound + } else if isForbidden(err) { + errCode = http.StatusForbidden + } else if isUnprocessable(err) { + errCode = http.StatusUnprocessableEntity + } + return errCode +} diff --git a/pkg/handlerutil/handlerutil_test.go b/pkg/handlerutil/handlerutil_test.go new file mode 100644 index 00000000000..291efd9bab5 --- /dev/null +++ b/pkg/handlerutil/handlerutil_test.go @@ -0,0 +1,29 @@ +package handlerutil + +import ( + "fmt" + "net/http" + "testing" +) + +func TestErrorCodeWithDefault(t *testing.T) { + type test struct { + err error + defaultCode int + expectedCode int + } + tests := []test{ + {fmt.Errorf("a release named foo already exists"), http.StatusInternalServerError, http.StatusConflict}, + {fmt.Errorf("release foo not found"), http.StatusInternalServerError, http.StatusNotFound}, + {fmt.Errorf("Unauthorized to get release foo"), http.StatusInternalServerError, http.StatusForbidden}, + {fmt.Errorf("release \"Foo \" failed"), http.StatusInternalServerError, http.StatusUnprocessableEntity}, + {fmt.Errorf("This is an unexpected error"), http.StatusInternalServerError, http.StatusInternalServerError}, + {fmt.Errorf("This is an unexpected error"), http.StatusUnprocessableEntity, http.StatusUnprocessableEntity}, + } + for _, s := range tests { + code := ErrorCodeWithDefault(s.err, s.defaultCode) + if code != s.expectedCode { + t.Errorf("Expected '%v' to return code %v got %v", s.err, s.expectedCode, code) + } + } +}