listener: add match all filter chain by lambdai · Pull Request #13449 · envoyproxy/envoy

lambdai · 2020-10-08T18:28:02Z

Commit Message:

The match all filter chain is chosen when no other filter chain matches
the request.

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Also update the doc on filter chain match and listener update.
Risk Level: LOW
Testing: unit tests
Docs Changes:
Release Notes:
Fixes #11088
Fixes #12572

The match all filter chain is chosen when no other filter chain matches the request. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only · 2020-10-08T18:28:09Z

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

🐱

Caused by: #13449 was opened by lambdai.

see: more, trace.

lambdai · 2020-10-08T18:41:27Z

@envoyproxy/api-shepherds

The reason to add match_all field: Theoretically we replace the match-all filter chain by a number of filter chains w/o introducing this match-all field.

However, it's not a trivial efforts to generate the above set of filter chains:

The code need to be replicated in multiple xds-servers
The number of the filter chains to fill each corner of the filter chain can be huge.

IMHO it's reasonable to promote the match-all semantic to API

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

htuch · 2020-10-09T00:38:59Z

@lambdai two questions:

Could we just have an explicit FilterChain that is called the default one? That would structurally guarantee that there isn't more than one filter chain marked as default.
I can't tell from filter_chain_match difference between {} and null is confusing/under documented #12572 last comments if this actually fully resolves concerns or is plan-of-record. Tagging @howardjohn @lizan @PiotrSikora @rshriram. In any case, I agree that having an explicit fallback is helpful.

PiotrSikora · 2020-10-09T00:54:07Z

I agree with @htuch that it should be an explicit fallback filter chain match, and not an option.

lambdai · 2020-10-09T01:01:23Z

Could we just have an explicit FilterChain that is called the default one? That would structurally guarantee that there isn't more than one filter chain marked as default.

The pain is that the default is semantically optional (0 or 1). So we have to use "oneof" to describe the "optional" because there is no "nullable" filter chain.
And we also need explain the "FilterChainMatch" field in default filter chain should be ignored.

I am fine to switch to one_of if we all agree on explicit at-most-one default chain.

lambdai · 2020-10-09T01:09:48Z

2. I can't tell from #12572 last comments if this actually fully resolves concerns or is plan-of-record. Tagging @howardjohn @lizan @PiotrSikora @rshriram. In any case, I agree that having an explicit fallback is helpful.

I think no one wishes to invest in implementing better-find because fallback probably resolve 95% of the corner cases.
What's more, the better find is good at recalling a second-best chain, however, it's hard to say if accidentally find 2nd best is breaking other user cases

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks API looks good to me with small comment, but I would like to see more documentation, release notes, etc.

/wait

api/envoy/config/listener/v3/listener.proto

api/envoy/config/listener/v3/listener_components.proto

docs/root/configuration/listeners/lds.rst

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

Writing doc...

api/envoy/config/listener/v3/listener.proto

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

howardjohn · 2020-10-11T22:26:15Z

I can't tell from #12572 last comments if this actually fully resolves concerns or is plan-of-record. Tagging @howardjohn @lizan @PiotrSikora @rshriram. In any case, I agree that having an explicit fallback is helpful.

I think the status is that this will fix our short term issues. Longer term, it does not seem feasible to migrate off of use_original_dst without the larger change. For example, we will want chains that (very very roughly) look like:

- match port 80, IP X
- match port 80, IP Y
- match port 90, IP Z
- match port 100, IP A
.... (100s of such matches) ....
- match any port, any HTTP traffic plaintext
- match any port, any traffic

At this point, we can neither use just the catch all, since we do not have just a single catch all. We also will struggle to implement this logic with the current FCM algorithm since we will need to duplicate all the matches per port.

mattklein123

Thanks generally looks great. Can you also add an integration test in whatever place we have the other integration tests for the intelligent update feature?

/wait

api/envoy/config/listener/v3/listener_components.proto

docs/root/configuration/listeners/lds.rst

docs/root/intro/arch_overview/listeners/network_filter_chain.rst

docs/root/configuration/listeners/lds.rst

docs/root/intro/arch_overview/listeners/network_filter_chain.rst

source/server/filter_chain_manager_impl.cc

mattklein123 · 2020-10-12T15:07:04Z

source/server/filter_chain_manager_impl.cc

+  const auto* origin = getOriginFilterChainManager();
+  if (origin == nullptr) {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+    return;
+  }
+  MessageUtil eq;
+  if (origin->default_filter_chain_message_.has_value() &&
+      eq(origin->default_filter_chain_message_.value(), *default_filter_chain)) {
+    default_filter_chain_ = origin->default_filter_chain_;
+  } else {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+  }
+}


I forget this code from the last time we reviewed this. Is this logic effectively duplicated elsewhere? Can you unify? If not can you add more comments?

The high level find-existing-or-add is duplicating. However, the source of the find and dest of add are not.
Previously we are building from a repeated field (filter_chains) to a map <message, filter_chain_runtime_info>, but now I am adding the default field under the goal of "at-most-one-default-chain"

OK, please add more comments. It's hard to understand what all of this logic does.

added the context and the inline comment to the if branches

source/server/listener_impl.cc

lambdai

Thank you! Now that the general flow is fine, I am adding more tests to cover the codes

lambdai · 2020-10-12T16:29:26Z

source/server/filter_chain_manager_impl.cc

+  const auto* origin = getOriginFilterChainManager();
+  if (origin == nullptr) {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+    return;
+  }
+  MessageUtil eq;
+  if (origin->default_filter_chain_message_.has_value() &&
+      eq(origin->default_filter_chain_message_.value(), *default_filter_chain)) {
+    default_filter_chain_ = origin->default_filter_chain_;
+  } else {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+  }
+}


The high level find-existing-or-add is duplicating. However, the source of the find and dest of add are not.
Previously we are building from a repeated field (filter_chains) to a map <message, filter_chain_runtime_info>, but now I am adding the default field under the goal of "at-most-one-default-chain"

htuch

API LGTM

htuch · 2020-10-13T15:11:42Z

api/envoy/config/listener/v3/listener_components.proto

+//
+// For destination port, filter chains specifying the destination port of incoming traffic are the
+// best match. If none of the filter chains specifies the exact destination port, the filter
+// chains which do not specify ports are the best match. Filter chains specifying the wrong


This example helps, but I can't help but think that "best match" would be best expressed as "most specifically match".

Thanks! That's more accurate in this context. Updated all over this file.

lizan · 2020-10-13T20:20:00Z

I can't tell from #12572 last comments if this actually fully resolves concerns or is plan-of-record. Tagging @howardjohn @lizan @PiotrSikora @rshriram. In any case, I agree that having an explicit fallback is helpful.

I think the status is that this will fix our short term issues. Longer term, it does not seem feasible to migrate off of use_original_dst without the larger change. For example, we will want chains that (very very roughly) look like:
- match port 80, IP X
- match port 80, IP Y
- match port 90, IP Z
- match port 100, IP A
.... (100s of such matches) ....
- match any port, any HTTP traffic plaintext
- match any port, any traffic
At this point, we can neither use just the catch all, since we do not have just a single catch all. We also will struggle to implement this logic with the current FCM algorithm since we will need to duplicate all the matches per port.

Yeah agreed we can't migrate off use_original_dst without larger change. I did some research of 2. in this doc and it is feasible but I didn't have time to bring it to full implementation yet.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks LGTM other than comment request. Please also make sure @htuch doc concerns are addressed so I will assign him also. Thank you!

/wait

mattklein123 · 2020-10-14T15:45:26Z

source/server/filter_chain_manager_impl.cc

+  const auto* origin = getOriginFilterChainManager();
+  if (origin == nullptr) {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+    return;
+  }
+  MessageUtil eq;
+  if (origin->default_filter_chain_message_.has_value() &&
+      eq(origin->default_filter_chain_message_.value(), *default_filter_chain)) {
+    default_filter_chain_ = origin->default_filter_chain_;
+  } else {
+    default_filter_chain_ =
+        filter_chain_factory_builder.buildFilterChain(*default_filter_chain, context_creator);
+  }
+}


OK, please add more comments. It's hard to understand what all of this logic does.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-10-14T19:49:02Z

/retest

repokitteh-read-only · 2020-10-14T19:49:06Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Verify examples run as documented), please wait.

🐱

Caused by: a #13449 (comment) was created by @lambdai.

see: more, trace.

mattklein123

Thanks LGTM. I will defer to @htuch for final review.

htuch

LGTM, thanks!

The match all filter chain is chosen when no other filter chain matches the request. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* master: (22 commits) delay health checks until transport socket secrets are ready. (envoyproxy#13516) test, oauth2: Make sure config test runs field validation (envoyproxy#13496) [http] swap codec implementations to default new (envoyproxy#13579) wasm: update proxy-wasm-cpp-host (envoyproxy#13606) postgres: do not copy and linearize received data when it is not going to be used (envoyproxy#13393) configs: Update configs v2 -> v3 (envoyproxy#13562) http2: Remove RELEASE_ASSERTs in sendPendingFrames() error handling (envoyproxy#13546) dependencies: track untracked implied dependencies, wrapup dashboard. (envoyproxy#13571) listener: add match all filter chain (envoyproxy#13449) fix mistakes in docstrings (envoyproxy#13603) ratelimit: add route entry metadata to ratelimit actions (envoyproxy#13269) cluster manager: avoid immediate activation for dynamic inserted cluster when initialize (envoyproxy#12783) ext_authz: Avoid calling check multiple times (envoyproxy#13288) docs: Unexclude remaining configs from validation (envoyproxy#13534) build: update rules_rust to allow Rustc in RBE (envoyproxy#13595) docs: Update sphinxext.rediraffe (envoyproxy#13589) Deprecate moonjit support on Windows before beta (envoyproxy#13541) dependencies: bump LuaJIT to 2.1 branch HEAD @ e9af1ab. (envoyproxy#13474) docs: add TLS stats to cluster stats doc (envoyproxy#13561) ci: stop building alpine-debug images in favor of ubuntu-based debug image (envoyproxy#13598) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

listener: add match all filter chain

5a6b159

The match all filter chain is chosen when no other filter chain matches the request. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot added the api label Oct 8, 2020

fix fallback match on ip

79f4ac5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

match_all to default filter chain

818c792

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 requested changes Oct 9, 2020

View reviewed changes

api/envoy/config/listener/v3/listener.proto Outdated Show resolved Hide resolved

api/envoy/config/listener/v3/listener_components.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Oct 9, 2020

mattklein123 self-assigned this Oct 9, 2020

lambdai commented Oct 9, 2020

View reviewed changes

docs/root/configuration/listeners/lds.rst Outdated Show resolved Hide resolved

api v3

cd0eb54

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 9, 2020

lambdai commented Oct 9, 2020

View reviewed changes

api/envoy/config/listener/v3/listener.proto Outdated Show resolved Hide resolved

lambdai added 4 commits October 10, 2020 03:14

various improvement

43f4c90

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'origin/master' into matchall

4e3f534

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

merge master

c41ff67

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

fix typo

0d32e47

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 requested changes Oct 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 12, 2020

lambdai commented Oct 12, 2020

View reviewed changes

htuch reviewed Oct 13, 2020

View reviewed changes

lambdai added 3 commits October 13, 2020 21:36

fix typo

fa4c180

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'origin/master' into matchall

b1a615f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

add filter chain only test

d6d6634

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai added 2 commits October 14, 2020 00:33

add integration test

197afac

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'me/matchall' into matchall

da2dd61

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai requested a review from PiotrSikora as a code owner October 14, 2020 00:48

repokitteh-read-only bot removed the waiting label Oct 14, 2020

mattklein123 added the waiting label Oct 14, 2020

rst

c9f8d55

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 14, 2020

mattklein123 requested changes Oct 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 14, 2020

mattklein123 assigned htuch Oct 14, 2020

add extra comment at copyOrRebuildDefaultFilterChain

4d8a4d5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Oct 14, 2020

mattklein123 approved these changes Oct 14, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Oct 14, 2020

mattklein123 removed their assignment Oct 14, 2020

htuch approved these changes Oct 16, 2020

View reviewed changes

mattklein123 merged commit e62c994 into envoyproxy:master Oct 16, 2020

lambdai added a commit to lambdai/envoy-dai that referenced this pull request Oct 16, 2020

listener: add match all filter chain (envoyproxy#13449)

e1bd101

The match all filter chain is chosen when no other filter chain matches the request. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Conversation

lambdai commented Oct 8, 2020

Uh oh!

repokitteh-read-only bot commented Oct 8, 2020

Uh oh!

lambdai commented Oct 8, 2020

Uh oh!

htuch commented Oct 9, 2020

Uh oh!

PiotrSikora commented Oct 9, 2020

Uh oh!

lambdai commented Oct 9, 2020

Uh oh!

lambdai commented Oct 9, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

howardjohn commented Oct 11, 2020

Uh oh!

mattklein123 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!

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!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch 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!

lizan commented Oct 13, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai commented Oct 14, 2020

Uh oh!

repokitteh-read-only bot commented Oct 14, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects