-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
feat(sources): multicast udp socket support #22099
base: master
Are you sure you want to change the base?
feat(sources): multicast udp socket support #22099
Conversation
This looks awesome🤯 |
Hi @nomalord perhaps you could test this? |
I don't really have experience with rust, but I'll try to compile the feature branch and update you🥹 |
Maybe this is helpful to you:
|
Thank you for popping in @pront 😃. I have left to include some tests to validate this PR and also add documentation & changelog. I will update this as soon as I can and mark the pr as non-draft then |
Hi @pront, this PR is now ready to review, just notifying, no hurries! |
Thank you 🚀 I will get to this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jorgehermo9, great work. Nothing major came out, mostly nits.
Thanks for the review @pront! I will happily address the comments as I prefer to fix those nits before merging! |
Co-authored-by: Pavlos Rontidis <[email protected]>
socket | ||
.join_multicast_v4(group_addr, interface) | ||
.map_err(|error| { | ||
emit!(SocketMulticastGroupJoinError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested https://github.com/vectordotdev/vector/pull/22099/files#r1927304213
does it look ok?
|
||
#[tokio::test] | ||
async fn udp_invalid_multicast_group() { | ||
assert_source_error(&COMPONENT_ERROR_TAGS, async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored this a little in dc09eb5. Didn't know that assert_source_error
existed. Is it ok to check that only the tag error_type
is updated? (COMPONENT_ERROR_TAGS
only contains that tag)
pub interface: Ipv4Addr, | ||
} | ||
|
||
impl<E: std::fmt::Display> InternalEvent for SocketMulticastGroupJoinError<E> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inspired from the SocketBindError
implementation
@@ -135,6 +137,37 @@ impl<E: std::fmt::Display> InternalEvent for SocketBindError<E> { | |||
} | |||
} | |||
|
|||
#[derive(Debug)] | |||
pub struct SocketMulticastGroupJoinError<E> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doubts about this naming. SocketMulticastJoinError
, UdpSocketMulticastJoinError
, UdpSocketMulticastGroupJoinError
....?
This error will be created only from UDP mode, as the multicast concept only exists UDP.
"group_addr" => group_addr, | ||
"interface" => interface, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to add those two variables (group_addr
and interface
) to counter tags? Or should only be included in the error!
log?
#[derive(Debug)] | ||
pub struct SocketMulticastGroupJoinError<E> { | ||
pub error: E, | ||
pub group_addr: Ipv4Addr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the group_addr
and interface
because they're the arguments required for join_multicast_v4
vector/src/sources/socket/udp.rs
Line 191 in a2930d2
.join_multicast_v4(group_addr, interface) |
error = %self.error, | ||
error_code = "socket_multicast_group_join", | ||
error_type = error_type::IO_FAILED, | ||
stage = error_stage::RECEIVING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error_stage receiving? I found that stage weird in this case. I think INITIALIZING
or so would fit more in this case... but I see that error_stage::RECEIVING
is used also for socket binding error and etc.. Just noticed this and wanted to raise it, not for addressing in this PR though
Hi @pront. Sorry for the delay, had no time these past weeks. Addressed all your comments and left a few doubts about the new changes. Feel free to resolve any conversation, just wanted to raise some things so you are aware of it. Thanks! |
Closes #5732
This PR is still in draft. I have a few pending
TODOs
and also missing tests to propertly check this. Although, the happy path is working.In order to test this, use this vector config:
and with this command
you can see logs in vector.
![image](https://private-user-images.githubusercontent.com/23034060/399499116-764c3224-be79-45a6-a184-64668262755a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4MzY0MjIsIm5iZiI6MTczOTgzNjEyMiwicGF0aCI6Ii8yMzAzNDA2MC8zOTk0OTkxMTYtNzY0YzMyMjQtYmU3OS00NWE2LWExODQtNjQ2NjgyNjI3NTVhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE3VDIzNDg0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTI2OTA1NmNmMjFhMGZhNTRjZDcwMjkzMGFjYzU4YWQ2Njc1NTlhZWRjY2UwMzM0ZTAzNzk0NDBmYjNmZjIzNmQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.MCLOK23QEXZP28Vis_d331ZTwg3gNNXIuk9iz1UlNok)
I would like to receive some feedback about how the configuration of this setting should look like.
Also, note that IPv6 is not supported. We can work on that, but maybe it is not worth it if no one request that.
@nomalord take a look into this please, It would be great if you can build the binary from this branch and test if it works in your systems.
@dalesample as you were the first requester of this I also ping you, just in case (although this issue was created 4 years ago)