Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Allow for path parameters by passing full URI to of-watchdog HTTP functions #716

Closed
telackey opened this issue Jun 6, 2018 · 20 comments

Comments

@telackey
Copy link
Contributor

telackey commented Jun 6, 2018

The new of-watchdog HTTP-style functions (cf. https://github.com/openfaas-incubator/node8-express-template) have a long-running HTTP server behind the actual function code. This is very similar to other FaaS solutions, such as AWS Lambda, and is mostly hidden from the function developer.

The end result offers greater flexibility as arbitrary headers, response codes, etc. can be more easily returned. It also eases the porting effort from other FaaS platforms.

One limitation of the current implementation is that only the query string of the incoming request, not its path, is passed along to the function. Both the API Gateway and of-watchdog reconstruct the URL without the full path. This means that I can make use of a request like:
/function/myfunc?category=123

but not:
/function/myfunc/category/123

Desired Behaviour

Pass the complete URI all the way to the function, so that the function developer can decide which is most appropriate for their needs: path parameters or query string parameters.

Current Behaviour

The full path is swallowed by the Gateway and by of-watchdog

Possible Solution

I have attached some very simple, proof-of-concept patches (against 0.8.1) showing a possible solution.

faas.diff.txt
of-watchdog.diff.txt

@ivanayov
Copy link
Contributor

This looks like a good-to-have.
@alexellis @rgee0 @ericstoekl any thoughts?

@ivanayov
Copy link
Contributor

Derek add label: proposal

@gmcelhoe
Copy link

This is very exciting, I was trying to write a REST-like API using OpenFaas and was getting stuck. I'll try applying these changes locally.

@alexellis
Copy link
Member

@gmcelhoe I would suggest you use query string and/or a body right now. Functions are not RESTful, they are HTTP-compatible.

@andrew-s
Copy link

@alexellis Is this likely to happen/roadmapped? I'm kind of in the same boat and work around would be to write a gateway that translates paths into query stringed function calls but, then that gateway needs to sit somewhere and be H/A too.

@telackey
Copy link
Contributor Author

telackey commented Jul 12, 2018

To put this in a different way, it is difficult for me to see what if anything is gained by stripping the path, while there seem to be definite gains from passing it.

@andrew-s

It is unfortunate to be forced to fork, but if you look at the example patches to the Gateway and of-watchdog in the initial proposal, I think you'll find they are simpler to maintain than the proxy you mention.

@alexellis
Copy link
Member

What is the requirement to use a path? I haven't heard a clear usecase yet.

@telackey
Copy link
Contributor Author

telackey commented Jul 12, 2018

@alexellis

I would suggest that it makes sense for anyone that might wish to have a path like/functionName/recordTypeA/recordId.

The fact that a query string could be used does not mean it must be used. There can be cases where a path appears as a better, more natural fit without it being strictly impossible to use a query. (As an analogy from another sphere, blogs could use query strings rather than paths for categorizing things, but most don't, since anything that implies any sort of hierarchy naturally maps better to a path.)

In limiting to a query string, it prevents anyone from choosing the best tool for the job at hand. Worse yet, if they have existing clients, it might be forcing them to rewrite them (cf. @andrew-s above).

This is doubly true of anyone coming from another FaaS service (Lambda, Google Cloud, etc.) because they all support this.

Beyond that, it makes sense to be able to host multiple, logically related endpoints in the same "function": /math/add, /math/sub, /math/div, etc. There is no earthly reason why someone would to have their 'addition' container scale independently of their 'subtraction' container. It is probably 99.9% the same code, same libraries, etc. Much easier to have one container. Yes, someone could do ?op=add|sub but why is that obviously better?

In my specific case, I have a hard requirement to be able to deploy the same "function" to Lambda as well as OpenFaaS. Running at Lambda, it can be hit at (not the real names, but for example): /status, /plan, /apply. That is impossible in OpenFaaS without this proposal... the Lambda version came first, so without this, to keep the code consistent I would need to: (A) change the Lambda code, (B) change all the other Lambda functions that are consumers of the current Lambda function, (C) change all the other non-Lambda clients (CLI, web, etc.) that call the Lambda function.

Or I could modify OpenFaaS simply not to swallow the path... Obviously I went the latter route for us.

I am not even convinced that (B) and (C) would be possible in many cases, since third-parties may be using the function as well, and you can't expect them to change their code.

That was the long version. The short version is that I would turn the question around a bit and ask: Why should a function that is normal and runs well on Lambda or Google Cloud be deliberately impossible to port to OpenFaaS?

@andrew-s
Copy link

andrew-s commented Jul 13, 2018

Not sure I could add much more to this but, for me for example, query strings in rest API's are only useful in some situations, e.g. you wouldn't want to do;

/category?catid=6

Instead, you'd want it to look like;

/category/6 or /category/cat-name

In REST this is cleaner, as you can do then do

PUT /category/cat-name to update the category
DELETE /category/cat-name to delete the category

Begs the question, would PUT-function-name be a different function from DELETE-function-name - in theory, yes as you wouldn't want to switch between in code. So really, path + method = function handler to call.

Definitely makes it easier to build requests or to organise them in the client application. I was thinking of a solution that didn't require a patch if it's deemed this is outside the scope of this project. A function could be made as a url-matching proxy then using an Nginx gateway in the same swarm with the configuration to point towards that singular function.

Hope that helps anyway.

@alexellis
Copy link
Member

alexellis commented Jul 20, 2018

@telackey I've had a chat with David from Wireline.io - he gave me some more details on the use-case for this feature. Let's move ahead with adding arbitrary data in the path after the /function//*

Please can you write a PR for this, the of-watchdog and the traditional watchdog too?

All of the of-watchdog based templates will need to be updated too: Go, Python, Node and Java - https://github.com/openfaas-incubator

I think it could potentially break backwards compatibility with all existing OpenFaaS images, so we would need your help in doing some due diligence around that too.

Alex

@alexellis
Copy link
Member

Derek assign: ericstoekl

@telackey
Copy link
Contributor Author

@alexellis

I'll open a PR.

@andrew-s
Copy link

Is this implementation set around just parameters as functions or an implementation or REST into OpenFaaS?

E.g. For true rest, we'd want to be able to specify method + path (with parameters, and ideally for these to be named parameters so they can be referenced within the function).

Would there also be an implementation of middleware like in AWS's API Gateway - you can reference a function to act as the authorisation middleware which you can specify on the endpoint. I would like to see a more flexible middleware implementation for REST (than in API Gateway) but I don't know how far that goes beyond this scope?

@alexellis
Copy link
Member

Hi Andrew,

Is this implementation set around just parameters as functions or an implementation or REST into OpenFaaS?

I spoke to the guys at https://wireline.io yesterday. about how they are using OpenFaaS and why they need the additional path after the function name, but I'm not clear on your specific use-case. Please can you tell me a bit more about how your business is using OpenFaaS?

There was a wild-card route for OpenFaaS which I think I removed at some stage, perhaps it would work better for your specific use-case if re-introduced?

It worked liked this:

  • Host: * <- put whatever you like
  • Path: * <- put whatever you like
  • Method: POST/GET

Specify function via header X-Function: nodeinfo

I would like to see a more flexible middleware implementation for REST (than in API Gateway) but I don't know how far that goes beyond this scope?

This is not well defined. Perhaps you should raise a separate issue with your specific requirements at this point.

Regards,

Alex

@alexellis
Copy link
Member

alexellis commented Jul 21, 2018

@telackey re: my comment on no-harm / smoke-testing:

#716 (comment)

As I recall all of the templates based-upon of-watchdog only have one route which is / - this works well with the data they are sent from the gateway. As soon as I broadened the of-watchdog to support any PATH, that was being sent down to the handler code (in Java/Node/Python etc) and then mis-matching the handler.

Example:

https://github.com/openfaas-incubator/node8-express-template/blob/master/template/node8-express/index.js#L83

The fix appeared to be to set the HTTP middle-ware to listen bind to the "*" route instead.

Here is the issue I opened in of-watchdog: openfaas/of-watchdog#16

So within this issue let's only address the work @telackey needs. I appreciate this looks like a small patch and an "easy problem" on the surface, but it could cause issues for existing users unless well-managed.

My prior research shows that this could be a breaking change which is not good for a project which is being used in production by businesses. Therefore I'd ask for the following changes and due diligence:

  • patch: extend the routes the gateway will allow such as: /function/<name>/<any>/<parameter>/<here>/<whatever>/<you>/<want>

  • test all templates based-upon of-watchdog to see if this is a breaking change, if it is to update the HTTP middleware to cope with that

  • regression test the legacy templates i.e. https://github.com/openfaas/templates - based-around Node, CSharp, Ruby etc.

https://github.com/openfaas-incubator/node8-express-template
https://github.com/openfaas-incubator/python-flask-template
https://github.com/openfaas-incubator/golang-http-template
https://github.com/openfaas/templates/tree/master/template/java8

Alex

@ivanayov
Copy link
Contributor

ivanayov commented Aug 1, 2018

I think this needs some regression testing with already existing functions

@alexellis
Copy link
Member

Derek close: released in 0.9.0

@alexellis
Copy link
Member

@gmcelhoe @andrew-s please go ahead and upgrade to get your new feature 🎉

@gmcelhoe
Copy link

gmcelhoe commented Sep 3, 2018 via email

@telackey
Copy link
Contributor Author

telackey commented Sep 7, 2018

Thanks, @alexellis !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants