From 673ca25cf2ecdcb385fba99f2b769a6927519dae Mon Sep 17 00:00:00 2001 From: Sanket Sudake Date: Mon, 14 Jun 2021 14:48:27 +0530 Subject: [PATCH] Support for multiple HTTP verbs in routes/HTTPTrigger (#2064) * Support for multiple HTTP verbs in routes/HTTPTrigger Signed-off-by: Sanket Sudake * Update pkg/apis/core/v1/types.go Co-authored-by: Harsh Thakur * Fix fallbackurl for ingress Signed-off-by: Sanket Sudake Co-authored-by: Harsh Thakur --- cmd/builder/Dockerfile.fission-builder | 2 +- crds/v1/fission.io_httptriggers.yaml | 7 ++++++- pkg/apis/core/v1/types.go | 5 +++++ pkg/apis/core/v1/validation.go | 21 ++++++++++++++----- pkg/apis/core/v1/zz_generated.deepcopy.go | 5 +++++ pkg/canaryconfigmgr/canaryConfigMgr.go | 16 +++++++++++++-- pkg/canaryconfigmgr/prometheusClient.go | 23 +++++++++++++-------- pkg/controller/api_test.go | 4 ++-- pkg/controller/httpTriggerApi.go | 14 ++++++++++++- pkg/fission-cli/cmd/function/create.go | 18 +++++++++++----- pkg/fission-cli/cmd/function/test.go | 12 ++++++++++- pkg/fission-cli/cmd/httptrigger/create.go | 25 +++++++++++++++++------ pkg/fission-cli/cmd/httptrigger/get.go | 9 +++++++- pkg/fission-cli/cmd/httptrigger/update.go | 11 ++++++++-- pkg/fission-cli/cmd/spec/list.go | 10 ++++++++- pkg/fission-cli/cmd/spec/spec.go | 3 +++ pkg/fission-cli/flag/flag.go | 2 +- pkg/router/httpTriggers.go | 18 ++++++++++++++-- 18 files changed, 166 insertions(+), 39 deletions(-) diff --git a/cmd/builder/Dockerfile.fission-builder b/cmd/builder/Dockerfile.fission-builder index 9eeeb830..19b5fb9e 100644 --- a/cmd/builder/Dockerfile.fission-builder +++ b/cmd/builder/Dockerfile.fission-builder @@ -1,4 +1,4 @@ -FROM golang:1.14-alpine as godep +FROM golang:1.15-alpine as godep RUN apk add bash ca-certificates git gcc g++ libc-dev ARG GOPKG=github.com/fission/fission diff --git a/crds/v1/fission.io_httptriggers.yaml b/crds/v1/fission.io_httptriggers.yaml index 568dd275..69d3b59b 100644 --- a/crds/v1/fission.io_httptriggers.yaml +++ b/crds/v1/fission.io_httptriggers.yaml @@ -77,8 +77,13 @@ spec: type: string type: object method: - description: HTTP method to access a function. + description: 'Deprecated: Use Methods instead of Method. HTTP method to access a function.' type: string + methods: + description: HTTP methods to access a function + items: + type: string + type: array prefix: description: 'Prefix with which functions are exposed. NOTE: Prefix takes precedence over URL/RelativeURL. Note that it does not treat slashes specially ("/foobar/" will be matched by the prefix "/foobar").' type: string diff --git a/pkg/apis/core/v1/types.go b/pkg/apis/core/v1/types.go index cd7b5dcb..a02530a9 100644 --- a/pkg/apis/core/v1/types.go +++ b/pkg/apis/core/v1/types.go @@ -658,10 +658,15 @@ type ( // +optional Prefix *string `json:"prefix,omitempty"` + // Deprecated: Use Methods instead of Method. // HTTP method to access a function. // +optional Method string `json:"method"` + // HTTP methods to access a function + // +optional + Methods []string `json:"methods,omitempty"` + // FunctionReference is a reference to the target function. FunctionReference FunctionReference `json:"functionref"` diff --git a/pkg/apis/core/v1/validation.go b/pkg/apis/core/v1/validation.go index ef4e5db1..0ca49f32 100644 --- a/pkg/apis/core/v1/validation.go +++ b/pkg/apis/core/v1/validation.go @@ -400,12 +400,23 @@ func (spec EnvironmentSpec) Validate() error { func (spec HTTPTriggerSpec) Validate() error { result := &multierror.Error{} + checkMethod := func(method string, result *multierror.Error) *multierror.Error { + switch method { + case http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut, http.MethodPatch, + http.MethodDelete, http.MethodConnect, http.MethodOptions, http.MethodTrace: // no op + default: + result = multierror.Append(result, MakeValidationErr(ErrorUnsupportedType, "HTTPTriggerSpec.Method", spec.Method, "not a valid HTTP method")) + } + return result + } + if len(spec.Methods) > 0 { + for _, method := range spec.Methods { + result = checkMethod(method, result) + } + } - switch spec.Method { - case http.MethodGet, http.MethodHead, http.MethodPost, http.MethodPut, http.MethodPatch, - http.MethodDelete, http.MethodConnect, http.MethodOptions, http.MethodTrace: // no op - default: - result = multierror.Append(result, MakeValidationErr(ErrorUnsupportedType, "HTTPTriggerSpec.Method", spec.Method, "not a valid HTTP method")) + if len(spec.Method) > 0 { + result = checkMethod(spec.Method, result) } result = multierror.Append(result, spec.FunctionReference.Validate()) diff --git a/pkg/apis/core/v1/zz_generated.deepcopy.go b/pkg/apis/core/v1/zz_generated.deepcopy.go index b700a329..abdeb765 100644 --- a/pkg/apis/core/v1/zz_generated.deepcopy.go +++ b/pkg/apis/core/v1/zz_generated.deepcopy.go @@ -512,6 +512,11 @@ func (in *HTTPTriggerSpec) DeepCopyInto(out *HTTPTriggerSpec) { *out = new(string) **out = **in } + if in.Methods != nil { + in, out := &in.Methods, &out.Methods + *out = make([]string, len(*in)) + copy(*out, *in) + } in.FunctionReference.DeepCopyInto(&out.FunctionReference) in.IngressConfig.DeepCopyInto(&out.IngressConfig) return diff --git a/pkg/canaryconfigmgr/canaryConfigMgr.go b/pkg/canaryconfigmgr/canaryConfigMgr.go index 491c9b12..68c46522 100644 --- a/pkg/canaryconfigmgr/canaryConfigMgr.go +++ b/pkg/canaryconfigmgr/canaryConfigMgr.go @@ -282,9 +282,21 @@ func (canaryCfgMgr *canaryConfigMgr) RollForwardOrBack(canaryConfig *fv1.CanaryC } else { urlPath = triggerObj.Spec.RelativeURL } - failurePercent, err := canaryCfgMgr.promClient.GetFunctionFailurePercentage(urlPath, triggerObj.Spec.Method, + methods := triggerObj.Spec.Methods + if len(triggerObj.Spec.Method) > 0 { + present := false + for _, m := range triggerObj.Spec.Methods { + if m == triggerObj.Spec.Method { + present = true + break + } + } + if !present { + methods = append(methods, triggerObj.Spec.Method) + } + } + failurePercent, err := canaryCfgMgr.promClient.GetFunctionFailurePercentage(urlPath, methods, canaryConfig.Spec.NewFunction, canaryConfig.ObjectMeta.Namespace, canaryConfig.Spec.WeightIncrementDuration) - if err != nil { // silently ignore. wait for next window to increment weight canaryCfgMgr.logger.Error("error calculating failure percentage", diff --git a/pkg/canaryconfigmgr/prometheusClient.go b/pkg/canaryconfigmgr/prometheusClient.go index 146eff72..818f4e62 100644 --- a/pkg/canaryconfigmgr/prometheusClient.go +++ b/pkg/canaryconfigmgr/prometheusClient.go @@ -51,21 +51,28 @@ func MakePrometheusClient(logger *zap.Logger, prometheusSvc string) (*Prometheus }, nil } -func (promApiClient *PrometheusApiClient) GetFunctionFailurePercentage(path, method, funcName, funcNs string, window string) (float64, error) { +func (promApiClient *PrometheusApiClient) GetFunctionFailurePercentage(path string, methods []string, funcName, funcNs string, window string) (float64, error) { + var reqs, failedReqs float64 // first get a total count of requests to this url in a time window - reqs, err := promApiClient.GetRequestsToFuncInWindow(path, method, funcName, funcNs, window) - if err != nil { - return 0, err + for _, method := range methods { + mreqs, err := promApiClient.GetRequestsToFuncInWindow(path, method, funcName, funcNs, window) + if err != nil { + return 0, err + } + reqs += mreqs } if reqs <= 0 { - return -1, fmt.Errorf("no requests to this url %v and method %v in the window: %v", path, method, window) + return -1, fmt.Errorf("no requests to this url %v and method %v in the window: %v", path, methods, window) } // next, get a total count of errored out requests to this function in the same window - failedReqs, err := promApiClient.GetTotalFailedRequestsToFuncInWindow(funcName, funcNs, path, method, window) - if err != nil { - return 0, err + for _, method := range methods { + mfailedReqs, err := promApiClient.GetTotalFailedRequestsToFuncInWindow(funcName, funcNs, path, method, window) + if err != nil { + return 0, err + } + failedReqs += mfailedReqs } // calculate the failure percentage of the function diff --git a/pkg/controller/api_test.go b/pkg/controller/api_test.go index 12b0c934..beb7ce6a 100644 --- a/pkg/controller/api_test.go +++ b/pkg/controller/api_test.go @@ -158,7 +158,7 @@ func TestHTTPTriggerApi(t *testing.T) { Namespace: testNS, }, Spec: fv1.HTTPTriggerSpec{ - Method: http.MethodGet, + Methods: []string{http.MethodGet}, RelativeURL: "/hello", FunctionReference: fv1.FunctionReference{ Type: fv1.FunctionReferenceTypeFunctionName, @@ -181,7 +181,7 @@ func TestHTTPTriggerApi(t *testing.T) { tr, err := g.Client().V1().HTTPTrigger().Get(m) panicIf(err) - assert(testTrigger.Spec.Method == tr.Spec.Method && + assert(len(testTrigger.Spec.Methods) == len(tr.Spec.Methods) && testTrigger.Spec.RelativeURL == tr.Spec.RelativeURL && testTrigger.Spec.FunctionReference.Type == tr.Spec.FunctionReference.Type && testTrigger.Spec.FunctionReference.Name == tr.Spec.FunctionReference.Name, "trigger should match after reading") diff --git a/pkg/controller/httpTriggerApi.go b/pkg/controller/httpTriggerApi.go index 395d8b38..0a3174b4 100644 --- a/pkg/controller/httpTriggerApi.go +++ b/pkg/controller/httpTriggerApi.go @@ -22,6 +22,7 @@ import ( "fmt" "io/ioutil" "net/http" + "sort" "github.com/emicklei/go-restful" restfulspec "github.com/emicklei/go-restful-openapi" @@ -136,7 +137,18 @@ func (a *API) checkHTTPTriggerDuplicates(t *fv1.HTTPTrigger) error { if ht.Spec.RelativeURL == t.Spec.RelativeURL || (ht.Spec.Prefix != nil && t.Spec.Prefix != nil && *ht.Spec.Prefix != "" && *ht.Spec.Prefix == *t.Spec.Prefix) { urlMatch = true } - if urlMatch && ht.Spec.Method == t.Spec.Method && ht.Spec.Host == t.Spec.Host { + methodMatch := false + if ht.Spec.Method == t.Spec.Method && len(ht.Spec.Methods) == len(t.Spec.Methods) { + methodMatch = true + sort.Strings(ht.Spec.Methods) + sort.Strings(t.Spec.Methods) + for i, m1 := range ht.Spec.Methods { + if m1 != t.Spec.Methods[i] { + methodMatch = false + } + } + } + if urlMatch && methodMatch && ht.Spec.Method == t.Spec.Method && ht.Spec.Host == t.Spec.Host { return ferror.MakeError(ferror.ErrorNameExists, fmt.Sprintf("HTTPTrigger with same Host, URL & method already exists (%v)", ht.ObjectMeta.Name)) diff --git a/pkg/fission-cli/cmd/function/create.go b/pkg/fission-cli/cmd/function/create.go index 8528675c..7fbd2e73 100644 --- a/pkg/fission-cli/cmd/function/create.go +++ b/pkg/fission-cli/cmd/function/create.go @@ -345,9 +345,17 @@ func (opts *CreateSubCommand) run(input cli.Input) error { if len(prefix) != 0 && len(triggerUrl) > 0 { console.Warn("Prefix will take precedence over URL/RelativeURL") } - method, err := httptrigger.GetMethod(input.String(flagkey.HtMethod)) - if err != nil { - return errors.Wrap(err, "error getting HTTP trigger method") + + methods := input.StringSlice(flagkey.HtMethod) + if len(methods) == 0 { + return errors.New("HTTP methods not mentioned") + } + + for _, method := range methods { + _, err := httptrigger.GetMethod(method) + if err != nil { + return err + } } triggerName := uuid.NewV4().String() @@ -359,7 +367,7 @@ func (opts *CreateSubCommand) run(input cli.Input) error { Spec: fv1.HTTPTriggerSpec{ RelativeURL: triggerUrl, Prefix: &prefix, - Method: method, + Methods: methods, FunctionReference: fv1.FunctionReference{ Type: fv1.FunctionReferenceTypeFunctionName, Name: opts.function.ObjectMeta.Name, @@ -371,7 +379,7 @@ func (opts *CreateSubCommand) run(input cli.Input) error { return errors.Wrap(err, "error creating HTTP trigger") } - fmt.Printf("route created: %v %v -> %v\n", method, triggerUrl, opts.function.ObjectMeta.Name) + fmt.Printf("route created: %v %v -> %v\n", methods, triggerUrl, opts.function.ObjectMeta.Name) return nil } diff --git a/pkg/fission-cli/cmd/function/test.go b/pkg/fission-cli/cmd/function/test.go index ab24a427..0122140d 100644 --- a/pkg/fission-cli/cmd/function/test.go +++ b/pkg/fission-cli/cmd/function/test.go @@ -109,9 +109,19 @@ func (opts *TestSubCommand) do(input cli.Input) error { defer closeCtx() } + methods := input.StringSlice(flagkey.HtMethod) + if len(methods) == 0 { + return errors.New("HTTP method not mentioned") + } else if len(methods) > 1 { + return errors.New("More than one HTTP method not supported") + } + method, err := httptrigger.GetMethod(methods[0]) + if err != nil { + return err + } resp, err := doHTTPRequest(ctx, functionUrl.String(), input.StringSlice(flagkey.FnTestHeader), - input.String(flagkey.HtMethod), + method, input.String(flagkey.FnTestBody)) if err != nil { return err diff --git a/pkg/fission-cli/cmd/httptrigger/create.go b/pkg/fission-cli/cmd/httptrigger/create.go index 43119b7f..4b0b07bb 100644 --- a/pkg/fission-cli/cmd/httptrigger/create.go +++ b/pkg/fission-cli/cmd/httptrigger/create.go @@ -89,6 +89,7 @@ func (opts *CreateSubCommand) complete(input cli.Input) error { triggerUrl := input.String(flagkey.HtUrl) prefix := input.String(flagkey.HtPrefix) + fallbackURL := "" if triggerUrl == "" && prefix == "" { console.Error("You need to supply either Prefix or URL/RelativeURL") @@ -108,10 +109,22 @@ func (opts *CreateSubCommand) complete(input cli.Input) error { if prefix != "" && !strings.HasPrefix(prefix, "/") { prefix = "/" + prefix } + if prefix != "" { + fallbackURL = prefix + } else { + fallbackURL = triggerUrl + } - method, err := GetMethod(input.String(flagkey.HtMethod)) - if err != nil { - return err + methods := input.StringSlice(flagkey.HtMethod) + if len(methods) == 0 { + return errors.New("HTTP methods not mentioned") + } + + for _, method := range methods { + _, err := GetMethod(method) + if err != nil { + return err + } } // For Specs, the spec validate checks for function reference @@ -146,7 +159,7 @@ func (opts *CreateSubCommand) complete(input cli.Input) error { createIngress := input.Bool(flagkey.HtIngress) ingressConfig, err := GetIngressConfig( input.StringSlice(flagkey.HtIngressAnnotation), input.String(flagkey.HtIngressRule), - input.String(flagkey.HtIngressTLS), triggerUrl, nil) + input.String(flagkey.HtIngressTLS), fallbackURL, nil) if err != nil { return errors.Wrap(err, "error parsing ingress configuration") } @@ -161,7 +174,7 @@ func (opts *CreateSubCommand) complete(input cli.Input) error { Spec: fv1.HTTPTriggerSpec{ Host: host, RelativeURL: triggerUrl, - Method: method, + Methods: methods, FunctionReference: *functionRef, CreateIngress: createIngress, IngressConfig: *ingressConfig, @@ -220,7 +233,7 @@ func GetMethod(method string) (string, error) { case http.MethodTrace: return http.MethodTrace, nil default: - return "", fmt.Errorf("invalid or unsupported HTTP Method %v", method) + return "", fmt.Errorf("invalid or unsupported HTTP Method '%v'", method) } } diff --git a/pkg/fission-cli/cmd/httptrigger/get.go b/pkg/fission-cli/cmd/httptrigger/get.go index 22791251..1b5f87d7 100644 --- a/pkg/fission-cli/cmd/httptrigger/get.go +++ b/pkg/fission-cli/cmd/httptrigger/get.go @@ -86,8 +86,15 @@ func printHtSummary(triggers []fv1.HTTPTrigger) { } ann := strings.Join(msg, ", ") + methods := []string{} + if len(trigger.Spec.Method) > 0 { + methods = append(methods, trigger.Spec.Method) + } + if len(trigger.Spec.Methods) > 0 { + methods = trigger.Spec.Methods + } fmt.Fprintf(w, "%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v\n", - trigger.ObjectMeta.Name, trigger.Spec.Method, trigger.Spec.RelativeURL, function, trigger.Spec.CreateIngress, host, path, trigger.Spec.IngressConfig.TLS, ann) + trigger.ObjectMeta.Name, methods, trigger.Spec.RelativeURL, function, trigger.Spec.CreateIngress, host, path, trigger.Spec.IngressConfig.TLS, ann) } w.Flush() } diff --git a/pkg/fission-cli/cmd/httptrigger/update.go b/pkg/fission-cli/cmd/httptrigger/update.go index 29041750..d8009753 100644 --- a/pkg/fission-cli/cmd/httptrigger/update.go +++ b/pkg/fission-cli/cmd/httptrigger/update.go @@ -80,8 +80,15 @@ func (opts *UpdateSubCommand) complete(input cli.Input) error { ht.Spec.RelativeURL = triggerUrl ht.Spec.Prefix = &prefix - if input.IsSet(flagkey.HtMethod) { - ht.Spec.Method = input.String(flagkey.HtMethod) + methods := input.StringSlice(flagkey.HtMethod) + if len(methods) > 0 { + for _, method := range methods { + _, err := GetMethod(method) + if err != nil { + return err + } + } + ht.Spec.Methods = methods } if input.IsSet(flagkey.HtFnName) { diff --git a/pkg/fission-cli/cmd/spec/list.go b/pkg/fission-cli/cmd/spec/list.go index a3471ad1..d5c44caf 100644 --- a/pkg/fission-cli/cmd/spec/list.go +++ b/pkg/fission-cli/cmd/spec/list.go @@ -329,8 +329,16 @@ func ShowHTTPTriggers(hts []fv1.HTTPTrigger) { } ann := strings.Join(msg, ", ") + methods := []string{} + if len(trigger.Spec.Method) > 0 { + methods = append(methods, trigger.Spec.Method) + } + if len(trigger.Spec.Methods) > 0 { + methods = trigger.Spec.Methods + } + fmt.Fprintf(w, "%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v\n", - trigger.ObjectMeta.Name, trigger.Spec.Method, trigger.Spec.RelativeURL, function, trigger.Spec.CreateIngress, host, path, trigger.Spec.IngressConfig.TLS, ann) + trigger.ObjectMeta.Name, methods, trigger.Spec.RelativeURL, function, trigger.Spec.CreateIngress, host, path, trigger.Spec.IngressConfig.TLS, ann) } fmt.Fprintf(w, "\n") w.Flush() diff --git a/pkg/fission-cli/cmd/spec/spec.go b/pkg/fission-cli/cmd/spec/spec.go index 1c7f7260..abfac70b 100644 --- a/pkg/fission-cli/cmd/spec/spec.go +++ b/pkg/fission-cli/cmd/spec/spec.go @@ -443,6 +443,9 @@ func (fr *FissionResources) Validate(input cli.Input) ([]string, error) { if len(t.Spec.Host) > 0 { warnings = append(warnings, "Host in HTTPTrigger spec.Host is now marked as deprecated, see 'help' for details") } + if len(t.Spec.Method) > 0 { + warnings = append(warnings, "Method in HTTTPTrigger spec.Method is deprecated, use spec.Methods instead") + } result = multierror.Append(result, t.Validate()) } for _, t := range fr.KubernetesWatchTriggers { diff --git a/pkg/fission-cli/flag/flag.go b/pkg/fission-cli/flag/flag.go index 737ee2b0..fbbcf822 100644 --- a/pkg/fission-cli/flag/flag.go +++ b/pkg/fission-cli/flag/flag.go @@ -116,7 +116,7 @@ var ( FnSubPath = Flag{Type: String, Name: flagkey.FnSubPath, Usage: "Sub Path to check if function internally supports routing"} HtName = Flag{Type: String, Name: flagkey.HtName, Usage: "HTTP trigger name"} - HtMethod = Flag{Type: String, Name: flagkey.HtMethod, Usage: "HTTP Method: GET|POST|PUT|DELETE|HEAD", DefaultValue: http.MethodGet} + HtMethod = Flag{Type: StringSlice, Name: flagkey.HtMethod, Usage: "HTTP Methods: GET,POST,PUT,DELETE,HEAD. To mention single method: --method GET and for multiple methods --method GET --method POST.", DefaultValue: []string{http.MethodGet}} HtUrl = Flag{Type: String, Name: flagkey.HtUrl, Usage: "URL pattern (See gorilla/mux supported patterns)"} HtHost = Flag{Type: String, Name: flagkey.HtHost, Usage: "Use --ingressrule instead", Deprecated: true, Substitute: flagkey.HtIngressRule} HtIngress = Flag{Type: Bool, Name: flagkey.HtIngress, Usage: "Creates ingress with same URL"} diff --git a/pkg/router/httpTriggers.go b/pkg/router/httpTriggers.go index 8a2c9763..17f3846f 100644 --- a/pkg/router/httpTriggers.go +++ b/pkg/router/httpTriggers.go @@ -173,11 +173,25 @@ func (ts *HTTPTriggerSet) getRouter(fnTimeoutMap map[types.UID]int) *mux.Router } else { ht = muxRouter.HandleFunc(trigger.Spec.RelativeURL, fh.handler) } - ht.Methods(trigger.Spec.Method) + methods := trigger.Spec.Methods + if len(trigger.Spec.Method) > 0 { + present := false + for _, m := range trigger.Spec.Methods { + if m == trigger.Spec.Method { + present = true + break + } + } + if !present { + methods = append(methods, trigger.Spec.Method) + } + } + ht.Methods(methods...) if trigger.Spec.Host != "" { ht.Host(trigger.Spec.Host) } - if trigger.Spec.Prefix == nil && trigger.Spec.RelativeURL == "/" && trigger.Spec.Method == "GET" { + + if trigger.Spec.Prefix == nil && trigger.Spec.RelativeURL == "/" && len(methods) == 1 && methods[0] == http.MethodGet { homeHandled = true } }