Adds NG states

[cheuschober]

This PR adds the ng states for the nginx formula. This changes the pattern considerably and adds support for managing virtual hosts files. In the interest of disclosure, there are a couple caveats:

• ng requires the merge parameter of salt.modules.pillar.get(), not formally available until Helium. This is explicitly noted in the README
• Much of the vhost management is clumsily done as a state and should have been implemented as an addition to the nginx execution module (and by extension, as a state module). Unfortunately, realized that a bit too late to reverse course this time around.
• There's a ton of reliance on jinja underneath the hood making it both complex and difficult to read but that's all to the benefit of making the pillar (the true user-interface), powerful, extensible, and intuitive

[whiteinge]

Nicely implemented! I like your sls_block approach. I'll pore over this in detail when I can find a few minutes.

I made some changes to the README in 3494514. I hope you don't mind -- feel free to adjust as you see fit.

[whiteinge]

I also really like your conf file management approach. Putting something like the vhost_config macro at the top is a very clean syntax for specifically formatting arbitrary config files in a very data-centric way. I'd like to see something like this make it into the best-practices doc.

Pondering aloud: I wonder how much overlap in formats there is between config formats for various tools. (In other words: whether it would be worth having a "library" of macros for common formats, or if that is instead a road to madness...)

[whiteinge]

(Still) pondering aloud: that said, there's a lot of value in seeing the formatting there at the top of the file. It removes any ambiguity as to what will be written to the file system.

It also keeps the templating logic quite clean. I prefer your approach compared to my approach.

[cheuschober]

Hi Seth. No problem: re changes.

I'm very curious to get your opinion on the whole pattern. The general pattern is a pseudo-mvc approach. Expanded, the mapfile is the model and the 'sole source' of data from whatever backend it exposes (pillar, grains, etc). The states are controllers and should not contain anything other than programmatic logic (states) and transactional controls (requisites). The conf files are obviously the views and operate much the same way any view would work.

I much prefer the slim view like vhost_config, if only because it saves us the hell of updating a bunch of formulas when the underlying config file params change and it also saves formula authors from having to have 1:1 parity with config file options/params.

I've had the same thought you had re: macro libraries and I think it's completely do-able. My ntp.ng state uses a similar pattern (I don't think I've sent a PR for that one for some reason or another).

[cheuschober]

The other reason to use the macro was recuriseness -- many config file formats can and do recurse fairly easily and indentation is not necessary but also not difficult to preserve.

I've been working on another formula for a php app we use in-house that has standard php variable declarations for its config options, eg:

$some_var = 'I am a string';
$some_other= True;
$some_nightmare = array(
    'associative' => 'array',
)

Even that was simple to map from python constructs.

[cheuschober]

er, recursiveness

[whiteinge]

One more note here. (Writing it down somewhere to solicit comments...and also so I don't forget.) A pattern that came up on IRC last week for denoting that a file is managed by Salt and also including the source file so you know where to find the original. Files templated through the file.managed stuffs have the contents of the state in the Jinja context so you can do:

# Nginx vhost configuration
#
# **** DO NOT EDIT THIS FILE ****
#
# This file is managed by Salt via {{ source }}.

Which will output the salt://path/to/file or whatever.

[whiteinge]

The other reason to use the macro was recuriseness -- many config file formats can and do recurse fairly easily

That is extremely cool.

[cheuschober]

That's a perfect idea (re: comments). Yes, good addition.

Another pattern worth considering including in the formal best practices is what's diving the sls_block bit. More than once I had headaches using formulas in a situation where, for example, I wanted to install nginx but I needed it pinned to a specific version. Expecting a formula author to re-implement every variable in pillar of every state method would be ridiculous but using a simple dictionary in pillar that can be expanded or not is a soft expose with hard overrides below it (relying on state compilation taking the last declaration of a particular key). It's not terribly complex to implement and makes the formula so much more flexible to individual use-cases.

[whiteinge]

Expecting a formula author to re-implement every variable in pillar of every state method

I agree whole-heartedly. I've run into this myself more than once. And using extend for this feels clunky. Readability suffers a bit since you have to reference an external file (the map file) for what all args will be used but I can't think of a good way around that offhand.

[cheuschober]

I agree.

I played with doing overrides in the map file itself (as I did with the user.name at the bottom) and basically constructing fully-operational state declarations as dictionaries in the map.jinja but it felt like overkill. It's not fundamentally flawed but it does smell ever so slightly. As you said, I don't think there's a particularly good way around it.

I would like the sls_block() macro to be unnecessary in the form of some type of generic addition to core states that accepts a dictionary and plays it back as default params for the state function that are then explicitly overridden by named parameters, eg:

somefile:
    file.managed:
        - create: True
        - default_params: {'create': False, 'replace': True}

Having that as a universal-starting point would reduce some of the formula clunk because we still want, or at least I still want, my map.jinja to be my sole source for data and specifically, the location I go to see what data defaults are being applied to my states. I like being able to look in there to see the big picture and know exactly what I want to override in pillar. It's easy for me because the merge approach ensures that the map.jinja is structured exactly the same as the pillar itself so it's a 1:1 api and only one type of comprehension split between two files (map and pillar).

[whiteinge]

Well said. I'll shop the idea around the office of having a global override parameter of some sort.

[cheuschober]

Since it's relevant, to see what the php macro looks like:

https://github.com/spsoit/resourcespace-formula/blob/master/resourcespace/files/config.php

[whiteinge]

Yeah, nice. Thanks for the link.

[cheuschober]

FYI, we re-did the php formula with the same pattern (and added support for fpm pool management):

https://github.com/spsoit/php-formula

This one requires the use of odict([]) (added as a jinja global in Helium) because some of the php.ini variables are order-sensitive. I fear it's not enough because I don't think the data coming out of pillars preserves dict order (perhaps you could confirm). With this pattern, we really don't want to turn these into lists if only because it makes recursive merging impossible.

I'm under the gun on a few projects so it might be a week or so before this one (and others) get properly doc'ed and submitted but thought you'd like a preview of the pattern at work again.

[whiteinge]

Thanks for the preview. Using lists is simple and bullet-proof but point taken on that breaking merging values. Wonder if it's worthwhile making a merge algorithm that works for lists of single key/val dictionaries. (We may have one already for merging requisites.)

[cheuschober]

I had wondered about that as a potential option but wanted to defer to your knowledge about it. Alternately, the blunt-instrument approach could be to make ordered dict the default mapping type.

[cheuschober]

Hey Seth, I've got one more brainteaser for thought, a place where the current pattern falls a bit:

How would you recommend implementing the vhosts management features of this state in other formulas? Here's the scenario:

I have a webapp, "fooapp" and fooapp, needs to be hosted on some type of webserver. I can create an fooapp.nginx state that pulls in my nginx.ng states but I (currently) have no way to inject a vhost from my fooapp pillar/states into the dict of vhosts managed in the nginx.ng pillar. I can specify that pillar independently in the nginx pillar but it breaks the 'state must run by default' paradigm and means that I, as a fooapp user, have to know what to configure in that particular vhost config. The vhost management pieces are too complex for a simple extend and I can't inject anything directly because the current pattern uses direct imports from map.jinja.

Do you k now of any way of instantiating the map variable once (eg, nginx), and treating it like some kind of global var where data could be modified? Any other ideas around this particular issue?

[cheuschober]

Just thinking aloud of the larger limitations, I'm going to assume that jinja2 is reinstantiated for each template being rendered which means that jinja isn't the right place for any type of persistent store between state renders. How insane would it be to spin up another dunder var like salt and expose it in Jinja with write access, eg, {{ data }}. The pattern could then include a map.loader that returns data.formulaname if it exists or lazy-loads it if it doesn't. I think that would keep pillar calls / merges down to a minimum in a complex state with a lot of includes and solve my cross-state-data issue.

Have any thoughts on that?

[whiteinge]

Slow reply...and I'm a tad sleepy from yesterday's sprint. I'll think on this more but here's a short reply.

There's no good way to have a global var that can be modified from different Pillar .sls files.

Offhand I'd say the best way to approach it is to have a myapp/nginx.sls file, like you noted. That includes and extends the Nginx vhost state. I need to sit down and go through the exercise of building one out to get the full picture of where using extend starts to break down.

The "must run by default" without Pillar thing should be reworded to specify that applies to the base functionality in a formula. E.g. the expected thing (install a package, start a service). There are some things you simply can't do without some configuration; I think vhosts are a good example of this.

[whiteinge]

Run into a hiccup while playing with this pattern today. The mapping test was introduced in Jinja 2.6 and Cent 6 ships with 2.2.1. A simple way to go would be to ship a duplicate Jinja test in Salt.

[cheuschober]

Boo. Not awesome. As you said, it wouldn't be too difficult to pick that up. You should see the fun hoops I had to drive through because file.managed(context) was double-quoting my passed data:

https://github.com/spsoit/php-formula/blob/master/php/ng/macro.jinja#L8

Added the macro file because it doesn't perform a pillar hit and doesn't require context. serialize() there was required to recursively turn my merge-friendly ordered dict into lists of single-key-dictionaries so order would be preserved in json() translation. For the record that deserialize() method doesn't really work as-expected and will be removed.

The only real issue I have with this approach is that it's not particularly recursion-friendly. After I go from an ordered dict into a list of single-key dicts, parsing a real list from a dict becomes quite the headache and would require a custom filter (not a macro) to deserialize since the return is a variable.

Probably not a real project until late June or July but I do want to play with idea of spinning up some kind of singleton object that the templates can use for cross-talk (much in the same way that salt['exec.module'] is available for execution modules I'd like to think that somevar['method'] could be available with some basic get,set,merge capabilities). Admittedly, I don't have near-enough a picture of the state execution and templating chain to know if that's all happening under a single call (where something could persist in memory) or if each state call is completely independent on the zmq bus. file.managed(context) is good but direct, lazy-loaded variable access is so much better.

[whiteinge]

Interesting. Might be helpful to expose this function to Jinja.

[whiteinge]

I haven't forgotten about this thread. Been swamped. Think I may have a good way to generate state yaml from a var that will work with an existing state...

[cheuschober]

Really? I'm certainly interested, I've been on some other tasks myself in the last couple of weeks. Should be back on formulas in a day or two. The function you pointed out would be extremely helpful in jinja so I'm probably going to make that an early priority to see if I can jerry it in before the helium gets cut.

[whiteinge]

I went to implement my idea...and it's already in there.

{% set defaults = [
    {'user': 'myuser'},
    {'group': 'mygroup'},
] %}

myfile:
  file:
    - managed
    - name: /tmp/myfile.txt
    {{ defaults | yaml(flow_style=False) | indent(4) }}

No where near as flexible as your default_params idea but at least you can add list items to an existing list. The pattern warrants a little exploration in any case.

[cheuschober]

Hah. Yep, I'm responsible for the flow_style patch.

[cheuschober]

I added that anticipating helium but have been using sls_block() in formulas for compatibility with existing releases. It's been my hope to transition to flow_style | indent after helium.

[cheuschober]

The reason I liked default_params was how explicit it is. Otherwise, what we rely upon is the parsing order (and the expectation that users know it). It is perhaps arguably more pythonic (and I say this as someone whose first language isn't python), to not be so explicit and use the flow_style option but just for readability, I prefer this:

statename:
  module.function:
    - hardcoded: value
    - softcoded: {{ value }}
    - defaults_params: {{ params }}

to this:

statename:
  module.function:
    {{ params | yaml(flow_style=False) | indent(4) }}
    - softcoded: {{ value }}
    - hardcoded: value

[cheuschober]

By the way, flow_style is also amazing in config templates that are based in yaml.

When you see my rewritten salt-formula I'm sure you can guess why.

[whiteinge]

When you see my rewritten salt-formula I'm sure you can guess why.

I like the sound of that.

[cheuschober]

The whole config file template:

# Salt configuration file
# 
# **** DO NOT EDIT THIS FILE ****
# 
# This file is managed by Salt via {{ source }}
{{ config | yaml(False) }}

[whiteinge]

It's a thing of beauty. Death to Jinja! Long live Jinja!

[cheuschober]

Hah. Yeah, I do feel a little guilty what with my heavy jinja formula patterns but I see it as a small evil for the much greater good of data-driven templates. Kinda flies in the face of the 'use less Jinja in states' practice recommendations.

[whiteinge]

Yes, but there isn't a reasonable alternative that I can think of. ~50 lines of unreadable Jinja == instant domain-specific data serializer.