matchers: add server name xds matcher by kyessenov · Pull Request #20652 · envoyproxy/envoy

kyessenov · 2022-04-04T16:26:47Z

Commit Message: Implements server name matcher in Envoy, as needed by the listener unified matcher for matching SNI.
Additional Description:
Risk Level: low
Testing: unit
Docs Changes: yes
Release Notes: no

Signed-off-by: Kuat Yessenov <kuat@google.com>

…cher

Signed-off-by: Kuat Yessenov <kuat@google.com>

…cher

repokitteh-read-only · 2022-04-04T16:26:54Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #20652 was opened by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

zhxie · 2022-04-06T02:15:33Z

 namespace Common {
 namespace Matcher {

+class NetworkTrieMatcherFactory : public TrieMatcherFactoryBase<Network::MatchingData> {};


Why should these class definitions be moved to .cc?

For consistency. It doesn't seem to be useful to export these in a header.

lambdai

Generally LGTM. I didn't look into the scary match...

lambdai · 2022-04-05T17:50:34Z

+.. _extension_envoy.matching.custom_matchers.domain_matcher:
+
+* :ref:`Server name matcher <envoy_v3_api_msg_.xds.type.matcher.v3.ServerNameMatcher>`.


Can you clarify that this matcher is supposed to match SNI?
Particularly, the wildcard matcher does not apply to wildcard cert "*.example.com"

It is not only SNI, it would work for virtual host as well. I'll make it clear that it matches domains, not wildcards.

Aha, yes, virtual host can reuse this matcher!

lambdai · 2022-04-05T17:52:04Z

+    typed_config:
+      "@type": type.googleapis.com/google.protobuf.StringValue
+  custom_match:
+    name: ip_matcher


server_name_matcher and below :)

lambdai · 2022-04-06T04:19:01Z

+using ::Envoy::Matcher::OnMatch;
+
+/**
+ * A "compressed" trie node with domain parts as edge values.


What does compress mean? Can two parts be compressed in a single node?

Compressed as in it's not a regular trie with letters as alphabet. It doesn't compress two domain parts. Added a little diagram.

Signed-off-by: Kuat Yessenov <kuat@google.com>

lambdai · 2022-04-06T22:11:33Z

+.. _extension_envoy.matching.custom_matchers.domain_matcher:
+
+* :ref:`Server name matcher <envoy_v3_api_msg_.xds.type.matcher.v3.ServerNameMatcher>`.


Aha, yes, virtual host can reuse this matcher!

lambdai · 2022-04-06T22:20:25Z

+        }
+        if (matched.match_state_ == MatchState::MatchComplete && matched.result_) {
+          return {MatchState::MatchComplete, OnMatch<DataType>{matched.result_, nullptr}};
+        }


Is it possible to escape the if (on_match) { block w/o return ?
In which case?

If the answer is yes, should on_match degrade from exact_on_match to prefix_on_match ?

Yes, it can have nested matchers so the match can complete without a result.
It does degrade, because it traverses the parent_prefix_ edges. E.g. it'll try wider and wider patters by going from the node to the root.

Signed-off-by: Kuat Yessenov <kuat@google.com>

phlax · 2022-04-07T11:00:08Z

/retest

repokitteh-read-only · 2022-04-07T11:00:16Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20652 (comment) was created by @phlax.

see: more, trace.

zuercher · 2022-04-15T16:53:10Z

ping @phlax

phlax

this lgtm, but its a bit outside my competence to land

@mattklein123 can i pass this over to you as a CODEOWNER

mattklein123

Thanks at a high level LGTM to ship and iterate. A few small doc comments:

Please add a release note
Can you add an example somewhere so people have something to crib from?

Thank you!

/wait

github-actions · 2022-06-01T12:01:31Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-06-08T12:01:37Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-06-15T20:01:19Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

kyessenov · 2022-06-15T20:40:39Z

Still want to get this done...

github-actions · 2022-07-16T00:03:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-07-23T04:09:29Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

kyessenov added 4 commits March 31, 2022 10:07

initial

af1e16c

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/main' into server_name_xds_mat…

863ae55

…cher

matchers: add server name matcher

3497460

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/main' into server_name_xds_mat…

3adcb7e

…cher

kyessenov requested a review from mattklein123 as a code owner April 4, 2022 16:26

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Apr 4, 2022

repokitteh-read-only Bot assigned phlax Apr 4, 2022

kyessenov added 4 commits April 4, 2022 09:33

doc fix-up

87b9c29

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix docs

f2ed038

Signed-off-by: Kuat Yessenov <kuat@google.com>

typo

d1db52c

Signed-off-by: Kuat Yessenov <kuat@google.com>

ci

4314c08

Signed-off-by: Kuat Yessenov <kuat@google.com>

zhxie reviewed Apr 6, 2022

View reviewed changes

lambdai reviewed Apr 6, 2022

View reviewed changes

review

1a3981c

Signed-off-by: Kuat Yessenov <kuat@google.com>

lambdai reviewed Apr 6, 2022

View reviewed changes

kyessenov added 2 commits April 6, 2022 15:35

workaround gcc bug

e86c0e2

Signed-off-by: Kuat Yessenov <kuat@google.com>

review

dc8ed26

Signed-off-by: Kuat Yessenov <kuat@google.com>

phlax reviewed Apr 20, 2022

View reviewed changes

phlax assigned mattklein123 Apr 20, 2022

mattklein123 requested changes Apr 20, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 20, 2022

phlax removed their assignment May 2, 2022

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 1, 2022

github-actions Bot closed this Jun 8, 2022

kyessenov reopened this Jun 8, 2022

github-actions Bot closed this Jun 15, 2022

kyessenov reopened this Jun 15, 2022

github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 16, 2022

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 16, 2022

github-actions Bot closed this Jul 23, 2022

		.. _extension_envoy.matching.custom_matchers.domain_matcher:

		* :ref:`Server name matcher <envoy_v3_api_msg_.xds.type.matcher.v3.ServerNameMatcher>`.

Uh oh!

Conversation

kyessenov commented Apr 4, 2022

Uh oh!

repokitteh-read-only Bot commented Apr 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

phlax commented Apr 7, 2022

Uh oh!

repokitteh-read-only Bot commented Apr 7, 2022

Uh oh!

zuercher commented Apr 15, 2022

Uh oh!

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

github-actions Bot commented Jun 1, 2022

Uh oh!

github-actions Bot commented Jun 8, 2022

Uh oh!

github-actions Bot commented Jun 15, 2022

Uh oh!

kyessenov commented Jun 15, 2022

Uh oh!

github-actions Bot commented Jul 16, 2022

Uh oh!

github-actions Bot commented Jul 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants