contents_newline in file.managed is effectively ignored

[tj90241]

Description of Issue

The file.managed state is said to have a contents_newline argument which can optionally be specified (as False) to prevent a newline from being appended to the end of the managed file when used in conjunction with contents, contents_grains, or contents_pillars. However, in 2018.3.4 and 2019.2, a newline is currently always added -- looks to be an unintended side-effect of #51252.

Namely this logic block:

            for part in validated_contents:
                for line in part.splitlines():
                    contents += line.rstrip('\n').rstrip('\r') + os.linesep
            if contents_newline and not contents.endswith(os.linesep):
                contents += os.linesep

The problem is that os.linesep is always applied to the last line as a result of the for loop -- the if statement does not work as intended because the line will always end in os.linesep already (unless contents is []).

Setup

manage-rabbitmq-erlang-cookie:
  file.managed:
    - name: /var/lib/rabbitmq/.erlang.cookie
    - contents:
      - SOME_EXAMPLE_COOKIE
    - contents_newline: False
    - user: rabbitmq
    - group: rabbitmq
    - mode: 0400
    - watch_in:
      - service: rabbitmq-server

(removing the watch_in requisite of course!)

Steps to Reproduce Issue

Apply the above state - the managed file should not have a newline at the end... but it will.

Versions Report

Salt Version:
           Salt: 2018.3.4
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          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.7.3 (default, Apr  3 2019, 05:39:12)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: debian 10.0 
         locale: UTF-8
        machine: x86_64
        release: 4.19.0-5-amd64
         system: Linux
        version: debian 10.0 

The bug also seems to be present in 2019.2.

[SEJeff]

Confirmed. We ran into this exact same bug when upgrading to the latest salt.

[dmurphy18]

Closing this since duplicate. The xix will be delivered in the next point release of Salt 2019.2.1 which is near completing final QA, The original issue and fix can be tracked via #51828

[tj90241]

@dmurphy18 , with all due respect I am quite certain this is not a duplicate of that issue. I was looking at the code in the develop branch of SaltStack prior to filing this issue and the bug appears there still.

Here is the line number in the develop branch today:
https://github.com/saltstack/salt/blob/develop/salt/states/file.py#L2715

[tj90241]

More specifically, that issue seems to address the issue with the user wanting to retain newlines, while this issue deals with the user not wanting to add in a trailing newline if one is not already present. Some services require that there be no newline at the end of the config, cookie, etc. file.

[SEJeff]

@dmurphy18 please re-open this as this is not a dupe. I've confirmed this is still a bug in the latest. I've hacked on salt long enough to know this is not fixed.

This is a separate bug from #51828.

[tj90241]

Good deal - thanks for confirming! 👍

I should be able to get in touch with one of the devs - I'll see if I can poke someone on the side to get a second look before re-reporting.

[dmurphy18]

Reopened, apologies, thought it was the same issue from a first reading.

[SEJeff]

Much appreciated gents. It is hard to maintain such a large project.

[xeacott]

So for this block of code...

            contents = ''
            for part in validated_contents:
                for line in part.splitlines():
                    contents += line.rstrip('\n').rstrip('\r') + os.linesep
            if contents_newline and not contents.endswith(os.linesep):
                contents += os.linesep

@tj90241 like you said, the for loop will always ensure the last line of text, in this case SOME_EXAMPLE_COOKIE evaluate to SOME_EXAMPLE_COOKIE\n, where it's the last line in the file because of os.linesep.

So the behavior of contents_newline: False should specify that the last line in the file should not contain a \n, or more simply in this case, SOME_EXAMPLE_COOKIE should be the final line, yes?

[tj90241]

bingo.

(as long as the contents themselves do not end in a newline already; it should not remove a newline if one is already present, at least per the documentation based on my understanding)

[xeacott]

Cool, I wanted first to make sure we're on the same page about expected behavior. And, although the documentation isn't fully incorrect, I'm going to make it more exhaustive and then address this issue. Thanks for responding @tj90241

[xeacott]

Alright I think this covers all the cases, see if this change works for you guys.

[tj90241]

lgtm!

[defanator]

JFTR, faced this one while debugging a case similar to #31709 - the proposed PR seems to fix an inconsistency.