[BUG] file.keyvalue doesn't preserve indentation

Description file.keyvalue doesn't preserve indentation of key:value pairs in configuration file.

Setup

Create the configuration file to be modified on the minion:

# cat << EOF > /tmp/test.txt
<VirtualHost *:80>
  ServerName YOUR_SERVER_FQDN
  ServerSignature Off

  RewriteEngine on
  RewriteCond %{HTTPS} !=on
  RewriteRule .* https://%{SERVER_NAME}%{REQUEST_URI} [NE,R,L]
</VirtualHost>
EOF

Create the following state:

file_keyvalue_test:
file.keyvalue:
- name: /tmp/test.txt
- key: ServerName
- value: host.example.com
- separator: ' '
- key_ignore_case: False
- append_if_not_found: False

Steps to Reproduce the behavior

Apply the state:

# salt host.example.com state.apply
host.example.com:
----------
----------
          ID: file_keyvalue_test
    Function: file.keyvalue
        Name: /tmp/test.txt
      Result: True
     Comment: Changed 1 lines
     Started: 22:46:00.444384
    Duration: 6.222 ms
     Changes:   
              ----------
              diff:
                  -   ServerName YOUR_SERVER_FQDN
                  + ServerName host.example.com

Summary for host.example.com
-------------
Succeeded: 1 (changed=1)
Failed:     0

View the file on minion:

# cat /tmp/test.txt 
<VirtualHost *:80>
ServerName host.example.com
ServerSignature Off

RewriteEngine on
RewriteCond %{HTTPS} !=on
RewriteRule .* https://%{SERVER_NAME}%{REQUEST_URI} [NE,R,L]
</VirtualHost>

Expected behavior file.keyvalue should preserve indentation of key:value pairs:

# cat /tmp/test.txt 
<VirtualHost *:80>
  ServerName host.example.com
  ServerSignature Off

  RewriteEngine on
  RewriteCond %{HTTPS} !=on
  RewriteRule .* https://%{SERVER_NAME}%{REQUEST_URI} [NE,R,L]
</VirtualHost>

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ``` # salt --versions-report Salt Version: Salt: 3001.1 Dependency Versions: cffi: 1.11.5 cherrypy: unknown dateutil: 2.6.1 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.10.1 libgit2: Not Installed M2Crypto: 0.35.2 Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 0.6.2 mysql-python: Not Installed pycparser: 2.14 pycrypto: Not Installed pycryptodome: Not Installed pygit2: Not Installed Python: 3.6.8 (default, Apr 16 2020, 01:36:27) python-gnupg: Not Installed PyYAML: 3.12 PyZMQ: 19.0.0 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.2 System Versions: dist: centos 8 Core locale: UTF-8 machine: x86_64 release: 4.18.0-193.19.1.el8_2.x86_64 system: Linux version: CentOS Linux 8 Core ```

TL;DR at the bottom

A little back-story:

When I initially wrote this function, one of the reasons was that file.replace would always do a replace in cases like this:

/etc/ssh/sshd_config:
  file.replace:
    - pattern: '^PermitRootLogin:.*'
    - repl: 'PermitRootLogin: 'without-password'

The engineers I worked with liked doing a run with test=True to see if anything changed, and the above was causing false positives. So I had to write things with a negative lookahead like:

/etc/ssh/sshd_config:
  file.replace:
    - pattern: '^PermitRootLogin:\s*(?!without-password$)\S+$'
    - repl: 'PermitRootLogin: without-password'

And of course I wanted everything in a Jinja templated state so I could do all the settings in one loop. All settings in one place instead of a bunch of separate states is better for a good overview:

{% for key, value in [('PermitRootLogin','without-password'), ('DisableForwarding','yes')] %}
update-sshd-setting-{{key}}:
  file.replace:
    - name: /etc/ssh/sshd_config
    - pattern: '{{ "^\s*(%s)\s*=\s*((?!%s$)\S+)$"|format(key, value) }}'
    - repl: '{{key}}={{value}}'
{% endfor %}

And even then it might still not work as expected, because I believe everything had just been upgraded to Hydrogen at that point, and append_if_not_found wasn't in there yet. file.replace doesn't have a nice option to uncomment lines somewhere in the file either.

Techical details:

This reason the leading spaces are stripped here is two-fold.

First

The fact that separator: ' ' instructs file.keyvalue to look for a space as separator. This technically exists at the beginning of the line. The reason this is not immediately apparent is because of the second cause.

Second

The fact that the uncomment defaults to None, and str.lstrip(None) will still get rid of leading whitespace: https://github.com/saltstack/salt/blob/7e357ae3c4da6cf54a61e9ba9935d46397997250/salt/states/file.py#L5394

Adding uncomment: '' will fix the stripping of whitespace at the start of the line, but when it's not stripped it is instead recognised as a separator. If the separator was a : then this combination would work and keep the two-space indentation:

<VirtualHost *:80>
  ServerName:YOUR_SERVER_FQDN
  ServerSignature:Off
</VirtualHost>

file_keyvalue_test:
  file.keyvalue:
    - name: /tmp/test.txt
    - key: '  ServerName'
    - value: host.example.com
    - separator: ':'
    - uncomment: ''

TL;DR:

I believe the file.replace state to be much nicer that it once was, and the use-case mentioned above seems better served by it rather than file.keyvalue (which was really intended for long config-files where ideally the key only exists once and may or may not be commented out).

@ggiesen: The desired behaviour of setting the ServerName to the correct value while leaving the leading whitespace intact can be achieved by the following state:

/tmp/test.txt:
  file.replace:
    - pattern: '(ServerName).*'
    - repl: '\1 host.example.com'

@sagetherage: Not sure if anyone has strong feelings about this. Technically the default value for uncomment can be changed from None to '' (empty string). However that may unexpectedly change the current behaviour for states depending on this to be stripped by default.

saltstack / salt