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

Allow binding to IPv6 (if present) #601

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

deavmi
Copy link
Contributor

@deavmi deavmi commented Nov 3, 2024

If an interface has an IPv6 address record associated with it then, and only then, prefer that.

Otherwise AF_INET is used (Ipv4 address)

  • get_address_for_if(string) done
  • get_broadcast_for_if(string)

If an interface has an IPv6 address record associated with it then, and only then, prefer that.

Otherwise AF_INET is used (Ipv4 address)
@deavmi
Copy link
Contributor Author

deavmi commented Nov 3, 2024

Need to probably do get_broadcast_for_if(string) @markqvist ?

@deavmi
Copy link
Contributor Author

deavmi commented Nov 4, 2024

Okay, all prepped. Need to figure out why the test failing tho :), doesn't seem to be related to the other PR me thinks.

@deavmi
Copy link
Contributor Author

deavmi commented Nov 4, 2024

@markqvist Not sure why but a unittest fails

@deavmi
Copy link
Contributor Author

deavmi commented Nov 4, 2024

Reverted changes to see if CI passes, looks like it didn't. I am even more
confused.

Added changes back anyways.

@deavmi
Copy link
Contributor Author

deavmi commented Nov 6, 2024

Nvm it is passing

@deavmi
Copy link
Contributor Author

deavmi commented Nov 6, 2024

Should be good to merge @markqvist

@rlaneth
Copy link

rlaneth commented Nov 18, 2024

Hi everyone,

I've noticed that rnsd doesn't seem to support binding TCP server interfaces to IPv6 addresses. Upon searching the repository to see if someone else had the same demand, I came across this PR. After reviewing the changes, I have observed:

  1. It appears that this PR does not enable binding to IPv6 via the listen_ip option. Instead, it only works when specifying a network interface with the device option; and

  2. If the device option is used and the specified interface has at least one IPv6 address, the TCP server interface will bind to the first available IPv6 address, and will not bind to the IPv4 address at all.

Am I understanding this correctly?

If so, I have some concerns about merging this. It seems to change the behavior of an existing configuration option in an unexpected way, which could potentially break many existing setups. For instance, consider a case where someone binds to an interface with both a public IPv4 and an IPv6 address, but the IPv6 address isn't actually usable for internet connectivity.

In fact, I believe the current behavior of the device option is already a bit counterintuitive. I would expect it to listen on all IP addresses — both IPv4 and IPv6 — associated with the specified interface. However, the code suggests that it only selects the first available address, which seems limiting.

From my perspective, there are a few key issues here that could be improved:

  1. When a device is specified, the server should listen on all addresses associated with the network interface, including both IPv4 and IPv6.

  2. The listen_ip option should support specifying an IPv6 address directly for the TCP server interface.

  3. Ideally, the TCP server configuration would allow multiple IP addresses, enabling bindings to both IPv4 and IPv6 in the same block (e.g., listen_ip = 0.0.0.0, ::0).

I would really appreciate @markqvist’s thoughts on this since he’s the project maintainer. If he agrees and you, @deavmi, don’t plan to implement these changes, I’d be happy to take this on myself and submit a PR.

Thanks!

@deavmi
Copy link
Contributor Author

deavmi commented Nov 18, 2024

Agreed, listening on all associated IP addresses (of both families) would be ideal and also what I would like.

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