to_type helper's handling of numbers is too loose

[threadwaste]

Description of problem

• What did you do?

Tried to pass a numerical identifier with leading zeroes (e.g. "012345") through client_custom.

• What happened?

The identifier was converted to an integer, lost its leading zeroes, and stored as a JSON number.

• What did you expect to happen?

The identifier would retain its leading zeroes, and be stored as a JSON string.

• How can someone reproduce the problem?

class { 'sensu':
  client_custom => {
    some_value => "0123"
  }
}

Anything else to add that you think will be helpful?

I'm trying to understand this helper better. The original commit suggests its tied to several type checks inside Sensu, which I'm guessing pose a problem when facts are used as parameter values. A quick ag of the source shows only a couple of these type checks present in HEAD for very specific values.

Similarly, the use of #to_type in this module is inconsistent. For example, the sensu_client_config type calls it for keepalive, but not safe_mode.

I have two branches of this local that deal with this. One uses more specific matchers for numeric types, the other simply removes the calling #to_type over value for the client_custom parameter. Happy to PR one, but figured I'd seek clarification and input on direction first.

[ghoneycutt]

This is related to issue #682 to use Puppet v4's data types. We should enforce data types for certain values while respecting data types such as a string that looks like an integer, instead of transforming them.

[ghoneycutt]

@cwjohnston Does Sensu have a need for integers to be cast as strings or to allow an integer with a leading zero?

[alvagante]

So, current situation, after migration to puppet 4 parameters validation, is that when integers are passed as strings they are accepted and should not be converted.
@threadwaste can you verify if you have your issue with current master?

[ghoneycutt]

@cwjohnston We need your guidance on what the right thing to do is regarding numbers.

@threadwaste why would you have a number that begins with zero?

[cwjohnston]

@ghoneycutt Sensu implements what we call strong configuration validation, meaning Sensu will refuse to start if it determines configuration is invalid. This extends to a number of configuration attributes where integer values are required, e.g. port numbers, check intervals and keepalive or timeout thresholds. This strong validation does not extend to custom client or check attributes.

Does Sensu have a need for integers to be cast as strings or to allow an integer with a leading zero?

I'm not aware of any case where an attribute requires both an integer value and a leading zero.

We should enforce data types for certain values while respecting data types such as a string that looks like an integer, instead of transforming them.

I'm fine with this in principle. For example, the rabbitmq_port value must be an integer because it's specified in the RabbitMQ configuration reference documentation. Therefore, transforming a string value for this parameter to an integer instead of raising an exception is a better experience for users of this module.

When we're looking at client_custom or the custom parameter of a sensu::check resource, I believe these values should be taken as-is, without transformation. Custom client and check attributes are allowed for in Sensu, and we don't know what the user will do with custom attribute values after they're handled by Sensu, e.g. passed on to some other data store or API.

[ghoneycutt]

Thanks @cwjohnston !

@alvagante does this provide enough info to proceed?

[alvagante]

@ghoneycutt definitively, thank you @cwjohnston
If I've interpreted correctly the approach here I would:

• Accept both integers and numbers in strings for parameters which expect numbers. In any case they are converted to integers in https://github.com/sensu/sensu-puppet/blob/master/lib/puppet_x/sensu/to_type.rb (this is the current status of this PR)
• For the "custom" parameter of "sensu_client_config" (which is passed as custom => $::sensu::client_custom,
  ) I'd remove any type manipulation of the custom hash keys

[threadwaste]

@alvagante Seems like this is down a path, already. But, for context, identifiers like an AWS account number in the client's custom configuration where the intended target type is a string. While I don't have concrete examples of other cases, I could imagine similar types of inputs to say, handlers and checks.

[alvagante]

So, commit 636b3a8 removes integer validation and conversion of the keys of the custom parameter hash in sensu_client_config and should avoid cases like the ones mentioned in this issue.
For other numeric params, there's validation on Integer type (or other Numerics) and strings are converted to integers, so also strings with only numbers are accepted.
This ticket may be closed, if no other remarks are done.