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

Fix TLS endpoint port propagation in gossip #1708

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Fix TLS endpoint port propagation in gossip #1708

merged 3 commits into from
Jan 13, 2025

Conversation

Mallets
Copy link
Member

@Mallets Mallets commented Jan 13, 2025

Properly store the TLS listener locator and endpoint as returned by the OS upon socket opening.

@Mallets Mallets added the bug Something isn't working label Jan 13, 2025
@Mallets Mallets requested a review from oteffahi January 13, 2025 13:32

let endpoint = EndPoint::new(
locator.protocol(),
locator.address(),
Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025

Choose a reason for hiding this comment

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

In QUIC link this is local_addr.to_string(). Not sure if it can be different from locator.address() in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

local_addr.to_string() allocates and then the creation of the EndPoint creation allocates a second time. locator.address() returns &str so it allocates one time less.

Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025

Choose a reason for hiding this comment

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

I was more concerned about if the contents of the two variables diverge. Indeed there is a case where they diverge, it seems that locator.address() contains the unresolved hostname, while local_addr contains the resolved one.

For example (on MacOS) when listening on localhost:0:

  • locator.address(): "localhost:49999"
  • local_addr.to_string(): "[::1]:49999"

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like multicast scouting fails to connect when the propagated locator is tls/[::1]:49999, so it's correct to use locator.address(). Will check if QUIC link also has this issue

Copy link
Member Author

Choose a reason for hiding this comment

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

The unresolved address, i.e. locator.address(), should be used in my opinion since the listener has been created on a DNS name. Propagating the already resolved address could lead to some hidden problem about DNS caches in zenoh. Let's every host resolve the IP address from the DNS name based on its own DNS configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some testing, I can confirm there is a similar issue on QUIC.

Copy link
Contributor

@oteffahi oteffahi Jan 13, 2025

Choose a reason for hiding this comment

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

local_addr.to_string() should be replaced by format!("{host}:{}", local_addr.port()) (after computing host variable like so).

let endpoint = EndPoint::new(
endpoint.protocol(),
local_addr.to_string(),
endpoint.metadata(),
endpoint.config(),
)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added

Copy link
Contributor

@oteffahi oteffahi left a comment

Choose a reason for hiding this comment

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

LGTM.

@Mallets Mallets enabled auto-merge (squash) January 13, 2025 15:05
@Mallets Mallets merged commit 554e782 into main Jan 13, 2025
24 of 25 checks passed
@Mallets Mallets deleted the fix/tls_endpoint branch January 13, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants