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

Jinja rendering of state .sls fails if pillar data ends with : #53501

Closed
r-pufky opened this issue Jun 16, 2019 · 5 comments
Closed

Jinja rendering of state .sls fails if pillar data ends with : #53501

r-pufky opened this issue Jun 16, 2019 · 5 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior P3 Priority 3
Milestone

Comments

@r-pufky
Copy link

r-pufky commented Jun 16, 2019

Description of Issue

When rendering a files.replace, the following error occurs if a trailing : is used in the search pattern:

Rendering SLS 'base:salt.master' failed: mapping values are not allowed

Setup

failing_search_replace:
        name: /my/file/with/colon/separated/user/pass
        pattern: 'mysearchpattern:'
        repl: |
          -----BEGIN PGP MESSAGE-----
          <GPG material removed>
          -----END PGP MESSAGE-----
        backup: False
        require:
          - failing_search_replace_file_template_creation

Steps to Reproduce Issue

failing_search_replace:
        name: /my/file/with/colon/separated/user/pass
        pattern: 'mysearchpattern:'
        repl: |
          -----BEGIN PGP MESSAGE-----
          <GPG material removed>
          -----END PGP MESSAGE-----
        backup: False
        require:
          - failing_search_replace_file_template_creation

During runtime this will fail with the above mentioned error.

Looking into the failure message (essentially reading it as salt is trying to interpret values in search pattern in the wrong context), removing the trailing : fixes the issue.

failing_search_replace:
        name: /my/file/with/colon/separated/user/pass
        pattern: 'mysearchpattern'
        repl: |
          -----BEGIN PGP MESSAGE-----
          <GPG material removed>
          -----END PGP MESSAGE-----
        backup: False
        require:
          - failing_search_replace_file_template_creation

Works fine.

Versions Report

salt-minion --versions-report
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.5.3 (default, Sep 27 2018, 17:25:39)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1

System Versions:
           dist: debian 9.9
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-9-amd64
         system: Linux
        version: debian 9.9
salt-master --versions-report
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.5.3 (default, Sep 27 2018, 17:25:39)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1

System Versions:
           dist: debian 9.9
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-9-amd64
         system: Linux
        version: debian 9.9
@xeacott xeacott added this to the Approved milestone Jun 17, 2019
@xeacott xeacott added Bug broken, incorrect, or confusing behavior P3 Priority 3 labels Jun 17, 2019
@xeacott xeacott self-assigned this Jun 24, 2019
@xeacott
Copy link
Contributor

xeacott commented Jun 24, 2019

Hi @r-pufky see if making this change is what you need...

failing_search_replace:
  file.replace:
    name: /my/file/with/colon/separated/user/pass
    pattern: 'mysearchpattern:'
    repl: |
      -----BEGIN PGP MESSAGE-----
      <GPG material removed>
      -----END PGP MESSAGE-----
    backup: False
    require:
      - failing_search_replace_file_template_creation

where I added file.replace:

@r-pufky
Copy link
Author

r-pufky commented Jul 1, 2019

My initial report was a bit unclear. Will go over in detail.

TL;DR: yaml-like parameters need to be quoted, which limits the usage of using pillar data to define dynamic SLS representations. Relates to #1543

Dynamic SLS:
Just defining this so that everyone is on the same page. If you have an SLS file that iterates through pillar data, you can dynamically define multiple states in pillar data itself (many reasons to do this, but that's outside of this conversation):

Some SLS file:

{%- for test, test_option in tests.items() %}
file_replace_{{ test }}:
  file.replace:
    {%- for key, value in test_option.items() %}
    - {{ key }}: {{ value }}
    {%- endfor %}
{%- endfor %}

Pillar

test:
  test1:
    name: /tmp/test1
    pattern: 'testing:'
    repl: |
      -----BEGIN PGP MESSAGE-----
      {CLIPPED}
      -----END PGP MESSAGE-----
    backup: False

Static SLS file with non-pillar data.

This performs as expected. Any yaml-like data must be quoted for processing to work correctly.
static-sls-with-non-pillar-data.zip

Static SLS file with pillar data.

This performs as expected. Any yaml-like data must be quoted for processing to work correctly.
static-sls-with-pillar-data.zip

Dynamic SLS file with pillar data.

(original bug submission) -- breaking this down, this is working as expected. The only real way to fix this is to use jinja to detect the datatype that is being rendered and quote it as needed.

It would be nice for this detection to automatically happen before hitting the renderer by default so that the exception cases (e.g. forcing a type cast when rendering) would require explicit use. For current implementation, this forces explicit casting for standard data, which makes the templating messy for basic usage. Maybe this should be an option that can be enabled on the salt-master.
dynamic-sls-with-pillar-data.zip

Fix:

# Dynamically generated file.replace using pillar data.
{%- for test, test_option in tests.items() %}
file_replace_{{ test }}:
  file.replace:
    {%- for key, value in test_option.items() %}
      {%- if value is string %}
    - {{ key }}: '{{ value }}'
      {%- else %}
    - {{ key }}: {{ value }}
      {%- endif %}
    {%- endfor %}
{%- endfor -%}

@r-pufky
Copy link
Author

r-pufky commented Jul 1, 2019

I should note that a workaround is to store the entire file contents in pillar (in this case, GPG encrypted) and just write that to the file. However, this isn't preferred because it obfuscates any non-secret material in the file that could be configuration options, etc.

@waynew waynew changed the title file.replace fails if pattern contains trailing : Jinja rendering of state .sls fails if pillar data ends with : Jul 2, 2019
@xeacott
Copy link
Contributor

xeacott commented Jul 2, 2019

Thanks for providing so much detail on this! After investigating and working through this with @waynew
for this case, adding | tojson might be what you want to use. It should provide you with the expected behavior, and globally we may move over to requiring | tojson in more places or a different approach. However, adding that filter when iterating over pillar data should do what you expect.

@r-pufky
Copy link
Author

r-pufky commented Jul 4, 2019

Thanks for the update; didn't even think about that.

I've confirm that tojson works as expected.

@r-pufky r-pufky closed this as completed Jul 4, 2019
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 P3 Priority 3
Projects
None yet
Development

No branches or pull requests

2 participants