matchers: extend network inputs to HTTP requests#20796
matchers: extend network inputs to HTTP requests#20796snowp merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
I think |
| class HttpMatchingDataImpl : public HttpMatchingData { | ||
| public: | ||
| explicit HttpMatchingDataImpl(const Network::ConnectionInfoProvider& connection_info_provider) | ||
| : connection_info_provider_(connection_info_provider) {} |
There was a problem hiding this comment.
Do you think it would be better to provide a StreamInfo directly for HTTP matching data, compared to ConnectionSocket for network matching data?
There was a problem hiding this comment.
StreamInfo is complicated, it can be nested for internal redirects so I wanted to avoid bringing all that complexity implicitly.
I think we'll probably end-up using it though.
|
Thanks @zhxie , added source type, too. |
Signed-off-by: Kuat Yessenov <kuat@google.com>
| const auto& address = data.socket().connectionInfoProvider().localAddress(); | ||
| template <class MatchingDataType> | ||
| Matcher::DataInputGetResult | ||
| DestinationIPInput<MatchingDataType>::get(const MatchingDataType& data) const { |
There was a problem hiding this comment.
I think these template implementations should be inlined and moved to "inputs.h". Otherwise, we may face with linking issues.
There was a problem hiding this comment.
Can you elaborate? I've seen primary templates in .cc files used, and that works as long as it's used only within the .cc file.
There was a problem hiding this comment.
The original contains a full template specialization implementation for MatchingData and another for UdpMatchingData. A template specialization implementation can be located in .cc, and the linker will recognize their symbols for linking. Now, we are going to change to a template member implementation, the implementation should be moved to ".h". We may come up with
// inputs.h
template <class MatchingDataType>
class SourceIPInput : public Matcher::DataInput<MatchingDataType> {
// template member implementation
Matcher::DataInputGetResult get(const MatchingDataType& data) const override {}
}
// template specialization definition for UdpMatchingData
template <>
Matcher::DataInputGetResult
DestinationIPInput<UdpMatchingData>::get(const UdpMatchingData& data) const;
// inputs.cc
// template specialization implementation for UdpMatchingData
template <>
Matcher::DataInputGetResult
DestinationIPInput<UdpMatchingData>::get(const UdpMatchingData& data) const {}|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
|
||
| template <class MatchingDataType> | ||
| Matcher::DataInputGetResult | ||
| DestinationIPInput<MatchingDataType>::get(const MatchingDataType& data) const { |
There was a problem hiding this comment.
Yes, and we can take a step further. The template member implementation can be inlined. We can directly move the code to line 37.
|
cc @snowp friendly ping. |
snowp
left a comment
There was a problem hiding this comment.
LGTM, one optional request
| public: | ||
| Matcher::DataInputGetResult get(const MatchingDataType& data) const override; | ||
| Matcher::DataInputGetResult get(const MatchingDataType& data) const override { | ||
| const auto& address = data.connectionInfoProvider().localAddress(); |
There was a problem hiding this comment.
I wonder if we could add a data.localAddress() to all the matching types and then we don't need the different template specializations? i.e make them all quack the same for these shared input types
Can defer this to another PR if you'd prefer, lmk
There was a problem hiding this comment.
I think the function get<T>(const T&) needs to be implemented for each data type T. We could use a base override getGeneric if all T implement some interface, but we still need to have the boilerplate to call getGeneric from get.
There was a problem hiding this comment.
Ok not important, we can merge for now
refer to envoyproxy#20796 Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Extend network inputs (e.g. source IP) to HTTP requests. Reuse all code with templates. Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
Extend network inputs (e.g. source IP) to HTTP requests. Reuse all code with templates. Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov kuat@google.com
Commit Message: Extend network inputs (e.g. source IP) to HTTP requests. Reuse all code with templates.
Additional Description:
Risk Level: low
Testing: unit
Docs Changes: yes
Release Notes: no
Issue: #20623