Skip to content

wasm: fix fail-close streams on VM failure.#16112

Merged
lizan merged 14 commits intoenvoyproxy:mainfrom
mathetake:wasm-fail-close
May 4, 2021
Merged

wasm: fix fail-close streams on VM failure.#16112
lizan merged 14 commits intoenvoyproxy:mainfrom
mathetake:wasm-fail-close

Conversation

@mathetake
Copy link
Copy Markdown
Member

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 takeshi@tetrate.io

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16112 was opened by mathetake.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 22, 2021
@mathetake
Copy link
Copy Markdown
Member Author

will add test cases..

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake
Copy link
Copy Markdown
Member Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16112 (comment) was created by @mathetake.

see: more, trace.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake marked this pull request as ready for review April 28, 2021 01:32
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Apr 28, 2021

Weird failure.. maybe I got stale merge, give me a moment fixed

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks!


TEST_P(WasmNetworkFilterTest, CloseUpstream) {
if (std::get<1>(GetParam()) == "rust") {
// TODO(mathetake): not yet supported in the Rust SDK.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is blocked on proxy-wasm/proxy-wasm-rust-sdk#64, I'll add this once it's merged, so feel free to re-assign to me (if there are other changes, but don't waste CI cycle on it).


TEST_P(WasmNetworkFilterTest, CloseDownstream) {
if (std::get<1>(GetParam()) == "rust") {
// TODO(mathetake): not yet supported in the Rust SDK.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is blocked on proxy-wasm/proxy-wasm-rust-sdk#64, I'll add this once it's merged, so feel free to re-assign to me (if there are other changes, but don't waste CI cycle on it).

@mathetake
Copy link
Copy Markdown
Member Author

kindly ping @envoyproxy/dependency-shepherds PTAL! :)

@PiotrSikora PiotrSikora requested a review from moderation April 30, 2021 08:22
@PiotrSikora
Copy link
Copy Markdown
Contributor

@moderation PTAL for deps.

Also, is there a way we could automate those dependency checks? We've been stuck many times for a multiple days on what appears to be a manual check to verify that the commit is merged into master / main branch.

@moderation
Copy link
Copy Markdown
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 1, 2021
@lizan lizan merged commit e0b03fe into envoyproxy:main May 4, 2021
@mathetake mathetake deleted the wasm-fail-close branch May 6, 2021 03:20
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 <takeshi@tetrate.io>
Signed-off-by: Gokul Nair <gnair@twitter.com>
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.

Wasm: provides useful response code/flag on VM failure

4 participants