Improve resolver interface and documentation

**What**
- Update the BaseURLResolver interface so that it returns `url.Url`
instead of `string`
- Revert changes to the `FaaSConfig` so that the implementer can decide
to override the proxy behavior
- Add additional unit tests to verify the behavior of the NewHandlerFunc
method
- Add tests to verify behavior of non-allowed HTTP methods
- Add docs explaining the configuration of the http client
- Add docs to explain the proxy package and make them godoc friendly

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

proxy/handler_test.go

@@ -0,0 +1,63 @@
+package proxy
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"time"
+)
+
+type testBaseURLResolver struct {
+	testServerBase string
+}
+
+func (tr *testBaseURLResolver) Resolve(name string) (url.URL, error) {
+	return url.URL{
+		Scheme: "http",
+		Host:   tr.testServerBase,
+	}, nil
+}
+func Test_NewHandlerFunc_Panic(t *testing.T) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("should panic if resolver is nil")
+		}
+	}()
+
+	NewHandlerFunc(time.Second, nil)
+}
+
+func Test_NewHandlerFunc_NoPanic(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("should not panic if resolver is not nil")
+		}
+	}()
+
+	proxyFunc := NewHandlerFunc(time.Second, &testBaseURLResolver{})
+	if proxyFunc == nil {
+		t.Errorf("proxy handler func is nil")
+	}
+}
+
+func Test_ProxyHandler_NonAllowedMethods(t *testing.T) {
+
+	proxyFunc := NewHandlerFunc(time.Second, &testBaseURLResolver{})
+
+	nonAllowedMethods := []string{
+		http.MethodHead, http.MethodConnect, http.MethodOptions, http.MethodTrace,
+	}
+
+	for _, method := range nonAllowedMethods {
+		t.Run(method+" method is not allowed", func(t *testing.T) {
+			w := httptest.NewRecorder()
+			req := httptest.NewRequest(method, "http://example.com/foo", nil)
+			proxyFunc(w, req)
+			resp := w.Result()
+			if resp.StatusCode != http.StatusMethodNotAllowed {
+				t.Errorf("expected status code `%d`, got `%d`", http.StatusMethodNotAllowed, resp.StatusCode)
+			}
+		})
+	}
+}

proxy/proxy.go

@@ -1,3 +1,23 @@
+// Package proxy provides a default function invocation proxy method for OpenFaaS providers.
+//
+// The function proxy logic is used by the Gateway when `direct_functions` is set to false.
+// This means that the provider will direct call the function and return the results.  This
+// involves resolving the function by name and then copying the result into the original HTTP
+// request.
+//
+// openfaas-provider has implemented a standard HTTP HandlerFunc that will handle setting
+// timeout values, parsing the request path, and copying the request/response correctly.
+// 		bootstrapHandlers := bootTypes.FaaSHandlers{
+// 			FunctionProxy:  proxy.NewHandlerFunc(timeout, resolver),
+// 			DeleteHandler:  handlers.MakeDeleteHandler(clientset),
+// 			DeployHandler:  handlers.MakeDeployHandler(clientset),
+// 			FunctionReader: handlers.MakeFunctionReader(clientset),
+// 			ReplicaReader:  handlers.MakeReplicaReader(clientset),
+// 			ReplicaUpdater: handlers.MakeReplicaUpdater(clientset),
+// 			InfoHandler:    handlers.MakeInfoHandler(),
+// 		}
+//
+// proxy.NewHandlerFunc is optional, but does simplify the logic of your provider.
 package proxy
 
 import (
@@ -20,15 +40,35 @@ const (
 // BaseURLResolver URL resolver for proxy requests
 //
 // The FaaS provider implementation is responsible for providing the resolver function implementation.
-// BaseURLResolver.Resolve will receive the function name and should return the base Address of the
+// BaseURLResolver.Resolve will receive the function name and should return the URL of the
 // function service.
 type BaseURLResolver interface {
-	Resolve(functionName string) (string, error)
+	Resolve(functionName string) (url.URL, error)
 }
 
-// NewHandlerFunc creates a standard http HandlerFunc to proxy function requests to the function.
+// NewHandlerFunc creates a standard http.HandlerFunc to proxy function requests.
+// The returned http.HandlerFunc will ensure:
+//
+// 	- proper proxy request timeouts
+// 	- proxy requests for GET, POST, PATCH, PUT, and DELETE
+// 	- path parsing including support for extracing the function name, sub-paths, and query paremeters
+// 	- passing and setting the `X-Forwarded-Host` and `X-Forwarded-For` headers
+// 	- logging errors and proxy request timing to stdout
+//
+// Note that this will panic if `resolver` is nil.
 func NewHandlerFunc(timeout time.Duration, resolver BaseURLResolver) http.HandlerFunc {
+	if resolver == nil {
+		panic("NewHandlerFunc: empty proxy handler resolver, cannot be nil")
+	}
+
 	proxyClient := http.Client{
+		// these Transport values ensure that the http Client will eventually timeout and prevents
+		// infinite retries. The default http.Client configure these timeouts.  The specific
+		// values tuned via performance testing/benchmarking
+		//
+		// Additional context can be found at
+		// - https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779
+		// - https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
 		Transport: &http.Transport{
 			Proxy: http.ProxyFromEnvironment,
 			DialContext: (&net.Dialer{
@@ -113,10 +153,16 @@ func proxyRequest(w http.ResponseWriter, originalReq *http.Request, proxyClient
 
 // buildProxyRequest creates a request object for the proxy request, it will ensure that
 // the original request headers are preserved as well as setting openfaas system headers
-func buildProxyRequest(originalReq *http.Request, baseURL, extraPath string) (*http.Request, error) {
+func buildProxyRequest(originalReq *http.Request, baseURL url.URL, extraPath string) (*http.Request, error) {
+
+	host := baseURL.Host
+	if baseURL.Port() == "" {
+		host = baseURL.Host + ":" + watchdogPort
+	}
+
 	url := url.URL{
-		Scheme:   "http",
-		Host:     baseURL + ":" + watchdogPort,
+		Scheme:   baseURL.Scheme,
+		Host:     host,
 		Path:     extraPath,
 		RawQuery: originalReq.URL.RawQuery,
 	}

proxy/proxy_test.go

@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/gorilla/mux"
@@ -17,6 +18,13 @@ func varHandler(w http.ResponseWriter, r *http.Request) {
 	w.Write([]byte(fmt.Sprintf("name: %s params: %s", vars["name"], vars["params"])))
 }
 
+func testResolver(functionName string) (url.URL, error) {
+	return url.URL{
+		Scheme: "http",
+		Host:   functionName,
+	}, nil
+}
+
 func Test_pathParsing(t *testing.T) {
 	tt := []struct {
 		name         string
@@ -102,7 +110,8 @@ func Test_buildProxyRequest_Body_Method_Query(t *testing.T) {
 		t.Fail()
 	}
 
-	upstream, err := buildProxyRequest(request, "funcName", "")
+	funcURL, _ := testResolver("funcName")
+	upstream, err := buildProxyRequest(request, funcURL, "")
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -134,7 +143,8 @@ func Test_buildProxyRequest_Body_Method_Query(t *testing.T) {
 func Test_buildProxyRequest_NoBody_GetMethod_NoQuery(t *testing.T) {
 	request, _ := http.NewRequest(http.MethodGet, "/", nil)
 
-	upstream, err := buildProxyRequest(request, "funcName", "")
+	funcURL, _ := testResolver("funcName")
+	upstream, err := buildProxyRequest(request, funcURL, "")
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -166,7 +176,8 @@ func Test_buildProxyRequest_HasXForwardedHostHeaderWhenSet(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	upstream, err := buildProxyRequest(request, "funcName", "/")
+	funcURL, _ := testResolver("funcName")
+	upstream, err := buildProxyRequest(request, funcURL, "/")
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -186,7 +197,8 @@ func Test_buildProxyRequest_XForwardedHostHeader_Empty_WhenNotSet(t *testing.T)
 		t.Fatal(err)
 	}
 
-	upstream, err := buildProxyRequest(request, "funcName", "/")
+	funcURL, _ := testResolver("funcName")
+	upstream, err := buildProxyRequest(request, funcURL, "/")
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -207,7 +219,8 @@ func Test_buildProxyRequest_XForwardedHostHeader_WhenAlreadyPresent(t *testing.T
 	}
 
 	request.Header.Set("X-Forwarded-Host", headerValue)
-	upstream, err := buildProxyRequest(request, "funcName", "/")
+	funcURL, _ := testResolver("funcName")
+	upstream, err := buildProxyRequest(request, funcURL, "/")
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -234,7 +247,8 @@ func Test_buildProxyRequest_WithPathNoQuery(t *testing.T) {
 		t.Fail()
 	}
 
-	upstream, err := buildProxyRequest(request, "xyz", functionPath)
+	funcURL, _ := testResolver("xyz")
+	upstream, err := buildProxyRequest(request, funcURL, functionPath)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -285,7 +299,8 @@ func Test_buildProxyRequest_WithNoPathNoQuery(t *testing.T) {
 		t.Fail()
 	}
 
-	upstream, err := buildProxyRequest(request, "xyz", functionPath)
+	funcURL, _ := testResolver("xyz")
+	upstream, err := buildProxyRequest(request, funcURL, functionPath)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -334,7 +349,8 @@ func Test_buildProxyRequest_WithPathAndQuery(t *testing.T) {
 		t.Fail()
 	}
 
-	upstream, err := buildProxyRequest(request, "xyz", functionPath)
+	funcURL, _ := testResolver("xyz")
+	upstream, err := buildProxyRequest(request, funcURL, functionPath)
 	if err != nil {
 		t.Fatal(err.Error())
 	}

serve.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/openfaas/faas-provider/auth"
-	"github.com/openfaas/faas-provider/proxy"
 	"github.com/openfaas/faas-provider/types"
 )
 
@@ -49,7 +48,6 @@ func Serve(handlers *types.FaaSHandlers, config *types.FaaSConfig) {
 	}
 
 	// System (auth) endpoints
-
 	r.HandleFunc("/system/functions", handlers.FunctionReader).Methods("GET")
 	r.HandleFunc("/system/functions", handlers.DeployHandler).Methods("POST")
 	r.HandleFunc("/system/functions", handlers.DeleteHandler).Methods("DELETE")
@@ -60,10 +58,9 @@ func Serve(handlers *types.FaaSHandlers, config *types.FaaSConfig) {
 	r.HandleFunc("/system/info", handlers.InfoHandler).Methods("GET")
 
 	// Open endpoints
-	functionProxy := proxy.NewHandlerFunc(config.ReadTimeout, config.FunctionProxyResolver)
-	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}", functionProxy)
-	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}/", functionProxy)
-	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}/{params:.*}", functionProxy)
+	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}", handlers.FunctionProxy)
+	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}/", handlers.FunctionProxy)
+	r.HandleFunc("/function/{name:[-a-zA-Z_0-9]+}/{params:.*}", handlers.FunctionProxy)
 
 	if config.EnableHealth {
 		r.HandleFunc("/healthz", handlers.Health).Methods("GET")

types/config.go

@@ -3,14 +3,15 @@ package types
 import (
 	"net/http"
 	"time"
-
-	"github.com/openfaas/faas-provider/proxy"
 )
 
 // FaaSHandlers provide handlers for OpenFaaS
 type FaaSHandlers struct {
 	FunctionReader http.HandlerFunc
 	DeployHandler  http.HandlerFunc
+	// FunctionProxy provides the function invocation proxy logic.  Use proxy.NewHandlerFunc to
+	// use the standard OpenFaaS proxy implementation or provide completely custom proxy logic.
+	FunctionProxy  http.HandlerFunc
 	DeleteHandler  http.HandlerFunc
 	ReplicaReader  http.HandlerFunc
 	ReplicaUpdater http.HandlerFunc
@@ -29,9 +30,4 @@ type FaaSConfig struct {
 	EnableHealth    bool
 	EnableBasicAuth bool
 	SecretMountPath string
-
-	// The FaaS provider implementation is responsible for providing the resolver function implementation.
-	// BaseURLResolver.Resolve will receive the function name and should return the base Address of the
-	// function service.
-	FunctionProxyResolver proxy.BaseURLResolver
 }