matcher: add server name matcher by kyessenov · Pull Request #24 · cncf/xds

kyessenov · 2021-12-22T20:32:24Z

This is intended to generalize SNI matching in TLS in Envoy's listener unified matcher implementation.
Signed-off-by: Kuat Yessenov kuat@google.com

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

kyessenov · 2021-12-22T20:32:37Z

cc @snowp @markdroth

markdroth

Overall, this looks fine to me, but I'd like input from @snowp as well.

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

kyessenov · 2022-03-03T21:02:17Z

@snowp Please take a look. I wonder what your thoughts are on compatibility with https://github.com/envoyproxy/envoy/blob/3aacf2bee32fe77f9eee3ffc668fc7942ddd1418/api/envoy/config/route/v3/route_components.proto#L69.

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

snowp

Seems good to me for what it tries to do, just one question on whether we can simplify

snowp · 2022-03-03T21:10:41Z

+  // Match a server name by multiple wildcard domain matchers. If there are
+  // multiple wildcards matching the server name, the order of declaration is
+  // used to select the first match.
+  repeated DomainMatcher domain_matchers = 1;


Would it be possible to reduce the nesting here and leverage the ListMatcher to express this "first match wins"?

It works in tandem with the longest suffix so I guess not. E.g. if we have two matchers for *.com but also have example.com that takes priority.

Ok so there can be overlapping domains coming from each entry in this list? At first glance it looked like a list of independent suffixes to match on, so maybe the docs can be clearer?

On a second though, let's just make all wildcards distinct across all matchers. WDYT?

Could you put all the exact matchs first (any order), then the wildcard matches (sorted by length) to get the same behavior?

The key is that the implementation should be non-linear. Lists require traversal which doesn't scale well. This one can be done with an efficient compressed trie.

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

kyessenov · 2022-03-15T17:53:17Z

Any more input @snowp? Can this be merged?

snowp

LGTM, thanks!

kyessenov · 2022-03-24T23:55:07Z

Gentle ping @markdroth .

kyessenov · 2022-03-29T16:57:46Z

@markdroth This is one bit of lost functionality. Can you approve this so we can implement it in Envoy?

howardjohn · 2022-03-29T19:40:23Z

+
+// Matches a fully qualified server name against a set of wildcard domain
+// names.
+message ServerNameMatcher {


Upfront, I completely agree we need something here to get parity with current Envoy config, and I have no problem with this being that solution.

Just wondering if we want a more general vs domain specific matcher. It seems like this could be a "PrefixOrExactMatcher" or something generic, where if there is a * we treat it as prefix, else exact?

I think the only behavioral implication would be that *w.example.com would be valid. I don't have any concerns with that though; its up to the control plane to reject that if it doesn't like it?

Just an idea, I don't have a strong opinion here

We can always add more sophisticated ones later. The * being an entire word is needed to create a trie, otherwise, we'd have to backtrack for some complicated patterns like *x.*y.*m.

A sophisticated template engine is hard to agree on. Google one is very opinionated https://github.com/google/http_pattern_matcher.

mattklein123

Thanks LGTM with small comments

mattklein123 · 2022-03-29T23:00:57Z

+
+// [#protodoc-title: Server name matcher]
+
+// Matches a fully qualified server name against a set of wildcard domain


Technically this can be used for exact name matching also, right? I'm not sure if anyone would use that, but will domains be validated that they have a wildcard in them in the appropriate place?

Yes, exact names are allowed. The wildcard, if present, must be the leftmost symbol before the dot.

mattklein123 · 2022-03-29T23:01:21Z

+  // Match a server name by multiple wildcard domain matchers. Each wildcard
+  // domain must appear at most once across the domain matchers.


Can you clarify what happens if a non-wildcard domain is specified? Is that allows per above or will it be rejected?

It's allowed. I spelled out that exact are allowed.

mattklein123 · 2022-03-29T23:01:41Z

+  // ``*``. Note that partial wildcards are not supported, and values like
+  // ``*w.example.com`` are invalid.
+  message DomainMatcher {
+    // A non-empty set of wildcard domain names, e.g. ``www.example.com``,


www.example.com isn't wildcard, so a little confusing, and related to my other comments.

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

kyessenov · 2022-03-29T23:33:29Z

Thanks, updated the documentation to make it easier to follow.

mattklein123

Thanks LGTM with small comments

mattklein123 · 2022-03-30T15:37:33Z

+  // the domain on a dot border. The wildcard matches one or more non-empty
+  // domain parts.
+  message DomainMatcher {
+    // A non-empty set of domain names with wildcards, e.g.


Suggested change

// A non-empty set of domain names with wildcards, e.g.

// A non-empty set of domain names with optional wildcards, e.g.

mattklein123 · 2022-03-30T15:37:52Z

+    // ``www.example.com``, ``*.com``, or ``*``.
+    repeated string domains = 1 [ (validate.rules).repeated = {min_items : 1} ];
+
+    // Match action to apply when the server name matches any of the wildcard


Suggested change

// Match action to apply when the server name matches any of the wildcard

// Match action to apply when the server name matches any of the

mattklein123 · 2022-03-30T15:39:04Z

+  // The server name will be matched against all wildcard domains starting from
+  // the longest suffix, i.e. ``www.example.com`` input will be first matched
+  // against ``www.example.com``, then ``*.example.com``, then ``*.com``, then
+  // ``*``, until the associated matcher action accepts the input. Note that


What is a partial wildcard? Clarify?

Re-worded, thanks. This was straight copied from envoy originally.

markdroth · 2022-03-30T15:49:05Z

Sorry for the delay here.

LGTM, modulo comments from @mattklein123.

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

mattklein123

Thanks!

matcher: add server name matcher

f50970f

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

markdroth reviewed Jan 4, 2022

View reviewed changes

Comment thread xds/type/matcher/v3/domain.proto Outdated

kyessenov mentioned this pull request Jan 13, 2022

listener: filter chain unified matchers envoyproxy/envoy#18871

Closed

kyessenov mentioned this pull request Feb 25, 2022

listeners: add unified matcher for filter chains envoyproxy/envoy#20110

Merged

kyessenov added 2 commits March 3, 2022 12:46

Merge remote-tracking branch 'upstream/main' into domain_matcher

cce1c2d

review

0c80a48

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

empty string

e66e21c

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

snowp reviewed Mar 3, 2022

View reviewed changes

simplify

211637d

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

snowp previously approved these changes Mar 15, 2022

View reviewed changes

mattklein123 self-assigned this Mar 29, 2022

howardjohn reviewed Mar 29, 2022

View reviewed changes

mattklein123 requested changes Mar 29, 2022

View reviewed changes

review

631f6ed

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

kyessenov dismissed snowp’s stale review via 631f6ed March 29, 2022 23:32

mattklein123 requested changes Mar 30, 2022

View reviewed changes

review

71eeb1d

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

mattklein123 approved these changes Mar 30, 2022

View reviewed changes

mattklein123 merged commit eded343 into cncf:main Mar 30, 2022

kyessenov mentioned this pull request Apr 4, 2022

matchers: add server name xds matcher envoyproxy/envoy#20652

Closed


		// [#protodoc-title: Server name matcher]

		// Matches a fully qualified server name against a set of wildcard domain

		// Match a server name by multiple wildcard domain matchers. Each wildcard
		// domain must appear at most once across the domain matchers.

	// A non-empty set of domain names with wildcards, e.g.
	// A non-empty set of domain names with optional wildcards, e.g.

	// Match action to apply when the server name matches any of the wildcard
	// Match action to apply when the server name matches any of the

Uh oh!

Conversation

kyessenov commented Dec 22, 2021

Uh oh!

kyessenov commented Dec 22, 2021

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kyessenov commented Mar 3, 2022

Uh oh!

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

kyessenov commented Mar 15, 2022

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Mar 24, 2022

Uh oh!

kyessenov commented Mar 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

kyessenov commented Mar 29, 2022

Uh oh!

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

markdroth commented Mar 30, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone