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

Add button to remove dynamic DHCP leases #1634

Merged
merged 1 commit into from
Nov 28, 2020
Merged

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Nov 23, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Add button to remove dynamic DHCP leases. They are immediately removed both from FTL and the dhcp.leases file.
FTL takes care of updating the file!) WITHOUT the need for a restart of the DHCP/DNS resolver.

See the new red trash icon on the dynamic leases table:
Screenshot at 2020-11-23 13-40-23

Clicking on the one non-blurred entry deletes the lease currently active for dominik-desktop. The progress is reported by a status message on the top right corner:

Screenshot at 2020-11-23 13-37-29

How does this PR accomplish the above?:

Adds a button to call the new delete-release API callback in FTL (see pi-hole/FTL#932)

What documentation changes (if any) are needed to support this PR?:

None

…d both from FTL and the dhcp.leases file (FTL takes care of updating the file!) WITHOUT the need for a restart of the DHCP/DNS resolver.

Signed-off-by: DL6ER <[email protected]>
@DL6ER DL6ER added Discourse Feature Request PR: Approval Required Open Pull Request, needs approval labels Nov 23, 2020
@DL6ER DL6ER requested a review from a team November 23, 2020 12:45
@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/remove-dhcp-leases-over-webgui/2277/49

@yubiuser
Copy link
Member

Could you use the new function to fix this issue (#1026) by deleting the current active lease when a new 'static' lease for that MAC is added?

@DL6ER
Copy link
Member Author

DL6ER commented Nov 24, 2020

@yubiuser yes, this should work. FTL forgets that it has ever seen a client when clicking on the trash icon

@PromoFaux
Copy link
Member

I shall try and test this later - I don't currently have a config set up with Pi-hole as the DHCP server (I've recently moved DHCP to my UDM)

@dschaper
Copy link
Member

dschaper commented Nov 24, 2020

I guess the $20USD question is: What happens on/to the client when the lease is deleted?

Edit: It may be out of scope of this feature, but we should warn users in documentation that this will not remove the IP from the client. If you have a long lease period (say 24H) then the client is going to use the IP until it's told it needs a new one. That could be up to 12 hours or so (in the case of a fresh lease and textbook backoff period) before the client checks in and get's NACK'd by the DHCP server. Removing the lease does not remove the IP.

@DL6ER
Copy link
Member Author

DL6ER commented Nov 24, 2020

What happens on/to the client when the lease is deleted?

Nothing (but $20USD is not very much so this may be okay).

Removing the lease does not remove the IP.

You're absolutely right, however, it should be clear that this removes only the lease (server) not the consequent allocation (client) itself. I checked a few resources, however, I didn't find anything the server could do to inform the client it has to do something. It all comes down to the client becoming active. This will only happen once the lease is expected to expire. See also this Discourse post.

At any time during the lease period, the DHCP client can send a DHCPRELEASE packet to the DHCP server to release the IP address configuration data and to cancel any remaining lease.

FTL will then reply with a new lease to this request. If the scenario is that a static lease has been added meanwhile, then then client will get the new static address.

@dschaper
Copy link
Member

it should be clear that this removes only the lease (server) not the consequent allocation (client) itself.

I completely agree, and there's nothing we really can do to inform the client as that particular extension is in RFC form only and nearly no one implements it.

My point was that we'll need to do some education for users as they will not know this fact and will expect that removing a lease will change the client IP.

@PromoFaux
Copy link
Member

I didn't find anything the server could do to inform the client it has to do something

Unifi do it on WiFi Clients (though not wired)... but I guess they do that by forcing a disconnect/reconnect for that client, the joys of it also being the router I guess.

Obviously this is not something we can reproduce on a Pi-hole device

@DL6ER
Copy link
Member Author

DL6ER commented Nov 25, 2020

but I guess they do that by forcing a disconnect/reconnect for that client, the joys of it also being the router I guess.

I completely agree. We could block DNS for said device such that the user will reboot the device by themselves ;-)

@pralor-bot
Copy link

This pull request 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants