Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kustodian opened this issue Aug 31, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@kustodian
Copy link

commented Aug 31, 2017

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.

@kustodian kustodian changed the title Make follow=yes the default options for all file modules All file modules should have a sane default `follow` parameter Aug 31, 2017

@bcoca

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

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

@kustodian

This comment has been minimized.

Copy link
Author

commented Aug 31, 2017

@abadger

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

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

This comment has been minimized.

Copy link
Author

commented Sep 21, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

@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

This comment has been minimized.

Copy link
Author

commented Sep 22, 2017

@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

This comment has been minimized.

Copy link
Author

commented Oct 5, 2017

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.