matching: add UDP support for network input types

[zhxie]

Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: matching: add UDP support for network input types
Additional Description: network input types and corresponding matching data introduced by #19493 is for network filters which support stream connections only, and this patch will extent the compatibility for UDP listener filters
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[zhxie]

It is interesting to find that a registered matcher factory for BaseClass cannot be created from a matcher tree factory of DerivedClass. The CI complains in test of trie matcher, it cannot find DestinationIPInput (whose factory is registered for MatchingDataBase) from match tree factory of MatchingData. I am not sure is it the right behavior? @mattklein123 @snowp

[mattklein123]

@mattklein123 @snowp

I'm not sure and would need to debug myself unfortunately. @snowp is on vacation and should be back in the next couple of days to help if you want to wait. Thank you.

/wait

[zhxie]

So, maybe we can wait and get some help from snowp. I have done a lot of attempts, but the only solution is to create a UDP matching data, and a lot of UDP inputs and UDP matchers, though these inputs and matchers do the same work with others of TCP, which looks not elegant enough.

[snowp]

The problem is that the factory registrations works via templated factory registrations, which won't honor inheritance. I don' think we have any precedence for this kind of interaction with the existing extension registries, so this will probably require some thinking to come up with a good solution. We do something like this to support "common inputs", where we look for registrations for both the data specific input registrations as well as the common ones.

I think perhaps we'd want some way to declaring a relationship between the more specific and least specific types so that the matcher factory can use this to find all the eligible inputs, but I'm not completely sure on the best way to do this

[zhxie]

It may be difficult to let these registered base factories support their derivations, so I will make these inputs and factories templated to make the code clean. I choose the simple way since this patch is required by #18791, but I think it is still better to find out some ways making the inheritance work.

[zhxie]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #20116 (comment) was created by @zhxie.

see: more, trace.

[snowp]

This seems like an OK solution for now, can you file an issue tracking better ergonomics for shared inputs? I think we generally shy away from template specialization if we can, but I don't have a better solution atm

Could you add a few more tests making sure that all the registration we're doing works as expected?

[snowp]

No need for explicit unless it's a 1-arg ctor

[zhxie]

Thanks, fixed.

[zhxie]

I have added some tests integrating matcher with network inputs.

And I have opened #20324 to track issues about templated factory registrations inheritance.

[zhxie]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #20116 (comment) was created by @zhxie.

see: more, trace.

[kyessenov]

@snowp I think templating based on TCP/UDP matching data will also cause the issue occur in the usage in listener matcher (https://github.com/envoyproxy/envoy/pull/20110/files#diff-33e2c942f20e7830dfae2aac2b4f5851e096028e94f29451ffff7b57487caa26R397). We'd have to create either a matcher for UdpMatchingData or TcpMatchingData as DataType unless we solve the base class factory lookup.

[kyessenov]

I think it would be simpler if we merged this into MatchingData and made socket optional. We can modify the inputs to avoid using socket whenever possible, so they apply to both TCP and UDP.

[zhxie]

I am not sure if it works, since MatchingData currently have inputs like ServerNameInput which only works on TCP now.

[kyessenov]

But you can have SNI in DTLS, no? So in theory, the input could work for datagrams. I'm not certain it's a good idea to combine TCP/UDP datatype, just thinking through.

[zhxie]

I get your idea. Do you think it would be better if we provide interfaces like onSocket, onDestinationIp which is similar to HttpMatchingDataImpl? @snowp

[kyessenov]

The issue is not the callback but the way we get the local address is different between TCP, HTTP, UDP. So I guess it's fine to have separate UDPMatchingData since we have to implement Input functions three times anyways. It's unfortunate that we can't reuse code as much as we'd like.

[zhxie]

Yes, I mean we can make Socket, DestinationIp in MatchingData as OptRefs and use certain methods to assign their value. It is also a feasible way, but I vote for separating MatchingData since the way to construct a MatchingData with socket looks more natural. And I wish some day we can have template inheritance, so their will be no waste of codes.

[snowp]

Given how icky the situation is without, can we take a stab at getting something generic working? I can imagine something like

struct Empty {};
struct A {
    using BaseClass = Empty;
    constexpr static std::string_view name = "a";
};

struct B : A {
    using BaseClass = A;
    constexpr static std::string_view name = "b";
};

template<class T>
void traverseInheritanceChain() {
    std::cout << T::name << std::endl;

    if constexpr (!std::is_same_v<typename T::BaseClass, Empty>) {
        traverseInheritanceChain<typename T::BaseClass>();
    }
}

which would require all the data impls to specify either their "base class" or a singleton type. We could probably come up with something clever to avoid having to specify terminal type but might not be worth the complexity

[zhxie]

We are going to register Factory<BaseMatchingData> for Factory<DerivedMatchingData>, and the method we make factories to find their base class may not work, since their is no direct inheritance between Factory<Base> and Factory<Derived>.

[snowp]

Thanks for digging into this, seems like it will require some more work so let's get this merged

[zhxie]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #20116 (comment) was created by @zhxie.

see: more, trace.

[snowp]

/retest

[repokitteh-read-only]

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

🐱

Caused by: a #20116 (comment) was created by @snowp.

see: more, trace.

[snowp]

Maybe @mattklein123 can give this a second set of maintainer eyes?

[mattklein123]

Thanks this makes sense to me. I just want to understand 1 thing: this is not adding new APIs, it's just implementing existing matchers for UDP, is that right? Either way, do we need any documentation updates here? It seems like there should be documentation in release notes, some examples in the UDP proxy filter docs, and I'm not sure if any of the docs that @kyessenov has been working on should be altered to talk about UDP?

@kyessenov do you mind weighing in here since you have done so much work recently on the docs around this? Thank you.

/wait

[zhxie]

Merge conflicts.

Yes, this patch just implements existing network inputs supports for UDP. And as far as I am concerned, documentation of network inputs is protocol agnostic, so there will be no doc changes. And for release notes, this patch does not bring new features or update current filters, it is just a prerequisite for #18791. I would like to add docs for UDP proxy filter, but it is sad that the filter has not supported router yet.

[kyessenov]

@zhxie Documentation-wise, the only thing that is important is to state which inputs work for UDP. This is done explicitly here https://github.com/envoyproxy/envoy/blob/main/docs/root/intro/arch_overview/advanced/matching/matching_api.rst#network-input-functions. We'll probably automate this table later, but for now it's manual.

[zhxie]

Ok, docs updated. Could you please take a look? @kyessenov

[kyessenov]

Docs LGTM, thanks.

[mattklein123]

Great thanks, we can add full docs in the UDP router feature PR. Thank you!