listener: add an option to continue on listener filters timeout by lizan · Pull Request #7859 · envoyproxy/envoy

lizan · 2019-08-07T23:19:48Z

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
Add an option to continue on listener filters timeout.

Risk Level: Med (mostly guarded by config)
Testing: unittest
Docs Changes: Added
Release Notes: Added
Fixes #7195

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan · 2019-08-07T23:20:02Z

no test yet, cc @silentdai

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-07T23:51:36Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-08T00:36:06Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-08T00:52:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-08T01:00:27Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

lizan · 2019-08-08T01:07:12Z

This is ready to review, PTAL

lambdai · 2019-08-08T07:29:20Z

If listener option is the solution, can you add the document that a listener with proxy_protocol should not set continue_on_listener_filters_timeout?
The reason is that proxy_protocol may consume bytes instead of PEEK.

That is the initial motivation of the my PR: It's harmless for the listener to decide to close the socket, but only listener filter itself can decide whether it's appropriate to continue.

LGTM if the above case is omitted

rshriram

This works as well!

mattklein123

Thanks, LGTM with some small nits.

/wait

api/envoy/api/v2/lds.proto

docs/root/intro/version_history.rst

include/envoy/network/listener.h

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-09T01:13:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-09T01:15:29Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-09T01:15:54Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

mattklein123

LGTM, also needs a format fix.

/wait

api/envoy/api/v2/lds.proto

Signed-off-by: Lizan Zhou <lizan@tetrate.io> Co-Authored-By: Matt Klein <mattklein123@gmail.com>

repokitteh-read-only · 2019-08-09T04:59:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

repokitteh-read-only · 2019-08-09T05:00:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #7859 was synchronize by lizan.

see: more, trace.

mattklein123

Nice!

htuch · 2019-08-09T19:42:13Z

include/envoy/network/listener.h

   * @return std::chrono::milliseconds the time to wait for all listener filters to complete
   *         operation. If the timeout is reached, the accepted socket is closed without a
-   *         connection being created. 0 specifies a disabled timeout.
+   *         connection being created unless continueOnListenerFiltersTimeout() returns true.


One random thought, thinking about the use case that @silentdai had, would it make sense to have a timeout in the listener filter config as well to enable override on a per-filter basis? In theory you might want to have different timeouts for TLS vs. some other filter. I think the point is probably moot right now, because folks have shallow (maybe typically 1 deep) filter chains, but just a consideration for future.

A listener filter can still do their own timeout, to continue or fail, from their opaque config, such as @silentdai did in #7853. This option applies only to when listener level timeout happens.

lambdai

Thank you for the update!
/approve

listener: add an option to continue on listener filters timeout

4f8d5b6

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

fix build first, no test yet

efa0e02

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lambdai mentioned this pull request Aug 8, 2019

config: add fallback timeout to tls_inspector #7853

Closed

add test, fix event leak

9208927

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

version history

93b2724

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan marked this pull request as ready for review August 8, 2019 00:52

lizan requested review from lambdai and rshriram August 8, 2019 00:57

header doc

ad3908c

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan assigned mattklein123 Aug 8, 2019

rshriram previously approved these changes Aug 8, 2019

View reviewed changes

mattklein123 requested changes Aug 8, 2019

View reviewed changes

api/envoy/api/v2/lds.proto Outdated Show resolved Hide resolved

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

include/envoy/network/listener.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 8, 2019

comment

40f4ba0

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

lizan dismissed rshriram’s stale review via 40f4ba0 August 9, 2019 01:13

repokitteh-read-only bot removed the waiting label Aug 9, 2019

word

8b8fbc7

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

format

7c438ed

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

mattklein123 requested changes Aug 9, 2019

View reviewed changes

api/envoy/api/v2/lds.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 9, 2019

address comment

c6b3e0c

Signed-off-by: Lizan Zhou <lizan@tetrate.io> Co-Authored-By: Matt Klein <mattklein123@gmail.com>

repokitteh-read-only bot removed the waiting label Aug 9, 2019

format

2cb4b67

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

mattklein123 approved these changes Aug 9, 2019

View reviewed changes

htuch reviewed Aug 9, 2019

View reviewed changes

lambdai approved these changes Aug 9, 2019

View reviewed changes

lizan merged commit 975c62d into envoyproxy:master Aug 9, 2019

lizan deleted the listener_filter_timeout branch August 9, 2019 23:10

yxue mentioned this pull request Aug 13, 2019

add protocol detection timeout istio/istio#16223

Merged

yxue mentioned this pull request Sep 4, 2019

[Istio-1.3.0-rc0] Short protocol detection timeout can fail https requests istio/istio#16581

Closed

Conversation

lizan commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Aug 7, 2019

Uh oh!

repokitteh-read-only bot commented Aug 7, 2019

Uh oh!

repokitteh-read-only bot commented Aug 8, 2019

Uh oh!

repokitteh-read-only bot commented Aug 8, 2019

Uh oh!

repokitteh-read-only bot commented Aug 8, 2019

Uh oh!

lizan commented Aug 8, 2019

Uh oh!

lambdai commented Aug 8, 2019

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 9, 2019

Uh oh!

repokitteh-read-only bot commented Aug 9, 2019

Uh oh!

repokitteh-read-only bot commented Aug 9, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 9, 2019

Uh oh!

repokitteh-read-only bot commented Aug 9, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lizan commented Aug 7, 2019 •

edited

Loading