Skip to content

matching: add UDP support for network input types #20116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 8 commits into from
Mar 25, 2022
Merged

matching: add UDP support for network input types #20116

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a914cf
matching: UDP support for network input types
zhxie Feb 25, 2022
d0865a8
Revert "matching: UDP support for network input types"
zhxie Feb 28, 2022
d322434
matching: UDP support for network input types
zhxie Feb 28, 2022
557b729
matching: templatize network inputs and their factories
zhxie Mar 8, 2022
6751438
nit: remove unwanted explicit
zhxie Mar 14, 2022
536e089
test: add integration test for network matching data
zhxie Mar 14, 2022
59b6bdb
Merge remote-tracking branch 'envoyproxy/main' into udp-network-inputs
zhxie Mar 25, 2022
8525928
docs: add UDP network inputs
zhxie Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions envoy/network/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,5 +531,18 @@ class MatchingData {
virtual const ConnectionSocket& socket() const PURE;
};

/**
* UDP listener filter matching context data for unified matchers.
*/
class UdpMatchingData {

@kyessenov kyessenov Mar 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zhxie Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kyessenov kyessenov Mar 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zhxie Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kyessenov Mar 16, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zhxie Mar 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 snowp Mar 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zhxie Mar 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 snowp Mar 24, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public:
static absl::string_view name() { return "network"; }

virtual ~UdpMatchingData() = default;

virtual const Address::InstanceConstSharedPtr& localAddress() const PURE;
virtual const Address::InstanceConstSharedPtr& remoteAddress() const PURE;
};

} // namespace Network
} // namespace Envoy
16 changes: 16 additions & 0 deletions source/common/network/matching/data_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,22 @@ class MatchingDataImpl : public MatchingData {
const ConnectionSocket& socket_;
};

/**
* Implementation of Network::UdpMatchingData, providing UDP data to the match tree.
*/
class UdpMatchingDataImpl : public UdpMatchingData {
public:
explicit UdpMatchingDataImpl(const Address::InstanceConstSharedPtr& local_address,

@snowp snowp Mar 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zhxie zhxie Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@zhxie zhxie Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some tests integrating matcher with network inputs.

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

const Address::InstanceConstSharedPtr& remote_address)
: local_address_(local_address), remote_address_(remote_address) {}
const Address::InstanceConstSharedPtr& localAddress() const override { return local_address_; }
const Address::InstanceConstSharedPtr& remoteAddress() const override { return remote_address_; }

private:
const Address::InstanceConstSharedPtr& local_address_;
const Address::InstanceConstSharedPtr& remote_address_;
};

} // namespace Matching
} // namespace Network
} // namespace Envoy
60 changes: 56 additions & 4 deletions source/common/network/matching/inputs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ namespace Envoy {
namespace Network {
namespace Matching {

Matcher::DataInputGetResult DestinationIPInput::get(const MatchingData& data) const {
template <>
Matcher::DataInputGetResult DestinationIPInput<MatchingData>::get(const MatchingData& data) const {
const auto& address = data.socket().connectionInfoProvider().localAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
Expand All @@ -19,7 +20,20 @@ Matcher::DataInputGetResult DestinationIPInput::get(const MatchingData& data) co
address->ip()->addressAsString()};
}

Matcher::DataInputGetResult DestinationPortInput::get(const MatchingData& data) const {
template <>
Matcher::DataInputGetResult
DestinationIPInput<UdpMatchingData>::get(const UdpMatchingData& data) const {
const auto& address = data.localAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
}
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable,
address->ip()->addressAsString()};
}

template <>
Matcher::DataInputGetResult
DestinationPortInput<MatchingData>::get(const MatchingData& data) const {
const auto& address = data.socket().connectionInfoProvider().localAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
Expand All @@ -28,7 +42,19 @@ Matcher::DataInputGetResult DestinationPortInput::get(const MatchingData& data)
absl::StrCat(address->ip()->port())};
}

Matcher::DataInputGetResult SourceIPInput::get(const MatchingData& data) const {
template <>
Matcher::DataInputGetResult
DestinationPortInput<UdpMatchingData>::get(const UdpMatchingData& data) const {
const auto& address = data.localAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
}
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable,
absl::StrCat(address->ip()->port())};
}

template <>
Matcher::DataInputGetResult SourceIPInput<MatchingData>::get(const MatchingData& data) const {
const auto& address = data.socket().connectionInfoProvider().remoteAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
Expand All @@ -37,7 +63,18 @@ Matcher::DataInputGetResult SourceIPInput::get(const MatchingData& data) const {
address->ip()->addressAsString()};
}

Matcher::DataInputGetResult SourcePortInput::get(const MatchingData& data) const {
template <>
Matcher::DataInputGetResult SourceIPInput<UdpMatchingData>::get(const UdpMatchingData& data) const {
const auto& address = data.remoteAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
}
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable,
address->ip()->addressAsString()};
}

template <>
Matcher::DataInputGetResult SourcePortInput<MatchingData>::get(const MatchingData& data) const {
const auto& address = data.socket().connectionInfoProvider().remoteAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
Expand All @@ -46,6 +83,17 @@ Matcher::DataInputGetResult SourcePortInput::get(const MatchingData& data) const
absl::StrCat(address->ip()->port())};
}

template <>
Matcher::DataInputGetResult
SourcePortInput<UdpMatchingData>::get(const UdpMatchingData& data) const {
const auto& address = data.remoteAddress();
if (address->type() != Network::Address::Type::Ip) {
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable, absl::nullopt};
}
return {Matcher::DataInputGetResult::DataAvailability::AllDataAvailable,
absl::StrCat(address->ip()->port())};
}

Matcher::DataInputGetResult DirectSourceIPInput::get(const MatchingData& data) const {
const auto& address = data.socket().connectionInfoProvider().directRemoteAddress();
if (address->type() != Network::Address::Type::Ip) {
Expand Down Expand Up @@ -91,9 +139,13 @@ Matcher::DataInputGetResult ApplicationProtocolInput::get(const MatchingData& da
}

REGISTER_FACTORY(DestinationIPInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(UdpDestinationIPInputFactory, Matcher::DataInputFactory<UdpMatchingData>);
REGISTER_FACTORY(DestinationPortInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(UdpDestinationPortInputFactory, Matcher::DataInputFactory<UdpMatchingData>);
REGISTER_FACTORY(SourceIPInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(UdpSourceIPInputFactory, Matcher::DataInputFactory<UdpMatchingData>);
REGISTER_FACTORY(SourcePortInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(UdpSourcePortInputFactory, Matcher::DataInputFactory<UdpMatchingData>);
REGISTER_FACTORY(DirectSourceIPInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(SourceTypeInputFactory, Matcher::DataInputFactory<MatchingData>);
REGISTER_FACTORY(ServerNameInputFactory, Matcher::DataInputFactory<MatchingData>);
Expand Down
109 changes: 77 additions & 32 deletions source/common/network/matching/inputs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ namespace Envoy {
namespace Network {
namespace Matching {

template <class InputType, class ProtoType>
class BaseFactory : public Matcher::DataInputFactory<MatchingData> {
template <class InputType, class ProtoType, class MatchingDataType>
class BaseFactory : public Matcher::DataInputFactory<MatchingDataType> {
protected:
explicit BaseFactory(const std::string& name) : name_(name) {}

public:
std::string name() const override { return name_; }

Matcher::DataInputFactoryCb<MatchingData>
Matcher::DataInputFactoryCb<MatchingDataType>
createDataInputFactoryCb(const Protobuf::Message&, ProtobufMessage::ValidationVisitor&) override {
return []() { return std::make_unique<InputType>(); };
};
Expand All @@ -29,56 +29,96 @@ class BaseFactory : public Matcher::DataInputFactory<MatchingData> {
const std::string name_;
};

class DestinationIPInput : public Matcher::DataInput<MatchingData> {
template <class MatchingDataType>
class DestinationIPInput : public Matcher::DataInput<MatchingDataType> {
public:
Matcher::DataInputGetResult get(const MatchingData& data) const override;
Matcher::DataInputGetResult get(const MatchingDataType& data) const override;
};

class DestinationIPInputFactory
template <class MatchingDataType>
class DestinationIPInputBaseFactory
: public BaseFactory<
DestinationIPInput,
envoy::extensions::matching::common_inputs::network::v3::DestinationIPInput> {
DestinationIPInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::DestinationIPInput,
MatchingDataType> {
public:
DestinationIPInputFactory() : BaseFactory("destination-ip") {}
DestinationIPInputBaseFactory()
: BaseFactory<DestinationIPInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::DestinationIPInput,
MatchingDataType>("destination-ip") {}
};

class DestinationPortInput : public Matcher::DataInput<MatchingData> {
class DestinationIPInputFactory : public DestinationIPInputBaseFactory<MatchingData> {};

class UdpDestinationIPInputFactory : public DestinationIPInputBaseFactory<UdpMatchingData> {};

template <class MatchingDataType>
class DestinationPortInput : public Matcher::DataInput<MatchingDataType> {
public:
Matcher::DataInputGetResult get(const MatchingData& data) const override;
Matcher::DataInputGetResult get(const MatchingDataType& data) const override;
};

class DestinationPortInputFactory
template <class MatchingDataType>
class DestinationPortInputBaseFactory
: public BaseFactory<
DestinationPortInput,
envoy::extensions::matching::common_inputs::network::v3::DestinationPortInput> {
DestinationPortInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::DestinationPortInput,
MatchingDataType> {
public:
DestinationPortInputFactory() : BaseFactory("destination-port") {}
DestinationPortInputBaseFactory()
: BaseFactory<DestinationPortInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::DestinationPortInput,
MatchingDataType>("destination-port") {}
};

class SourceIPInput : public Matcher::DataInput<MatchingData> {
class DestinationPortInputFactory : public DestinationPortInputBaseFactory<MatchingData> {};

class UdpDestinationPortInputFactory : public DestinationPortInputBaseFactory<UdpMatchingData> {};

template <class MatchingDataType>
class SourceIPInput : public Matcher::DataInput<MatchingDataType> {
public:
Matcher::DataInputGetResult get(const MatchingData& data) const override;
Matcher::DataInputGetResult get(const MatchingDataType& data) const override;
};

class SourceIPInputFactory
: public BaseFactory<SourceIPInput,
envoy::extensions::matching::common_inputs::network::v3::SourceIPInput> {
template <class MatchingDataType>
class SourceIPInputBaseFactory
: public BaseFactory<SourceIPInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::SourceIPInput,
MatchingDataType> {
public:
SourceIPInputFactory() : BaseFactory("source-ip") {}
SourceIPInputBaseFactory()
: BaseFactory<SourceIPInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::SourceIPInput,
MatchingDataType>("source-ip") {}
};

class SourcePortInput : public Matcher::DataInput<MatchingData> {
class SourceIPInputFactory : public SourceIPInputBaseFactory<MatchingData> {};

class UdpSourceIPInputFactory : public SourceIPInputBaseFactory<UdpMatchingData> {};

template <class MatchingDataType>
class SourcePortInput : public Matcher::DataInput<MatchingDataType> {
public:
Matcher::DataInputGetResult get(const MatchingData& data) const override;
Matcher::DataInputGetResult get(const MatchingDataType& data) const override;
};

class SourcePortInputFactory
: public BaseFactory<SourcePortInput,
envoy::extensions::matching::common_inputs::network::v3::SourcePortInput> {
template <class MatchingDataType>
class SourcePortInputBaseFactory
: public BaseFactory<SourcePortInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::SourcePortInput,
MatchingDataType> {
public:
SourcePortInputFactory() : BaseFactory("source-port") {}
SourcePortInputBaseFactory()
: BaseFactory<SourcePortInput<MatchingDataType>,
envoy::extensions::matching::common_inputs::network::v3::SourcePortInput,
MatchingDataType>("source-port") {}
};

class SourcePortInputFactory : public SourcePortInputBaseFactory<MatchingData> {};

class UdpSourcePortInputFactory : public SourcePortInputBaseFactory<UdpMatchingData> {};

class DirectSourceIPInput : public Matcher::DataInput<MatchingData> {
public:
Matcher::DataInputGetResult get(const MatchingData& data) const override;
Expand All @@ -87,7 +127,8 @@ class DirectSourceIPInput : public Matcher::DataInput<MatchingData> {
class DirectSourceIPInputFactory
: public BaseFactory<
DirectSourceIPInput,
envoy::extensions::matching::common_inputs::network::v3::DirectSourceIPInput> {
envoy::extensions::matching::common_inputs::network::v3::DirectSourceIPInput,
MatchingData> {
public:
DirectSourceIPInputFactory() : BaseFactory("direct-source-ip") {}
};
Expand All @@ -99,7 +140,8 @@ class SourceTypeInput : public Matcher::DataInput<MatchingData> {

class SourceTypeInputFactory
: public BaseFactory<SourceTypeInput,
envoy::extensions::matching::common_inputs::network::v3::SourceTypeInput> {
envoy::extensions::matching::common_inputs::network::v3::SourceTypeInput,
MatchingData> {
public:
SourceTypeInputFactory() : BaseFactory("source-type") {}
};
Expand All @@ -111,7 +153,8 @@ class ServerNameInput : public Matcher::DataInput<MatchingData> {

class ServerNameInputFactory
: public BaseFactory<ServerNameInput,
envoy::extensions::matching::common_inputs::network::v3::ServerNameInput> {
envoy::extensions::matching::common_inputs::network::v3::ServerNameInput,
MatchingData> {
public:
ServerNameInputFactory() : BaseFactory("server-name") {}
};
Expand All @@ -124,7 +167,8 @@ class TransportProtocolInput : public Matcher::DataInput<MatchingData> {
class TransportProtocolInputFactory
: public BaseFactory<
TransportProtocolInput,
envoy::extensions::matching::common_inputs::network::v3::TransportProtocolInput> {
envoy::extensions::matching::common_inputs::network::v3::TransportProtocolInput,
MatchingData> {
public:
TransportProtocolInputFactory() : BaseFactory("transport-protocol") {}
};
Expand All @@ -137,7 +181,8 @@ class ApplicationProtocolInput : public Matcher::DataInput<MatchingData> {
class ApplicationProtocolInputFactory
: public BaseFactory<
ApplicationProtocolInput,
envoy::extensions::matching::common_inputs::network::v3::ApplicationProtocolInput> {
envoy::extensions::matching::common_inputs::network::v3::ApplicationProtocolInput,
MatchingData> {
public:
ApplicationProtocolInputFactory() : BaseFactory("application-protocol") {}
};
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/common/matcher/trie_matcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace Matcher {

REGISTER_FACTORY(NetworkTrieMatcherFactory,
::Envoy::Matcher::CustomMatcherFactory<Network::MatchingData>);
REGISTER_FACTORY(UdpNetworkTrieMatcherFactory,
::Envoy::Matcher::CustomMatcherFactory<Network::UdpMatchingData>);

} // namespace Matcher
} // namespace Common
Expand Down
1 change: 1 addition & 0 deletions source/extensions/common/matcher/trie_matcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ class TrieMatcherFactoryBase : public ::Envoy::Matcher::CustomMatcherFactory<Dat
};

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

} // namespace Matcher
} // namespace Common
Expand Down
Loading