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

Can we support FilterHeadersStatus::StopIteration ? #367

Open
gengleilei opened this issue Sep 21, 2023 · 2 comments
Open

Can we support FilterHeadersStatus::StopIteration ? #367

gengleilei opened this issue Sep 21, 2023 · 2 comments

Comments

@gengleilei
Copy link

We have a usage scenario where we need to extract data from the request body, then store it in the request header, and perform routing matching based on the request header during routing, sample code:

FilterHeadersStatus PluginContext::onRequestHeaders(uint32_t, bool) {
  auto content_type_ptr = getRequestHeader("content-type");
  auto transfer_encoding_ptr = getRequestHeader("transfer-encoding");
  if (!content_type_ptr && !transfer_encoding_ptr) {
    return FilterHeadersStatus::Continue;
  }

  stopIteration_ = true;
  return FilterHeadersStatus::StopIteration;
}

FilterDataStatus PluginContext::onRequestBody(size_t body_size, bool) {
  auto body =
      getBufferBytes(WasmBufferType::HttpRequestBody, 0, body_size);
  LOG_INFO("body: " + body->toString());

  if (stopIteration_ && (0 != body->size())) {
    addRequestHeader("body-test", body->toString());
  }

  return FilterDataStatus::Continue;
}

however, StopIteration is forced to be converted to StopAllIterationAndWatermark in proxy-wasm-cpp-host, can we remove this forced conversion?

FilterHeadersStatus ContextBase::convertVmCallResultToFilterHeadersStatus(uint64_t result) {
  if (wasm()->isNextIterationStopped() ||
      result > static_cast<uint64_t>(FilterHeadersStatus::StopAllIterationAndWatermark)) {
    return FilterHeadersStatus::StopAllIterationAndWatermark;
  }
  if ((wasm_->on_request_headers_abi_01_ || wasm_->on_request_headers_abi_02_) &&
      result == static_cast<uint64_t>(FilterHeadersStatus::StopIteration)) {
    // Always convert StopIteration (pause processing headers, but continue processing body)
    // to StopAllIterationAndWatermark (pause all processing), since the former breaks all
    // assumptions about HTTP processing.
    return FilterHeadersStatus::StopAllIterationAndWatermark;
  }
  return static_cast<FilterHeadersStatus>(result);
}

I think it would be better to let developers choose which state to use.

@gengleilei
Copy link
Author

@PiotrSikora any suggestions?

@martijneken
Copy link
Contributor

AIUI #397 was sent for this. Do you want to discuss?

This comment implies StopIteration is not good:

// Always convert StopIteration (pause processing headers, but continue processing body)
// to StopAllIterationAndWatermark (pause all processing), since the former breaks all
// assumptions about HTTP processing.

Source enum:
https://github.com/envoyproxy/envoy/blob/92118595813ac208b0072ab280d7d1b199ab83dd/envoy/http/filter.h#L109

And I think these issues are likely related:

Want to close this issue as a dupe of #143?

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

No branches or pull requests

2 participants