From 2f98ca8802286bae1c5d5fb4580166949bcadfc0 Mon Sep 17 00:00:00 2001 From: "Alex Ellis (VMware)" Date: Fri, 24 Aug 2018 10:03:26 +0100 Subject: [PATCH] Review changes for HTTP paths This reviews the code and fixes up suggestions made by team for the HTTP paths PR #789. - Removed feature-flag (this is backwards-compatible, so I see no value in adding the flag) - There was a URL transform happening for calls proxied to the back end, I changed this for the nil-transform - i.e. it does not change anything in the URL - Introduced variables to describe the regex indicies used in the URL trimming. Tested with Docker Swarm with a ruby-microservice, with system calls and with function calls using the UI. Signed-off-by: Alex Ellis (VMware) --- gateway/build.sh | 6 +++-- gateway/handlers/forwarding_proxy.go | 20 +++++++++++----- gateway/handlers/forwarding_proxy_test.go | 4 ++-- gateway/server.go | 29 +++++++++-------------- gateway/types/readconfig.go | 6 ----- 5 files changed, 31 insertions(+), 34 deletions(-) diff --git a/gateway/build.sh b/gateway/build.sh index 96c80fd0..763879e9 100755 --- a/gateway/build.sh +++ b/gateway/build.sh @@ -19,7 +19,9 @@ if [ "$1" ] ; then fi fi -echo Building openfaas/gateway:$eTAG +NS=openfaas + +echo Building $NS/gateway:$eTAG GIT_COMMIT_MESSAGE=$(git log -1 --pretty=%B 2>&1 | head -n 1) GIT_COMMIT_SHA=$(git rev-list -1 HEAD) @@ -28,4 +30,4 @@ VERSION=$(git describe --all --exact-match `git rev-parse HEAD` | grep tags | se docker build --build-arg https_proxy=$https_proxy --build-arg http_proxy=$http_proxy \ --build-arg GIT_COMMIT_MESSAGE="$GIT_COMMIT_MESSAGE" --build-arg GIT_COMMIT_SHA=$GIT_COMMIT_SHA \ --build-arg VERSION=${VERSION:-dev} \ - -t openfaas/gateway:$eTAG . -f $dockerfile --no-cache + -t $NS/gateway:$eTAG . -f $dockerfile --no-cache diff --git a/gateway/handlers/forwarding_proxy.go b/gateway/handlers/forwarding_proxy.go index 83cca058..cff99ea8 100644 --- a/gateway/handlers/forwarding_proxy.go +++ b/gateway/handlers/forwarding_proxy.go @@ -19,6 +19,14 @@ import ( // Parse out the service name (group 1) and rest of path (group 2). var functionMatcher = regexp.MustCompile("^/?(?:async-)?function/([^/?]+)([^?]*)") +// Indicies and meta-data for functionMatcher regex parts +const ( + hasPathCount = 3 + routeIndex = 0 + nameIndex = 1 + pathIndex = 2 +) + // HTTPNotifier notify about HTTP request/response type HTTPNotifier interface { Notify(method string, URL string, statusCode int, duration time.Duration) @@ -29,7 +37,7 @@ type BaseURLResolver interface { Resolve(r *http.Request) string } -// RequestURLPathTransformer Transform the incoming URL path for upstream requests +// URLPathTransformer Transform the incoming URL path for upstream requests type URLPathTransformer interface { Transform(r *http.Request) string } @@ -151,8 +159,8 @@ func getServiceName(urlValue string) string { // will be the service name we need, and at `2` the rest of the path. matcher := functionMatcher.Copy() matches := matcher.FindStringSubmatch(urlValue) - if 3 == len(matches) { - serviceName = matches[1] + if len(matches) == hasPathCount { + serviceName = matches[nameIndex] } } return strings.Trim(serviceName, "/") @@ -224,7 +232,7 @@ func (f FunctionPathTruncatingURLPathTransformer) Transform(r *http.Request) str // In the following regex, in the case of a match the r.URL.Path will be at `0`, // the function name at `1` and the rest of the path (the part we are interested in) // at `2`. For this transformer, all we need to do is confirm it is a function. - if 3 == len(parts) { + if len(parts) == hasPathCount { ret = "/" } } @@ -249,8 +257,8 @@ func (f FunctionPrefixTrimmingURLPathTransformer) Transform(r *http.Request) str // rest of the path (the part we are interested in) at `2`. matcher := functionMatcher.Copy() parts := matcher.FindStringSubmatch(ret) - if 3 == len(parts) { - ret = parts[2] + if len(parts) == hasPathCount { + ret = parts[pathIndex] } } diff --git a/gateway/handlers/forwarding_proxy_test.go b/gateway/handlers/forwarding_proxy_test.go index a16d72f5..963d6b74 100644 --- a/gateway/handlers/forwarding_proxy_test.go +++ b/gateway/handlers/forwarding_proxy_test.go @@ -78,7 +78,7 @@ func Test_buildUpstreamRequest_HasHostHeaderWhenSet(t *testing.T) { t.Fatal(err) } - upstream := buildUpstreamRequest(request, "/") + upstream := buildUpstreamRequest(request, "/", "/") if request.Host != upstream.Host { t.Errorf("Host - want: %s, got: %s", request.Host, upstream.Host) @@ -95,7 +95,7 @@ func Test_buildUpstreamRequest_HostHeader_Empty_WhenNotSet(t *testing.T) { t.Fatal(err) } - upstream := buildUpstreamRequest(request, "/") + upstream := buildUpstreamRequest(request, "/", "/") if request.Host != upstream.Host { t.Errorf("Host - want: %s, got: %s", request.Host, upstream.Host) diff --git a/gateway/server.go b/gateway/server.go index 4e925dd8..e280c7f1 100644 --- a/gateway/server.go +++ b/gateway/server.go @@ -74,23 +74,17 @@ func main() { functionURLResolver = urlResolver } - urlTransformer := handlers.FunctionPathTruncatingURLPathTransformer{} - var functionURLTransformer handlers.URLPathTransformer - - if config.PassURLPathsToFunctions { - functionURLTransformer = handlers.FunctionPrefixTrimmingURLPathTransformer{} - } else { - functionURLTransformer = urlTransformer - } + nilURLTransformer := handlers.TransparentURLPathTransformer{} + functionURLTransformer := handlers.FunctionPrefixTrimmingURLPathTransformer{} faasHandlers.Proxy = handlers.MakeForwardingProxyHandler(reverseProxy, functionNotifiers, functionURLResolver, functionURLTransformer) - faasHandlers.RoutelessProxy = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) - faasHandlers.ListFunctions = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) - faasHandlers.DeployFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) - faasHandlers.DeleteFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) - faasHandlers.UpdateFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) - queryFunction := handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) + faasHandlers.RoutelessProxy = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) + faasHandlers.ListFunctions = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) + faasHandlers.DeployFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) + faasHandlers.DeleteFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) + faasHandlers.UpdateFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) + queryFunction := handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) alertHandler := plugin.NewExternalServiceQuery(*config.FunctionsProviderURL) faasHandlers.Alert = handlers.MakeAlertHandler(alertHandler) @@ -110,7 +104,7 @@ func main() { faasHandlers.ListFunctions = metrics.AddMetricsHandler(faasHandlers.ListFunctions, prometheusQuery) faasHandlers.Proxy = handlers.MakeCallIDMiddleware(faasHandlers.Proxy) - faasHandlers.ScaleFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer) + faasHandlers.ScaleFunction = handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer) if credentials != nil { faasHandlers.UpdateFunction = @@ -123,7 +117,6 @@ func main() { handlers.DecorateWithBasicAuth(faasHandlers.ListFunctions, credentials) faasHandlers.ScaleFunction = handlers.DecorateWithBasicAuth(faasHandlers.ScaleFunction, credentials) - } r := mux.NewRouter() @@ -147,7 +140,7 @@ func main() { r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}/{params:.*}", functionProxy) r.HandleFunc("/system/info", handlers.MakeInfoHandler(handlers.MakeForwardingProxyHandler( - reverseProxy, forwardingNotifiers, urlResolver, urlTransformer))).Methods(http.MethodGet) + reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer))).Methods(http.MethodGet) r.HandleFunc("/system/alert", faasHandlers.Alert) @@ -181,7 +174,7 @@ func main() { metricsHandler := metrics.PrometheusHandler() r.Handle("/metrics", metricsHandler) - r.HandleFunc("/healthz", handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, urlTransformer)).Methods(http.MethodGet) + r.HandleFunc("/healthz", handlers.MakeForwardingProxyHandler(reverseProxy, forwardingNotifiers, urlResolver, nilURLTransformer)).Methods(http.MethodGet) r.Handle("/", http.RedirectHandler("/ui/", http.StatusMovedPermanently)).Methods(http.MethodGet) diff --git a/gateway/types/readconfig.go b/gateway/types/readconfig.go index 5146efb1..5f0986e9 100644 --- a/gateway/types/readconfig.go +++ b/gateway/types/readconfig.go @@ -105,8 +105,6 @@ func (ReadConfig) Read(hasEnv HasEnv) GatewayConfig { cfg.DirectFunctions = parseBoolValue(hasEnv.Getenv("direct_functions")) cfg.DirectFunctionsSuffix = hasEnv.Getenv("direct_functions_suffix") - cfg.PassURLPathsToFunctions = parseBoolValue(hasEnv.Getenv("pass_url_path_to_functions")) - cfg.UseBasicAuth = parseBoolValue(hasEnv.Getenv("basic_auth")) secretPath := hasEnv.Getenv("secret_mount_path") @@ -152,10 +150,6 @@ type GatewayConfig struct { // If set this will be used to resolve functions directly DirectFunctionsSuffix string - // If set to true, the requested path will be passed along to the function, minus the "/function/xyz" - // prefix, else the path will be truncated to "/" regardless of what the client sends. - PassURLPathsToFunctions bool - // If set, reads secrets from file-system for enabling basic auth. UseBasicAuth bool