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

Use the same options for acquiring, renewing lease #755

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

akhilles
Copy link
Contributor

Currently, hostname is set in the original DHCPREQUEST but not the
renewal. With some DHCP server implementations (such as FreeBSD dhcpd),
this leads to the hostname being cleared in the lease table.

This behavior is inconsistent with other DHCP clients such as dhclient
which set the hostname on the renewal request as well. To fix, use the
same options for acquire and renew.

This is compatible with RFC 2131 (see table 5).

@mccv1r0
Copy link
Member

mccv1r0 commented Jun 23, 2022

Can you squash?

Currently, hostname is set in the original DHCPREQUEST but not the
renewal. With some DHCP server implementations (such as FreeBSD dhcpd),
this leads to the hostname being cleared in the lease table.

This behavior is inconsistent with other DHCP clients such as dhclient
which set the hostname on the renewal request as well. To fix, use the
same options for acquire and renew.

This is compatible with RFC 2131 (see table 5).

Signed-off-by: Akhil Velagapudi <[email protected]>
@akhilles
Copy link
Contributor Author

Squashed

@dcbw
Copy link
Member

dcbw commented Jul 13, 2022

@akhilles Should we also do the same for Release()? If the DHCP server doesn't respect ClientID for renew, I'll bet it doesn't respect it for Release either?

@akhilles
Copy link
Contributor Author

The DHCP server is respecting client ID on renew, but it doesn't know what to do with the other options (and it's not well-defined in the RFC from what I can tell). So, it basically does an upsert and replaces all options with what's in the renewal request. I think it's because both request and renew use DHCPREQUEST, so the server implementations kinda do the same thing for both. This also accommodates scenarios where the DHCP options do legitimately change on renewal. It's totally possible for hostname to change from request to renewal, for example.

It seems like you're only allowed to set client ID for DHCPRELEASE request according to the RFC. "All others" is defined as MUST NOT which includes hostname.

Option                     DHCPDISCOVER  DHCPREQUEST      DHCPDECLINE,
                           DHCPINFORM                     DHCPRELEASE
------                     ------------  -----------      -----------
Requested IP address       MAY           MUST (in         MUST
                           (DISCOVER)    SELECTING or     (DHCPDECLINE),
                           MUST NOT      INIT-REBOOT)     MUST NOT
                           (INFORM)      MUST NOT (in     (DHCPRELEASE)
                                         BOUND or
                                         RENEWING)
IP address lease time      MAY           MAY              MUST NOT
                           (DISCOVER)
                           MUST NOT
                           (INFORM)
Use 'file'/'sname' fields  MAY           MAY              MAY
DHCP message type          DHCPDISCOVER/ DHCPREQUEST      DHCPDECLINE/
                           DHCPINFORM                     DHCPRELEASE
Client identifier          MAY           MAY              MAY
Vendor class identifier    MAY           MAY              MUST NOT
Server identifier          MUST NOT      MUST (after      MUST
                                         SELECTING)
                                         MUST NOT (after
                                         INIT-REBOOT,
                                         BOUND, RENEWING
                                         or REBINDING)
Parameter request list     MAY           MAY              MUST NOT
Maximum message size       MAY           MAY              MUST NOT
Message                    SHOULD NOT    SHOULD NOT       SHOULD
Site-specific              MAY           MAY              MUST NOT
All others                 MAY           MAY              MUST NOT

@dcbw
Copy link
Member

dcbw commented Jul 27, 2022

Thanks for the explanation!

@dcbw dcbw merged commit e2a7138 into containernetworking:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants