projectcalico
networking_calico: Do not checksum DHCP packets
Description
Very old versions of isc-dhcp-client broke when UDP packets had a checksum field not properly filled in as part of GSO/checksum offload support being introduced for virtio_net: https://lwn.net/Articles/373209/
The missing/incorrect checksum in the UDP header was problematic for isc-dhcp-client (and possibly other DHCP clients) because they use raw sockets, and were unaware of how to handle TP_STATUS_CSUMNOTREADY.
OpenStack has historically worked around this by adding an iptables mangle rule to always rewrite the UDP header for DHCP packets. But, isc-dhcp-client has also been patched to account for this since 2014, and likewise has qemu (for virtio_net) since 2009: https://git.qemu.org/?p=qemu.git;a=commit;h=1d41b0c
The consequence of patching the mangle rule in iptables is that the underlying OpenStack mechanisms tend to add the rules in an order that upsets Felix. When Felix sees this (often during live-migration, when the VM has no networking), it can spend a fair amount of time resyncing the rules... thus leaving a live-migrating instance with no networking for an appreciable amount of time.
As this should not be needed for any Linux distribution in the last decade and it can perturb Felix, simply do not fill in DHCP UDP checksums here with the assumption that it has been fixed/is being done elsewhere.
Signed-off-by: Tyler Stachecki [email protected]
Related issues/PRs
- https://lwn.net/Articles/373209/
- https://git.qemu.org/?p=qemu.git;a=commit;h=1d41b0c
- https://github.com/isc-projects/dhcp/commit/7ff6ae5aa85754119319def3c7f225a40da299c4
Todos
- [ ] Tests
- [ ] Documentation
- [ ] Release note
Release Note
TBD
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one docs-* label.
docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.
Every PR needs one release-note-* label.
release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.
Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.
I should mention, the VirtIO "offload" in this case is to simply not compute the checksum if the packet isn't leaving the physical host.
Thumbing over the qemu commit, it does explicitly check for both a) IPv4 and b) MTU <= 1500... so this may be impactful to IPv6 DHCP clients, or if jumbo frames are used to renew a lease. Also, this probably needs to be present in virtio_net drivers in the kernel, and it's not (was rejected on the basis that the kernel shouldn't monkey patch like that).
Really, we can't rely on the qemu code, then, and must require clients to do the right thing. Fortunately, all remotely modern clients do seem to have been patched to account for this, already... (I've tried: Cirros, Win Server 2019, RHEL6, Ubuntu 14, ...)
Still, if simply removing this code is deemed insufficient, I can look into seeing what can be done to patch the OpenStack code that's doing the iptables management closer.
Great work @tj90241 , for us it would be perfectly fine to merge this as-is.
@tj90241 @beddari Do you happen to know which versions of CirrOS are fixed so as not to need this DHCP checksumming rule? Our internal system tests are using CirrOS 0.3.2, and that version does not boot successfully with the change in this PR.
Actually I probably don't need a detailed answer here. I've done an initial test with CirrOS 0.6.0 and that seems to be working.