http: unified the filter chain creation in the FilterManager by wbpcode · Pull Request #37112 · envoyproxy/envoy

wbpcode · 2024-11-12T15:36:13Z

Commit Message: http: unified the filter chain creation in the FilterManager
Additional Description:

This refactor ensure the all the http filter chain creation is done in the FilterManager. (This also make sure we can do more control to the filter chain creations.)

In the previous implementation, the createFilterChain method (of FilterManager) is only used by the downstream filter chain. And the upstream filter chain use another way to create filter chain. It make that it's hard to control the behavior of filter chain creation in uniform way.

The createFilterChain will return a value to tell whether the upgrade is rejected. And use upgradeFilterChainCreated callback to tell whether the upgrade is accepted. If both are none, then, that means no upgrade.

In the new implementation, we unified the way of filter chain creation. The createFilterChain method will be used for both downstream and upstream. And a uniform way will be sued to report the upgrade results.

Risk Level: mid.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk · 2024-11-13T16:40:57Z

source/common/http/filter_manager.h

+   *        explicitly. This only makes sense for upstream HTTP filter chain.
+   *
+   */
+  CreateFilterChainResult createFilterChain(const FilterChainFactory& filter_chain_factory,


Can you comment a bit more about the motivation for this refactor?
Having a single createFilterChain function where one argument only works for downstream and one for upstream feels ugly. Why not retain createDownstream/createUpstream and only pass the relevant args?
/wait-any

Can you comment a bit more about the motivation for this refactor?
/wait-any

The initial motivation is I wanna to refactor the http filter chain to use vector rather than list as the container of filters because the filter chain needn't O(1) modification anyway. And rather than ActiveStremDecoderFilterPtr, I want to use ActiveStremDecoderFilter directly. This could reduce unnecessary heap allocations.

This refactoring requires us to set filter callbacks at the end of filter chain creation. So I need to ensure the filter chain creation is under the control of FilterManager.

And even not for that, make sure all filter chain creation logic in same way is better choice.

Having a single createFilterChain function where one argument only works for downstream and one for upstream feels ugly. Why not retain createDownstream/createUpstream and only pass the relevant args?

Yeah, I can make the create filter chain a shared private method and create a wrapper method for upstream filter chain creation.

I found I acutally needn't the upgrade option, because we can check whether the downstream stream callbacks is valid or not to determine to try upgrade or not.
Now, I removed it, code is cleaner now.

By the way, after a check to the upstream filter chain creation, I think even only_create_if_configured is unnecessary. Rather add some specific logic/parameter to the FilterChainFactory.createFilterChain, it would be better to use a default FilterChainFactory instance. When no valid filters are provided by both router and cluster, then we can construct a default FilterChainFactory to create the filter chain. (Only contains UpstreamCodecFilter)

This way should be much cleaner.

(If it's ok to you, I can also do a minor refactoring to this with a new PR).

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk

sorry about slow turnaround - was busy at EnvoyCon :-)
/wait

alyssawilk · 2024-11-14T18:55:59Z

source/common/http/filter_manager.cc

-    return false;
-  }
-  bool upgrade_rejected = false;
+FilterManager::CreateFilterChainResult


why change this one to CreateFilterChainResult? If it creates isn't the upgrade automatically not rejected? Seems simpler as a boolean

But the created may be true when the upgrade is rejected. And the created status is necessary by the upstream filter chain creation.

#37150 will make thing become more complex. So, I think I have to make some changes here to make it's easy to work with #37150

alyssawilk · 2024-11-14T18:56:41Z

source/common/http/filter_manager.cc

+FilterManager::createFilterChain(const FilterChainFactory& filter_chain_factory,
+                                 bool only_create_if_configured) {
+  if (state_.created_filter_chain_) {
+    return {true, false};


is this necessarily correct? Doesn't it depend on what happened previously? maybe at least comment if this is preserving legacy behavior

Yeah, this is mainly for preserving legacy behavior. But I think you are right because the created_filter_chain_ flag only ensure we have created filter chain. Set the upgrade_rejected to false may be wrong. We may need other way to expose the upgrade failure.

I found we have added a upgradeFilterChainCreated callback to expose/tell the success status of upgrade filter chain creation. This callabck typically could be reused to expose the failure status of upgrade filter chain creation. Then the createFilterChain() could only return a bool to tell if the filter chain is created or not.

source/common/http/filter_manager.cc

alyssawilk · 2024-11-14T19:00:59Z

source/common/http/filter_manager.h

+   * Set up the Encoder/Decoder filter chain.
+   * @param filter_chain_factory the factory to create the filter chain.
+   * @param only_create_if_configured whether to only create the filter chain if it is configured
+   *        explicitly. This only makes sense for upstream HTTP filter chain.


ooc is it ever not true for upstream filter chain? I'm wondering if we can infer this as we do the upgrade allowed option if they're just the inverse of each other

ooc is it ever not true for upstream filter chain?

Yeah...it will be false when we try to create filters from router and cluster at second time. As I said before, i think we should remove this by providing a default filter chain factory for upstream filter chain.

Would it be worth doing that now? default filter chain is just codec filter right?

Would it be worth doing that now? default filter chain is just codec filter right?

I think it deserves. It not just make code simple also reduce memory overhead of cluster because we now need keep a default filter chain for every cluster.

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-15T07:25:15Z

source/common/http/conn_manager_impl.h

-      State()
-          : codec_saw_local_complete_(false), codec_encode_complete_(false),
-            on_reset_stream_called_(false), is_zombie_stream_(false), successful_upgrade_(false),
-            is_internally_destroyed_(false), is_internally_created_(false), is_tunneling_(false),
-            decorated_propagate_(true), deferred_to_next_io_iteration_(false),
-            deferred_end_stream_(false) {}
-
      // It's possibly for the codec to see the completed response but not fully
      // encode it.
-      bool codec_saw_local_complete_ : 1; // This indicates that local is complete as the completed
-                                          // response has made its way to the codec.
-      bool codec_encode_complete_ : 1;    // This indicates that the codec has
-                                          // completed encoding the response.
-      bool on_reset_stream_called_ : 1;   // Whether the stream has been reset.
-      bool is_zombie_stream_ : 1;         // Whether stream is waiting for signal
-                                          // the underlying codec to be destroyed.
-      bool successful_upgrade_ : 1;


These changes mainly used to fix clang.tidy.

Hm. I think that removing the : 1 means that each of these fields will get 8x larger. What's the clang tidy error?

Here is an example:

Use default member initializer for 'codec_saw_local_complete_' (fix available)clang-tidy(modernize-use-default-member-init)

Hm. I think we'd prefer to avoid making this struct larger if we don't need to. Can you do a sizeof(State) before and after this change to see what it's doing? I wonder if we could use both : 1 and = false to get the best of both worlds?

Yeah, you are right, it make the state memory footprint from 2 bytes to 11 bytes.

Although I am not sure if that make sense or not for software like envoy. we never pursue extreme low memory overhead.

But I still revert all these changes. I think @phlax is working on clang-tidy related things. we can do all these at there or change our clang-tidy rule. :)

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk

Sorry about all the back and forth - just trying to make sure as we move this we don't make the already confusing code more confusing. :-/
/wait

source/common/http/conn_manager_impl.h

source/common/http/conn_manager_impl.cc

wbpcode · 2024-11-19T02:05:15Z

Sorry about all the back and forth - just trying to make sure as we move this we don't make the already confusing code more confusing. :-/ /wait

Never mind. Be careful to core code always make sense. :)

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk

Almost there!

source/common/http/conn_manager_impl.h

alyssawilk · 2024-11-21T18:12:42Z

source/common/http/filter_manager.h

+   * Set up the Encoder/Decoder filter chain.
+   * @param filter_chain_factory the factory to create the filter chain.
+   * @param only_create_if_configured whether to only create the filter chain if it is configured
+   *        explicitly. This only makes sense for upstream HTTP filter chain.


Would it be worth doing that now? default filter chain is just codec filter right?

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

RyanTheOptimist

Sorry for the delay. Overall this looks reasonable but a few comments. The optional<bool> in particular is a bit troublesome.

RyanTheOptimist · 2024-11-28T00:38:03Z

source/common/http/conn_manager_impl.h

-      State()
-          : codec_saw_local_complete_(false), codec_encode_complete_(false),
-            on_reset_stream_called_(false), is_zombie_stream_(false), successful_upgrade_(false),
-            is_internally_destroyed_(false), is_internally_created_(false), is_tunneling_(false),
-            decorated_propagate_(true), deferred_to_next_io_iteration_(false),
-            deferred_end_stream_(false) {}
-
      // It's possibly for the codec to see the completed response but not fully
      // encode it.
-      bool codec_saw_local_complete_ : 1; // This indicates that local is complete as the completed
-                                          // response has made its way to the codec.
-      bool codec_encode_complete_ : 1;    // This indicates that the codec has
-                                          // completed encoding the response.
-      bool on_reset_stream_called_ : 1;   // Whether the stream has been reset.
-      bool is_zombie_stream_ : 1;         // Whether stream is waiting for signal
-                                          // the underlying codec to be destroyed.
-      bool successful_upgrade_ : 1;


Hm. I think that removing the : 1 means that each of these fields will get 8x larger. What's the clang tidy error?

source/common/http/conn_manager_impl.h

source/common/http/filter_manager.h

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

…nify-http-filter-chain

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-28T06:06:55Z

/retest

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-12-02T06:04:27Z

/retest

http: unified the filter chain creation in the FilterManager

d28e7cf

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode requested a review from kyessenov as a code owner November 12, 2024 15:36

wbpcode assigned alyssawilk Nov 12, 2024

fix the test case where no filters

b37f5b7

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk reviewed Nov 13, 2024

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Nov 13, 2024

address comments

4a73cb7

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk reviewed Nov 14, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 14, 2024

make the code more simpler

64eb0f9

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

repokitteh-read-only bot removed the waiting label Nov 15, 2024

wbpcode commented Nov 15, 2024

View reviewed changes

fix test

0308ca1

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk reviewed Nov 18, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 18, 2024

address comments

e13c9a9

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

repokitteh-read-only bot removed the waiting label Nov 20, 2024

fix logic

dcd4bdf

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk reviewed Nov 21, 2024

View reviewed changes

remove unnecessary field

2bd2592

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

alyssawilk assigned RyanTheOptimist and unassigned alyssawilk Nov 25, 2024

RyanTheOptimist reviewed Nov 28, 2024

View reviewed changes

wangbaiping(wbpcode) added 3 commits November 28, 2024 02:24

address comments

f2958c5

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-u…

b7c67b4

…nify-http-filter-chain

style

8e4b115

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

revert state change

cd2e199

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

RyanTheOptimist approved these changes Dec 4, 2024

View reviewed changes

wbpcode merged commit bc1ed87 into envoyproxy:main Dec 4, 2024

wbpcode deleted the dev-unify-http-filter-chain branch December 4, 2024 14:12

Conversation

wbpcode commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

wbpcode Nov 14, 2024 • 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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Nov 15, 2024 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Nov 28, 2024 • 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!

wbpcode Dec 2, 2024 • 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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Nov 19, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Nov 28, 2024

Uh oh!

wbpcode commented Dec 2, 2024

Uh oh!

Reviewers

Assignees

wbpcode commented Nov 12, 2024 •

edited

Loading

wbpcode Nov 14, 2024 •

edited

Loading

wbpcode Nov 15, 2024 •

edited

Loading

wbpcode Nov 28, 2024 •

edited

Loading

wbpcode Dec 2, 2024 •

edited

Loading