Skip to content
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

Add relay ip override e2e tests #6028

Conversation

hulthe
Copy link
Contributor

@hulthe hulthe commented Mar 25, 2024


This change is Reviewable

Copy link

linear bot commented Mar 25, 2024

@hulthe hulthe force-pushed the write-relay-selection-tests-for-relay-ip-overrides-des-579 branch 5 times, most recently from 085a825 to 88e14fd Compare March 27, 2024 13:05
@hulthe hulthe marked this pull request as ready for review March 27, 2024 13:07
@MarkusPettersson98 MarkusPettersson98 self-requested a review March 27, 2024 13:20
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1, 5 of 5 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@hulthe hulthe force-pushed the write-relay-selection-tests-for-relay-ip-overrides-des-579 branch 2 times, most recently from 2b425b2 to 121af32 Compare April 2, 2024 09:36
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hulthe)


test/test-manager/src/tests/relay_ip_overrides.rs line 352 at r6 (raw file):

/// Returns a handle that will stop the proxy when dropped.
async fn spawn_tcp_proxy(destination: SocketAddr, port: u16) -> anyhow::Result<AbortOnDrop<()>> {
    let socket = TcpListener::bind((Ipv4Addr::UNSPECIFIED, port)).await?;

On further inspection, I don't think that we should bind to all local interfaces - just the temporary bridge interface created for running the tests.

On Linux, there is this constant network called test_manager::vm::network::linux::TEST_SUBNET which could be used to only bind to the bridge interface on the host. The caveat is that this constant is not available on macOS, but a hacky way to define it would be to let it equal NON_TUN_GATEWAY.

Anyway, I suggest changing Ipv4Addr::UNSPECIFIED to TEST_SUBNET:

let socket = TcpListener::bind((TEST_SUBNET.ip(), port)).await?;

This comment is true for the UDP-socket created in spawn_udp_proxy as well 😊

Code quote:

let socket = TcpListener::bind((Ipv4Addr::UNSPECIFIED, port)).await?;

@hulthe hulthe force-pushed the write-relay-selection-tests-for-relay-ip-overrides-des-579 branch from 5167887 to dc66a13 Compare April 2, 2024 14:13
Copy link
Contributor Author

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


test/test-manager/src/tests/relay_ip_overrides.rs line 352 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

On further inspection, I don't think that we should bind to all local interfaces - just the temporary bridge interface created for running the tests.

On Linux, there is this constant network called test_manager::vm::network::linux::TEST_SUBNET which could be used to only bind to the bridge interface on the host. The caveat is that this constant is not available on macOS, but a hacky way to define it would be to let it equal NON_TUN_GATEWAY.

Anyway, I suggest changing Ipv4Addr::UNSPECIFIED to TEST_SUBNET:

let socket = TcpListener::bind((TEST_SUBNET.ip(), port)).await?;

This comment is true for the UDP-socket created in spawn_udp_proxy as well 😊

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


test/test-manager/src/tests/relay_ip_overrides.rs line 352 at r6 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Done.

Nice! 🔥

@MarkusPettersson98 MarkusPettersson98 force-pushed the write-relay-selection-tests-for-relay-ip-overrides-des-579 branch from dc66a13 to 291f115 Compare April 8, 2024 09:01
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@MarkusPettersson98 MarkusPettersson98 merged commit 86d0ce1 into main Apr 8, 2024
50 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the write-relay-selection-tests-for-relay-ip-overrides-des-579 branch April 8, 2024 09:27
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