matchers: add input types for network streams #19493

kyessenov · 2022-01-11T22:13:27Z

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

Commit Message: Introduce data inputs for connection matching as part of #18871
Risk Level: low
Testing: unit
Docs Changes: none
Release Notes: none

repokitteh-read-only · 2022-01-11T22:13:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19493 was opened by kyessenov.

see: more, trace.

kyessenov · 2022-01-11T22:14:57Z

api/envoy/type/matcher/v3/network_inputs.proto

+//   However, the use of ALPN is pretty much limited to the HTTP/2 traffic on the Internet,
+//   and matching on values other than ``h2`` is going to lead to a lot of false negatives,
+//   unless all connecting clients are known to use ALPN.
+message ApplicationProtocolInput {


@snowp This field is problematic because it's a list of values. Do we have any support for "list" inputs? I think comma-separate join is fine, except it's hard to distinguish between "h2" and "h2c" with string matching.

I made them quoted to make it easier to say "first element is h2, not h2c".

When this came up originally there was talks about an aggregate matcher that would allow applying a matcher against elements in the list of elements (ContainsMatching, AllMatching, etc.), though we never implemented that cc @markdroth

There is an implementation problem in that input is always a string when passed to matchers. I think we need either a dynamic value (e.g. something like CelValue variant) or typed inputs/matchers. Given the constraint, let's use a string serialization for now? We can always add another input once lists are supported.

Yeah, I recall that we talked about making it templatized so that the matcher framework could support types other than string. I'm not sure how much implementation work is involved in doing that, but it sounds like the right approach.

In the short term, I don't have a conceptual problem with using a string serialization, but I think we're going to get into situations where the overhead of serializing and deserializing is way too high to be efficient. So I would like to see us move to a templatized approach before that happens.

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

markdroth · 2022-01-13T16:06:08Z

api/envoy/type/matcher/v3/network_inputs.proto

+// [#protodoc-title: Common Network Matching Inputs]
+
+// Specifies that matching should be performed by the destination IP address.
+message DestinationIPInput {


Are these matchers going to be used anywhere other than in Listener? If so, this is definitely the right place for them. But if not, I wonder if it would be more appropriate to put them somewhere in config/listener/v3.

I think so. RBAC uses port input, for example, which is not really an HTTP property. All of these could be read from HTTP matching data, and probable deserves a follow up. I should rephrase the comments to be less about listeners.

Another option would be to express this as extensions (e.g. api/extensions), that might help avoid bloating envoy/type/matcher?

I think extensions is fine, too. Is envoy/extensions/matching/common_inputs/network good enough?

That path sounds fine.

Hmm. Can we put this in the cncf/xds repo? The more new things we create there, the less we'll have to migrate over time. You can create an extensions/matching/common_inputs/network tree there. Is that going to cause any problems for Envoy extension documentation? CC @envoyproxy/api-shepherds

Moved to extensions. I am fine moving over to CNCF. I think some are somewhat envoy-specific.

I think we need some guidance on which inputs should go to CNCF and which ones should reside in envoy. In particular, the ALPN list is awkward right now, and the "direct" addresses assume a capability to override addresses by a client.

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

kyessenov · 2022-01-19T21:06:28Z

/cc @snowp

snowp

Looks reasonable to me apart from the open(?) question about where to put the protos. Maybe add tests that verify that the factory registration is done correctly?

snowp · 2022-01-19T21:16:23Z

api/envoy/type/matcher/v3/network_inputs.proto

+// [#protodoc-title: Common Network Matching Inputs]
+
+// Specifies that matching should be performed by the destination IP address.
+message DestinationIPInput {


Another option would be to express this as extensions (e.g. api/extensions), that might help avoid bloating envoy/type/matcher?

source/common/network/matching/data_impl.h

kyessenov · 2022-01-20T23:38:39Z

@snowp I updated the trie matcher test to use a real network input to confirm factory registration works.

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

snowp

One nit otherwise this LGTM

source/common/network/matching/data_impl.h

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

snowp

Code LGTM, thanks!

@markdroth still needs to review the API i believe

kyessenov · 2022-02-01T18:56:33Z

@markdroth Could you please review the API changes?

markdroth · 2022-02-08T23:50:49Z

The API changes look good, but I'm still trying to get feedback from the other @envoyproxy/api-shepherds about establishing a policy for when new files should be created in cncf/xds.

We have an API shepherds meeting next Monday, so I've put this on the agenda for that meeting, in case I don't get any input before then.

I'll keep you posted.

kyessenov · 2022-02-15T19:57:41Z

@markdroth Any outcome on the policy? This PR is not immediately usable until unified matchers land for listeners, so there's time to move files around.

markdroth · 2022-02-16T15:36:41Z

We have consensus on a long-term plan to move everything over en masse, with some appropriate automation. So I guess it's fine to leave this here for now, and we'll move it later with everything else.

/lgtm api

kyessenov · 2022-02-16T16:57:03Z

@markdroth Thanks for the update. I think this PR is then ready to be merged.

kyessenov · 2022-02-17T16:20:50Z

@snowp Can we merge this so we can start hashing out docs for #18871?

repokitteh-read-only bot added the api label Jan 11, 2022

repokitteh-read-only bot assigned mattklein123 Jan 11, 2022

kyessenov commented Jan 11, 2022

View reviewed changes

add matching types for network streams

aea58eb

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

kyessenov force-pushed the tcp_data_inputs branch from 1d182f5 to aea58eb Compare January 11, 2022 22:18

kyessenov added 4 commits January 11, 2022 14:31

quoted

f4ed5de

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

docs

41e60d6

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

tests

89eaf62

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

coverage

ebb0634

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

kyessenov changed the title ~~matchers: add input types for network streams (WIP)~~ matchers: add input types for network streams Jan 12, 2022

kyessenov mentioned this pull request Jan 13, 2022

listener: filter chain unified matchers #18871

Closed

kyessenov added 2 commits January 12, 2022 19:58

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

b8b15f8

coverage

7427533

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

kyessenov requested a review from mattklein123 as a code owner January 13, 2022 04:44

markdroth reviewed Jan 13, 2022

View reviewed changes

kyessenov added 2 commits January 13, 2022 10:58

limit listener language

35850bb

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

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

eba5305

snowp reviewed Jan 19, 2022

View reviewed changes

kyessenov added 3 commits January 24, 2022 11:33

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

2eeae31

review

eeef2bf

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

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

8da956e

mattklein123 assigned snowp and unassigned mattklein123 Jan 24, 2022

mattklein123 added the waiting label Jan 24, 2022

fix docs

93fd7fb

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

repokitteh-read-only bot removed the waiting label Jan 24, 2022

snowp reviewed Jan 26, 2022

View reviewed changes

source/common/network/matching/data_impl.h Outdated Show resolved Hide resolved

kyessenov added 2 commits January 26, 2022 09:18

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

bc14d3e

review

a35a28a

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

snowp approved these changes Jan 26, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Feb 16, 2022

snowp merged commit a118134 into envoyproxy:main Feb 17, 2022

This was referenced Feb 25, 2022

udp: add router for UDP proxy #18791

Merged

matching: add UDP support for network input types #20116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

matchers: add input types for network streams #19493

matchers: add input types for network streams #19493

kyessenov commented Jan 11, 2022 •

edited

Loading

repokitteh-read-only bot commented Jan 11, 2022

kyessenov Jan 11, 2022

kyessenov Jan 12, 2022

snowp Jan 19, 2022

kyessenov Jan 20, 2022

markdroth Jan 21, 2022

markdroth Jan 13, 2022

kyessenov Jan 13, 2022

snowp Jan 19, 2022

kyessenov Jan 20, 2022

markdroth Jan 21, 2022

kyessenov Jan 24, 2022

kyessenov Jan 27, 2022

kyessenov commented Jan 19, 2022

snowp left a comment

snowp Jan 19, 2022

kyessenov commented Jan 20, 2022

snowp left a comment

snowp left a comment

kyessenov commented Feb 1, 2022

markdroth commented Feb 8, 2022

kyessenov commented Feb 15, 2022

markdroth commented Feb 16, 2022

kyessenov commented Feb 16, 2022

kyessenov commented Feb 17, 2022

matchers: add input types for network streams #19493

matchers: add input types for network streams #19493

Conversation

kyessenov commented Jan 11, 2022 • edited Loading

repokitteh-read-only bot commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jan 19, 2022

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Jan 20, 2022

snowp left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

kyessenov commented Feb 1, 2022

markdroth commented Feb 8, 2022

kyessenov commented Feb 15, 2022

markdroth commented Feb 16, 2022

kyessenov commented Feb 16, 2022

kyessenov commented Feb 17, 2022

kyessenov commented Jan 11, 2022 •

edited

Loading