envoy icon indicating copy to clipboard operation
envoy copied to clipboard

conn_manager: add overload header if request is dropped due to OM

Open leosarra opened this issue 1 year ago • 9 comments

Commit Message: Add x-envoy-overloaded flag to local reply sent by connection manager in case Overload Manager led to the need of shedding some load. Added an API field in connection manager to allow the suppression of the x-envoy-overload flag.

Additional Description: This is different from the handling in the http filter where x-envoy-overloaded flag added when maintenance mode is enabled or when the upstream circuit breaking is triggered. The connection manager does not initialize the filters when "envoy overload" is detected thus requiring special handling. N.B: For the time being I used the existing x-envoy-overloaded header, if you prefer I can introduce a different header exclusive to the case where the envoy itself is overloaded (e.g: x-envoy-local-overloaded).

To prevent malicious actors from gaining information about Envoy's status, a flag has been added in the API to suppress the header.

Fixes #15106

Risk Level: Low Testing: Added unit-tests and integration test for verifying the header being added Docs Changes: Done Release Notes: Done Platform Specific Features: -

leosarra avatar Jan 19 '24 15:01 leosarra

Hi @leosarra, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/31907 was opened by leosarra.

see: more, trace.

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/). envoyproxy/api-shepherds assignee is @adisuissa CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/31907 was opened by leosarra.

see: more, trace.

Thank you everyone of the feedback :). I will soon have some bandwidth and I will start addressing all the comments

leosarra avatar Feb 03 '24 15:02 leosarra

Based on the feedback the following changes were performed:

  • The header that is added in case envoy itself is overloaded (based on OM) is now "x-envoy-local-overloaded", I do not reuse anymore the "x-envoy-overloaded" header which remains exclusive to upstream circuit breaking usecases
  • The "x-envoy-local-overloaded" is no longer added by default, it is controlled by a skip_local_overload_append flag in connection manager (default: true)
  • doc changed accordingly
  • test that the header is not added by default. I'm a bit lost on how to add a test to verify that the header is added when the flag is set, I'm having difficulty in mocking connectionManagerConfig for conn_manager_impl_test_2.cc :(

leosarra avatar Feb 06 '24 14:02 leosarra

I'm still implementing the integration tests

leosarra avatar Feb 20 '24 09:02 leosarra

I have rebased and made the requested changes. Test coverage via integration tests has been added as well. Let me know if it looks good to you

leosarra avatar Feb 20 '24 12:02 leosarra

Thank you all of you for the feedback :)

leosarra avatar Feb 23 '24 14:02 leosarra

/assign-from @envoyproxy/senior-maintainers

adisuissa avatar Feb 23 '24 14:02 adisuissa

@envoyproxy/senior-maintainers assignee is @wbpcode

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/31907#issuecomment-1961436917 was created by @adisuissa.

see: more, trace.