All file modules should have a sane default `follow` parameter

[kustodian]

Proposal: All file modules should have a sane default follow parameter

Author: Strahinja Kustudic @kustodian

Date: 2017/08/31

• Status: New
• Proposal type: file modules defaults
• Targeted Release: 2.5
• Associated PR: N/A
• Estimated time to implement: 2 days

Motivation

Currently file modules have mixed "feelings" about weather should they follow symlinks, or not. Here is a table how it is currently implemented:

Module	Follow Symlinks Default
acl	yes
archive	N/A
assemble	N/A
blockinfile	no
copy	no
fetch	N/A
file	no
find	no
ini_file	N/A
iso_extract	N/A
lineinfile	yes (hard coded)
patch	N/A
replace	no
stat	no
synchronize	N/A
tempfile	N/A
template	N/A
unarchive	N/A
xattr	yes

N/A means follow parameter doesn't exist and I didn't check in the code how it works.

It would be great if all modules used the same and sane default value, because like this it doesn't make any sense, later on for new modules that default value could be forced.

Also I would recommend to make the default value depend on how the Unix commands which these modules emulate work. In most cases this should be to follow symlinks, e.g. if I'm editing a symlink to a file (e.g. vim linktofile), it will actually edit a file, it won't edit a symlink. On the other hand cp file1 linktofile would actually replace a link with file1, but cp file1 linktodir would copy the file1 into the directory the link points to.

Problems

Some of the modules don't work as you would expect them to work and the work inconsistently:

• If you want to edit a file which is a symlink e.g. on RHEL/CentOS if you edit with blockinfile /etc/rc.local which actually is a symlink to /etc/rc.d/rc.local it will actually replace the symlink with the file it generates, and basically break the /etc/rc.local functionality.
• This is also dangerous, because it's not what the user would expect. If I'm telling the module to edit a symlink to a file, I expect it to edit the destination file and not remove a symlink and create a file instead of it.
• It's not consistent across the modules. Some modules have a default yes, some no, for some of them you don't even know until you try it out.

Solution proposal

• I would suggest that we revise all file modules and set a sane default value (or if the follow parameter doesn't exist, hard code it, or just in case implement it).
• For all modules that edit files (lineinfile, replace, blockinfile, patch, template, assemble, ini_file) the sane default value is yes. If for some reason you don't want to follow symlinks, you should set it manually to no because you are doing something out of the ordinary.
• For other modules we should check what is default in in Unix and use those. For example find and stat don't follow symlinks by default, so those modules shouldn't as well.

This shouldn't make a big impact to the users, but it can still be announced as a breaking changed, even though most of the users shouldn't notice any issues.

UPDATE 05. Oct 2017.

After the discussion on the community meeting, here are all the modules categorized by their type, so that we know what needs to changed on each module.

Replacers

• Replace a file on the remote system with a new one.
• default follow=no

Module	follow (current)
archive	no (hard-coded)
assemble	no (hard-coded)
copy	no
fetch (flat=yes)	yes (hard-coded)
template	no

Bulk Replacers

• Anything that can target a directory (therefore the directory can be a symlink that should have a default for follow).
• For directory as dest -> default folloy=yes
• For a file as dest -> default follow=no

Module	follow (current)
copy (recursive)	yes (hard-coded)
fetch (flat=no)	yes (hard-coded)
synchronize	yes (hard-coded)
unarchive	yes (hard-coded)
iso_extract	N/A

Modifiers

• Modify attributes of the file, not the file's contents
• default follow=yes because most attributes on most systems can't be applied to symlinks, only to the files they point to

Module	follow (current)
acl	yes
file	no
xattr	yes

Editors

• take a file on the remote system and modify it
• hardcoded yes (no follow option as it wouldn't make sense) because they cannot function without a real file with contents on the remote system

Module	follow (current)
blockinfile	no
ini_file	yes (hard-coded)
lineinfile	yes (hard-coded)
patch	fails on symlink -> this is how it works with command patch, so no changes
replace	no

Stat/find

• This is off by itself. It is informational only and you really are asking for information about the specific path
• default follow=no

Module	follow (current)
stat	no
find	no

Other

• This is only tempfile module which generates a new temp file, so follow is not important for it.

Module	Follow Symlinks Default
tempfile	N/A

Summary

This means only fetch (flat=yes), file, blockinfile, patch and replace need to be updated.

[bcoca]

stat might be the exception we want to keep as is, just because you are trying to identify what a path is.

[kustodian]

Yeah, I mentioned both stats and find, but we should check with other modules as well.

[abadger]

Talked about at this week'd Core meeting and had the following feedback:

There's really several categories of file module which seem like they should have different default values for follow (names negotiable):

• replacers (copy, template, assemble ...)
  • replace a file on the remote system with a new one.
  • default follow=no
• bulk replacers (syncronize/unarchive/copy recursive)
  • anything that can target a directory (therefore the directory can be a symlink that should have a default for follow)
  • default follow=no
• modifiers (file, xattr/acl/etc)
  • Modify attributes of the file, not the file's contents
  • default follow=yes because most attributes on most systems can't be applied to symlinks, only to the files they point to
• editors (line/block/patch/...)
  • take a file on the remote system and modify it
  • hardcoded yes (no follow option as it wouldn't make sense) because they cannot function without a real file with contents on the remote system
• stat
  • This is off by itself. It is informational only and you really are asking for information about the specific path
  • default follow=no

Action plan:

• Add the categories to the proposal
• Modify the list of file modules in the proposal to place the modules into groups
• Add creating or verifying that there are good integration tests for symlinks for each of the modules that will be changed (and possibly unit tests to cover race scenarios easier).

[kustodian]

Sorry for not being able to join you guys during the meeting, but I wasn't near a workstation at the time.

I agree with all, I'm only not sure if replacers should be follow=no, but when I think about it if I choose on a unix system copy src_file symlink it will actually replace the symlink with the src file, so it sounds good that replacers should not follow as well.

Very happy about the decided changes and thanks for being fast and responsive about this (important) issue. I guess this change will be expected for 2.5?

[abadger]

@kustodian I forgot to ask, could you make the updates to the proposal?

We were just talking today about how to allocate manpower to work on implementing proposals as a general problem. When one of the Core devs submits a proposal it often means they are willing to work on it. When a community member submits a proposal because they want some guidance on how to implement a PR then that contributor usually is going to work on it once the proposal is approved. It's when a proposal is raised by someone who sees a problem but isn't going to work on it themselves that we have an issue. Step 1 is approving the proposal so everyone can see what the plan is. But that still leaves Step 2 of finding someone with the time to actually make the changes to the modules....

[kustodian]

@abadger I will certainly update the proposal, that is not a problem. If it's approved I should be able to work on it as well, because the changes to the modules should be pretty straight forward, even though there is a lot of them. My biggest concern would be tests, not sure how much time those would take, last time I need to modify a unit test for a connection plugin wasn't a good experience. I noticed that you wrote integration tests, which means playbooks, that should be fine.

Now about the update, what exactly did you mean by:

Add the categories to the proposal

Did you mean adding more categories if not all modules fit into these ones?

[kustodian]

@abadger I sorted all the modules into categories, tried all of them to see how they work with following symlinks and updated the proposal.

As far as I can see only 5 modules need to be updated fetch (flat=yes), file, blockinfile, patch and replace.