Commits on Oct 18, 2024

1. Don't pause processing when send_local_response fails …

   For context see the Envoy issue envoyproxy/envoy#28826.
   Here is a shorter summary:
   
   1. A wasm plugin calls proxy_send_local_response from both onRequestHeaders and
      onResponseHeaders
   2. When proxy_send_local_reply is called from onRequestHeaders it triggers
      a local reply and that reply goes through the filter chain in Envoy
   3. The same plugin is called again as part of the filter chain processing
      but this time onResponseHeaders is called
   4. onResponseHeaders calls proxy_send_local_response which ultimately does
      not generate a local reply, but it stops filter chain processing.
   
   As a result we end up with a stuck connection on Envoy - no local reply
   and processing is stopped.
   
   I think that proxy wasm plugins shouldn't use proxy_send_local_response this
   way, so ultimately whoever created such a plugin shot themselves in the foot.
   That being said, I think there are a few improvements that could be made here
   on Envoy/proxy-wasm side to handle this situation somewhat better:
   
   1. We can avoid stopping processing in such cases to prevent stuck connections
      on Envoy
   2. We can return errors from proxy_send_local_response instead of silently
      ignoring them.
   
   Currently Envoy implementation of sendLocalResponse can detect when a second
   local response is requested and returns an error in this case without actually
   trying to send a local response.
   
   However, even though Envoy reports an error, send_local_response ignores the
   result of the host specific sendLocalResponse implementation and stops
   processing and returns success to the wasm plugin.
   
   With this change, send_local_response will check the result of the
   host-specific implementation of the sendLocalResponse. In cases when
   sendLocalResponse fails it will just propagate the error to the caller and
   do nothing else (including stopping processing).
   
   I think this behavior makes sense in principle because on the one hand we don't
   ignore the failure from sendLocalResponse and on the other hand, when the
   failure happens we don't trigger any side-effects expected from the successful
   proxy_send_local_response call.
   
   NOTE: Even though I do think that this is a more resonable behavior, it's
   still a change from the previous behavior and it might break existing
   proxy-wasm plugins. Specifically:
   
   1. C++ plugins that proactively check the result of proxy_send_local_response
      will change behavior (e.g., before proxy_send_local_response failed silently)
   2. Rust plugins, due to the way Rust SDK handles errors from
      proxy_send_local_response will crash in runtime in this case.
   
   On the bright side of things, the plugins that are affected by this change
   currently just cause stuck connections in Envoy, so we are changing one
   undesirable behavior for another, but more explicit.
   
   Signed-off-by: Mikhail Krinkin <[email protected]>

   

   krinkinmu committed Oct 18, 2024
   Configuration menu

   • View commit details
   • Copy full SHA for 98fe532
   • Browse repository at this point

   Copy the full SHA

   98fe532 View commit details

   Browse the repository at this point in the history