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

dynamic_modules: adds send response #37856

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions source/extensions/dynamic_modules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ envoy_cc_library(
"-Wl,--export-dynamic-symbol=envoy_dynamic_module_callback_http_get_response_trailers",
"-Wl,--export-dynamic-symbol=envoy_dynamic_module_callback_http_get_response_trailers_count",
"-Wl,--export-dynamic-symbol=envoy_dynamic_module_callback_http_set_response_trailer",
"-Wl,--export-dynamic-symbol=envoy_dynamic_module_callback_http_send_response",
],
deps = [
":abi_version_lib",
Expand Down
36 changes: 34 additions & 2 deletions source/extensions/dynamic_modules/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@
#ifdef __cplusplus
#include <cstdbool>
#include <cstddef>
#include <cstdint>

extern "C" {
#else

#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>

#endif

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -128,8 +131,19 @@ typedef char* envoy_dynamic_module_type_buffer_module_ptr;
typedef char* envoy_dynamic_module_type_buffer_envoy_ptr;

/**
* envoy_dynamic_module_type_Header represents a key-value pair of an HTTP header owned by Envoy's
* HeaderMap.
* envoy_dynamic_module_type_module_http_header represents a key-value pair of an HTTP header owned
* by the module.
*/
typedef struct {
envoy_dynamic_module_type_buffer_module_ptr key_ptr;
size_t key_length;
envoy_dynamic_module_type_buffer_module_ptr value_ptr;
size_t value_length;
} envoy_dynamic_module_type_module_http_header;

/**
* envoy_dynamic_module_type_http_header represents a key-value pair of an HTTP header owned by
* Envoy's HeaderMap.
*/
typedef struct {
envoy_dynamic_module_type_buffer_envoy_ptr key_ptr;
Expand Down Expand Up @@ -606,6 +620,24 @@ bool envoy_dynamic_module_callback_http_set_response_trailer(
envoy_dynamic_module_type_buffer_module_ptr key, size_t key_length,
envoy_dynamic_module_type_buffer_module_ptr value, size_t value_length);

/**
* envoy_dynamic_module_callback_http_send_response is called by the module to send the response
* to the downstream.
*
* @param filter_envoy_ptr is the pointer to the DynamicModuleHttpFilter object of the
* corresponding HTTP filter.
* @param status_code is the status code of the response.
* @param headers_vector is the array of envoy_dynamic_module_type_module_http_header that contains
* the headers of the response.
* @param headers_vector_size is the size of the headers_vector.
* @param body is the pointer to the buffer of the body of the response.
* @param body_length is the length of the body.
*/
void envoy_dynamic_module_callback_http_send_response(
envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, uint32_t status_code,
envoy_dynamic_module_type_module_http_header* headers_vector, size_t headers_vector_size,
envoy_dynamic_module_type_buffer_module_ptr body, size_t body_length);

#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/dynamic_modules/abi_version.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace DynamicModules {
#endif
// This is the ABI version calculated as a sha256 hash of the ABI header files. When the ABI
// changes, this value must change, and the correctness of this value is checked by the test.
const char* kAbiVersion = "27dbe7918b67b12bfcf43e050ce142a300feb73fb3bd5779c754d1d1d3c8645f";
const char* kAbiVersion = "5867e2259f40fd9e63a47a61e89ab7fe2e87d22e09aa5c8c198a5ec9105a9608";

#ifdef __cplusplus
} // namespace DynamicModules
Expand Down
30 changes: 29 additions & 1 deletion source/extensions/dynamic_modules/sdk/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,17 @@ pub trait EnvoyHttpFilter {
///
/// Returns true if the operation is successful.
fn set_response_trailer(&mut self, key: &str, value: &[u8]) -> bool;

/// Send a response to the downstream with the given status code, headers, and body.
///
/// The headers are passed as a list of key-value pairs.
#[allow(clippy::needless_lifetimes)] // Explicit lifetime specifiers are needed for mockall
bplotnick marked this conversation as resolved.
Show resolved Hide resolved
fn send_response<'a, 'b, 'c>(
&mut self,
status_code: u32,
headers: Vec<(&'a str, &'b [u8])>,
body: Option<&'c [u8]>,
);
Comment on lines +338 to +343
Copy link
Member

Choose a reason for hiding this comment

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

nit: I doubt independent lifetimes here are actually needed. The trait impl below doesn't even repeat them either (there is a clippy lint that usually fires about this, maybe we don't have it on.) I would just remove them all. The default lifetime should be ok I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without them it fails with

`&` without an explicit lifetime name cannot be used here

AFAICT, it's not possible due with mockall. asomers/mockall#61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, at the top of the trait, we explicitly disable clippy::needless_lifetimes because of this:

#[allow(clippy::needless_lifetimes)] // Explicit lifetime specifiers are needed for mockall.

}

/// This implements the [`EnvoyHttpFilter`] trait with the given raw pointer to the Envoy HTTP
Expand Down Expand Up @@ -453,7 +464,6 @@ impl EnvoyHttpFilter for EnvoyHttpFilterImpl {
}
}


fn get_response_trailer_value(&self, key: &str) -> Option<EnvoyBuffer> {
self.get_header_value_impl(
key,
Expand Down Expand Up @@ -490,6 +500,24 @@ impl EnvoyHttpFilter for EnvoyHttpFilterImpl {
)
}
}

fn send_response(&mut self, status_code: u32, headers: Vec<(&str, &[u8])>, body: Option<&[u8]>) {
let body_ptr = body.map(|s| s.as_ptr()).unwrap_or(std::ptr::null());
let body_length = body.map(|s| s.len()).unwrap_or(0);

let headers_ptr = headers.as_ptr() as *mut abi::envoy_dynamic_module_type_module_http_header;
Copy link
Member

Choose a reason for hiding this comment

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

I'm very, very surprised this works. I don't really see how it can so I must be missing something. Is this covered by a test? I don't see what could properly convert this into the struct form that the ABI expects? If it does work I would be concerned that it's basically by accident. I'm not sure if layout guarantees this will always be true. Maybe it does?

Copy link
Member

@mathetake mathetake Jan 8, 2025

Choose a reason for hiding this comment

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

so this is an (unstable) secret that I have used in another project... basically Rust hasn't changed the memory layout of &[u8] (and &str which is exactly the same as the former at memory level) before since the release v1.0 which is a pair of (ptr and size). Also it hasn't changed the layout of tuple in the sense that it always packs in the order in which element appear. That's the reason why this works. Having said that though, this is also not guaranteed by Rust team whose ABI is not stable.

We can copy the vector here into envoy_dynamic_module_type_module_http_header with the stable repr(C) guaranteed by the Rust compiler to ensure the stability but not sure if we want to do that or not. On one hand, I think it should be fine and detected by the CI when the new Rust toolchain breaks this, which hans't happened before at all, but on the other hand, we can eliminate the concern over the (negligible?) gain.

Alternatively, we can accept the iterator at this API that allows us to construct the C stable vector without constructing vectors twice (by user and internally here with the current code).


unsafe {
abi::envoy_dynamic_module_callback_http_send_response(
self.raw_ptr,
status_code,
headers_ptr,
headers.len(),
body_ptr as *mut _,
body_length,
)
}
}
}

impl EnvoyHttpFilterImpl {
Expand Down
25 changes: 25 additions & 0 deletions source/extensions/filters/http/dynamic_modules/abi_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,31 @@ bool envoy_dynamic_module_callback_http_get_response_trailers(
DynamicModuleHttpFilter* filter = static_cast<DynamicModuleHttpFilter*>(filter_envoy_ptr);
return getHeadersImpl(filter->response_trailers_, result_headers);
}

void envoy_dynamic_module_callback_http_send_response(
envoy_dynamic_module_type_http_filter_envoy_ptr filter_envoy_ptr, uint32_t status_code,
envoy_dynamic_module_type_module_http_header* headers_vector, size_t headers_vector_size,
envoy_dynamic_module_type_buffer_module_ptr body_ptr, size_t body_length) {
DynamicModuleHttpFilter* filter = static_cast<DynamicModuleHttpFilter*>(filter_envoy_ptr);

std::function<void(ResponseHeaderMap & headers)> modify_headers = nullptr;
if (headers_vector != nullptr && headers_vector_size != 0) {
modify_headers = [headers_vector, headers_vector_size](ResponseHeaderMap& headers) {
for (size_t i = 0; i < headers_vector_size; i++) {
const auto& header = &headers_vector[i];
const absl::string_view key(static_cast<const char*>(header->key_ptr), header->key_length);
const absl::string_view value(static_cast<const char*>(header->value_ptr),
header->value_length);
headers.addCopy(Http::LowerCaseString(key), value);
}
};
}
const absl::string_view body =
body_ptr ? absl::string_view(static_cast<const char*>(body_ptr), body_length) : "";

bplotnick marked this conversation as resolved.
Show resolved Hide resolved
filter->sendLocalReply(static_cast<Http::Code>(status_code), body, modify_headers, 0,
"dynamic_module");
}
}
} // namespace HttpFilters
} // namespace DynamicModules
Expand Down
38 changes: 36 additions & 2 deletions source/extensions/filters/http/dynamic_modules/filter.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include "source/extensions/filters/http/dynamic_modules/filter.h"

#include "envoy/server/filter_config.h"

namespace Envoy {
namespace Extensions {
namespace DynamicModules {
Expand All @@ -17,19 +15,31 @@ void DynamicModuleHttpFilter::onDestroy() { config_->on_http_filter_destroy_(in_

FilterHeadersStatus DynamicModuleHttpFilter::decodeHeaders(RequestHeaderMap& headers,
bool end_of_stream) {
if (sent_local_reply_) {
return FilterHeadersStatus::Continue;
}

request_headers_ = &headers;
const envoy_dynamic_module_type_on_http_filter_request_headers_status status =
config_->on_http_filter_request_headers_(thisAsVoidPtr(), in_module_filter_, end_of_stream);
return static_cast<FilterHeadersStatus>(status);
};

FilterDataStatus DynamicModuleHttpFilter::decodeData(Buffer::Instance&, bool end_of_stream) {
if (sent_local_reply_) {
return FilterDataStatus::Continue;
}

const envoy_dynamic_module_type_on_http_filter_request_body_status status =
config_->on_http_filter_request_body_(thisAsVoidPtr(), in_module_filter_, end_of_stream);
return static_cast<FilterDataStatus>(status);
};

FilterTrailersStatus DynamicModuleHttpFilter::decodeTrailers(RequestTrailerMap& trailers) {
if (sent_local_reply_) {
return FilterTrailersStatus::Continue;
}

request_trailers_ = &trailers;
const envoy_dynamic_module_type_on_http_filter_request_trailers_status status =
config_->on_http_filter_request_trailers_(thisAsVoidPtr(), in_module_filter_);
Expand All @@ -48,19 +58,31 @@ Filter1xxHeadersStatus DynamicModuleHttpFilter::encode1xxHeaders(ResponseHeaderM

FilterHeadersStatus DynamicModuleHttpFilter::encodeHeaders(ResponseHeaderMap& headers,
bool end_of_stream) {
if (sent_local_reply_) {
return FilterHeadersStatus::Continue;
}

response_headers_ = &headers;
const envoy_dynamic_module_type_on_http_filter_response_headers_status status =
config_->on_http_filter_response_headers_(thisAsVoidPtr(), in_module_filter_, end_of_stream);
return static_cast<FilterHeadersStatus>(status);
};

FilterDataStatus DynamicModuleHttpFilter::encodeData(Buffer::Instance&, bool end_of_stream) {
if (sent_local_reply_) {
return FilterDataStatus::Continue;
}

const envoy_dynamic_module_type_on_http_filter_response_body_status status =
config_->on_http_filter_response_body_(thisAsVoidPtr(), in_module_filter_, end_of_stream);
return static_cast<FilterDataStatus>(status);
};

FilterTrailersStatus DynamicModuleHttpFilter::encodeTrailers(ResponseTrailerMap& trailers) {
if (sent_local_reply_) {
return FilterTrailersStatus::Continue;
}

response_trailers_ = &trailers;
const envoy_dynamic_module_type_on_http_filter_response_trailers_status status =
config_->on_http_filter_response_trailers_(thisAsVoidPtr(), in_module_filter_);
Expand All @@ -71,6 +93,18 @@ FilterMetadataStatus DynamicModuleHttpFilter::encodeMetadata(MetadataMap&) {
return FilterMetadataStatus::Continue;
}

void DynamicModuleHttpFilter::sendLocalReply(
Code code, absl::string_view body,
std::function<void(ResponseHeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status, absl::string_view details) {
if (sent_local_reply_) {
return;
}

sent_local_reply_ = true;
decoder_callbacks_->sendLocalReply(code, body, modify_headers, grpc_status, details);
}

void DynamicModuleHttpFilter::encodeComplete(){};

} // namespace HttpFilters
Expand Down
8 changes: 8 additions & 0 deletions source/extensions/filters/http/dynamic_modules/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ class DynamicModuleHttpFilter : public Http::StreamFilter,
}
void encodeComplete() override;

void sendLocalReply(Code code, absl::string_view body,
std::function<void(ResponseHeaderMap& headers)> modify_headers,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view details);

// The callbacks for the filter. They are only valid until onDestroy() is called.
StreamDecoderFilterCallbacks* decoder_callbacks_ = nullptr;
StreamEncoderFilterCallbacks* encoder_callbacks_ = nullptr;
Expand All @@ -60,6 +65,9 @@ class DynamicModuleHttpFilter : public Http::StreamFilter,
ResponseTrailerMap* response_trailers_ = nullptr;

private:
// This ensures that we do not re-enter the dynamic filter hooks when sending a local reply
bool sent_local_reply_ = false;
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Did it crash without this? I haven't been in the Envoy code deeply in a long time but historically I'm pretty sure that once a local reply is sent the filter should no longer be called again as everything will be reset.

Copy link
Member

Choose a reason for hiding this comment

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

back in the day i remember this crashes the wasm plugins because of the reentrance causing the multiple mutable borrowing, and that's why I suggested to do this - but i think it's worth revisiting this and see if this actually crashes... maybe the envoy behavior has changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to come up with a crasher (but that doesn't mean it's not possible).

The similar guard in the wasm filter was added here: #16593

But since that PR merged, b361fb6#diff-e91327a311d8130daac76a7c43dca2ae70d5a6a37dc7fdb658cf25b2fde88cc9R534 and #24367 were added that seems like it would make the filter callbacks non-reentrant after a local reply.

Copy link
Member

Choose a reason for hiding this comment

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

But since that PR merged, b361fb6#diff-e91327a311d8130daac76a7c43dca2ae70d5a6a37dc7fdb658cf25b2fde88cc9R534 and #24367 were added that seems like it would make the filter callbacks non-reentrant after a local reply.

hah i didn't know that. I think let's remove this flag then, and fix it if someone encounters the problem


/**
* This is a helper function to get the `this` pointer as a void pointer which is passed to the
* various event hooks.
Expand Down
Loading