Code review PR of #789 - HTTP paths for functions #832

alexellis · 2018-08-24T08:35:13Z

Description

Closes #789

Docker image for testing: alexellis/gateway:http-paths

…atchdog. Previously, only the query string of the URL was passed through the Gateway. With this change, the entire path requested by the client is passed through as well as the query string. While fwatchdog already supported passing the path through, in practice this would not happen since the Gateway would have swallowed it before forwarding the request to the watchdog. With this change, the path portion after the function name is added to the Http_Path environment variable, provided that cgiHeaders are enabled. This is similar to the of-watchdog equivalent. Signed-off-by: Thomas E Lackey <[email protected]>

…tion prefix trimming instead of baking it in. Also add a configuration option, 'pass_url_path_to_functions' to control whether the full path is passed to the functions or not. Signed-off-by: Thomas E Lackey <[email protected]>

Signed-off-by: Thomas E Lackey <[email protected]>

…ync functions as well. Signed-off-by: Thomas E Lackey <[email protected]>

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) <[email protected]>

alexellis · 2018-08-24T09:17:29Z

Async Path passing is now tested:

faas-cli deploy --name env --fprocess env --network func_functions --image functions/alpine:latest

curl -d "" http://127.0.0.1:8080/async-function/env/path_here

func_queue-worker.1.qk96166nz9ji@nuc    | [#3] Received on [faas-request]: 'sequence:6 subject:"faas-request" data:"{\"Header\":{\"Accept\":[\"*/*\"],\"Content-Length\":[\"0\"],\"Content-Type\":[\"application/x-www-form-urlencoded\"],\"User-Agent\":[\"curl/7.55.1\"],\"X-Call-Id\":[\"09577dcb-46bd-473b-9bf6-ede3f44e96a4\"],\"X-Start-Time\":[\"1535102101843119850\"]},\"Host\":\"127.0.0.1:8080\",\"Body\":\"\",\"Method\":\"POST\",\"Path\":\"/path_here\",\"QueryString\":\"\",\"Function\":\"env\",\"CallbackUrl\":null}" timestamp:1535102101843442385 '

"Path\":\"/path_here\"

Also tested with ?qs=1 and saw that passed through.

Path is empty when nothing is passed after /async-function/env

I tested the path work via Ivana's Ruby guestbook including GET/POST endpoints at different paths.

faas-cli deploy --name guestbook --network func_functions --image docwareiy/guestbook:0.1

Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis · 2018-08-24T09:31:05Z

@telackey please confirm that the original intent is still matched in this follow-up PR.

I tested a guestbook microservice in Ruby using GET/POST - worked fine.
I tested the UI + CLI + paths were passed to the back-end and routed as expected
I tested the Java of-watchdog template which still worked fine when no path was passed
I tested async functions include the path variable.

Signed-off-by: Alex Ellis (VMware) <[email protected]>

This was altered to "alexellis" for building a testing image, but shouldn't have been pushed. Reverting. Signed-off-by: Alex Ellis (VMware) <[email protected]>

ivanayov · 2018-08-24T10:47:14Z

I tested with docwareiy/guestbook:0.2 for path parameters + url query:
http://127.0.0.1:8080/function/guestbook/signatures?guest=foo%20bar&firstname=foo&lastname=bar
Output:
Visitor foo bar has signed the Guestbook..

http://127.0.0.1:8080/function/guestbook/signatures?guest=test%20bar&firstname=test&lastname=bar
Output:
Visitor test bar has not signed the Guestbook..

Also tested async functions with path parameters.

ivanayov · 2018-08-24T13:19:10Z

Using this gateway, function invocation count does not increase. Tested both in the UI and Prometheus by gateway_function_invocation_total{function_name="figlet"}.

After reverting to openfaas/gateway:0.8.11 it works as expected.

alexellis · 2018-08-24T13:20:56Z

Can you find out why?

ivanayov · 2018-08-24T13:22:20Z

@alexellis I'm looking into it

ivanayov · 2018-08-24T15:27:10Z

gateway/handlers/forwarding_proxy.go

 	return func(w http.ResponseWriter, r *http.Request) {
 		baseURL := baseURLResolver.Resolve(r)

-		requestURL := r.URL.Path
+		requestURL := urlPathTransformer.Transform(r)


This line seems to break the invocation count bug

This prevented Prometheus metrics from being gathered from the URL. Signed-off-by: Alex Ellis (VMware) <[email protected]>

ivanayov · 2018-08-27T07:35:50Z

Derek add label: blocked

ivanayov · 2018-08-27T07:36:41Z

Derek remove label: blocked

telackey · 2018-08-28T16:13:19Z

@alexellis

please confirm that the original intent is still matched in this follow-up PR.

Yes, looks good.

LucasRoesler

Everything looks good to me, no glaring holes that I can see.

alexellis · 2018-09-01T17:15:44Z

Appreciated Lucas.

alexellis · 2018-09-01T17:16:16Z

@telackey do the upstream templates have the ability to pass on a path to their handlers yet?

telackey and others added 5 commits August 24, 2018 09:33

Only truncate '/function/...' paths.

3830bc8

Signed-off-by: Thomas E Lackey <[email protected]>

Add a non-capturing subgroup so that the function regex will match as…

acaee31

…ync functions as well. Signed-off-by: Thomas E Lackey <[email protected]>

alexellis mentioned this pull request Aug 24, 2018

Implement proposal 716, passing full URL paths #789

Closed

11 tasks

Add test coverage for buildUpstreamRequest

c211de4

Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis added 2 commits August 24, 2018 10:40

Add test coverage for FunctionPrefixTrimmingURLPathTransformer

c91bb25

Signed-off-by: Alex Ellis (VMware) <[email protected]>

Set Docker image NS to openfaas

294d45e

This was altered to "alexellis" for building a testing image, but shouldn't have been pushed. Reverting. Signed-off-by: Alex Ellis (VMware) <[email protected]>

ivanayov reviewed Aug 24, 2018

View reviewed changes

Fix bug introduced by URL transformer

4eae367

This prevented Prometheus metrics from being gathered from the URL. Signed-off-by: Alex Ellis (VMware) <[email protected]>

derek bot added the blocked label Aug 27, 2018

derek bot removed the blocked label Aug 27, 2018

alexellis merged commit 9c2f6dd into master Aug 29, 2018

LucasRoesler reviewed Aug 29, 2018

View reviewed changes

alexellis deleted the alexellis/function_http_paths branch September 1, 2018 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review PR of #789 - HTTP paths for functions #832

Code review PR of #789 - HTTP paths for functions #832

alexellis commented Aug 24, 2018 •

edited

Loading

alexellis commented Aug 24, 2018

alexellis commented Aug 24, 2018

ivanayov commented Aug 24, 2018

ivanayov commented Aug 24, 2018

alexellis commented Aug 24, 2018

ivanayov commented Aug 24, 2018

ivanayov Aug 24, 2018 •

edited

Loading

ivanayov commented Aug 27, 2018

ivanayov commented Aug 27, 2018

telackey commented Aug 28, 2018

LucasRoesler left a comment

alexellis commented Sep 1, 2018

alexellis commented Sep 1, 2018

Code review PR of #789 - HTTP paths for functions #832

Code review PR of #789 - HTTP paths for functions #832

Conversation

alexellis commented Aug 24, 2018 • edited Loading

Description

alexellis commented Aug 24, 2018

alexellis commented Aug 24, 2018

ivanayov commented Aug 24, 2018

ivanayov commented Aug 24, 2018

alexellis commented Aug 24, 2018

ivanayov commented Aug 24, 2018

ivanayov Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

ivanayov commented Aug 27, 2018

ivanayov commented Aug 27, 2018

telackey commented Aug 28, 2018

LucasRoesler left a comment

Choose a reason for hiding this comment

alexellis commented Sep 1, 2018

alexellis commented Sep 1, 2018

alexellis commented Aug 24, 2018 •

edited

Loading

ivanayov Aug 24, 2018 •

edited

Loading