Bug #8727
closedDiscovered hosts with same MAC address are reported as DHCP conflicts
Description
In our DHCP validation code, we check for conflicts for each leases represented by both MAC and IP. We should not validate records based on MAC because this particular case is not valid reason we (me/Greg) think to present an interactive confirmation form to the user.
Updated by Greg Sutcliffe almost 10 years ago
To expand a bit on Lukas' brief description :P
Current Situation¶
Currently we take the record we're proposing to create (a mac/ip pair) can then request all the records the proxy can find either by mac or ip. This returns an array, and we remove entries matching the proposed record. If there's anything left, we report it as a conflict. This is executed with this code:
conflicts = [proxy.record(network, mac), proxy.record(network, ip)].delete_if { |c| c self }.compact
This causes an issue in the case of multiple-ip leases. If a host boots with a PXE UUID, and then gets a new lease from dhcpcd within the OS, we now have:
[mac1, ip1] - PXE lease
[mac1, ip2] - dhcpcd lease
Given the above code, if we provision this system and change nothing, we will still have an entry for [mac1, ip1] in the conflicts array. This is potentially reported to the user via an overwrite confirmation, but that wouldn't be available via the API (or any plugin which is hiding the host edit form). The same happens if you try to change the IP of a pre-existing host - you have to confirm the Overwrite when you save the host.
Analysis¶
We're struggling to come up with a scenario where we would care about leases with the same mac. Macs are a hardware identifier, and as such, if I see a lease for my mac with a different IP, I would interpret that as "My host is releasing the old IP and requesting a lease for the new IP". Since ISC's lease file is a last-lease-wins system, we don't even need to delete the old leases for this mac (although we should, ideally).
It's still true that we care about an IP conflict. In the opposite case to above, where we vary the mac and keep the same IP, we now have a record like [mac2, ip1]. I would interpret that as "My host (mac2) is requesting an IP that belongs to another host (mac1)". In this scenario, we do indeed need to flag this as a conflict, and raise it to the user for investigation.
Proposal¶
What we're proposing is to change the logic to (note that this can be written much more neatly, the below code is for illustration of the logic):
- conflicts = [proxy.record(network, mac), proxy.record(network, ip)].delete_if { |c| c self }.compact
+ conflicts = [proxy.record(network, ip)].delete_if { |c| c == self }.compact
In other words, do not request leases-by-mac from the proxy during conflict detection, since we do not actually care about them. We might request leases-by-mac during the orchestration, since we would want to find and remove any leases not matching the current mac/ip pair.
Have we missed a usecase for mac conflicts, or does this sound sensible?
Updated by The Foreman Bot almost 10 years ago
- Status changed from New to Ready For Testing
- Pull request https://github.com/theforeman/foreman/pull/2022 added
- Pull request deleted (
)
Updated by Lukas Zapletal over 9 years ago
- Status changed from Ready For Testing to Closed
- % Done changed from 0 to 100
Applied in changeset 37bb1b44a9753382c6858e22ac55031e9662c4e4.
Updated by Dominic Cleal over 9 years ago
- Translation missing: en.field_release set to 28
Updated by Lukas Zapletal over 9 years ago
- Related to Bug #9776: foreman does not show a warning if a dhcp reservation already exists and simply fails added
Updated by The Foreman Bot almost 9 years ago
- Pull request https://github.com/theforeman/foreman/pull/2941 added
Updated by Lukas Zapletal almost 9 years ago
- Related to Bug #12637: Improve DHCP lease unit test added
Updated by Ohad Levy over 8 years ago
- Related to Bug #13725: Orchestration fails with 409 when there is a MAC DHCP conflict added
Updated by Lukas Zapletal over 8 years ago
- Related to Bug #8538: ISC DHCP provider does not respect expired and free leases added