Repository: ansible/ansible
Pull request: Preserving line endings in blockinfile module
##### SUMMARY
Preserve line endings in blockinfile module.

Fixes #64966

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
blockinfile

##### ADDITIONAL INFORMATION
I'm not sure if this is the desired way to fix this ... it looks kinda "hacky", but OTOH allows us to remove other hacks (e.g. fixing new lines at the end of files).

In #65677 it was suggested the changed line endings might be related to missing `newline=''` parameter when calling `open()`. But trying to add this you get the following error: `ValueError: binary mode doesn't take a newline argument`. Instead it looks like `splitlines()` is the actual culprit ... at least in this case. :neutral_face: 


Problem statement:
Ansible blockinfile ^M bug!?


### SUMMARY
In SuSe SLES there is a file: /etc/vimrc. It contains ^M characters, like in line:  101 containing:
    map! <ESC>OM  ^M
When running module blockinfile on this file it removes the ^M characters.
The blockinfile just needs to adds some lines on the end of the file. Even when the lines are already there it removes the ^M characters.

It looks like that it reads the file and cannot handle the ^M characters and writes the buffer without the characters to the file.

##### ISSUE TYPE
- Module blockinfile

##### COMPONENT NAME
blockinfile

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
ansible 2.4.2.0
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /bin/ansible
  python version = 2.7.5 (default, Aug  7 2019, 00:51:29) [GCC 4.8.5 20150623 (Red Hat 4.8.5-39)]


```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below

```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->


##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
```yaml

```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->


##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/files/blockinfile.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/files/blockinfile.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @yaegashi
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Is this still an issue with supported versions of Ansible (>= 2.7.0)?

needs_info
I'm going to hazard a guess that this is still an issue, due to `.splitlines()` in python translating windows newlines into unix newlines, and stripping them.

Simply using `.readlines()` will also translate.

It looks like to get around it, you need to use `io.open` with `newlines=''` and `.readlines()` to prevent universal newline translation.  But this will require a rewrite in that we would need to change the assumption that the lines don't contain the newline.

We then run into a problem with us using unix newlines to be inserted, and what happens if there are multiple newline types in a single file?
@ansible-sam This issue is waiting for your response. Please respond or the issue will be closed.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: needs_info_base --->
@ansible-sam You have not responded to information requests in this issue so we will assume it no longer affects you. If you are still interested in this, please create a new issue with the requested information.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: needs_info_base --->

Commit message: Fix sanity checks complaining about test fixtures with mixed line endings

Patch to review:
diff --git a/changelogs/fragments/66461-blockinfile_preserve_line_endings.yaml b/changelogs/fragments/66461-blockinfile_preserve_line_endings.yaml
new file mode 100644
index 00000000000000..c3fdae8e6b2548
--- /dev/null
+++ b/changelogs/fragments/66461-blockinfile_preserve_line_endings.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - blockingfile - preserve line endings on update (https://github.com/ansible/ansible/issues/64966)
diff --git a/lib/ansible/modules/blockinfile.py b/lib/ansible/modules/blockinfile.py
index 2db20fe5f792d7..f11d7f8eeb9fd6 100644
--- a/lib/ansible/modules/blockinfile.py
+++ b/lib/ansible/modules/blockinfile.py
@@ -237,7 +237,7 @@ def main():
         f = open(path, 'rb')
         original = f.read()
         f.close()
-        lines = original.splitlines()
+        lines = original.splitlines(True)
 
     diff = {'before': '',
             'after': '',
@@ -266,13 +266,13 @@ def main():
     else:
         insertre = None
 
-    marker0 = re.sub(b(r'{mark}'), b(params['marker_begin']), marker)
-    marker1 = re.sub(b(r'{mark}'), b(params['marker_end']), marker)
+    marker0 = re.sub(b(r'{mark}'), b(params['marker_begin']), marker) + b(os.linesep)
+    marker1 = re.sub(b(r'{mark}'), b(params['marker_end']), marker) + b(os.linesep)
     if present and block:
         # Escape sequences like '\n' need to be handled in Ansible 1.x
         if module.ansible_version.startswith('1.'):
             block = re.sub('', block, '')
-        blocklines = [marker0] + block.splitlines() + [marker1]
+        blocklines = [marker0] + block.splitlines(True) + [marker1]
     else:
         blocklines = []
 
@@ -306,9 +306,7 @@ def main():
     lines[n0:n0] = blocklines
 
     if lines:
-        result = b('\n').join(lines)
-        if original is None or original.endswith(b('\n')):
-            result += b('\n')
+        result = b''.join(lines)
     else:
         result = b''
 
diff --git a/test/integration/targets/blockinfile/tasks/main.yml b/test/integration/targets/blockinfile/tasks/main.yml
index 86e066d944eaa5..5accf5b5875c5b 100644
--- a/test/integration/targets/blockinfile/tasks/main.yml
+++ b/test/integration/targets/blockinfile/tasks/main.yml
@@ -47,7 +47,7 @@
   register: blockinfile_test0
 
 - name: check content
-  shell: 'grep -e "Match User ansible-agent" -e "PasswordAuthentication no" {{ output_dir_test }}/sshd_config'
+  shell: 'grep -c -e "Match User ansible-agent" -e "PasswordAuthentication no" {{ output_dir_test }}/sshd_config'
   register: blockinfile_test0_grep
 
 - debug:
@@ -65,7 +65,7 @@
       - 'blockinfile_test0.msg is defined'
       - 'blockinfile_test0.changed'
       - 'blockinfile_test0.msg == "Block inserted"'
-      - 'blockinfile_test0_grep.stdout_lines | length == 2'
+      - 'blockinfile_test0_grep.stdout == "2"'
 
 - name: check idemptotence
   blockinfile:
@@ -112,3 +112,28 @@
       - empty_test_2 is changed
       - "'Block removed' in empty_test_2.msg"
       - empty_test_stat.stat.size == 0
+
+- name: create line_endings_test.txt in the test dir
+  copy:
+    dest: "{{ output_dir_test }}/line_endings_test.txt"
+    # generating the content like this instead of copying a fixture file
+    # prevents sanity checks from warning about mixed line endings
+    content: "unix\nunix\nunix\n\ndos\r\ndos\r\ndos\r\n\nunix\nunix\n# BEGIN ANSIBLE MANAGED BLOCK\ndos\r\n# END ANSIBLE MANAGED BLOCK\nunix\nunix\nunix\nunix\n"
+
+- name: insert/update "dos" configuration block in line_endings_test.txt
+  blockinfile:
+    path: "{{ output_dir_test }}/line_endings_test.txt"
+    block: "dos\r\ndos\r\ndos\r\n"
+  register: blockinfile_test2
+
+- name: check content
+  # using the more precise `grep -Pc "^dos\\r$" ...` fails on BSD/macOS
+  shell: 'grep -c "^dos.$" {{ output_dir_test }}/line_endings_test.txt'
+  register: blockinfile_test2_grep
+
+- name: validate line_endings_test.txt results
+  assert:
+    that:
+      - 'blockinfile_test2 is changed'
+      - 'blockinfile_test2.msg == "Block inserted"'
+      - 'blockinfile_test2_grep.stdout == "6"'


Code review comments:
diff_hunk: @@ -0,0 +1,2 @@
+bugfixes:
+  - blockingfile - preserve line endings on update (https://github.com/ansible/ansible/issues/64966)
path: changelogs/fragments/66461-blockinfile_preserve_line_endings.yaml
text: @user1:
```suggestion
  - blockinfile - preserve line endings on update (https://github.com/ansible/ansible/issues/64966)
```

@author:
embarrassing :sweat_smile: 

@user1:
I do stuff like that all the time. 😊