Salt: blockreplace marker_end isn't applied with newline

[PhilippeAB]

Description of Issue/Question

blockreplace marker_end isn't applied with newline
like #33686

Setup

Using apache-formula there is missing newline in the resulting file before the end mark
example

# START managed zone -DO-NOT-EDIT-
ServerSignature off
TraceEnable off
FileETag None
ServerTokens Prod# END managed zone --

{% if salt['file.file_exists' ]('/etc/apache2/conf-available/security.conf') %}
apache_security-block:
  file.blockreplace:
    - name: /etc/apache2/conf-available/security.conf
    - marker_start: "# START managed zone -DO-NOT-EDIT-"
    - marker_end: "# END managed zone --"
    - append_if_not_found: True
    - show_changes: True
    - require:
      - pkg: apache
    - watch_in:
      - module: apache-reload

{% for option, value in salt['pillar.get']('apache:security', {}).items() %}
apache_manage-security-{{ option }}:
  file.accumulated:
    - filename: /etc/apache2/conf-available/security.conf
    - name: apache_manage-security-add-{{ option }}
    - text: "{{ option }} {{ value }}"
    - require_in:
      - file: apache_security-block
{% endfor %}

Steps to Reproduce Issue

Configure security parameter in apache-formula

Versions Report

Salt Version:
Salt: 2017.7.2

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.4.2
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.8
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: 1.0.3
msgpack-pure: Not Installed
msgpack-python: 0.4.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.12 (default, Nov 19 2016, 06:48:10)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 15.2.0
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.2.1
ZMQ: 4.1.4

System Versions:
dist: Ubuntu 16.04 xenial
locale: UTF-8
machine: x86_64
release: 4.4.0-89-generic
system: Linux
version: Ubuntu 16.04 xenial

[Ch3LL]

Looks like the issue you tagged was resolved with a new argument here: https://github.com/saltstack/salt/pull/36273

append_newline if you set that do you see your newline?

[PhilippeAB]

set append_newline: True in the blocreplace give me a warning

Warnings: 'append_newline' is an invalid keyword argument for
'file.blockreplace'.

There is nothing in the doc about append_newline for file.blocreplace

[PhilippeAB]

Hello
Do you need more information ?
Thanks

[Ch3LL]

Ahh sorry looks like that argument was only added to the execution module, looks like we need to add this argument to the state module as well. Want to take a stab at it?

[PhilippeAB]

Maybee I'm missing something but I don't see why we need this "append_newline"
AFAICS on bug report everyone want the marker alone one the line, so why can't we just do this with no extra newline ?
Is there some use case where we want the marker appended at the end of the content last line ?
Anyway there is a bug because without 'append_newline' the last line of content is duplicated.
So the marker is not correctly handled.

[Ch3LL]

From my understanding of the issue if you look at this comment https://github.com/saltstack/salt/issues/33686#issuecomment-245127773 we did not change the behavior to add a newline due to another fix he is referring to which is this fix:

https://github.com/saltstack/salt/pull/33049

Which fixed and issue here: https://github.com/saltstack/salt/issues/12422 where it caused issues when using multiline content.

but @techhat can correct me if i'm wrong about that analysis as to why we would want this to be an argument versus default behavior.

[PhilippeAB]

both behavior are wrong to me as the result are wrong
Duplicate line (hence config crash) vs modified config line (config false)

[Ch3LL]

Can i get anyone form @saltstack/team-core 's opinion here to see if we want to change the default behavior here or not?

[Zaunei]

Is there anything new on the issue? This bug is pretty annoying.

[thatch45]

Yes, this is annoying, I think it just got lost in the fray. We should look into this, and it should be an easy fix

[terminalmage]

As noted above, the append_newline argument was added to the execution module but not the state. This value simply needs to be passed through to the execution module.

But I think more than _just_ that needs to be done. For instance, if we do not know precisely whether or not a newline will be present in the content, then it's hard to know whether or not to pass True or False for this value. Therefore, I propose the following behavior for append_newline:

• If True, newline will be appended to content
• If False, newline will _not_ be appended to content.
• If None, newline will be appended to content _only if content does not already end in a newline_.

The first two are existing behavior for this argument in the file.blockreplace remote-execution function, so the only change here would be that there would be a third option.

To me, it seems like once we add append_newline to the file.blockreplace state, it should default to None. This would mean that the default behavior of the state would be to ensure that there is a newline before the end marker. The question would be what do we do to the remote-execution function? Right now, if append_newline is not passed, it is assumed to be False (i.e. no newline will be appended). I think that we should keep the same default behavior until the next feature release (i.e. Fluorine, later this year) at minimum.

So, to summarize:

1. Add append_newline argument to file.blockreplace state in 2017.7 bugfix branch (targeted for 2017.7.5, 2018.3.1 releases). Default value of None.
2. Also in 2017.7 bugfix branch, modify file.blockreplace remote-execution function to conditionally append the newline if content does not end in a newline, when append_newline is None. Leave the default value of append_newline as False
3. In develop, modify default value of append_newline to None. This change would take effect in the Fluorine release.

I am also open to just keeping the default value in the remote execution function set to False indefinitely. It would be less disruptive to those who invoke the remote-execution function manually, and there must be people that do this because it was added to the remote-execution function and not the state. Why would this be done if there weren't people using it as a standalone function? Leaving the default value as-is would leave an incongruity between the state and remote-execution function, but it may be worth it to keep disruption to a minimum.

Note: In the case of minions running salt <= 2017.7.4 or 2018.3.1, passing None to file.blockreplace would simply take the behavior of False, which is a sane fallback IMO as it preserves the prior behavior.

@thatch45 @Ch3LL @PhilippeAB @Zaunei what are your thoughts?

[rallytime]

I think that makes a lot of sense and I agree with your assessment. The only thing I would add is to make sure there will be the appropriate documentation/warnings that the default behavior will change in Fluorine.

[terminalmage]

I've opened https://github.com/saltstack/salt/pull/46430 to fix this.

[Hybrid512]

Salt Version:
Salt: 2018.3.0

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.5.3
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
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: 2.7.13 (default, Nov 24 2017, 17:33:09)
python-gnupg: Not Installed
PyYAML: 3.12
PyZMQ: 16.0.2
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.4.3
ZMQ: 4.2.1

System Versions:
dist: debian 9.4
locale: UTF-8
machine: x86_64
release: 4.9.0-6-amd64
system: Linux
version: debian 9.4

I still get this warning when adding 'append_newline' to my state files :

Warnings: 'append_newline' is an invalid keyword argument for
'file.blockreplace'. If you were trying to pass additional data to
be used in a template context, please populate 'context' with
'key: value' pairs. Your approach will work until Salt Fluorine is
out. Please update your state files.

[terminalmage]

Yes, that is because this fix came in after the code freeze for 2018.3.0 and will be in 2018.3.1.