Allow HWADDR to be disabled

[anders-larsson]

Inspired by #42. However since nothing happened with that pull request I've created my own take on it. Tried to cherry-pick @divansantana commit however it failed because of some structural changes that would require me to patch it to even work.

Parameter manage_hwaddr is set true as a default which keeps the current defaults. Setting it to false removes the HWADDR line in ifcfg-X.

[esalberg]

I copied this pull request into my own because I'm working with hiera, and not setting macaddress there leaves a blank HWADDR= line even with the "if @macaddress" in the template (undef doesn't translate well).

+1 to merge. :)

[anders-larsson]

@esalberg I'm currently running on a fork of this module as well with hiera support and support to disable HWADDR. In an attempt to try to use the origin module I made this pull request to add support for HWADDR. If you have a clean commit which add hiera support it would be awesome if you could open a pull request to add it to this module :)

I'm unable to test it with hiera right now though. Perhaps it would be better to use the same logic as divansantana and use "[email protected]?"?

[esalberg]

@anders-larsson
I actually updated my fork after I made the above comment but hadn't come back here to update. For my usage, it was sufficient to update the ifcfg-eth.erb template with the following:

<% if @macaddress and @macaddress != '' %>HWADDR=<%= @macaddress %>
<% end -%>

(I did the same to ipaddress, netmask, and gateway)

This lets the hwaddr be disabled (e.g. not show up as an unnecessary HWADDR= line) without having to add a new parameter throughout the code. It also leaves the default for macaddress as is (e.g. for bonds where hwaddr is optional, it's set to '' ).

Looking at if::static, I think that the config will still fail if the macaddress is not available either as hardcoded data or from facter. If the default of undef from that manifest (no macaddress set) could be valid, then perhaps it would just be enough to update the validation statement, either removing it or allowing undef (e.g. add "and $macaddy != undef")?

if ! is_mac_address($macaddy) {
fail("${macaddy} is not a MAC address.")
}

For hiera, I'm currently managing that in my own profile::network file separate from the module, but I do think it should be moved into the module, e.g. following the method used in Saz's puppet-ssh. I just haven't had a chance to move it in yet. It's not difficult really, just a bit time-consuming due to the number of defines involved, spec updates, testing, etc. I can hopefully get to it soon.

[Phil-Friderici]

@esalberg: I am also interessted in adding hiera functionality to this module. Maybe I can help finalise the feature ? I would be very glad if you could add your hiera-aware branch to your Github repository to have a look at your solution.

Thanks & have phun
Phil

[esalberg]

I'm at the PuppetConf contributor summit today - I may see if I can get to working on this as part of that.

[razorsedge]

I am at the Contributor Summit as well. This module is on the top of my list to get some attention.

[razorsedge]

The Hiera requests are being tracked in issue #45.

[razorsedge]

OK. I am trying to understand what this PR (and #42) is trying to solve. Is it only Define['network::if::static'] that needs to have HWADDR= removed, or do other defines also need it? If more than one define, then I would go with @esalberg 's solution of just modifying the template's logic.

[anders-larsson]

I don't think @esalberg's commit helps in this case. The reason an extra parameter is added is because even if you pass an empty string for macaddress to network::if::static it will default to the current interface's macaddress because of this logic on line 78-84 network::if::static. Please correct me if I'm wrong though.

This is the code I used to test it:

  network::if::static { 'ens192':
    ensure     => 'up',
    ipaddress  => '123.123.123.123',
    netmask    => '255.255.255.0',
    macaddress => '',
  }

[esalberg]

So a potential answer for that could be to update the validator, e.g.:

if ! is_mac_address($macaddress) and $macaddress != '' {
# Strip off any tailing VLAN (ie eth5.90 -> eth5).
$title_clean = regsubst($title,'^(\w+).\d+$','\1')
$macaddy = getvar("::macaddress_${title_clean}")
} else {
$macaddy = $macaddress
}

If that's considered too dangerous (per @razorsedge ?), then adding manage_hwaddr makes sense, but I would suggest using it to bypass the validation / set macaddress to '' - which means that the template shouldn't need the change in the pull request (shouldn't need to reference @manage_hwaddr - it won't have the entry as long as macaddress remains '' ).

[razorsedge]

@esalberg I think the whole point of that code was to provide a sane default for anyone not supplying $macaddress.

I am fine with adding manage_hwaddr. Unfortunately, this PR does not merge cleanly, so I may not get to it for a little while (unless someone beats me to it). Also ::network::if::dynamic will be in need of the same treatment as ::network::if::static.

[anders-larsson]

I've updated the pull request. It now possible to use manage_hwaddr with network::if::dynamic as well.

[Phil-Friderici]

THANK YOU, Thank You, thank you @esalberg, @razorsedge and @anders-larsson for taking care :)

[anders-larsson]

Any updates @razorsedge ?

[esalberg]

I can't believe I completely forgot about all of this - my apologies! @razorsedge - do @anders-larsson and I need to add our names and contributors anywhere, or make any other changes?

[razorsedge]

@esalberg Honestly, I don't know what I was waiting on for this PR (besides free time). If @anders-larsson wants to put his name in the various places, that is fine with me. I will try to get to merging this at some point soon.

[anders-larsson]

@razorsedge It's OK for me as it is. You can merge this if you feel it's satisfactory.

[anders-larsson]

@razorsedge Any updates? Sorry for bump.

[Phil-Friderici]

@razorsedge Thanks for asking, but we need the code not the fame ;)

[razorsedge]

I have not forgotten this. It's been a complete lack of time and higher priorities (family issues) on my part.

[Phil-Friderici]

Got it, sorry for bumping.

¡ Good luck to you and your family !

[Phil-Friderici]

@esalberg & @razorsedge THANKS A LOT !!!