Refactor around VM failure check on Http/Tcp callbacks. #155
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
mathetake
commented
Apr 21, 2021
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
mathetake
changed the title
|
OK now envoy test passes. |
PiotrSikora
reviewed
Apr 26, 2021
Signed-off-by: Takeshi Yoneda <[email protected]>
PiotrSikora
reviewed
Apr 27, 2021
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
PiotrSikora
approved these changes
Apr 27, 2021
There was a problem hiding this comment.
I'm wondering if those pre-checks are even necessary? We catch all initialization failures in configuration phase, and all runtime failures should be caught by those new post-checks.
Am I missing something?
In any case, this is definitely an improvement, so feel free to merge as-is once Envoy tests pass.
|
Good point. For other stream contexts than the context where VM traps, the processing normally proceeds even after the VM failure. That's why we must have prechecks as well..? |
|
Oh yeah, good point. |
lizan
pushed a commit
to envoyproxy/envoy
that referenced
this pull request
May 4, 2021
Fixes #14947 and properly closes streams. This commit differentiates `failStream` from `closeStream` where the former is called when a VM fails, and the latter is called via `close_stream` or `grpc_close` by user Wasm codes. Notably, we try to send local response with 503 for http streams as expected by the description of `fail_open` api. The change here is a little and mostly done in Proxy-Wasm C++ host implementation(proxy-wasm/proxy-wasm-cpp-host#155). Signed-off-by: Takeshi Yoneda <[email protected]>
gokulnair
pushed a commit
to gokulnair/envoy
that referenced
this pull request
May 6, 2021
Fixes envoyproxy#14947 and properly closes streams. This commit differentiates `failStream` from `closeStream` where the former is called when a VM fails, and the latter is called via `close_stream` or `grpc_close` by user Wasm codes. Notably, we try to send local response with 503 for http streams as expected by the description of `fail_open` api. The change here is a little and mostly done in Proxy-Wasm C++ host implementation(proxy-wasm/proxy-wasm-cpp-host#155). Signed-off-by: Takeshi Yoneda <[email protected]> Signed-off-by: Gokul Nair <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant to envoyproxy/envoy#14947.
Refactored around VM failure check on Http and Tcp callbacks in order to handle the VM failure right after it happens. Previously, for example, when panic happens in OnResponseHeaders, then we return Continue since we didn't check the
isFailafter the Wasm calls. That means Envoy sends the response headers to the client even if the VM fails in OnResponseHeaders, and the failClose is called on OnResponseBody. This seems problematic and unintended.This is WIP since I haven't passed Envoy tests.Signed-off-by: Takeshi Yoneda [email protected]