dhcpv4: support RENEW requests and option deletion

[twelho]

When using nclient4.RequestFromOffer with an existing offer to issue lease renewals, OptionRequestedIPAddress and OptionServerIdentifier must be cleared to comply with RFC 2131, section 4.3.2 (RENEW). Updating these with modifiers using dhcpv4.WithOption does work, but setting the IP addresses to nil causes Options keep them which results in an invalid DHCP packet that has entries with without values. This fix implements a new WithoutOption modifier that can be used to clear those options.

Additionally, it also enables native issuing of renew requests with a new nclient4.Client.Renew method using the aforementioned functionality.

[twelho]

@insomniacslk @pmazzini could I get a review? This is needed for https://github.com/siderolabs/talos/pull/5897 which is lined up for the 1.2 release of Talos.

[pmazzini]

I really don't like making the Update method removing an option, from its name it is not clear it will do this. I also don't like setting the value of options to nil, it looks hacky.

How about having a modifier removing those options or a NewRenewFromOffer?

[twelho]

I can add a Del function to Options (similar to dchpv6) and corresponding a modifier for it. It might be a good idea to also provide NewRenewFromOffer as a shorthand. I will update this PR shortly.

However, it is still possible to cause the library to send invalid DHCP packets with nil values in Options. In my opinion this should be prevented somehow. Thoughts?

[hugelgupf]

It need not be prevented. Options that have no value are perfectly valid, and in Go it is often non-idiomatic and non-obvious to treat nil and []byte{} as different (both have length 0).

DHCPv4 spec afaik only has the Pad and End options without value, but it doesn't keep a vendor from specifying and implementing an option without a value in vendor-specific options, for example. (DHCPv6 has Rapid Commit, for example.)

On Thu, Jul 14, 2022, 00:08 Dennis Marttinen @.***> wrote:

I can add a Del function to Options (similar to dchpv6) and corresponding a modifier for it. It might be a good idea to also provide NewRenewFromOffer as a shorthand. I will update this PR shortly.

However, it is still possible to cause the library to send invalid DHCP packets with nil values in Options. In my opinion this should be prevented somehow. Thoughts?

— Reply to this email directly, view it on GitHub https://github.com/insomniacslk/dhcp/pull/469#issuecomment-1184077206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPG3ETHESKAMPXTZQSRKPLVT64HJANCNFSM53AYD5BA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[twelho]

Hmm, then really what seems to be missing is some kind of input validation for each of the known options as specified by RFC 2132 in the various Opt* functions. That will have to be a separate PR if such functionality is desired.

[twelho]

Alright, inspired by @pmazzini's suggestions I present a new approach.

• https://github.com/insomniacslk/dhcp/pull/469/commits/7cd86e1b80be3a3ca310b447519e887b94a2a240 adds the required functionality and a dedicated modifier for option deletion.
• https://github.com/insomniacslk/dhcp/pull/469/commits/575f18f584ca106c1af28f533d245629092f6403 essentially implements NewRenewFromOffer, but in a more generic way that covers all four variants of a DHCPREQUEST as specified by RFC 2131.

Could I get a re-review? I will implement unit tests after the design has been accepted, but before this PR is merged. Also, if you prefer to have the two functionalities in separate PRs, just let me know.

[twelho]

How about making the renew a method of the lease?

https://github.com/insomniacslk/dhcp/blob/master/dhcpv4/nclient4/lease.go#L25

This make sense. I don't have time to proto this today anymore, but will look into it implementing it that way tomorrow. How about the approach to clearing options, specifically https://github.com/insomniacslk/dhcp/commit/7cd86e1b80be3a3ca310b447519e887b94a2a240? It is now a bit tangentially related, but could still be useful (since dhcpv6 has it).

[pmazzini]

How about the approach to clearing options, specifically 7cd86e1? It is now a bit tangentially related, but could still be useful (since dhcpv6 has it).

I am ok with it, no objections from my side there.

[twelho]

8dc422bf899950c56f65968ea86966a0e964c51d now contains the approach where Renew is a method of Client next to Release. This is the smallest changeset I could come up with, the architecture is not very flexible to enable implementing this without almost duplicating RequestFromOffer due to the tight coupling between request and offer in that method.

[codecov[bot]]

Codecov Report

Merging #469 (0bb6da2) into master (69cde97) will decrease coverage by 0.02%. The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
- Coverage   67.41%   67.39%   -0.03%     
==========================================
  Files          90       90              
  Lines        3769     3797      +28     
==========================================
+ Hits         2541     2559      +18     
- Misses       1055     1061       +6     
- Partials      173      177       +4     

Flag	Coverage Δ
integtests	∅ <ø> (∅)
unittests	67.39% <64.28%> (-0.03%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dhcpv4/nclient4/client.go	58.41% <ø> (ø)
dhcpv4/nclient4/lease.go	46.15% <41.17%> (-9.41%)	:arrow_down:
dhcpv4/dhcpv4.go	74.63% <100.00%> (+0.59%)	:arrow_up:
dhcpv4/modifiers.go	85.71% <100.00%> (+0.52%)	:arrow_up:
dhcpv4/options.go	86.06% <100.00%> (+0.11%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pmazzini]

I took a quick look and in general it looks good to me. I'll take a deeper look with more time later on and wait for other comments in the meantime.

[twelho]

I've managed to integrate Renew with the existing RequestFromOffer method. The functionality should be the same, but this approach avoids the code duplication and maintenance overhead. Will now write some unit tests for both this and the option removal.

[twelho]

Alright, unit tests added. This should now be ready for final review.

[twelho]

Review feedback assessed.

[pmazzini]

I'll leave time for other people to leave comments. If there are no comments, I'll merge it on Monday.

[twelho]

https://github.com/insomniacslk/dhcp/commit/b0e331049c1e6f7c7be239051ac993ff586c536a just moves the Renew function up a bit in anticipation for the next PR to avoid unnecessary merge conflicts. No functional changes were made, this should be ready now.

[twelho]

Seems like there are no more comments, could this be merged @pmazzini?

I've submitted a follow-up changeset at https://github.com/insomniacslk/dhcp/pull/470 which would also be great to have reviewed and merged together with this. It is more straight forward and should hopefully be easier to review compared to this PR.

[twelho]

@pmazzini could this be reviewed and merged? I think I have addressed all of the feedback, and don't plan on doing any more changes. This PR and https://github.com/insomniacslk/dhcp/pull/470 are really blocking https://github.com/siderolabs/talos/pull/5897 now.

[pmazzini]

@twelho Sorry for dropping this for a while. Is this ready to be merged?

[twelho]

Yes, this PR is ready to be merged from my side. I will fix up https://github.com/insomniacslk/dhcp/pull/470 this weekend as well, apologies for the delays there as I've been quite busy the last couple of weeks.

[pmazzini]

Looking back at this, should it have been NewRenewFromAck instead of NewRenewFromOffer?

[twelho]

There's a problem with that, the server may decide to drop the client IP from the ACK response (see ciaddr here), which means that a purely ACK-based solution would not know the IP to unicast the renewal request with.

[pmazzini]

There's a problem with that, the server may decide to drop the client IP from the ACK response (see ciaddr here), which means that a purely ACK-based solution would not know the IP to unicast the renewal request with.

All you need in the Review is the yiaddr: https://github.com/insomniacslk/dhcp/blob/master/dhcpv4/dhcpv4.go#L260 Am I missing something?

[twelho]

Indeed, didn't look at the correct row. The change to NewRenewFromAck is now available here: https://github.com/insomniacslk/dhcp/pull/498