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

Feature/network manager dhcp support #5109

Conversation

libklein
Copy link

@libklein libklein commented Jan 5, 2023

What does this PR aim to accomplish?:

This PR implements support for setting a static IP via NetworkManager. See #4783.

How does this PR accomplish the above?:

The codes adds a check if network-manager is installed and if it manages the selected interface. If so, nmcli is used to configure the interface.

Note that this is a preliminary implementation that will need to get tested once Raspberry OS makes the change. Until then it could be used to support more operating systems.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@libklein libklein changed the base branch from master to development January 5, 2023 23:39
@yubiuser
Copy link
Member

yubiuser commented Jan 6, 2023

Thanks for your PR.

However, this will need some discussion. Some time ago we decided to drop dhcpcd as a dependency (#4260) to not interfere with user's selected network configuration software. We decided to only offer setting a static IP addresses when dhcpcd is already installed to lower entry barriers for new users (mainly targeting Raspbian). With the announced switch from dhcpcd to NM (#4902) our decision needs to be revised.

Do we want to continue to offer setting static address during installation? Do we want to include NM already now? Or only after Raspbian made the change?

__

In any case, we would greatly appreciate a contribution to our documentation (https://docs.pi-hole.net/main/prerequisites/) on how to set a static address for different network managers. We had a draft PR pi-hole/docs#547 but it was abandoned. Maybe you could build on that and write a guide for NM?

@yubiuser yubiuser requested a review from a team January 6, 2023 13:19
@PromoFaux
Copy link
Member

My personal take on this is that we drop the static IP hand-holding once Raspberry Pi OS goes to NM, though it's not a hill I will die on.

Since we dropped the static address setting on all but systems with dhcpd5 pre-installed - is there a rough idea of how many people have had to ask us how they set a static IP address on their machine? A quick search and skim on Discourse does not turn up much in the way of requests.

NetworkManager support is already included in Raspberry Pi OS, though it has to be enabled manually and any configuration re-done. It's probably worth nothing that so far there is no other mention of NetworkManager on their blog, so I don't think we should be making any steps towards this just yet, unless someone can point to some documentation/blog/announcment beyonf the one already linked (which states "but in some future release, NetworkManager will become the default.")

Reads to me rather like it's coming soon™


I'm also of the opinion that static IP addressing should be handled at the router via dhcp reservation, but other peoples opinions on that are certainly valid, too 🙃

@dschaper
Copy link
Member

dschaper commented Jan 6, 2023

I'm happy to drop configuring server networking completely.

When we started 6 years ago it was easier to be a one-shot install that tried to do as much work for people as possible. We didn't have the resources to walk people through everything and explain the basics of IP addressing along the way. Things certainly have changed since then, there are tons of guides and resources and help with getting people that are new to Linux or new to networking set up. A great driver of that has been the Raspberry Pi and lowering the barrier to entry for people to have access.

I think we are at the point that we can let this go. Originally the idea was to drop static networking completely but I argued for keeping dhcpcd5 support there to help people with Raspberry Pi directly. It looks like they are going away from that IP manager and I assume that they are prepared with support and documentation for a major change like dhcpcd5 to NetworkManager. Their OS is rather opinionated and I don't think we should be in the middle of that, let the OS handle the IP stack using whatever tools they have or will have.

When you get down to it, Pi-hole is a DNS server and needs to have a stable IP address for clients to use. How you get that stability is up to the user now, we may get an increase of support requests for people that lose Pi-hole from not having a stable IP address but the debugger is pretty accurate with dhcp leases and payloads so we can often tell when the issue is a bad or changed IP address.

@yubiuser
Copy link
Member

yubiuser commented Jan 7, 2023

We decided internally to not further offering the ability to set a static IP address during installation once Raspbian made the change to network manager. I therefore created #5111

@libklein
Copy link
Author

libklein commented Jan 7, 2023

This is my first PR to this project so weight my opinion accordingly.

I do not see enough value in further offering (automatic) static IP configuration by the installer. In fact, I assume that it creates more harm than good:
Users who have a valid use case for a configuration like this will know how to configure it manually and probably prefer that. For new users, it's just another option that adds to the confusion.

Moreover, it's something that potentially limits the number of supported distros and causes a maintenance overhead.

Im happy to close this PR in favour of #5111. I'd suggest closing #4783 #4902 as well.

@dschaper
Copy link
Member

How does #4783 relate to the IP address of the Pi-hole node?

@libklein
Copy link
Author

How does #4783 relate to the IP address of the Pi-hole node?

Sorry, i guess that was a typo on my side. I was referring to the original issue (#4902). To the best of my knowledge, but correct me if I'm wrong here, there is no further action on the side of Pi-Hole required to support NM.

@libklein libklein closed this Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants