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

send local response: clarify call sequence #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kyessenov
Copy link

Per discussion in envoyproxy/envoy#36809

@@ -1156,7 +1156,8 @@ Plugin must return one of the following values:

Sends HTTP response with body (`body_data`, `body_size`) and
[serialized] headers (`serialized_headers_data`,
`serialized_headers_size`).
`serialized_headers_size`). The response is sent after the current
callback completes and triggers response callbacks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the opposite of the intended behavior (which is that proxy_send_local_response() is always a final call).

We want plugins to do something like this:

proxy_on_request_headers() {
   ...
   return proxy_send_local_response(...);
}

and not something like this:

proxy_on_request_headers() {
   ...
   proxy_send_local_response(...);
   do_something_very_important();
   return Pause;
}

The reason this is deferred in Envoy is because the response callbacks (i.e. proxy_on_response_headers() and friends) are called immediately upon sending local response, which leads to re-entrancy problems, but that's another issue (I don't think that plugins should "receive" the local responses they generated themselves).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpwarres I think we have to specify it one way or another. How does WaaS behave here?

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

Successfully merging this pull request may close these issues.

2 participants