Address review feedback

envoyproxy · Jan 8, 2025 · af3cf1f · af3cf1f
1 parent db1d1d2
commit af3cf1f
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 35 deletions.
diff --git a/source/extensions/dynamic_modules/sdk/rust/src/lib.rs b/source/extensions/dynamic_modules/sdk/rust/src/lib.rs
@@ -335,11 +335,11 @@ pub trait EnvoyHttpFilter {
   /// 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>(
+  fn send_response<'a>(
     &mut self,
     status_code: u32,
-    headers: Vec<(&'a str, &'b [u8])>,
-    body: Option<&'c [u8]>,
+    headers: Vec<(&'a str, &'a [u8])>,
+    body: Option<&'a [u8]>,
   );
 
   /// Get the number-typed dynamic metadata value with the given key.
@@ -528,6 +528,13 @@ impl EnvoyHttpFilter for EnvoyHttpFilterImpl {
     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);
 
+    // Note: Casting a (&str, &[u8]) to an abi::envoy_dynamic_module_type_module_http_header works
+    // not because of any formal layout guarantees but because:
+    // 1) tuples _in practice_ are laid out packed and in order
+    // 2) &str and &[u8] are fat pointers (pointers to DSTs), whose layouts _in practice_ are a
+    //    pointer and length
+    // If these assumptions change, this will break. (Vec is guaranteed to point to a contiguous
+    // array, so it's safe to cast to a pointer)
     let headers_ptr = headers.as_ptr() as *mut abi::envoy_dynamic_module_type_module_http_header;
 
     unsafe {

diff --git a/source/extensions/filters/http/dynamic_modules/filter.cc b/source/extensions/filters/http/dynamic_modules/filter.cc
@@ -25,31 +25,19 @@ void DynamicModuleHttpFilter::destroy() {
 
 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_);
@@ -68,31 +56,19 @@ 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_);
@@ -107,11 +83,6 @@ 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);
 }
 

diff --git a/source/extensions/filters/http/dynamic_modules/filter.h b/source/extensions/filters/http/dynamic_modules/filter.h
@@ -78,9 +78,6 @@ 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;
-
   /**
    * This is a helper function to get the `this` pointer as a void pointer which is passed to the
    * various event hooks.