feature: inbound only graceful draining by keithmattix · Pull Request #37873 · envoyproxy/envoy

keithmattix · 2025-01-04T00:23:16Z

Commit Message:

Fixes https://github.com/envoyproxy/envoy/issues/35020. The inbound_only query param for the drain_listeners admin
endpoint doesn't work with the graceful query parameter. As a result,
outbound listeners will send connection: close headers to upstreams
which is undesired. This PR adds the ability for drain_manager to drain
in a single direction.

Additional Description: Prior to this change, the inbound_only query param on the drain_listeners admin endpoint only modified which listeners were stopped. If graceful is set, listeners of all directions are drained, regardless if inbound_only is set. If skip_exit is set, inbound_only has zero effect. This PR adds the ability to drain only inbound listeners, allowing outbound listeners to continue functioning as normal. This is useful in the Kubernetes sidecar use-case where outbound traffic should not set connection: close headers to the upstream.

Risk Level: Medium
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA] fixes #35020
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2025-01-04T00:23:21Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #37873 was opened by keithmattix.

see: more, trace.

keithmattix · 2025-01-04T00:23:42Z

Fixes #35020

Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

KBaichoo

Thank you for working on this -- few comments / questions:

Was directional draining not working at all prior to this?
Can you add release notes?
Can you fillout the PR description
Can you add an integration test draining inbound only listeners using the admin endpoint so we can have something testing this e2e.
I think some other network filters support draining but probably don't support directionality, can those also be documented?

/wait

envoy/network/drain_decision.h

source/server/admin/listeners_handler.cc

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-01-10T17:53:48Z

Thank you for working on this -- few comments / questions:

1. Was directional draining not working at all prior to this?

2. Can you add release notes?

3. Can you fillout the PR description

4. Can you add an integration test draining inbound only listeners using the admin endpoint so we can have something testing this e2e.

5. I _think_ some other network filters support draining but probably don't support directionality, can those also be documented?

/wait

Directional draining "worked" in the since that only inbound listeners were stopped, but the drain manager drained all listeners regardless of direction.
Done
Done
Done
Documented where? Can you expand a bit on the ask?

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

jmarantz · 2025-01-15T14:14:51Z

needs main merge

/wait

KBaichoo

Sorry for the delay, this mostly looks good. Thank you!

Documented where? Can you expand a bit on the ask?

According to https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/operations/draining some other network filters
support draining (Redis, Mongo, Thrift) but wouldn't support this newly added directional component?

Maybe documented in https://www.envoyproxy.io/docs/envoy/latest/operations/admin#operations-admin-interface-drain?

/wait

envoy/network/drain_decision.h

test/integration/integration_admin_test.cc

test/extensions/filters/network/common/fuzz/utils/fakes.h

source/server/admin/listeners_handler.cc

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-01-23T22:23:28Z

@KBaichoo I think this is ready for another round of review; thanks for the comments!

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

KBaichoo

This looks great. Few small comments, almost there. Thank you.

/wait

source/server/admin/listeners_handler.cc

source/server/drain_manager_impl.cc

test/integration/integration_admin_test.cc

test/common/http/conn_manager_impl_test_2.cc

source/common/listener_manager/listener_manager_impl.h

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix · 2025-01-24T20:08:45Z

/retest

KBaichoo

Thank you LGTM.

/assign @ggreenway

for senior maintainer pass

alyssawilk · 2025-01-31T12:59:02Z

@ggreenway ping?

Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. Prior to this change, the `inbound_only` query param on the [`drain_listeners` admin endpoint ](https://www.envoyproxy.io/docs/envoy/latest/operations/admin#operations-admin-interface-drain) only modified which listeners were stopped. If `graceful` is set, listeners of all directions are drained, regardless if `inbound_only` is set. If `skip_exit` is set, `inbound_only` has zero effect. This PR adds the ability to drain only inbound listeners, allowing outbound listeners to continue functioning as normal. This is useful in the Kubernetes sidecar use-case where outbound traffic should not set connection: close headers to the upstream. Risk Level: Medium fixes envoyproxy#35020 Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

This reverts commit 916594d. Signed-off-by: Ryan Northey <ryan@synca.io>

Fix #38330 Signed-off-by: Ryan Northey <ryan@synca.io>

@yanavlasov

Trying #37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after #38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes #35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

@yanavlasov

Trying envoyproxy#37873 again to get to the bottom of the flakes that caused us to need to revert. This got a lot more complex after envoyproxy#38333, so I may need @yanavlasov to help me out with the proper semantics Commit Message: ``` Fixes envoyproxy#35020. The inbound_only query param for the drain_listeners admin endpoint doesn't work with the graceful query parameter. As a result, outbound listeners will send connection: close headers to upstreams which is undesired. This PR adds the ability for drain_manager to drain in a single direction. ``` Additional Description: Risk Level: Medium Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix changed the title ~~Feature/inbound only graceful draining~~ feature: inbound only graceful draining Jan 4, 2025

keithmattix marked this pull request as ready for review January 6, 2025 05:41

keithmattix requested review from adisuissa, alyssawilk and mattklein123 as code owners January 6, 2025 05:41

keithmattix force-pushed the feature/inbound-only-graceful-draining branch from cfa48b0 to 5bd97d9 Compare January 6, 2025 16:19

keithmattix added 3 commits January 6, 2025 17:07

Fix existing tests

3d4b602

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Add draining tests

18fa880

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

zuercher assigned KBaichoo Jan 6, 2025

keithmattix force-pushed the feature/inbound-only-graceful-draining branch from 5bd97d9 to 18fa880 Compare January 6, 2025 18:20

KBaichoo reviewed Jan 8, 2025

View reviewed changes

envoy/network/drain_decision.h Show resolved Hide resolved

source/server/admin/listeners_handler.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jan 8, 2025

Add admin integration test and release note

1f49279

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

repokitteh-read-only bot removed the waiting label Jan 10, 2025

Merge branch 'main' into feature/inbound-only-graceful-draining

3c9528d

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix added 2 commits January 10, 2025 19:00

Remove backticks

206c337

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Fix spelling

d7b4400

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix requested a review from KBaichoo January 14, 2025 16:14

repokitteh-read-only bot added the waiting label Jan 15, 2025

KBaichoo reviewed Jan 15, 2025

View reviewed changes

keithmattix added 2 commits January 15, 2025 17:54

Merge branch 'main' into feature/inbound-only-graceful-draining

6178bf1

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Address PR feedback

ddef26a

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

repokitteh-read-only bot removed the waiting label Jan 15, 2025

keithmattix added 2 commits January 15, 2025 19:30

Add some docs

cb202e7

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Try to make drainDirection irrelevant

869832b

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

keithmattix requested a review from KBaichoo January 23, 2025 22:22

Add spelling fix

c67faf2

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Just remove comment

186a380

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

KBaichoo reviewed Jan 24, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 24, 2025

Add test back

2e7b766

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

repokitteh-read-only bot removed the waiting label Jan 24, 2025

keithmattix requested a review from KBaichoo January 24, 2025 17:07

keithmattix added 2 commits January 24, 2025 19:07

Fix spelling in comments

92ab8d4

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

Fixup docs and comments

ec78510

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>

KBaichoo approved these changes Jan 27, 2025

View reviewed changes

KBaichoo assigned ggreenway Jan 27, 2025

KBaichoo assigned yanavlasov and unassigned ggreenway Feb 4, 2025

yanavlasov approved these changes Feb 6, 2025

View reviewed changes

yanavlasov merged commit 916594d into envoyproxy:main Feb 6, 2025

keithmattix mentioned this pull request Feb 6, 2025

[cherrypick/v1.33] feature: inbound only graceful draining (#37873) #38331

Closed

phlax added a commit to phlax/envoy that referenced this pull request Feb 6, 2025

Revert "feature: inbound only graceful draining (envoyproxy#37873)"

e9deef2

This reverts commit 916594d. Signed-off-by: Ryan Northey <ryan@synca.io>

phlax added a commit to phlax/envoy that referenced this pull request Feb 6, 2025

Revert "feature: inbound only graceful draining (envoyproxy#37873)"

5b91d05

This reverts commit 916594d. Signed-off-by: Ryan Northey <ryan@synca.io>

phlax added a commit that referenced this pull request Feb 6, 2025

Revert "feature: inbound only graceful draining (#37873)" (#38329)

3435d69

Fix #38330 Signed-off-by: Ryan Northey <ryan@synca.io>

howardjohn mentioned this pull request Feb 11, 2025

Native Sidecars draining erroneously sending connection: close for outbound requests istio/istio#51855

Closed

2 tasks

keithmattix mentioned this pull request Feb 13, 2025

feat: inbound only graceful draining #38430

Merged

Conversation

keithmattix commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 4, 2025

Uh oh!

keithmattix commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keithmattix commented Jan 10, 2025

Uh oh!

jmarantz commented Jan 15, 2025

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithmattix commented Jan 23, 2025

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithmattix commented Jan 24, 2025

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jan 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

keithmattix commented Jan 4, 2025 •

edited

Loading

keithmattix commented Jan 4, 2025 •

edited

Loading