voxpupuli / puppet-network

Types and providers to manage network interfaces
https://forge.puppet.com/puppet/network
Apache License 2.0
66 stars 107 forks source link

Get rid of type *property* defaults and use of Puppet::Property::Boolean #240

Open alexjfisher opened 6 years ago

alexjfisher commented 6 years ago

trevor.viljoen reported this issue on slack

Any idea why the puppet-network module would inadvertently set boot protocol to dhcp when it was set to static and the only property we wanted to change with the network_config type was mtu? The resource declaration looks like this:

network_config { "${network_interfaces}":
mtu => 8900,
}

The problem is that we use defaults for properties. Defaults are ok for parameters, but not so much for properties. Sometimes a default is needed for resource creation, but this code can go in the provider create method instead.

Worse still are the properties that default to true and use Puppet::Property::Boolean. Puppet::Property::Boolean is completely broken. If you try and set any of those properties to false, nothing will happen.

dhollinger commented 6 years ago

@alexjfisher looks like there is an overarching issue with boolean properties in resource types: https://tickets.puppetlabs.com/browse/PUP-2368

Based on @DavidS's comment at the end of the ticket, this issue affects even the new Resource API.

alexjfisher commented 6 years ago

@dhollinger Correct. I've bumped into this in a few other modules. eg. https://github.com/theforeman/puppet-pulp/pull/284

Shame about the resource API. I think after his talk at FOSDEM, David told me booleans properties were fixed in the new API. I guess not though.

DavidS commented 6 years ago

Writing boolean properties, yes.

Can't do much about core puppet being a jerk, though. That's @joshcooper's sisyphus.

joshcooper commented 6 years ago

Is it nil, false, or :false... all I got is tears.

traylenator commented 4 years ago

Is this why I see with:

network_config{ 'ifcfg-enp5s0f0':
  ensure => "present",
  onboot => true,
  family => "inet",
  method => "static",
  ipaddress => "138.137.160.71",
  netmask => "255.255.255.224",
  options => {
              "GATEWAY" => "138.137.160.65",
              "USERCTL" => "false"
              },
}

I get the error:

Error: Value 'true':TrueClass cannot be determined as a boolean value
Info: Unknown failure using insync_values? on type: Network_config[enp5s0f0] / property: hotplug to compare values [false] and absent

when there is no HOTPLUG entry in ifcfg-enp5s0f0

Property is defined as :

newproperty(:hotplug, required_features: :hotpluggable, parent: Puppet::Property::Boolean) do
    desc 'Allow/disallow hotplug support for this interface'
    defaultto :true
  end

Not possible to produce the error with a trivial puppet manifest and puppet apply - only a full puppet run.

strigazi commented 3 years ago

We get the same for onboot:

Error: Value 'true':TrueClass cannot be determined as a boolean value
Info: Unknown failure using insync_values? on type: Network_config[eth2] / property: onboot to compare values [:absent] and absent
Error: /Stage[main]/Hg_cloud_compute::Nova::Compute/Network_config[eth2]/onboot: change from 'absent' to true failed: Value 'true':TrueClass cannot be determined as a boolean value