Regression in 0.7.0: incorrect data type on location_custom_cfg_prepend and similar parameters

[dominics]

Affected module versions/distributions

• Module version: 0.7.0
• All platforms
• I'm using Puppet 5.0.1, but I imagine it's similar on any recent version with data type support

How to reproduce

nginx::resource::location { "foo":
  location_custom_cfg_prepend => ['location /bar {return 404;}'],
  [...]
}

Note that this is a nested location block. e.g. useful for implementing this pattern (see the last bullet point in that section; "Use a nested location block"...)

What are you seeing

Evaluation Error: Error while evaluating a Resource Statement, Nginx::Resource::Location[foo]: parameter 'location_custom_cfg_prepend' expects a value of type Undef or Hash, got Tuple at <ref>

What behaviour did you expect instead

• The documentation for location_custom_cfg_prepend claims "Expects an array with extra directives to put before anything else inside location (used with all other types except custom_cfg). Used for logical structures such as if."
• The actual use of those parameters does allow an array (it will do the correct thing), see: https://github.com/voxpupuli/puppet-nginx/blob/v0.7.0/templates/server/location_header.erb#L46
• Worked in 0.5.0 and 0.6.0 (and probably since March 2014)

Other Notes

• Caused by the addition of strict data types in #1050 and #1031 - the CHANGELOG claims those "replace validate_* calls", but they don't - there were no validate_* calls for these parameters in earlier versions.
• At some point, the tests of this functionality have been removed, allowing the regression

[alexjfisher]

@dominics Thanks for your bug reports. Looks like there are a few things that will need fixing. I thought this one would be an easy fix. But the more I look, the more I'm not so sure.

The actual use of those parameters does allow an array

I'm not too sure about that. Isn't it saying the the 'value' of any of the hash keys can be a hash or an array? That doesn't seem to have changed since 0.6.0.

<% if @location_custom_cfg_prepend -%>
  <%- @location_custom_cfg_prepend.each do |key,value| -%>
    <%- if value.is_a?(Hash) -%>
      <%- value.sort_by {|k,v| k}.each do |subkey,subvalue| -%>
        <%- Array(subvalue).each do |asubvalue| -%>
    <%= key %> <%= subkey %> <%= asubvalue %>
        <%- end -%>
      <%- end -%>
    <%- else -%>
      <%- Array(value).each do |asubvalue| -%>
    <%= key %> <%= asubvalue %>
      <%- end -%>
    <%- end -%>
  <%- end -%>
<% end -%>

I think maybe it was accidentally working?? If location_custom_cfg_prepend is an array. then in the each block, 'key' will contain the array value and 'value' will be nil? Then in Array(value).each do |asubvalue| we actually iterate an array of nils? This works anyway (even though asubvalue is nil), as we just print the original 'key' (which isn't actually a key, but your original array value).

Does any of that make sense? (Probably not; I've just read back what I've written! :) ) Is raw_prepend of any help?

Even if this isn't a type that needs fixing, the documentation certainly needs improving.

[dominics]

I think maybe it was accidentally working??

Looking more and more like that the more times I stare at that template iteration logic.

Does any of that make sense?

It does make sense, yeah. At first I thought maybe there would be something not supported by the hash syntax, but it seems like any array that was previously working can be converted to an equivalent hash (by moving the first "word" to the key, and making the hash value an array if you need e.g. multiple location blocks nested within another). I hope that's not just a lack of imagination on my part though.

E.g. my example can become something like:

nginx::resource::location { "foo":
  location_custom_cfg_prepend => { 'location' => ['/bar {return 404;}', '/baz {return 403;}'] },
  [...]
}

Feel free to consider this a request to fix up the documentation unless someone can come up with something that actually doesn't work with hashes.

[benh57]

Hmm, well, I could change my code from arrays to hashes, but it's nicer to keep the backwards compatibility and make the data type Optional[Variant[Hash, Array]]

[wyardley]

I can see this both ways, I've found the module's current behavior confusing and not well documented at times (the fact that so much of the logic is in the template makes it especially hard to understand, I think), but at the same time, I've found that the current behavior is very flexible, even when it's a bit too "magic" in terms of allowing different types of data structures.

I'm not sure whether the behavior was unintentional or just clever but kind of obscure / hard to read; I guess I would slightly prefer to see a doc fix than to limiting behavior that worked in the past, but maybe it would be better to just force a specific way of doing things (as long as we still allow enough flexibility to deal with situations where a hash might limit the ability to have >1 of a given directive)? I would be Ok with hurting backwards compatibility if it made the docs and templates easier to read and understand.

I know there had been some talk about moving some more of the logic out of the templates, after taking a stab at changing some of the existing templates again recently, I definitely think that could be a good idea, though I don't want to do it myself.