[BUG] libmatchers.jinja doesn't respect delimiter for grain/pillar-based yaml files

[blu-base]

Your setup

Formula commit hash / release tag

latest, fd8430b

Versions reports (master & minion)

latest, bug effectively independent from version

Pillar / config used

A nested pillar, such as:

my_pillar:
  nested_pillar_key: value

Customization of the map sources in parameters/map_jinja.yaml, such as

values:
  sources:
    - Y:I:!@my_pillar!nested_pillar_key

# include the defaults

Custom parameters TEMPLATE/parameters/my_pillar!nested_pillar_key/value.yaml, such as

values:
  parameter: override

---

Bug details

Describe the bug

The map function, and more specifically the libmatchers macro is not correctly respecting non-default delimiters for yaml-parameters selected by grains or pillars.

currently, the matchers macro would look up the value of my_pillar!nested_pillar_key instead of the value of nested_pillar_key within the parent my_pillar.

Since the my_pillar!nested_pillar_key key does not exist custom parameters of the specified source type (yaml, pillar, with non-default delimiter) are ignored.

However, Y:C:!@my_pillar!nested_pillar_key is respecting custom delimiters.

Using non-default delimiters is often required, that developers on windows-based clients can use repositories with custom parameters based on nested grains or pillar.

Steps to reproduce the bug

• Provide a nested pillar as specified in the setup
• Add a custom map source, or add it to the map.jinja defaults
• run formula._mapdata with debug output
• inspect the query result of Y:I:!@my_pillar!nested_pillar_key with is likely None
• inspect the mapdata yaml with is likely missing the custom parameter provided by grain/pillar-selected yaml-source

Expected behaviour

Non-default delimiters should be respected for grain-based or pillar-based yaml-file selection.

Attempts to fix the bug

• identification of the problem
• argumentation in additional context below
• preliminary workaround (patch):

iff --git a/TEMPLATE/libmatchers.jinja b/TEMPLATE/libmatchers.jinja
index 10ff8bd..391ad50 100644
--- a/TEMPLATE/libmatchers.jinja
+++ b/TEMPLATE/libmatchers.jinja
@@ -182,8 +182,12 @@
               ~ cli
               ~ "'"
             ) %}
+{%-         set query_opts = {} %}
+{%-       else %}
+{%-         set query_opts = {
+              "delimiter": parsed.query_delimiter,
+            } %}
 {%-       endif %}
-{%-       set query_opts = {} %}
 {%-       set query_opts_msg = "" %}
 {%-     endif %}
 

• I need a brief discussion whether to implement tests, since this might require adding test parameter files, including them through the kitchen.yml, and finally updating the mapdata results.

Additional context

I had to convert custom parameters for a formula to windows compatible delimiters since colons are invalid characters in filenames on windows. The previously used default delimiter, a colon, worked fine to work on repositories on linux clients. Though this condition was preventing coworkers from cloning on windows machines...

After that change, custom parameters where not respected in formulas anymore.

Anyhow, I had to "dive deep" to find the cause why the explicitly stated pattern [<TYPE>[:<OPTION>[:<DELIMITER>]]@]<KEY> wasn't fully usable. Below I try to give an analysis.

Boundary conditions

Approximately, i have this boundary condition,

1. A nested pillar:

my_pillar:
  nested_pillar_key: value

2. Customization of the map sources in parameters/map_jinja.yaml

values:
  sources:
    - Y:I:!@my_pillar!nested_pillar_key

# include the defaults

3. a formula (closely) derived from the template-formula, using unmodified jinja macros as given in the template formula
4. conventional minion and master, no salt-ssh, api calls, or unknown
5. no merging strategies for specified for custom parameters

Issue

The source Y:I@my_pillar:nested_pillar_key was correctly using TEMPLATE/parameters/my_pillar:nested_pillar_key/value.yaml. But Y:I:!@my_pillar!nested_pillar_key was resulting in querying TEMPLATE/parameters/my_pillar!nested_pillar_key.yaml

Drilling down

Identifying the resulting query

Searching for the query which should use pillar.get resulted in

{%-     set values = salt[parsed.query_method](
          parsed.query,
          default=[],
          **query_opts
        ) %}

https://github.com/saltstack-formulas/template-formula/blob/fd8430b203101eee8d124f5734ba678e71ac3e11/TEMPLATE/libmatchers.jinja#L199C1-L203C13

For Y:I:!@my_pillar!nested_pillar_key i would expect this jinja call to effectively be rendered to:

{%- set values = salt['pillar.get'](
       'my_pillar!nested_pillar_key',
       default=[],
       delimiter='!'
     ) %}

but for some reason query_opts did not contain the delimiter argument.

Where is query_opts set

Searching for the symbol query_opts shows that it is only referenced (and assigned) in this section:
https://github.com/saltstack-formulas/template-formula/blob/fd8430b203101eee8d124f5734ba678e71ac3e11/TEMPLATE/libmatchers.jinja#L164C1-L188C17

query_opts is only non-empty if this expression is true, including boundary condition 4:
parsed.query_method == "config.get" and config_get_strategy
otherwise it's set to an empty dict in line 186.

There is a hole in this logic sequence. only config.get and non-empty config_get_strategy would allow to use custom delimiters. As "specified" in the docs for map.jinja the pillar and grain lookups for yaml files should support custom delimiters. But never setting query_opts results in missing this feature for the concrete lookups (instead of config.get).

Source Parser always assigns delimiter

The section where the map source state is parsed always sets the default delimiter or custom delimiter if present into the parsed dictionary.
Compare to
https://github.com/saltstack-formulas/template-formula/blob/fd8430b203101eee8d124f5734ba678e71ac3e11/TEMPLATE/libmatchers.jinja#L49C1-L144C17

I haven't found a logic issue here, and therefore assume parsing always correctly dissects the map source DSL - returning a dictionary with these keys

parsed = {
  "type": 
  "option": 
  "query_method": 
  "query_delimiter": 
  "query": 
}

query_method is finalized in
https://github.com/saltstack-formulas/template-formula/blob/fd8430b203101eee8d124f5734ba678e71ac3e11/TEMPLATE/libmatchers.jinja#L146C1-L162C17

Why is only the config.get handled?

I don't know the history of the section which sets query_opts
https://github.com/saltstack-formulas/template-formula/blob/fd8430b203101eee8d124f5734ba678e71ac3e11/TEMPLATE/libmatchers.jinja#L164C1-L188C17

I can understand why the ssh/unknown cli flag are handled.
handling only config.get is likely done to apply the config_get_strategy option.

The argument merge (line 167) is not exclusive for config.get, but since in line 201 default=[] is set, merge is only effective for config.get. I think that's why config.get with config_get_strategy was handled.

Proposal

I believe, query_opts should contain delimiter in the default case, and should only be emptied when cli is in "ssh", "unknown".

This could be done in different fashions, for example one of these:

• minimally invasive by extending the else case in line 185 as shown in the patch at the top of this issue.
• rearranging and flatten the assignments, such as

{%-     set query_opts = {
            "delimiter": parsed.query_delimiter,
          } %}
{%-     set query_opts_msg = (
            ", delimiter='"
            ~ parsed.query_delimiter
            ~ "'"
          ) %}
{#-     Add `merge:` option to `salt["config.get"]` if configured #}
{%-     if parsed.query_method == "config.get" and config_get_strategy %}
{%-       do query_opts.update({
              "merge": config_get_strategy
            } %}
{%-       set query_opts_msg = (
            ", delimiter='"
            ~ parsed.query_delimiter
            ~ "', merge: strategy='"
            ~ config_get_strategy
            ~ "'"
          ) %}
{%-     endif  %}
{#-     reset 'query_opts' and 'query_opts_msg' if 'cli' is 'ssh' or 'unknown' #}
{%-     if cli in ["ssh", "unknown"] %}
{%-       do salt["log.warning"](
              log_prefix
              ~ "the 'delimiter' and 'merge' options of 'config.get' are skipped when the salt command type is '"
              ~ cli
              ~ "'"
            ) %}
{%-     set query_opts = {} %}
{%-     set query_opts_msg = "" %}
{%-     endif %}

Testing

I think, it would be useful to include nested metadata as edge cases in the test-pipelines. but this would require to extend the config and test files.

For example,

the kitchen.yml would further include test-only parameters such as test/salt/TEMPLATE/parameters/nested:pillar/value.yml. The test/salt/pillar/defaults.sls would include an additional nested key structure, including the respective mapdata renderings.