http/transport tap: Add a new UDP sink#37172
http/transport tap: Add a new UDP sink#37172wbpcode merged 11 commits intoenvoyproxy:mainfrom coolg92003:main
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for working on this!
I've looked at the PR description and the issue, but couldn't find an answer: why a custom_sink (see a test example) cannot be used?
cc @mattklein123 @xu1zhou as TAP filter code owners.
thanks |
Then it probably should be fixed, instead of adding a new field. |
|
hi @adisuissa ,
you can see issue #28838, the owner doesn't give an example on how to use this sink. |
I think the config should be similar to the following: Or something of this structure. |
|
Hi @adisuissa, hi @ashishb-solo thanks |
|
Hi @fredyw Would you help check it? |
|
Hi @coolg92003 my apologies for not being more responsive here, I have been very busy lately. Adi's suggestion in this comment is spot-on. I would try changing your configuration to the one that he recommended and continue from there. If you can post the issues that you're experiencing when you try that, we may be able to help you and figure out how to fix it. |
|
I think a close example can be the stateful-session filter (api, config-cc. Similar to "tap" the stateful-session filter is a filter that can be added to the filter chain. In your case the sub-extension will be the UDP sink. |
|
Hi @ashishb-solo , then I try your contribution about custom_sink, I assume that it doesn't need change any envoy code when using this sink, right??? when configure as below: Does this mean that there is no available extension for custom_sink because they don't implement TapSinkFactory as state in link (#28808)? Look forward to you! |
|
Hi @adisuissa ,
Let's wait the answer from @ashishb-solo . thanks |
|
Hi Cliff, You are correct that
So what this means is that you have to take the following steps:
Hopefully I'm not missing any steps here, but I believe that is all that you have to do. Hope this helps, -Ashish |
|
hi @ashishb-solo , |
|
hi @mattklein123 If you prefer to the UDP extension, then would you comment below my procedure? As you know, I have little experience on envoy development, even little on extension.
Look forward to you! Thanks |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
hi @ashishb-solo ,
Code details as below:
package envoy.extensions.tap_sinks.udp_sink.v3;
import "envoy/config/core/v3/address.proto";
import "udpa/annotations/status.proto";
import "validate/validate.proto";
import "udpa/annotations/versioning.proto";
option java_package = "io.envoyproxy.envoy.extensions.tap_sinks.udp_sink.v3";
option java_outer_classname = "UdpSinkProto";
option java_multiple_files = true;
option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/tap_sinks/udp_sink/v3;udp_sinkv3";
option (udpa.annotations.file_status).package_version_status = ACTIVE;
// [#protodoc-title: Udp sink configuration]
// Udp sink configuration.
message UDPSink {
// Configure UDP Address
//
config.core.v3.SocketAddress udp_address = 1;
}
|
|
Great work Cliff!
I think that's a really nice change. The definition of
It looks fine to me. Does it compile? I would try configuring it using the static configuration that Adi linked above (with the Overall, your code looks very good to me. I think you're on the right track! Best, -Ashish |
|
/retest |
|
Hi @adisuissa , |
I'm not sure what is happening in your case, but looking at the link you provided I see the following:
Have you committed your local changes? |
|
Hi @ashishb-solo Hi @adisuissa For now, how we do next? See below for static configuration file transport tap; I will continue to do the test and refine the code. |
|
Hi @ashishb-solo, |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
In addition to the comments, I suggest adding an integration test to make sure this sink works as expected.
| // [#protodoc-title: Udp sink configuration] | ||
| // [#extension: envoy.tap_sinks.udp_sink] | ||
|
|
||
| // Udp sink configuration. |
There was a problem hiding this comment.
Consider adding an example how to configure this sink.
|
Thanks for the contribution. But similar to @adisuissa I do have no more bandwidth to sponsor a feature that I never concerned/used before. I will suggest you to maintain this extension in your fork or your extensions repo (like istio/proxy, etc). If you don't want to maintain a fork, you can still move this extension to contrib which needn't additional sponsor. You can use a @unowned to complete your owner list first if no another one to support this feature. |
|
/wait |
|
Hi @wbpcode
Really appreciated your comments and suggestion! |
|
Hi @adisuissa,
Sure, I can do that. this is complicated because there is no udp server there. I am still thinking... one more question, except Matt and wbpcode, who else can sponsor this extension? code is very easy and it is configurable. Therefore, the risk is the lowest. From the senior owner in this link, who can be contacted? The reason that I don't prefer to go contrib is that it doesn't match current design. At same time, let me study on how to write code in contrib. thanks |
|
Seems that envoy has lots of limitation on time and thread and still thinking integration test case... |
|
/retest |
| verifyStaticFilePerTap(fmt::format(filter_config, getTempPathPrefix())); | ||
| } | ||
|
|
||
| TEST_P(TapIntegrationTest, StaticExtTapSinkUdp) { |
There was a problem hiding this comment.
This should probably be added in a dedicated integration test file for tap_sinks/udp_sink.
For non-contrib extensions it is better to find an owner that has some need for this extension (or a very close one). Each new extension adds some overhead to the maintainers, as they will need to address issues and direct the future development of the extension.
Many companies add their own extensions (not necessarily in the Envoy-proxy repo), and link them together.
|
|
Hi @adisuissa, @ashishb-solo @wbpcode ,
I prefer to 2), any changes can be updated continuously in comment of issue, and no extra other work. thanks |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for your great contribution. Some initial comments to start the review.
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for your contribution. Some new comments are added.
| TapCommon::SinkPtr UdpTapSinkFactory::createTransportSinkPtr( | ||
| const Protobuf::Message& config, | ||
| Server::Configuration::TransportSocketFactoryContext& tsf_context) { | ||
| ENVOY_LOG_MISC(trace, "{}: Create UDP sink in transport context", __func__); |
There was a problem hiding this comment.
I think __func__ is unnecessary because ENVOY_LOG macro will print the line number also?
| TapCommon::SinkPtr | ||
| UdpTapSinkFactory::createHttpSinkPtr(const Protobuf::Message& config, | ||
| Server::Configuration::FactoryContext& http_context) { | ||
| ENVOY_LOG_MISC(trace, "{}: Create UDP sink in http context", __func__); |
| config, http_context.serverFactoryContext() | ||
| .messageValidationContext() | ||
| .staticValidationVisitor())); |
There was a problem hiding this comment.
Please use messageValidationVisitor from http_context directly if there is no special reason.
| class UdpTapSinkFactory : public TapCommon::TapSinkFactory { | ||
| public: | ||
| ~UdpTapSinkFactory() override = default; | ||
| std::string category() const override { return "envoy.tap.sinks.udp"; } |
There was a problem hiding this comment.
I think you shouldn't overwrite the category() method.
| // below one is only for UT because | ||
| // it can't control return value of writePacket if not add below function. | ||
| UdpTapSink(Network::UdpPacketWriterPtr&& utUdpPacketWriter) | ||
| : udp_packet_writer_(std::move(utUdpPacketWriter)) { | ||
| std::string utUdpServerAddress("127.0.0.1"); | ||
| udp_server_address_ = | ||
| Network::Utility::parseInternetAddressNoThrow(utUdpServerAddress, 8080, false); | ||
| } |
There was a problem hiding this comment.
If this is only used for unit, it would be better to create a derived class in the unit test files and move this method to the test files.
Or it would be better to let the constructor to take both the address and writer.
|
LGTM. Thanks. Please merge main and resolve the conflict. Thanks again. :) |
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
Signed-off-by: fchen7 <cliff.chen@nokia-sbell.com>
|
hi @wbpcode , @adisuissa @ashishb-solo, |
4 participants
Commit Message:
Add an UDP sink for HTTP/Transport Tap
Additional Description:
See more detailed information in issue #36945
Risk Level: low
Testing:
Manual test has been done
Also CPR test is done
see issue 36945 for more information
Docs Changes:
Build is passed
Release Notes:
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]