Boolean properties are overriden by default values when set to false

[bodgix]

This snippet from providers/instance.rb demonstrate the issue:

@create_account = new_resource.create_account || Logstash.get_attribute_or_default(node, @name, 'create_account')

when new_resource.create_account is set to false, Logstash.get_attribute_or_default(node, @name, 'create_account') is getting invoked because this is how the || operator works.

I think that an explicit check if the property is nil should be used instead of the simplified check if it's truthy.

[martinb3]

When new_resource.create_account is set to false, these should return the same thing, right?

new_resource.create_account || Logstash.get_attribute_or_default(node, @name, 'create_account')
new_resource.create_account.nil? || Logstash.get_attribute_or_default(node, @name, 'create_account')

Is the issue that someone has set create_account on the node object? I'd say that's a logic error in the cookbook doing it.

[bodgix]

no the two return different things.

the first one calls

Logstash.get_attribute_or_default(node, @name, 'create_account')

which reads the value of the node['logstash']['instance_default']['create_account'] which is set to true in attributes/default.rb

rendering setting this property to false impossible.

The second line, does not call Logstash.get_attribute_or_default(node, @name, 'create_account') and the value of new_resource.create_account remains false

[martinb3]

Wouldn't it be easier to simply override the node attribute, if you're going to use one and default to true?

[martinb3]

(or get rid of the node attribute)

[bodgix]

Ok but what's the point of letting the user set a resource property if it's getting overriden anyway and is always true?

To me this is clearly a bug.

[bodgix]

I've just spent an hour trying to figure out why

logstash_instance 'name' do
   ...
   create_account false
end

is trying to create the account.

[martinb3]

Having a resource property and looking at the node object allows us to support both styles of usage (resources vs. recipes). But if someone tries to mix them, there's actually lots of ways in which this will break (separate resource collections & run contexts, chefspec, chef-shell, etc).

I've just spent an hour trying to figure out why

From what I understand, your situation is actually:

default['logstash']['name']['create_account'] = true

logstash_instance 'name' do
   ...
   create_account false
end

Is that more accurate? I feel like that's inconsistent. I'd almost rather raise an error there.

[bodgix]

no. I do not set

default['logstash']['name']['create_account'] = true

I'm only calling the resource.

[martinb3]

@bodgix can you show me a minimal example? I suspect something is setting node['logstash']['name']. otherwise both the property and the utility method should return false.

[martinb3]

Ah, I see what's happening. We should probably adjust the utility method to never return true if node['logstash'][@name] isn't set.

[bodgix]

IMHO the utility method should only be called IFF the property isn't set.

[martinb3]

IMHO the utility method should only be called IFF the property isn't set.

My concern is that it's a backwards-incompatible change. We should just drop support for the node object entirely if we do that 👍 (and make this a library cookbook, like it should be.)

[bodgix]

well but the utility method isn't called for String properties when they're set.

The problem is the use of || operator. It's works as expected for String/Array because strings or arrays can only be falsey when they're nil.

Boolean are special and this is why I propose to drop the use of

property || default_property

For booleans.

[bodgix]

the root of the problem is that an unset boolean and a boolean set to false are both falsey. and I believe that the utility function should only be called for unset properties. and it still will be with my change.

[bodgix]

Thank you for pointing me to the workaround.

Since my patch may break backward compatibility, I decided to try it.

I set ['logstash']['instance'][instance_name]['create_account'] to false in my recipe so now I have:

node.override['logstash']['instance'][instance_name]['create_account'] = false

logstash_instance instance_name do
  base_directory base_dir
  install_type 'tarball'
  version '1.5.6'
  source_url "file://#{File.join(base_dir, 'logstash-1.5.6.tar.gz')}"
  checksum '812a809597c7ce00c869e5bb4f87870101fe6a43a2c2a6586f5cc4d1a4986092'
  create_account false
  user 'some_user'
  group 'some_group'
end

but the provider is still trying to create the user.

I believe there is another problem and it's in the utility method this time:

chef-logstash/libraries/logstash_util.rb

Line 33 in d4c2749

instance_attr || default_attr

I believe that the intention of the utility method is to return the instance_attr if it's set or the default_attr otherwise. The problem is that when the instance_attr is set to false, this expression becomes

false || default_attr

which evaluates to default_attr.

I finally managed to disable account creation by overriding

node.default['logstash']['instance_default']['create_account'] = false

in my recipe.