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

Change callbacks to return specific response types #39

Closed
wants to merge 1 commit into from
Closed

Change callbacks to return specific response types #39

wants to merge 1 commit into from

Conversation

gbrail
Copy link
Contributor

@gbrail gbrail commented Oct 21, 2020

This replaces the "Action" enum that is used when returning from
the various callbacks to the specific callbacks types that are
also used by the C++ SDK, such as FilterHeadersStatus,
FilterDataStatus, and so on.

I understand that the original API was trying to present a somewhat simplified view of callbacks than the raw WASM ABI does, and that there are efforts to produce a better ABI. However, at the moment this change makes this SDK more understandable because it matches what internal Envoy code as well as the C++ SDK do, and since this is a low-level SDK, I think it should faithfully reproduce the whole ABI.

This replaces the "Action" enum that is used when returning from
the various callbacks to the specific callbacks types that are
also used by the C++ SDK, such as FilterHeadersStatus,
FilterDataStatus, and so on.
@gbrail gbrail requested a review from PiotrSikora as a code owner October 21, 2020 21:13
unleashed added a commit to 3scale/proxy-wasm-rust-sdk that referenced this pull request Nov 5, 2020
This is based on PR proxy-wasm#39 upstream.

Brings us a step closer to what the C++ SDK uses.

Signed-off-by: Alejandro Martinez Ruiz <[email protected]>
unleashed added a commit to 3scale/proxy-wasm-rust-sdk that referenced this pull request Nov 5, 2020
This is based on PR proxy-wasm#39 upstream.

Brings us a step closer to what the C++ SDK uses.

Signed-off-by: Alejandro Martinez Ruiz <[email protected]>
unleashed added a commit to 3scale/proxy-wasm-rust-sdk that referenced this pull request Nov 5, 2020
This is based on PR proxy-wasm#39 upstream.

Brings us a step closer to what the C++ SDK uses.

Signed-off-by: Alejandro Martinez Ruiz <[email protected]>
@PiotrSikora
Copy link
Member

What's the actual reasoning behind this change? Which return values are you missing?

In any case, API simplification was not the reason for Action, the two main reasons were:

a) It was developed while I was working on the updated ABI (proxy-wasm/spec#1), and Action is aligned with the return values there, but limited to what we supported in ABI v0.1.0, which Rust SDK implements. C++ SDK is closely tied to Envoy implementation, since it was extracted from that codebase, but it should be updated to match updated ABI, not the other way around.

b) The Proxy-Wasm effort is proxy-agnostic, and there is already work underway to implement it in other proxies, so adding Envoy specific return values is not desirable from that point of view.

@gbrail
Copy link
Contributor Author

gbrail commented Nov 12, 2020

In the fullness of time, I can imagine needing to return things like FilterDataStatus::StopAllIterationAndWatermark, for instance, but my main problem was that I needed a way to return StopAllIterationAndWatermark instead of StopIteration.

@PiotrSikora
Copy link
Member

In the fullness of time, I can imagine needing to return things like FilterDataStatus::StopAllIterationAndWatermark, for instance

That should be fixed within the next few weeks once the ABI and SDKs are updated.

but my main problem was that I needed a way to return StopAllIterationAndWatermark instead of StopIteration.

That's fixed now with proxy-wasm/proxy-wasm-cpp-host#95.

@gbrail
Copy link
Contributor Author

gbrail commented Dec 7, 2020

I think I can hold off on this and if I get back to a specific problem, I'll see what I can do.

@gbrail gbrail closed this Dec 7, 2020
@PiotrSikora
Copy link
Member

Thanks! I expect to have the ABI finalized and SDKs updated in O(weeks), so I don't think this will be needed.

@aweis89
Copy link

aweis89 commented May 26, 2021

@PiotrSikora when using send_http_response and then returning Action::Pause, I'm seeing the following in Envoy logs:

"scope":"envoy_bug","msg":"envoy bug failure: !state_.local_complete_ || status == FilterHeadersStatus::StopIteration. Details: Filters should return FilterHeadersStatus::StopIteration after sending a local reply."

@PiotrSikora
Copy link
Member

@aweis89 this is a bug in Envoy v1.17.x, see: envoyproxy/envoy#15496.

@aweis89
Copy link

aweis89 commented May 26, 2021

Owe thanks @PiotrSikora for your speedy reply! Since you seem rather familiar with the fix 😉 , would you happen to know off the top if this will lead to a more serious issue other than a log message?

@PiotrSikora
Copy link
Member

@aweis89 Envoy crashes in debug builds... To be honest, I'm not 100% sure what happens in release builds, but it's either a crash or no side-effects.

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.

3 participants