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

Adding DHCP lease management #437

Merged
merged 8 commits into from
Mar 24, 2021

Conversation

ryan-summers
Copy link
Contributor

@ryan-summers ryan-summers commented Mar 12, 2021

This PR fixes #436 by adding an API to check if the current DHCP lease has expired, which allows external users to deconfigure the IP address from the interface.

This fixes #307 by updating DHCP to utilize the lease duration provided by the DHCP server.

TODO:

  • Add tests for parsing DHCP lease duration
  • Test updates on hardware

@ryan-summers ryan-summers marked this pull request as ready for review March 12, 2021 16:36
@ryan-summers
Copy link
Contributor Author

CC @whitequark / @Dirbaio - This should now be ready for review when either of you get a chance. I've verified that it's functioning as intended on hardware by sniffing the DHCPACK and verifying that with a 10 minute lease time, the client properly re-requests after 5 minutes

@Dirbaio
Copy link
Member

Dirbaio commented Mar 12, 2021

Very nice! Is my understanding on what changed correct?

Previous behavior:

  • Start renew 60 seconds after ACK
  • retry every 60 seconds, give up after 3 retries

New behavior:

  • Start renew lease_time/2 after ACK
  • retry every 60 seconds, give up when lease expires

If that's the case, 👍 from me.

There's one concern: routers typically set very high lease times, so a smoltcp device could now become offline for hours if a network changes its IP range or some other configuration. I'd like to see some option to set a "ceiling" on the renew/lease time, so users can choose to mitigate this. We can add this later, I'm fine with merging this PR without it.

@ryan-summers
Copy link
Contributor Author

ryan-summers commented Mar 12, 2021

Very nice! Is my understanding on what changed correct?

Previous behavior:

  • Start renew 60 seconds after ACK
  • retry every 60 seconds, give up after 3 retries

New behavior:

  • Start renew lease_time/2 after ACK
  • retry every 60 seconds, give up when lease expires

If that's the case, 👍 from me.

This summary is correct. lease_time / 2 is recommended by RFC 2131

There's one concern: routers typically set very high lease times, so a smoltcp device could now become offline for hours if a network changes its IP range or some other configuration. I'd like to see some option to set a "ceiling" on the renew/lease time, so users can choose to mitigate this. We can add this later, I'm fine with merging this PR without it.

As far as my reading of the RFC, even if the router changes the port range, the assigned IP address to the device is still respected for the entire lease duration, so the device isn't "off-line". The DHCP server will be checking to see if addresses are already assigned before handing them out even between configuration changes. As far as I understand it, the RFC essentially states that the provided IP + lease is guaranteed to be valid for the whole duration, regardless of server reboots/configurations (it explicitly states the server should store IP assignments in persistent storage)

Additionally, reconfiguration of DHCP servers is (as I understand it) very much not the norm, so I don't think we should be designing intentionally around that. Reconfiguration is common in development environments, but pretty rare in production imo.

@ryan-summers
Copy link
Contributor Author

@Dirbaio / @whitequark Is there any feedback that needs to be incorporated before this can be merged?

@Dirbaio Dirbaio enabled auto-merge March 24, 2021 02:26
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

Looks great!

Regarding the DHCP server reconfiguration: In my company we've had problems where a customer changed their address ranges and our devices stopped working and had to be powercycled because the leases were hours long.

A "max lease time" where it caps the lease time if the dhcp server's is longer would be handy. We can always add this later though.

@Dirbaio Dirbaio merged commit 1bff999 into smoltcp-rs:master Mar 24, 2021
@jordens
Copy link

jordens commented Mar 24, 2021

Not honoring the lease time and instead renewing significantly more frequently is somewhat problematic. It can reasonably be seen as non-compliant and causing unnecessary or unwanted traffic. To migrate DHCP configurations, you'd do something similar to DNS migration, decrease the lease time/TTL on the server, then migrate, then increase again.

@Dirbaio
Copy link
Member

Dirbaio commented Mar 24, 2021

I know, but our customers have no idea about that, are using crappy consumer-grade routers where these options may not be even configurable, and expect our devices to Just Work. :)

@ryan-summers ryan-summers deleted the feature/dhcp-lease-updates branch March 24, 2021 12:09
@ryan-summers
Copy link
Contributor Author

Yeah, but I would argue that should be baked into the application logic (e.g. you have a ping service going every minute) and not in the underlying DHCP implementation

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

Successfully merging this pull request may close these issues.

Detecting DHCP lease renewal timeout DHCP lease renewal time not respected
3 participants