listener filter: hiding envoy internal from ListenerFilterCallbacks interface by soulxu · Pull Request #22732 · envoyproxy/envoy

soulxu · 2022-08-17T05:07:28Z

Signed-off-by: He Jie Xu hejie.xu@intel.com

Commit Message: listener filter: hiding envoy internal from ListenerFilterCallbacks interface
Additional Description:
Since #20728 remove the need for listener filter to register their own event, the dispatch() and continueFilterChain(bool) interfaces are needn't by any listener filter anymore, and we can hide those envoy internal from the listener filter now.

Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

…nterface Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

KBaichoo

Two high level questions:

Moving continueFilterChain from interface to the concrete class, but how can the listener filters communicate the continue filter chain?

Would this remove the ability to have failure in the onAccept calls?

soulxu · 2022-08-18T14:47:05Z

Two high level questions:

Moving continueFilterChain from interface to the concrete class, but how can the listener filters communicate the continue filter chain?

In the old day, the listener filter use continueFilterChain to trigger the processes (move to next filter or reject the connection) of envoy core code, since the listener filter registered its own callback with the file event. That is what #20728 removed.

Today, after having #20728, the listener filter only uses the FilterStatus to communicate with listener. The file event is registered by envoy core code now, and that callback will trigger the processes based on the listener filter returned FilterStatus.

envoy/source/server/active_tcp_socket.cc

Line 87 in aa9ada6

Network::FilterStatus status = (*iter_)->onData(filter_buffer);

Would this remove the ability to have failure in the onAccept calls?

The listener filter just needs to close the sockets and returns Network::FilterStatus::StopIteration, then the envoy core code knows that the listener filter rejected the connection:

envoy/source/server/active_tcp_socket.cc

Line 103 in aa9ada6

continueFilterChain(false);

Thanks for the review!

KBaichoo

Thanks for the explanation and the change. We should just put this new understanding back into the code.

KBaichoo · 2022-08-18T17:44:34Z

source/server/active_tcp_socket.h

  Network::ConnectionSocket& socket() override { return *socket_.get(); }
-  Event::Dispatcher& dispatcher() override;
-  void continueFilterChain(bool success) override;
+  void continueFilterChain(bool success);


Can we move this elsewhere since it's no longer part of the interface commented above and have it documented?

It seems like it's now split into either internal usage or external continueFilterChain(true) call that really just starts the filter chain. We could increase encapsulation splitting this out into startFilterChain() and continueFilterChain with the latter being private and startFilterChain just deferring to it.

Likewise comments in https://github.com/envoyproxy/envoy/pull/22732/files/e8563f3153394c2ffe8653ca9c9beef7e70dacc0#diff-6708370e112a5097f06e16009c3730b34f5ab0ee4f1e736f8816848409435888R122

and

envoy/envoy/network/filter.h

Line 333 in fb2d0a0

virtual FilterStatus onAccept(ListenerFilterCallbacks& cb) PURE;

should be updated since the convention is changing e.g the filter doesn't call continue

Can we move this elsewhere since it's no longer part of the interface commented above and have it documented?

The ActiveTcpSocket is a struct, its all members are public now, I guess that is for convenient for ActiveTcpListener access. And ActiveTcpListener is only place ActiveTcpSocket, and probably we think ActiveTcpSocket is kind of internal struct for ActiveTcpListener(since its only alive short time, it will be deleted when the connection passed all the listener filters or the connection rejected by listener filter)

It seems like it's now split into either internal usage or external continueFilterChain(true) call that really just starts the filter chain. We could increase encapsulation splitting this out into startFilterChain() and continueFilterChain with the latter being private and startFilterChain just deferring to it.

Yea, that is good idea! The ActiveTcpListener is the place to call startFileChain(), and that make the code clear. Just as above said the whole ActiveTcpSocket are public, not sure I should make continueFilterChain as private. But let me know if this is still something you think should be changed.

Likewise comments in https://github.com/envoyproxy/envoy/pull/22732/files/e8563f3153394c2ffe8653ca9c9beef7e70dacc0#diff-6708370e112a5097f06e16009c3730b34f5ab0ee4f1e736f8816848409435888R122

and

envoy/envoy/network/filter.h

Line 333 in fb2d0a0

virtual FilterStatus onAccept(ListenerFilterCallbacks& cb) PURE;

should be updated since the convention is changing e.g the filter doesn't call continue

got it, will update the comments

Thanks this looks great! It makes a lot of sense that a lot of the class was open as you said, but it seems like it has grown in responsibility e.g. internally buffering listener data.

Having some methods private (continueFilterChain, createListenerFilterBuffer) would make the interface more compact / easier to grok.

got it, let me try to improve that.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-08-19T03:16:09Z

Sorry for having to do a force push due to I mess up with the DCO since my daily dev machine is down and the temp machine has the wrong git config.

KBaichoo

Thanks this looks great!

KBaichoo · 2022-08-19T13:20:23Z

envoy/network/filter.h

  /**
-   * Called when data read from the connection. If the filter chain doesn't get
-   * enough data, the filter chain can be stopped, then waiting for more data.
+   * Called when data read from the connection. If the filter doesn't get


s/data read/data is read

got it, thanks!

KBaichoo · 2022-08-19T13:28:49Z

source/server/active_tcp_socket.h

  Network::ConnectionSocket& socket() override { return *socket_.get(); }
-  Event::Dispatcher& dispatcher() override;
-  void continueFilterChain(bool success) override;
+  void continueFilterChain(bool success);


Thanks this looks great! It makes a lot of sense that a lot of the class was open as you said, but it seems like it has grown in responsibility e.g. internally buffering listener data.

Having some methods private (continueFilterChain, createListenerFilterBuffer) would make the interface more compact / easier to grok.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-08-22T12:53:37Z

/wait

that is something failed real

KBaichoo · 2022-08-22T13:29:53Z

Seems like issue might be resolved by #22792

soulxu · 2022-08-22T14:07:55Z

Seems like issue might be resolved by #22792

thanks!, let me wait for that merged

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

KBaichoo

lgtm, thanks!

…backs interface (envoyproxy#22732)" This reverts commit 350c7f9. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu force-pushed the cleanup_listener_filter_callbacks branch from bb9d6e6 to ec413bb Compare August 17, 2022 05:08

listener filter: hiding envoy internal from ListenerFilterCallbacks i…

669384e

…nterface Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu force-pushed the cleanup_listener_filter_callbacks branch from 4f0c793 to 669384e Compare August 17, 2022 10:05

fix clang-tidy

e8563f3

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

jmarantz assigned KBaichoo Aug 17, 2022

KBaichoo reviewed Aug 18, 2022

View reviewed changes

KBaichoo added the waiting label Aug 18, 2022

KBaichoo reviewed Aug 18, 2022

View reviewed changes

repokitteh-read-only bot removed the waiting label Aug 19, 2022

address comment

8679577

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu force-pushed the cleanup_listener_filter_callbacks branch from 61f9cd7 to 8679577 Compare August 19, 2022 03:15

KBaichoo reviewed Aug 19, 2022

View reviewed changes

KBaichoo added the waiting label Aug 19, 2022

repokitteh-read-only bot removed the waiting label Aug 22, 2022

soulxu added 2 commits August 22, 2022 19:17

turn ActiveTcpSocket from struct to class

5016686

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix comment

ac12792

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu force-pushed the cleanup_listener_filter_callbacks branch from 763057e to ac12792 Compare August 22, 2022 11:20

soulxu added 2 commits August 22, 2022 20:35

refine

f1f7191

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into cleanup_listener_filter_callbacks

b5bb43c

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot added the waiting label Aug 22, 2022

Merge branch 'main' into cleanup_listener_filter_callbacks

b5a1412

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Aug 22, 2022

KBaichoo reviewed Aug 22, 2022

View reviewed changes

KBaichoo approved these changes Aug 22, 2022

View reviewed changes

KBaichoo merged commit 350c7f9 into envoyproxy:main Aug 22, 2022

soulxu added a commit to soulxu/envoy that referenced this pull request Oct 13, 2022

Revert "listener filter: hide envoy internals from ListenerFilterCall…

3aeddb5

…backs interface (envoyproxy#22732)" This reverts commit 350c7f9. Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu mentioned this pull request Oct 13, 2022

Revert "listener filter: hide envoy internals from ListenerFilterCallbacks interface (#22732) #23470

Closed

Conversation

soulxu commented Aug 17, 2022 • 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!

soulxu commented Aug 18, 2022

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Aug 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Aug 22, 2022

Uh oh!

KBaichoo commented Aug 22, 2022

Uh oh!

soulxu commented Aug 22, 2022

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

soulxu commented Aug 17, 2022 •

edited

Loading

soulxu Aug 19, 2022 •

edited

Loading

soulxu commented Aug 19, 2022 •

edited

Loading