voxpupuli / puppet-consul

A Puppet Module to Manage Consul
Apache License 2.0
121 stars 314 forks source link

config_hash converts strings to integers => breaks port mappings #119

Closed weitzj closed 9 years ago

weitzj commented 9 years ago

In the consul config_hash it is possible to disable the http port by setting it to -1, and enabling the https service instead by setting it to the appropiate port, 8500:

      'ports' =>              {
        'http'  =>  -1,
        'https' => 8500,
      },

The generated config.json results in:

  "ports": {
    "https": "8500",
    "http": -1
  },

Current behaviour (Release 1.0.0)

-1 is looking good, and is still an integer, whilst the https ports was converted to a string. The consul agent does not support strings as port numbers and therefore fails to launch.

Expected behaviour

-1 as well as 8500 should be integers.

Workaround

Tell puppet to treat "8500" as an integer by multiplying with 1 *:

      'ports' =>              {
        'http'  =>  -1,
        'https' =>  1 * 8500,
      },
solarkennedy commented 9 years ago

Yea, we explicitly try to "cast" these into ints for service: https://github.com/solarkennedy/puppet-consul/blob/master/manifests/service.pp#L49

Then we do it on a bit more keys here: https://github.com/solarkennedy/puppet-consul/blob/master/manifests/config.pp#L82-L98

I'm... up for suggestions?

We could take a advantage of the fact that we have a custom function to write the json down: https://github.com/solarkennedy/puppet-consul/blob/master/lib/puppet/parser/functions/consul_sorted_json.rb

In ruby we could look at each key, and if it looks like an int, make it an int? Seems a little dangerous, but a tad better than whitelisting keys?

aj-jester commented 9 years ago

Something like this?

irb(main):001:0> iffy_integers = [-1, '+1', '-goats', 'moats-', '8500', '2+3', '-3454', '-this is A-']
=> [-1, "+1", "-goats", "moats-", "8500", "2+3", "-3454", "-this is A-"]

irb(main):002:0> iffy_integers.map { |i| (i.kind_of?(String) and i.match(/\A[-]?[0-9]+\z/)) ? i.to_i : i }
=> [-1, "+1", "-goats", "moats-", 8500, "2+3", -3454, "-this is A-"]
solarkennedy commented 9 years ago

Yea, that looks like it achieves the desired effect. @aj-jester want me to whip that into a PR or did you want to?

aj-jester commented 9 years ago

@solarkennedy https://github.com/solarkennedy/puppet-consul/pull/156

solarkennedy commented 9 years ago

@weitzj this should be fixed on the latest master thanks to @aj-jester

weitzj commented 9 years ago

Thank you. :)