-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[WIP] - removal of hard dependency on dhcpcd 5. #3715
Conversation
There is no dependency on dhcpcd for the non-debian side. |
Great step, many thanks for this, most non-Raspbian users will like it. Keep in mind to remove that part in the docs as well: https://github.com/pi-hole/docs/blob/master/docs/main/prerequisites.md#ip-addressing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly: Yes. This is a good move.
Secondly: This change may easily cause the new number one support request when users set up Pi-hole. They install the enter-mashing way and - at the next restart - they may end up with a different IP and "Pi-hole broke the Internet". Or they set up Pi-hole as DHCP server and booting doesn't work because the device itself wants to get an IP address even before it's own DHCP server could hand out one (dnsmasq
only does this when an interface is already up so it knows which IP range this interface is responsible for).
@@ -2160,7 +1992,9 @@ Your Admin Webpage login password is ${pwstring}" | |||
IPv4: ${IPV4_ADDRESS%/*} | |||
IPv6: ${IPV6_ADDRESS:-"Not Configured"} | |||
|
|||
If you set a new IP address, you should restart the Pi. | |||
If you have not done so already, the above IP should be set to static. Depending on your operating system, there are many ways to do this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should print this at least in bold-face. Maybe even in red.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that it doubles already with the whiptail prompt at install start: https://github.com/pi-hole/pi-hole/blob/master/automated%20install/basic-install.sh#L654
But that is not necessarily a bad thing.
EDIT: Ah, saying "whiptail", not possible to apply text colour or style in whiptail, capital letter or such is the best we can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require positive acknowledgement of the conditions. ("Type P
to confirm ")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interactive action, more than just hitting "return" is probably the best if you worry about user locking out themselves from SSH access after reboot.
Instead of having two places where this info is prompted + interactive input, what about changing the msgbox from welcome dialogues into a yesno with "no" as default, so that an interactive switch to "yes" is required. The information/text that is added here could be moved into the whiptail as replacement, and then as question:
Do you acknowledge this information and configured your system accordingly?
If "no" is chosen (respectively return pressed), the installer could exit and print again the information to apply a static IP or IP reservation (if router stays DHCP server) before installing Pi-hole.
For unattended installs the new printf
below could stay and there red colour and bold can be done as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would need to be in a whiptail, as we cannot wait for user input when piping the curled script to bash. (Unless we changed the install command to bash -c "$(curl -sSL http://install.pi-hole.net)"
- but that's a bit more of a mouthful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misread your comment.
I kind of like the idea of the first whiptail requiring a switch to yes
from a defaulted no
Sorry, been a while since I looked at this. What do we want to do RE:Removing/keeping the code that sets static via |
I have been installing PI-Hole on a Ubuntu Focal machine, and stumbled over that problem. the install script installed dhcpcd5, my physical Ethernet interface received an additional IP via DHCP and the 5 VLANs got an additional LLC address (as there was no DHCP server on those VLANs). My understanding is that Raspberry OS would in any case use dhcpcd5 to assign even static IPs ? That seems a bit strange to me. Given that we talk about a DNS server which should be (really!) static and it's IP should be existing before the DNS daemon ist started. I can imagine the installer checking which of the common network manager tools are installed and act accordingly (which could mean, act not at all as the network configuration could be in place already, just as in my case and just query which of the interfaces/bridges to use as DNS listener), OR apply the Raspberry OS default case. In fact I had a VPN end point configured on that machine and Pi-Hole installer wanted to re-assign that VPN IP(!) to the machine, not taking multiple interfaces into consideration at all apparently. I removed and hid dhcpcd5 from apt so that the installation would eventually succeed, but this is more a workaround. I think even though Pi-Hole comes from the Pi itself, it shouldn't limit itself. I'm happy to discuss, even if I'm rather new to Pi-Hole and not "digging deep" in the code. |
Indeed, the official RPi docs intend it that way: https://www.raspberrypi.org/documentation/configuration/tcpip/README.md But that is all not an issue of Pi-hole (following just the RPi-ways) and since Pi-hole officially supports a number of other distributions for a while, it totally makes sense to not force an RPi-way network setup that at least creates an overhead of even breaks other distros network setup 😉. |
78c8d7a
to
b8723db
Compare
I mean for using Pi-hole, one must configure either the router or the clients manually to use Pi-hole, hence its IP address for DNS resolution. I cannot really believe that more than literally a hand full users will enter a dynamically assigned IP address and then start wondering that DNS resolution fails after DHCP lease times out 🤔. And if such happens, then my only suggestion would be for that user(s) to stop administrating network services with such few basic knowledge, as the risk for major security breakage is simply too high... I'm joking 😄. The issue still is that there are many ways (many tools, high-level as well as low-level) to setup the network, so any attempt to check for either DHCP or static IP being used will end up with the same issue as before: It will work for most systems but fail and mess with others, hence limit the flexibility/freedom for admins, like dhcpcd currently does in a way.
See above the suggestion to remove this prompt, respectively merging it with the one that is newly added with this PR and an interactive yes/no selection which must be manually switched from "no" to "yes" to proceed with the install, as a hopefully sufficient reminder about the static/reserved IP topic: #3715 (comment) |
… already installed Signed-off-by: Adam Warner <me@adamwarner.co.uk>
b8723db
to
7cd2bb0
Compare
Latest version of Pi-hole debug process:
|
Are you sending out a DHCP request and catch answers? Indeed a smart way, if so (EDIT: I see: |
I'm not exactly sure what method Dom picked for the check but it should be fairly easy to see if the address returned in the lease offer matches an IP address on the interface.
|
Thank you for coming up with this. This change of removing static addressing will also allow local install on devices that often switch networks and subnets to continue work when IPs change (e.g. installing on a PinePhone). |
Also remove static address setting of any kind, display message to user at the end of the script advising that they should set it as static.
Very much open to change on all of this, especially the wording of the final message. If this goes ahead, I'll tidy up the commits. @bcambl can you take a look over the dependency section for the non-debian part?