Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YAML refs aren't dereferenced in local Pillars #53291

Open
eliasp opened this issue May 29, 2019 · 4 comments
Open

YAML refs aren't dereferenced in local Pillars #53291

eliasp opened this issue May 29, 2019 · 4 comments
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@eliasp
Copy link
Contributor

eliasp commented May 29, 2019

Description of Issue/Question

When using YAML refs/anchors in Pillar SLS of a local/masterless setup, refs aren't de-referenced before passing them into other contexts which leads to some quite unexpected results e.g. when processing those Pillar data further in a Jinja context, where updating a value within the data suddenly changes other data as well, as they all share the same referenced object.

Setup

State Template: bug.sls:

{% load_yaml as yaml_input %}
anchors:
    project:
        repositories: &id001
            files:
            -   name: salt-top-states
projects:
    a:
        repositories: *id001
    b:
        repositories: *id001
    c:
        repositories:
            files:
            -   name: foobar
    d:
        repositories: *id001
{% endload %}

{% set projects = salt['pillar.get']('projects', {}) %}
{# set projects = yaml_input['projects'] #}

{% for project, projectdata in projects.items() %}
  {% do projectdata.update({
    'config': [],
  }) %}
  {% for repo in projectdata.repositories.files %}
    {% do repo.update({
      "path": project,
    }) %}
  {% endfor %}
{% endfor %}

{% for project, projectdata in projects.items() %}
  {% for repo in projectdata.repositories.files %}
    {% do projectdata.config.append(project) %}
    {% do projectdata.config.append(repo.path) %}
  {% endfor %}
{% endfor %}

{%- for project, projectdata in projects.items() %}
{{ project }}:
    - {{ projectdata.config|yaml }}
{%- endfor %}

Steps to Reproduce Issue

When using bug.sls provided above with the line {% set projects = yaml_input['projects'] %} not commented out ({# ... #}), the output generated by salt-call slsutil.renderer salt://bug.sls looks correct/as expected:

local:
    ----------
    a:
        |_
          - a
          - a
    b:
        |_
          - b
          - b
    c:
        |_
          - c
          - c
    d:
        |_
          - d
          - d

When using the inlined YAML as Pillar SLS instead and commenting out the line {% set projects = yaml_input['projects'] %} again, the result looks like this (please note the non-matching elements (d in a, d in b):

local:
    ----------
    a:
        |_
          - a
          - d
    b:
        |_
          - b
          - d
    c:
        |_
          - c
          - c
    d:
        |_
          - d
          - d

This indicates that YAML refs are simply passed through as Python references up to the point where Jinja processes the data of the dictionary, so instead of working on loop-scoped data in Jinja, one actually processes referenced data crossing loop-scope boundaries.

This can be reproduced using this Python-only code-snippet:

#!/usr/bin/env python3

import pprint

from copy import deepcopy

from salt.utils.jinja import SerializerExtension
from salt.renderers.yaml import render as yaml_render

indoc = """
anchors:
    project:
        repositories: &id001
            files:
            -   name: salt-top-states
projects:
    a:
        repositories: *id001
    b:
        repositories: *id001
    c:
        repositories:
            files:
            -   name: foobar
    d:
        repositories: *id001
"""

def domagic(data, name=None):
  for project, projectdata in data.items():
    projectdata.update({
      'config': [],
    })
    for repo in projectdata['repositories']['files']:
      repo.update({
        "path": project,
      })

  for project, projectdata in data.items():
    for repo in projectdata['repositories']['files']:
      projectdata['config'].append(project)
      projectdata['config'].append(repo['path'])

  print("{}, {}".format(name, type(data)))
  for project, projectdata in data.items():
    print(project)
    pprint.pprint(projectdata['config'])

via_yaml_load = SerializerExtension.load_yaml(None, indoc)
via_yaml_render = yaml_render(indoc)

domagic(via_yaml_load['projects'], 'yaml_load')
domagic(via_yaml_render['projects'], 'yaml_render')
domagic(deepcopy(via_yaml_render['projects']), 'yaml_render+deepcopy')

Output:

yaml_load, <class 'dict'>
a
['a', 'a']
b
['b', 'b']
c
['c', 'c']
d
['d', 'd']
yaml_render, <class 'salt.utils.odict.OrderedDict'>
a
['a', 'd']
b
['b', 'd']
c
['c', 'c']
d
['d', 'd']
yaml_render+deepcopy, <class 'salt.utils.odict.OrderedDict'>
a
['a', 'd']
b
['b', 'd']
c
['c', 'c']
d
['d', 'd']

The output shows a few things:

  • SerializerExtension.load_yaml from salt.utils.jinja provides a regular dictionary and works as expected
  • render from salt.renderers.yaml doesn't work as expected and provides an OrderedDictionary (via salt.utils.odict).
  • trying to de-reference the OrderedDictionary using copy.deepcopy doesn't work, but I'm not sure whether that's a general problem of OrderedDictionary or due to the custom Py27/Py3 compatibility layer in salt.utils.odict

In the end, I want to be able to simply not having to care about implementation internals when processing YAML/Pillar data in a Renderer/Jinja context - I expect Pillar data to be fully de-referenced when working with them.

Versions Report

Also reproduced in 2018.3.3.

Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.5 (default, Apr  1 2018, 05:46:30)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-48-generic
         system: Linux
        version: Ubuntu 18.04 bionic
@dmurphy18
Copy link
Contributor

dmurphy18 commented May 29, 2019

Different recollections internally for supporting yaml variables on Salt:

  1. We don't due to performance issues
  2. Technically it should work, but never promised anyone that it would.

So an internal difference of opinion, and not seeing anything obvious in the code to support it.
Hence it may well be a while before this is resolved and may I suggest a better alternative which is to utilize Jinja variables in the meantime.

@dmurphy18 dmurphy18 added Bug broken, incorrect, or confusing behavior P4 Priority 4 labels May 29, 2019
@dmurphy18 dmurphy18 added this to the Approved milestone May 29, 2019
@eliasp
Copy link
Contributor Author

eliasp commented May 31, 2019

This is not so much about supporting YAML refs/anchors - they're already supported and apparently working.

This is about not leaking the references created by YAML refs/anchors into other contexts, where they're completely unexpected and only prone to create errors like the one described. The fact, that SerializerExtension.load_yaml just works as expected shows it can be done this way.

Data passed as Pillars and/or into Jinja contexts should always be fully de-referenced. The equivalent of dumping the effective YAML structure into a reference-less format like JSON and then parsing/de-serializing it again.

@stale
Copy link

stale bot commented Jan 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 8, 2020
@waynew waynew added the Confirmed Salt engineer has confirmed bug/feature - often including a MCVE label Jan 8, 2020
@stale
Copy link

stale bot commented Jan 8, 2020

Thank you for updating this issue. It is no longer marked as stale.

@stale stale bot removed the stale label Jan 8, 2020
@sagetherage sagetherage removed the P4 Priority 4 label Jun 3, 2020
@sagetherage sagetherage added the severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around label Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

4 participants