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

DHCP lease changes have no visual indication of whether they've been applied/saved or not. #2276

Closed
fake-name opened this issue Jul 30, 2022 · 9 comments

Comments

@fake-name
Copy link

fake-name commented Jul 30, 2022

Versions

durr@lxchole:~$ pihole -v
  Current Pi-hole version is v5.11.4
  Current AdminLTE version is v5.13
  Current FTL version is v5.16.1

Platform

  • OS and version: Ubuntu 20.04 LTS
  • Platform: LXC

Expected behavior

When you add a static DHCP lease, and then reboot the device that the lease specifies the IP for, I'd expect the device to use the newly configured address.

Actual behavior / bug

Adding a new DHCP static lease has lower priority then any active dynamic leases, and the process of adding a static lease does not clear any dynamic lease for the device's MAC. As such, the device will continue to pull the dynamic lease's specified IP until that lease expires. You have to manually delete the lease to make the device pull the correct IP (or, well, wait 24 hours).

Steps to reproduce

Steps to reproduce the behavior:

  1. Have PiHole doing DHCP
  2. Have a device use the PiHole DHCP to pull an IP
  3. Add a static lease for the device which already has a dynamic lease.
  4. Cause device to try to pull a new IP address.
  5. Address from dynamic lease is given to device, rather then the static lease.

Debug Token

  • URL: Not relevant, this is a logic issue.

Screenshots

N/A

Additional context

Slightly off topic, but the sorting of IP addresses on the DHCP page is also incorrect. It currently sorts using pure alphanumeric ordering, which means you get

  • 192.168.1.2
  • 192.168.1.20
  • 192.168.1.200
  • 192.168.1.3

At least the IP addresses should be sorted numerically, rather then alphanumerically.

@fake-name
Copy link
Author

fake-name commented Jul 30, 2022

Ok, the above may not be completely correct. I have no dynamic leases for my device, a manually configured static lease, and it's still being given the old IP.

127 durr@Khadas:/etc/network$ sudo dhclient -v eth0
[sudo] password for durr:
Internet Systems Consortium DHCP Client 4.4.1
Copyright 2004-2018 Internet Systems Consortium.
All rights reserved.
For info, please visit https://www.isc.org/software/dhcp/

Listening on LPF/eth0/c8:63:14:72:89:99
Sending on   LPF/eth0/c8:63:14:72:89:99
Sending on   Socket/fallback
DHCPDISCOVER on eth0 to 255.255.255.255 port 67 interval 3 (xid=0x415c0f2e)
DHCPOFFER of 10.1.2.31 from 10.1.1.207
DHCPREQUEST for 10.1.2.31 on eth0 to 255.255.255.255 port 67 (xid=0x2e0f5c41)
DHCPACK of 10.1.2.31 from 10.1.1.207 (xid=0x415c0f2e)
RTNETLINK answers: File exists
bound to 10.1.2.31 -- renewal in 32741 seconds.

image

@fake-name
Copy link
Author

Ok, I'm an idiot. You have to hit save after adding static leases.

On the other hand, there is zero indication of this, other then scrolling to the bottom of the page, and noticing there's a random "Save" button. This is despite any changes persisting to the server (adding static leases causes the page to do a full reload!).

There really should be some sort of visual indicator that you have un-saved changes.

@fake-name fake-name changed the title Active DHCP leases take precedence over manually added static leases. DHCP lease changes have no visual indication of whether they've been applied/saved or not. Jul 30, 2022
@dschaper dschaper transferred this issue from pi-hole/pi-hole Jul 30, 2022
@yubiuser
Copy link
Member

Slightly off topic, but the sorting of IP addresses on the DHCP page is also incorrect. It currently sorts using pure alphanumeric ordering, which means you get

192.168.1.2
192.168.1.20
192.168.1.200
192.168.1.3

I can't confirm this behavior.

Bildschirmfoto zu 2022-07-31 07-58-54

I don't have current active leases, but it uses the same code as the static leases, so I expect it give the same results.

https://github.com/pi-hole/AdminLTE/blob/1714b082c3a233ea3a0c344b50352c59818676fc/settings.php#L634
https://github.com/pi-hole/AdminLTE/blob/1714b082c3a233ea3a0c344b50352c59818676fc/settings.php#L674

@yubiuser
Copy link
Member

and the process of adding a static lease does not clear any dynamic lease for the device's MAC

No it doesn't do this by it's own. We have a button for this.

PR #1634

@fake-name
Copy link
Author

I don't have current active leases, but it uses the same code as the static leases, so I expect it give the same results.

Aaaand it seems to work fine. Man I must have been having a bad evening or something.

No it doesn't do this by it's own. We have a button for this.

I know about the button, but I'm struggling to come up with a context where you want a existing dynamic lease to persist when adding a manual lease. Maybe I'm bringing expectations, but OPNSense/PFSense the static leases take priority.

@yubiuser
Copy link
Member

One could think about automatically delete current lease if a users adds a static lease for the same device.

@fake-name
Copy link
Author

One could think about automatically delete current lease if a users adds a static lease for the same device.

Yep, basically that seems to be how at least OPNSense works.

@pralor-bot
Copy link

This issue has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/automatically-delete-current-dchp-lease-when-static-assignment-is-added-for-the-same-device/56837/1

@yubiuser
Copy link
Member

As this turned out to be a feature request I added one to our discourse forum (where all feature requests should go) and close this issue here.

https://discourse.pi-hole.net/t/automatically-delete-current-dchp-lease-when-static-assignment-is-added-for-the-same-device/56837

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

No branches or pull requests

3 participants