matchers: add input types for network streams#19493
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I made them quoted to make it easier to say "first element is h2, not h2c".
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
1d182f5 to
aea58eb
Compare
| // [#protodoc-title: Common Network Matching Inputs] | ||
|
|
||
| // Specifies that matching should be performed by the destination IP address. | ||
| message DestinationIPInput { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Another option would be to express this as extensions (e.g. api/extensions), that might help avoid bloating envoy/type/matcher?
There was a problem hiding this comment.
I think extensions is fine, too. Is envoy/extensions/matching/common_inputs/network good enough?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Moved to extensions. I am fine moving over to CNCF. I think some are somewhat envoy-specific.
There was a problem hiding this comment.
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>
|
/cc @snowp |
snowp
left a comment
There was a problem hiding this comment.
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?
| // [#protodoc-title: Common Network Matching Inputs] | ||
|
|
||
| // Specifies that matching should be performed by the destination IP address. | ||
| message DestinationIPInput { |
There was a problem hiding this comment.
Another option would be to express this as extensions (e.g. api/extensions), that might help avoid bloating envoy/type/matcher?
|
@snowp I updated the trie matcher test to use a real network input to confirm factory registration works. |
snowp
left a comment
There was a problem hiding this comment.
One nit otherwise this LGTM
snowp
left a comment
There was a problem hiding this comment.
Code LGTM, thanks!
@markdroth still needs to review the API i believe
|
@markdroth Could you please review the API changes? |
|
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. |
|
@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. |
|
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 |
|
@markdroth Thanks for the update. I think this PR is then ready to be merged. |
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