Add host-name DHCP option to fixedaddress records

[mporrato]

The behaviour of DHCP reservations created by the Infoblox smart proxy currently does not match the behaviour of DHCP reservations created by the isc-dhcpd smart proxy: specifically, Infoblox does not seem to send the client host name in the offer (option 12, "host-name") unless the reservation contains an explicit DHCP option for that.
In other words, setting the "name" field when creating the fixedaddress is not enough for Infoblox to send the "host-name" DHCP option.
On the other hand, the isc-dhcpd smart proxy creates reservations with supersede host-name, causing the host name to be included in the response.
This patch adds an explicit "host-name" option to all fixedaddress records, making the Infoblox smart proxy behave more like the isc-dhcpd one.

[theforeman-bot]

Can one of the admins verify this patch?

[lzap]

Sorry for the late reply.

Small nitpick, this can go in. However, I have a concern. DHCP module uses record comparison to find conflicts in subnets and it does take hostname into account if entered. This can cause issues on existing installations. There is not much we can do about it tho, all records would need to have host-name entries entered via some migration script or something.

Have you tested this in production?

[mporrato]

Thank you lzap. Sorry about the messy code. First line of ruby code for me.
I have tested it in a staging environment and it behaves as I expected. That being said, It's very unlikely I would hit a conflict in that environment because I have just a couple of hosts on a /24 subnet. If you have any suggestion on how I can try to trigger the issue with the conflict detection, I'd be happy to try it in my environment. Or, if you just can point me to point in the code where it happens, I might be able to figure it out myself.

[lzap]

Sure ok let's get this in. Would you mind pushing and amending the two nitpicks I have? Then this can go in.

[Ron-Lavi]

Hey @mporrato, is there any update with this PR?

[ekohl]

DHCP module uses record comparison to find conflicts in subnets and it does take hostname into account if entered.

All I can find is this:

smart_proxy_dhcp_infoblox/lib/smart_proxy_dhcp_infoblox/common_crud.rb

Lines 60 to 62 in 4402d84

	if options[:mac] != existing_host.mac || options[:hostname] != existing_name
	raise Proxy::DHCP::Collision, "Record #{options[:ip]} conflicts with an existing record."
	end

And that doesn't look like it would affect anything.

So I'd be ok with merging this.

[mporrato]

Sorry for the late reply: I missed the notification.
I won't be able to test the suggested changes because I don't have access anymore to the environment where this was needed.
FWIW, we had to resort to using a hook script to fix the Infoblox records in order to deal with other issues we found in the way foreman sets the bootfile and nextserver options, so we decided to have the script deal with the host-name option as well.

[ekohl]

Thank you for that update. Do you still see value in this PR for other users?
For our CI: ok to test

[ekohl]

I took the liberty of rebasing and fixing the Rubocop violations.

[mporrato]

@ekohl yes, I believe this is definitely needed in order to make the Infoblox smart proxy work more like the standard dhcp smart proxy, and also the issues about nextserver and bootfile should be addressed, but this is off-topic for this PR: I can provide more details about those if needed.

[ekohl]

I'd like to hear about those yes.

[mporrato]

@ekohl this is the hook script I created to address all those issues: https://gist.github.com/mporrato/d1cfed082c5bc0e2a458306607cc2b3f

If I recall correctly, the issues were related to hosts without a bootfile or nextserver field: those fields were created in the Infoblox FixedAddress object as empty strings instead of being marked as not in use (see my script), causing the infoblox server to send the dhcp option with an empty content instead of omitting it entirely, and therefore leading to boot failures.

Hope this helps.

[ekohl]

@timdeluxe any thoughts on this?

[timdeluxe]

@ekohl Good question. As we use "Host" objects and do not have DHCP ranges, we do not work with "fixedaddress".
In the web UI i can't find any way to overwrite any DHCP option on the object itself - maybe it is some higher admin setting or it was changed, i don't know or it is only possible via the API. Same goes with "Host" objects by the way. We have set the options (nextserver f.e.) at the network level and so far did not have any problems with it.
The build_host method for "Host" objects also sets those options (nextserver, bootfile) the same way as the method for fixedaddress does, so if there would be a problem by having Foreman setting empty values there, we should also notice issues which we don't.

We don't use fixedaddress right now, so i can't confirm if the issue the PR opens with is a real one. I also can't reproduce right now or the next days, since we/i do not have a time budget to do so at the moment. Looks like @mporrato worked heavily with it (in the past) and recognized it as a problem, so it should be merged.

I vote for merging this PR as is after the failing tests have been fixed (which come from the "merge!" code, where you @ekohl also mentioned about above). The problem with the other "empty" DHCP options should be handled as soon someone else can confirm it. @mporrato obviously can't anymore, because he does not have access to his environment anymore.

[mporrato]

@timdeluxe the bootfile and nextserver issues are not directly related to this PR, which is about the host-name option alone.

IIRC, bootfile and nextserver are not treated like other dhcp options in infoblox and they are presented in a dedicated tab in the web ui (the bootp tab, if I'm not mistaken) and I'm pretty sure you can override the network level settings on a per-fixedaddress (and probably also per-host) basis.
I believe the ability to reproduce the issue depends on the client: if it honors the bootfile and nextserver options, then it's affected by this issue.

One detail that I forgot to mention but I think might be relevant: we only had this problem with networks were we had to use iPXE to provision the hosts and therefore we had to configure the infoblox network to serve the undionly.kpxe file to regular PXE clients.

[timdeluxe]

@mporrato Well, the documentation for NIOS 8.5 (we are using right now) reflects what you are saying (DHCP options per single host or fixedaddress and inheritance), the only issue is that our Web-UI doesn't. I don't know why. I checked everywhere to see if there is any option preventing that, but couldn't find any (i am super-admin in the tool, so no permission issue). We can only configure DHCP options / BOOTP down to the network level, not deeper - i don't know why. So i also can not test / confirm any fixes of the plugin regarding that.
Also we use normal PXElinux, not iPXE, so i also can't help testing/confirming that.

So if there is anyone willing writing code for this issues AND has an environment to test that, we can work on getting the code into the plugin and released. So far it looks like no one else is having the issue yet.

Maybe i repeat myself, but let's work on getting this PR clean. Do you want to fix your code yourself or shall one of us active here in the PR do it?
Or maybe you decide to ditch the PR at all for the moment, since nobody is able to confirm/test it against a real environment (including you and me)?