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

Enhancement proposal: process controlled HTTP response code and headers #3

Open
kayila opened this issue Jan 15, 2018 · 13 comments
Open

Comments

@kayila
Copy link

kayila commented Jan 15, 2018

Brief summary including motivation/context

This change is to the streaming mode of of-watchdog to add a named pipe with which the running subprograms can send control messages back to of-watchdog in specify the HTTP response code and headers. This design will be done in such a way so as to not impact the behavior for any called functions which do not make use of the pipe provided. The motivation behind this change is that I need functions to be able to return specific response codes other than 200 for functions which I call.

Any design changes

The of-watchdog process will create a new named pipe when a function is called, then set the environment variable CONTROL_PIPE before calling the sub process. The of-watchdog will then listen on the control pipe file, if it recieves a JSON blob of the following format, it will use it to set the response code and headers before writing the output of the sub command. If no JSON blob is received before the subprocess begins to output on it's standard out, an HTTP response code of 200 will be sent with the first message.

Pros + Cons

Pros: Functions will now be able to return custom response codes and headers per request.
Cons: None.

Effort required up front

A few days of coding time.

Effort required for CI/CD, release, ongoing maintenance

Minimal, if any.

Migration strategy / backwards-compatibility

This change will be 100% backwards compatible with the existing program.

Mock-up screenshots or examples of how the CLI would work

N/A

@kayila
Copy link
Author

kayila commented Jan 18, 2018

@alexellis Could you please take a look at this?

@alexellis
Copy link
Member

So in the streaming mode your HTTP response header is returned immediately as soon as the first byte is transferred. You cannot send a HTTP response header a second time.

@kayila
Copy link
Author

kayila commented Jan 20, 2018

Correct. All the headers and status code must be sent in a single bundle in this design.

@alexellis
Copy link
Member

Unless I'm not following along then what I just mentioned would mean your change couldn't work in the streaming mode. Perhaps if you need an error after the fact link headers might be better suited or checking on the client-end to create a checksum from the result.

What is your concrete use-case here?

@kayila
Copy link
Author

kayila commented Jan 22, 2018

My concrete use case is using an openfaas function to provide authentication responses for nginx subrequest authentication. The nginx server needs a 200 code for access granted and a 400 code for access denied.

This proposal can be accomplished by putting a shim struct between the http.ResponseWriter and the stdout of the called process which will detect the first byte of the output and at that point, if there was a http response code or headers provided by in the pipe, using those for the response, defaulting to a 200 code if nothing was provided. This shim would only need to do this check on the first byte and would otherwise skip that test, instead passing the data along to the http.ResponseWriter transparently.

@duckinator
Copy link

I'd like to see this, too. Most usecases I can think of for openfaas or similar things require the ability to return non-200 status codes.

@kayila
Copy link
Author

kayila commented Jan 28, 2018

@alexellis I'm thinking perhaps I didn't do the best job explaining what my idea is in full. In my proposal, a side channel will be opened and, when the first byte of output would be sent back to the client, that side pipe is checked first. If that pipe contains any data, it dumps that before the first byte of stdout. After the first byte is sent to the client, that check no longer occurs. So in this design the program would still be required to set status code and headers before sending the body of the response, but it would allow something other than a 200 to be returned.

@alexellis
Copy link
Member

Hi guys - we are working on something that will allow a http status to be returned.

Using a pipe as well as stdio seems orthogonal and I'm not keen on opening additional files or pipes per request - as these have to be managed and garbage collected. Under high load these could result in undesired side-effects.

If you're on the OpenFaaS slack already you can read more about the plans in #contributors with some related notes at openfaas/faas#494

To join the Slack community just email [email protected]

@kayila
Copy link
Author

kayila commented Jan 29, 2018

The issue you've linked to doesn't cover the streaming mode, only the forking and afterburn methods. My proposed solution works for streaming, and could easily be used for both forking and afterburn as well. But lets discuss it over slack rather than continuing to go back and forth in this thread.

@alexellis
Copy link
Member

alexellis commented Jan 29, 2018

Going forward the new of-watchdog will most likely retain the forking mode and the HTTP mode only. The HTTP mode will allow a user to return any response status code / header.

The forking mode I would not see changing much beyond its current interface. It is possible to set an alternative HTTP code with a non-zero exit code. This currently maps like this:

  • Zero = 200
  • Non-zero = 500

What programming language would you like to use to return the 200/400 status code?

@kayila
Copy link
Author

kayila commented Jan 29, 2018

Based on your wording, and looking at the README to confirm the mode types, I must assume that by 'forking mode' you mean 'Serializing Fork' and not 'Streaming Fork' because my proposed solution is primarily aimed at the Streaming Fork mode, which is still a forking mode.

It now sounds like you are saying you are planning to remove the Streaming Forking mode all together, which would break backwards compatibility which you expressly state that the of-watchdog process will maintain. Serializing Fork mode, as stated in the readme, reads everything into a buffer before passing it to the end user. This means you are limited to the memory your computer has and prevents doing large files which is the primary use case of the Streaming Fork mode.

As for the current interface of Serializing Fork, which again I must assume you mean as your post is ambiguous, returning either 200 or 500 is not sufficient for some use cases, including the use case I stated above of using Openfaas for subrequest authentication of an nginx server.

To more directly address the HTTP mode you refer to, I am uncertain why someone would bother using the of-watchdog process if their code is already providing an HTTP interface, instead of just having their process listen on the port directly. Adding of-watchdog in front of it seems to add unneeded overhead.

The language used to return the status codes should be irrelevant per your post as the Serializing Fork mode cannot return the 400 status codes, and the HTTP interface does not need to know anything about the language in process function's language.

Again, I would be happy to discuss this with you in slack, and have already sent a request to your email address to join the slack server, as you suggested. I await your invite.

@alexellis
Copy link
Member

Thank you for sharing your thoughts and opinions. For those who do not want to to consume HTTP within their process the forking mode will still be available.

The OpenFaaS-incubator repository contains experimental projects which do not provide a 1.0 API - that means that they are subject to change. Future versions of the watchdog will of course maintain compatibility with the existing watchdog.

To summarise before locking the thread - there will be two options for returning a HTTP status code:

@alexellis
Copy link
Member

Derek lock

@openfaas openfaas locked as too heated and limited conversation to collaborators Jan 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants