Skip to content

Passthrough traffic mirroring #3279

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

Conversation

Razz4780
Copy link
Contributor

@Razz4780 Razz4780 commented Apr 23, 2025

Oh well

@Razz4780 Razz4780 force-pushed the michals/mbe-705-passthrough-traffic-mirroring branch from 9a309eb to 7707492 Compare April 24, 2025 12:34
@t4lz
Copy link
Member

t4lz commented May 13, 2025

One general thing: does passthrough_mirroring make sense as a user facing name?
Where does this name come from? Is that a well known term for what we're doing here?
If not, we should try to name it in a way that helps users understand what it does, especially what effect it has on them (which I am still not sure about). Like reset_mirrored_connections or iptables_mirroring (though this one is also a bit TMI).

/// Handles mirrord's file operations, see [`FileManager`].
file_manager: FileManager,
connection: ClientConnection,
/// [`None`] when targetless.
tcp_sniffer_api: Option<TcpSnifferApi>,
tcp_mirror_api: Option<Box<dyn TcpMirrorApi>>,
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand why we need/want dynamic dispatch and can't/don't want to use normal generics, with a trait bounded type variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have needed to change the way ClientConnectionHandler is created, and it was a bit messy :V
Are you against dynamic dispatch? We can do an enum as well

Comment on lines +508 to +514
if let Some(tcp_stealer_api) = self.tcp_steal_api.as_mut() {
if let Err(error) = tcp_stealer_api.handle_client_message(message).await {
self.respond(DaemonMessage::Close(format!(
"invalid HTTP filter: {error}"
)))
.await?;
}
Copy link
Member

Choose a reason for hiding this comment

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

"invalid HTTP filter" looks pretty arbitrary in here. Even if it's the only possible reason for an error (is it?) wouldn't it be better to determine that with a match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error type of handle_client_message is fancy_regex::Error. Not sure what you mean by match. Do you want sth like this?

                    if let Err(error) = tcp_stealer_api.handle_client_message(message).await {
                        let error: fancy_regex::Error = error;
                        self.respond(DaemonMessage::Close(format!(
                            "invalid HTTP filter: {error}"
                        )))
                        .await?;
                    }

This would work as a reminder to update the error message, when someone changes the error type.
Or should the method return just Result<_, String> (message for DaemonMessage::Close) 🤔

@Razz4780
Copy link
Contributor Author

One general thing: does passthrough_mirroring make sense as a user facing name? Where does this name come from? Is that a well known term for what we're doing here? If not, we should try to name it in a way that helps users understand what it does, especially what effect it has on them (which I am still not sure about). Like reset_mirrored_connections or iptables_mirroring (though this one is also a bit TMI).

I didn't think about it much tbh. I believe I came up with this name when presenting the idea internally. I agree that iptables_mirroring makes more sense. I wouldn't worry about it being too specific, as this config option is temporary. Eventually we'll drop packet sniffing and make iptables the only implementation.

@Razz4780 Razz4780 force-pushed the michals/mbe-705-passthrough-traffic-mirroring branch from 105eea0 to 2339fa0 Compare May 20, 2025 17:04
@Razz4780 Razz4780 force-pushed the michals/mbe-705-passthrough-traffic-mirroring branch from 5e54163 to 7f28da9 Compare May 21, 2025 08:06
@Razz4780 Razz4780 force-pushed the michals/mbe-705-passthrough-traffic-mirroring branch from b731d61 to 15163e2 Compare May 21, 2025 08:46
@Razz4780 Razz4780 force-pushed the michals/mbe-705-passthrough-traffic-mirroring branch from e0b72c6 to 13a0b32 Compare May 21, 2025 09:30
@Razz4780
Copy link
Contributor Author

Ok, there's something wrong here, some kind of a race.
I'm splitting this PR further, cause it's too big to find it ;_;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants