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 refactor #459

Merged
merged 12 commits into from
May 28, 2021
Merged

DHCP refactor #459

merged 12 commits into from
May 28, 2021

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Apr 1, 2021

Tested against these DHCP servers:

  • mikrotik hAP AC2, RouterOS v6.45.9 (long-term)
  • dnsmasq 2.84rc2 (linux router)
  • Askey rtf8115vw (Movistar fiber, Spain)
  • TP-Link TD-W8970
  • Linksys E2000 with Tomato 1.28
  • Pixel 4a Android 11 USB-Ethernet tethering
  • Ubiquiti EdgeRouter X, many thanks to @adamgreig
  • TP-Link Archer C7 v2 on OpenWrt 19.07.3, many thanks to @GrantM11235

Testing on more routers would be highly appreciated, especially crappy ISP routers. It requires a linux machine with wired Ethernet connection, and only 5 minutes of your time! Instructions here

@Dirbaio Dirbaio force-pushed the dhcp-socket branch 3 times, most recently from b33541f to b2e5d2c Compare April 6, 2021 23:32
This makes UdpRepr work like IpRepr, where it only emits the header, and the user
must emit the payload.

This makes it easier to emit UDP packets with payloads that come from protocol-specific
reprs, like DHCP and in the future DNS.
@Dirbaio Dirbaio force-pushed the dhcp-socket branch 5 times, most recently from 21ca7eb to 5dff97a Compare April 7, 2021 13:38
@Dirbaio Dirbaio changed the title wip: convert DHCP client to a socket. DHCP refactor Apr 7, 2021
@Dirbaio Dirbaio marked this pull request as ready for review April 7, 2021 17:20
@Dirbaio Dirbaio marked this pull request as draft April 8, 2021 00:09
@Dirbaio
Copy link
Member Author

Dirbaio commented Apr 8, 2021

Marking as draft until I remove the "fast renew" hack commit

tp-link routers pad the DNS server list with 0.0.0.0 to a fixed size :(
@Dirbaio Dirbaio marked this pull request as ready for review April 13, 2021 19:14
Copy link
Contributor

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

This is all looking much nicer - thanks for putting in the work to get this refactored.

I'm going to test integrating this into our hardware platform and verify that things function as intended shortly, and will let you know how things go. Other feedback is attached in-line below - most is general questions on design decisions. I don't believe I found anything problematic during review.

src/wire/udp.rs Outdated Show resolved Hide resolved
src/wire/udp.rs Show resolved Hide resolved
src/socket/dhcpv4.rs Outdated Show resolved Hide resolved
src/socket/dhcpv4.rs Outdated Show resolved Hide resolved
src/socket/dhcpv4.rs Show resolved Hide resolved
src/socket/dhcpv4.rs Outdated Show resolved Hide resolved
src/socket/dhcpv4.rs Outdated Show resolved Hide resolved
src/socket/dhcpv4.rs Show resolved Hide resolved
src/socket/dhcpv4.rs Show resolved Hide resolved
src/socket/dhcpv4.rs Outdated Show resolved Hide resolved
@ryan-summers
Copy link
Contributor

ryan-summers commented May 10, 2021

Follow-up, I have now tested this on Stabilizer and monitored network traffic with Wireshark. I can confirm that the DHCP client renewed after 4 minutes (on an 8 minute lease) and the ICMP message is absent as well.

I also want to note that the buffer removal is also quite significant and this refactor makes DHCP much easier to use. Thanks for the hard work here!

@ryan-summers
Copy link
Contributor

@Dirbaio Ping on this - is there any chance we can work towards getting this merged? This is a very nice improvement to DHCP.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 28, 2021

@ryan-summers Thank you for the review! and sorry for the delay, I've had a really bad month.

Addressed all the comments, could you take another look?

@ryan-summers
Copy link
Contributor

No worries! I appreciate the great work on the refactor. I believe there's only one discussion still in place (re: DHCP source/dest ports), but other than that, the changes look good

@Dirbaio
Copy link
Member Author

Dirbaio commented May 28, 2021

Changed the port check to a hard assert, I believe this should be now good to go.

... except the CI now fails due to a defmt semver-breaking change :( knurling-rs/defmt#494

@Dirbaio Dirbaio merged commit 7e937c4 into master May 28, 2021
@Dirbaio Dirbaio deleted the dhcp-socket branch May 28, 2021 16:54
@astro astro mentioned this pull request Nov 17, 2021
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.

DHCP retransmits request too fast, breaks if server is slow. Convert DHCP client to a socket
2 participants