Feature standardize function proxy handler #11
Conversation
**What** - Add standardized proxy handler based on the implementations for swarm and kubernetes **Why** - Reduce duplicated code amoung the various providers - Ensure the proxy behavior is consistent - Addresses openfaas#9 Signed-off-by: Lucas Roesler <[email protected]>
**What** - Remove depricated support for specifying the function name via the X-Function header Signed-off-by: Lucas Roesler <[email protected]>
**What** - Refactor the function proxy handler to enable calling the function with a subpath - Pull and refactor unit tests from the faas gateway Signed-off-by: Lucas Roesler <[email protected]>
**What** - Wrap the proxy request with the original context Signed-off-by: Lucas Roesler <[email protected]>
Signed-off-by: Lucas Roesler <[email protected]>
**What** - Do not pass the functionproxy handler to the server, only pass the resolver so that we can construct the proxy function **Why** - This ensures that the core logic is consistently implemented Signed-off-by: Lucas Roesler <[email protected]>
**What** - Explicitly return an error from the Resolve method instead of forcing the caller to infer an error from an empty string Signed-off-by: Lucas Roesler <[email protected]>
LucasRoesler
changed the title
|
I have started testing this in faas-swarm and have pushed a docker image here |
**What** - Do not follow redirect responses from functions, these should be passed back to the user - Copied from openfaas/faas#925 Signed-off-by: Lucas Roesler <[email protected]>
|
I have tested the functions the demo stack from the |
| Proxy: http.ProxyFromEnvironment, | ||
| DialContext: (&net.Dialer{ | ||
| Timeout: timeout, | ||
| KeepAlive: 1 * time.Second, |
There was a problem hiding this comment.
@LucasRoesler i'm curious I've seen these adjustments to the default transport in faas-cli as well
- DialContex.KeepAlive
- IdleConnTimeout
- ExpectContinueTimeout
The differ substantially from DefaultTransport is there a way to infer to the reader of the code why we are doing this?
There was a problem hiding this comment.
I could add some comments. The core reason, that I understand, for these settings is that some are just not set by the default transport and client
There was a problem hiding this comment.
A common issue with Golang's HTTP library was inappropriate defaults which lead to a "DoS" attack or infinite timeouts when doing performance testing/benchmarking. We tuned these values but the Golang team has also improved upon their defaults since we made these changes originally. cc @stefanprodan
There was a problem hiding this comment.
You should also see some context in the git history in the faas repo where this code was taken from.
There was a problem hiding this comment.
Please take a look at the newly added comment and let me know if more context would be useful
**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 <[email protected]>
**What** - Upgrade gorilla mux from 1.6.0 -> 1.6.2 so that we can use the new SetURLVars method in unit tests - Add additional tests for the early error cases in the proxy handler Signed-off-by: Lucas Roesler <[email protected]>
|
To help with the testing, I updated to gorilla mux 1.6.2, i have also opened a ticket in openfaas/faas#945 so that we can do a similar version bump across the project |
|
Currently the biggest hole in the test coverage is testing the response from a "function" but at the moment coverage ~58% and should cover a wide variety of cases, including the proxy request construction. |
This addresses #9 by creating a standard Proxy handler and updating the provider config to accept a resolver method that is then provided by the provider implementation. This ensures that the proxy logic is consistent between the various providers while allowing them to customize the function lookup implementation details.
This also implements the sub-path routing to functions. Unit tests from the
gatewayimplementation have been ported here as well.Please note that this includes an update to the Gorilla Mux package. This upgrade primarily makes testing easier by introducing the
SetURLVarsmethod.