magfest-archive / ubersystem-puppet

A puppet module that manages install/config for the Magfest ubersystem registration system
3 stars 1 forks source link

The Final Puppetization #28

Open EliAndrewC opened 9 years ago

EliAndrewC commented 9 years ago

At the moment, our puppet manifests suffer from the problem that if we add a config option, we need to go into our puppet manifest and template and then add it there as well, or else it isn't really configurable. Ditto for config options which are not currently tracked by Puppet; they're technically configurable, but not really.

As it happens, our configuration is currently just a giant dictionary. I mean, technically it's an ini file, but it ends up parsing to a dictionary. So instead of having a bunch of different hiera variables, each controlling a single option, I'd like to have a single dictionary which maps to the entire config. For example, if we had this yaml

config:
    foo: 5
    bar:
        baz: 6
        spam:
            hello: "world"

that would render the following to our development.ini file

foo = 5
[bar]
baz = 6
[[spam]
hello = "world"

Then we could have a single nested hiera variable which will always be able to contain any/all of our config options without having to individually puppetize each one.

binary1230 commented 9 years ago

So, looked into this and there's one gotcha that has to do with the recommended way to make hiera inherit hierarchies of config files.

The idea being that you could have a base config like this:

in base.yaml:

config:
 key1: "value1"
 section2:
    key2: "value2"

and then another file that inherits from that:

in derived.yaml:

config:
 key3: "value3"

the resulting config might look like:

config:
 key1: "value1"
 section2:
    key2: "value2"
  key3: "value3"

HOPEFULLY that's what it would look like. However, the merging behavior of hiera gets a little weird occasionally and it doesn't come out clean. In addition, there's no real way to remove params from base levels too.

We're currently using something called deep-merge which does a decent job of guessing the right thing.

That all being said, before embarking on this, it would be worth having a conversation with @thaeli about the redis config (which would be necessary for other stuff like docker deploys) and to figure out how to do a bunch of layers of inheritance of config values there without necessarily having to use hiera for it. We're going to need to think carefully about that as our plugins become more complex too and will need their own configs tracked independently of the main config.

So: TLDR: I hear what you're saying here, and we should figure out and get everyone on the same page for a comprehensive plan for config stuff.

EliAndrewC commented 9 years ago

Two minor notes:

In addition, there's no real way to remove params from base levels too.

Strictly speaking, nothing is ever "removed" from a configspec-based ConfigObj instance. You would merely set it back to its old default value. For example, if the configspec.ini has key1 = string(default="") and base.yaml has

config:
    key1: "Hello World"

then in order to "unset" it, derived.yaml would need to just say

config:
    key2: ""

redis config

If we do end up moving in that direction, this will be less of a big deal, but we're always going to have some ConfigObj config, for things we don't want stored in Redis like account creds for Stripe, Postrgres, AWS, and of course Redis itself. So this would still be helpful to avoid needing to individually puppetize every individual config option.

thaeli commented 9 years ago

If we move to a Docker-based deployment, some of the specifics of storing that info may change. Breaking it out into a separate ConfigObj is reasonable for now though, and will make that refactor easier.

binary1230 commented 9 years ago

so I looked into two things on this recently.

1) it's looking generally frowned up to do hiera lookups directly from .erb templates (i.e. having a template pull in 'all the config data' from one var and paste it in). while that may be true, it may also be worth doing it that way just because yea it would make it faster to iterate. this method has also been warned against because it makes it harder to debug inheritance issues with hiera.

the idea is the that the .erb is supposed to only work with data passed into it by the puppet class it's refernced from, so that the class can perform validations and such against it.

2) we might though be able to do exactly this by having some kind of class like this in puppet:

class uber::config ($c) {
...
}

and that $configdata is what you're proposing like:

uber::config::c::prereg_opens '-date-'

Then you only have to specify things in hiera.

I'm only guessing that this is possible, I don't know for sure.

If that does work, we just have to look into how merging an array of config data works between different levels of the hierarchy. We were doing this fine before, but with it all under one variable, could get tricky, not sure.

EliAndrewC commented 9 years ago

Yeah, I'm guessing this would be a pretty huge amount of work, and it might just never be worth doing. I mean, it's not like it's that hard to puppetize a new option. I've just been spoiled by the fact that I used to be able to just update a config then run uberpull and then everything is on the production server. Now when I want to make a config change it's three pull requests across three repos, and what used to be a 30 second change is now a 5 minute one.

Again, not a huge deal and the new stuff is definitely better, but I definitely feel the difference in terms of the friction on "I'm just trying to do a thing but now I have to stop what I'm doing to go puppetize another config option". Probably not worth the tons of hours and hours required to implement this ticket, though that would be amazing if it turned out to be easy.

binary1230 commented 8 years ago

btw, maybe relevant: https://forge.puppet.com/puppetlabs/inifile

EliAndrewC commented 7 years ago

Someone implemented this where I work and it turned out to be pretty straightforward for the simple case (which wouldn't completely apply to us). If we have a foo project then we just have a single hiera variable. Then we put this in the erb template for our development.ini:

<% 
def hash_to_config(any_name, any_hash, level)
   if any_hash.empty?
      return ""
   end

   if level > 0 
      ret_val = "\n" + "[" * level + any_name + "]" * level + "\n"
   else
      ret_val = ""
   end

   any_hash.sort_by{|n,v| if v.is_a?(Hash) then "z"+n else n end}.each do |data, value|
      if value.is_a?(Hash)
         ret_val += hash_to_config(data, value, level+1)
      elsif value.is_a?(Array)
         ret_val += data + " = " + value.collect{|x| "\'#{x}\'"}.join(',') + "\n"
      elsif value.is_a?(Integer)
         ret_val += data + " = " + value.to_s + "\n"
      elsif value == !!value
         ret_val += data + " = " + value.to_s.capitalize + "\n"
      elsif ["true","false"].include? value.downcase
         ret_val += data + " = " + value.capitalize + "\n"
      else
         ret_val += data + " = \"" + value + "\"\n"
      end
   end
   return ret_val
end 
-%>
<% if not foo_options.empty? -%>
<%= hash_to_config("", foo_options, 0) -%>
<% end -%>

Obviously it would be a pretty big refactor to go from what we have now to taking this approach. And the difficult part which this ducks merging data from different sub-parts of the has. Still, I figured I'd post this snippet since it's at least half the battle (even if it is the easy half).

binary1230 commented 7 years ago

So hey btw, the good news is -i think- all the hiera inheritance stuff happens before this is called so -in theory- integrating this with the merging shouldn't be hard, assuming the 'deep merge' strategy plays nice. which it may not. can look into it though.

binary1230 commented 7 years ago

at any rate, very cool

EliAndrewC commented 7 years ago

According to the guy who cooked up that recipe, we can use hiera_array and hiera_hash instead of hiera and it'll do the deep merging like we want: https://ask.puppet.com/question/13592/when-to-use-hiera-hiera_array-and-hiera_hash/

FWIW, we've never actually tested this where I work since so far all the manifests which use this pattern have defined all their options in a single YAML file and haven't needed to override default values and such through hiera data.