-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
base: main
Are you sure you want to change the base?
Changes from all commits
f16c3a6
2cf5cc6
45971ea
c86821f
86fe96a
304c395
e59e224
c12d800
bfb49f9
006082d
3ad32e4
b8a7ef1
f94fb8b
d56ac0e
db1d1d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -332,6 +332,16 @@ 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. | ||
fn send_response<'a, 'b, 'c>( | ||
&mut self, | ||
status_code: u32, | ||
headers: Vec<(&'a str, &'b [u8])>, | ||
body: Option<&'c [u8]>, | ||
); | ||
|
||
/// Get the number-typed dynamic metadata value with the given key. | ||
/// If the metadata is not found or is the wrong type, this returns `None`. | ||
fn get_dynamic_metadata_number(&self, namespace: &str, key: &str) -> Option<f64>; | ||
|
@@ -477,7 +487,6 @@ impl EnvoyHttpFilter for EnvoyHttpFilterImpl { | |
} | ||
} | ||
|
||
|
||
fn get_response_trailer_value(&self, key: &str) -> Option<EnvoyBuffer> { | ||
self.get_header_value_impl( | ||
key, | ||
|
@@ -515,6 +524,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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
) | ||
} | ||
} | ||
|
||
fn get_dynamic_metadata_number(&self, namespace: &str, key: &str) -> Option<f64> { | ||
let namespace_ptr = namespace.as_ptr(); | ||
let namespace_size = namespace.len(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -73,6 +78,9 @@ class DynamicModuleHttpFilter : public Http::StreamFilter, | |
} | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
AFAICT, it's not possible due with mockall. asomers/mockall#61
There was a problem hiding this comment.
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: