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 to listen on multiple IP addresses #174

Merged
merged 1 commit into from
Aug 1, 2022
Merged

Conversation

smortex
Copy link
Member

@smortex smortex commented Jun 28, 2022

Allow to pass multiple IP addresses. The default behavior is to listen on IPv4 only so this is only a breaking change because the interface change.

@smortex smortex changed the title Allow to setup multiple listening addresses Allow to setup multiple listening addresses and listen on IPv6 Jun 28, 2022
@smortex smortex added the enhancement New feature or request label Jun 28, 2022
@smortex smortex marked this pull request as ready for review June 28, 2022 05:14
@smortex smortex requested a review from neomilium June 28, 2022 05:15
@smortex
Copy link
Member Author

smortex commented Jun 28, 2022

But maybe since #173 breaks backward compatibility, no need to support a String for the parameters and only accept Array[String[1]]?

@neomilium
Copy link
Contributor

But maybe since #173 breaks backward compatibility, no need to support a String for the parameters and only accept Array[String[1]]?

I agree, this makes code more simple.

@smortex
Copy link
Member Author

smortex commented Jun 28, 2022

☑️ backward compatibility code removed

@neomilium
Copy link
Contributor

@smortex Whould you like I merge too on this project after a review?

@smortex
Copy link
Member Author

smortex commented Jun 30, 2022

I would prefer to have a second approval by somebody external of @opus-codium we both belong to before merging 😉

If we have no news in a few day, no objection to pull the trigger.

manifests/client.pp Outdated Show resolved Hide resolved
@smortex smortex changed the title Allow to setup multiple listening addresses and listen on IPv6 Allow to setup multiple listening addresses Jul 12, 2022
@smortex smortex added backwards-incompatible This change will lead to a major version bump for the next release and removed enhancement New feature or request labels Jul 12, 2022
@smortex smortex changed the title Allow to setup multiple listening addresses Allow to listen on multiple IP addresses Jul 12, 2022
@smortex
Copy link
Member Author

smortex commented Jul 12, 2022

I updated the PR to not set an explicit listen address by default, which bacula understands as listen on all IPv4 only (so same behavior as before).

Copy link
Contributor

@neomilium neomilium left a comment

Choose a reason for hiding this comment

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

I updated the PR to not set an explicit listen address by default, which bacula understands as listen on all IPv4 only (so same behavior as before).

IMHO, this behavior should be shorty documented. Could you put a note in the variable documentation?

@smortex
Copy link
Member Author

smortex commented Jul 13, 2022

The fact that :: also listen on IPv4 is linked to Linux's default of /proc/sys/net/ipv6/bindv6only which is 0. I just checked on FreeBSD where the default is not to do this kind of thing and the service only listen on IPv6 with such a configuration. Putting both 0.0.0.0 and :: on FreeBSD listen on both addresses, doing so on Linux fail because the service wants to bind twice on the same port for IPv4. Setting /proc/sys/net/ipv6/bindv6only to 1 "fix" this behavior and we can put explicitly :: and 0.0.0.0 as listen addresses and the service will be reachable over IPv4 and IPv6.

The only thing that annoys me here is that the service only listen on IPv4 by default (which is an issue at Bacula's level), and I do not think the module documentation is the right place to describe this since this can change at any point upstream?

@neomilium
Copy link
Contributor

The only thing that annoys me here is that the service only listen on IPv4 by default (which is an issue at Bacula's level), and I do not think the module documentation is the right place to describe this since this can change at any point upstream?

OK, so we had to support OS-independent and idempotent way. What about putting some hiera files, OS-dependant to reach the same behavior on each OS?

@smortex
Copy link
Member Author

smortex commented Jul 29, 2022

The only thing that annoys me here is that the service only listen on IPv4 by default (which is an issue at Bacula's level), and I do not think the module documentation is the right place to describe this since this can change at any point upstream?

OK, so we had to support OS-independent and idempotent way. What about putting some hiera files, OS-dependant to reach the same behavior on each OS?

I really don't know how we could deal with this: system dependent config also look wrong to me: the default behavior of FreeBSD and e.g. Debian is not the same in the way that Debian offer IPv6 mapped IPv4 by default while FreeBSD doesn't. But as stated above, Debian can be tuned to opt-out, and of course FreeBSD can be tuned to opt-in 🤷 Above I wrote "Linux" but I really meant "Debian", I don't know if the default is the same for every Linux distro, and I would not be surprised to see it is not. And since it is tunable, I think that sticking to upstream default is the best way to not break POLA.

Maybe in the end a note in the README is better? What information would you like to see there?

@smortex
Copy link
Member Author

smortex commented Aug 1, 2022

Could you put a note in the variable documentation?

"variable documentation" 🤯

Yeah! Did that! Thanks!

By default, the daemons listen on the system IPv4 address only.  The
year is 2022, we can listen on IPv6 too :-)
Copy link
Contributor

@neomilium neomilium left a comment

Choose a reason for hiding this comment

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

Excellent explanation (in reference documentation)!

@neomilium neomilium merged commit 23f47c2 into master Aug 1, 2022
@neomilium neomilium deleted the listen-on-ipv6-too branch August 1, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatible This change will lead to a major version bump for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants