Salt: multiple file.line problems

[Jille]

Hi,

file.line has a bunch of problems, this is what I found with a quick search: #41474, #40953, #40821, #39849, #38670, #39218, #38171, #37360, #32412, #27295.

Also with a quick look at the code it looks like it makes some dangerous assumptions, is in urgent need of a refactor and has surprising semantics.

What are our options here? If I can find the time I might be willing do refactor the entire thing into something sensible, but that'll surely break some edge case behavior someone somewhere was depending on.

Should we just deprecate it? file.replace seems a reasonable alternative and is probably what users want anyway.

I just did some fixes (https://github.com/Jille/salt/tree/file-line) for the problem I had, but I think it could use a bigger hammer.

[gtmanfred]

@isbm could you take a look at this?

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

I believe that file.line was implemented to mimic the way puppet does it.

Thanks,
Daniel

[gtmanfred]

@isbm when you have time could you comment here?

Thanks,
Daniel

[reinierhamstra]

Okay so after the closing of #42322 just coming back as requested to provide possible requirements for file.line now that there are discussions on reworking the function.

The 'ensure' option should to my mind do what is stated: 'IF line DOES NOT exist, it will be ADDED', either BEFORE or AFTER the provided positional marker, instead of replacing lines using what currently seems to be non-exact matching.

To determine IF the line exists we should be able to search for and match exactly the full 'content' line, before or after the provided marker, unless the 'match' option is used to allow for what would probably end up as a more loose match.

Personally I think just matching the line directly before or after the positional marker would be too tight a match, since some use cases have 'saltstack dynamically managed blocks' inside otherwise static files, so the content of those locations is not always known beforehand. It might therefore be an idea to provide both a Start AND End marker to file.line, so it provides scoping at the file level. This would increase the accuracy of manipulation and decrease the number of 'false positives' while matching inside files. Also this way not the entire file would have to be parsed by definition, which would probably cut down possible difficulties in the number of characters that need to be escaped/dealt with?

Additionally I can also see a clear use case in adding Start and End marker functionality to the 'replace' function, in combination with the 'match' function, which provides scoping on the line level. This would allow dynamically changing parameters inside a firewall rule or proxy configuration to be replaced, but have the rule itself retain it's positioning inside the configuration or rulebase, which is crucially important in many cases.

A similar case could be made for the 'delete' function and the scoping provided by Start and End markers. It is quite possible to have the same configuration element such as a firewall rule that occurs in more than one place, yet it might only need to be removed from a specific part of the configuration and not the other.

Anyways, my two cents so far. Given some love from the team and the community the 'ensure', 'insert', 'delete' and 'replace' options could really take the game up to another level. Looking forward to seeing how this develops and what other people might like to see.

[lordcirth]

Some possibly interesting facts about file.py and file.line():

file.py is the single largest state, over twice as big as the next, pkg.py.
$ wc -l file.py pkg.py
6423 file.py
3136 pkg.py
Source:
$ wc -l * | sort -n

file.py has the second most public functions, 25. (bigip.py has 29, pkg.py has 12)

Source:
$ for f in *.py; do echo -n "$f: " >> ~/funcs ; grep '^def [^_]' $f | wc -l >> ~/funcs; done
$ sort -n -k 2,2 ~/funcs -o ~/funcs

file.py has 134 linting errors, one of which is:
C: 1, 0: Too many lines in module (6423/3000) (too-many-lines)

(anecdotal:) I have several times tried to find bugs in file.py and given up because it was too big/intimidating. I am new to Python, however. I did once manage to fix a minor bug in it, which was actually in an execution module it uses.

Now sure, "managing files" is a core feature and a broad category; you'd expect it to be large. But it's a clear outlier in it's complexity. In my (subjective & novice) opinion we should look into breaking some bits out of file.py into new states. Naturally this would be painful and make backwards compatibility hard. Alternatively, maybe refactoring some code down into modules/file.py might help.

file.line() is 171 lines including docstrings; which is longer than 104 full state files.
Source:
$ salt/salt/states# wc -l *.py | sort -n | head -104.

file.line() has 4 modes which do related but different actions. Anecdotally, I find these modes unclear and confusing.

Perhaps we should consider making states/line.py, (or similar; text.py?) and then we can have line.delete, etc instead of modes. Perhaps a few other file.* functions might belong there as well. I think I have seen this suggested elsewhere but I can't find it. If someone knows if this is already discussed somewhere, please let me know.

[isbm]

Anecdotally, I find these modes unclear and confusing.

I wrote it. Three or four years ago. Can you elaborate what exactly is confusing? I agree that this part needs a better unit testing, but please be constructive, so I can go and just fix it. Apparently, we have exactly a bugfixing time right now for exactly these kind of issues.

Thanks.

[isbm]

@gtmanfred Oops. Was quite a while... :laughing: OK. I am going to bite this after a week (we start bugfixing sprint here).

[lordcirth]

The confusion results from the documentation being vague, and/or me being dense. Docs are harder to write than code, IMHO. I will write out my thought process as I read the docs.

'ensure': If line does not exist, it will be added.

Seems similar to file.append(), except that with the other options the user gets to specify where. What if I want to add it at - location: start or end? location just says: 'Note this option is only used when mode=insert is specified'. No mention of why this couldn't work with ensure like one would think, or pointing the user to append() or prepend() to avoid duplication, which is presumably why this wasn't implemented. Altogether there's a lot of apparent overlap here, both in docs and code, that I don't really understand.

replace: If line already exists, it will be replaced.

Shouldn't it be 'replaced' seeing as it's a state, and not an execution module? Also, isn't there also 'file.replace()' (same naming problem)? What's the difference between the two? Why do we have both?

delete: Delete the line, once found.

Perfect, simple and very useful. I don't see a duplicate anywhere.

insert: Insert a line.

'insert' is a decidedly imperative/ not idempotent name. Is this going to insert the line every time and report Changed? (Presumably not, but that's what the docs imply). It doesn't say "if it's not in the file" or "if it's not at that location". Also, doesn't 'ensure' add lines? What does this do that 'ensure' doesn't?
The 'mode' argument sounds like a workaround to me. 'insert' sounds like a non-idempotent function that 'ensure' or 'inserted' might call. Might not eg line.replaced, line.deleted, line.inserted be clearer?

'location' says: "Defines where to place content in the line" but then says "start: Place the content at the beginning of the file." Typo s/line/file/ ?

And then there's this odd "Parameters" section at the end, that changes the doc formatting. But I think that's what https://github.com/saltstack/salt/issues/27295 is talking about.

Hope this helps everyone understand why I don't understand :P

[mnieber]

Since imho it makes sense to use the location argument in the ensure mode, I added this to my fork. Just posting it here in case it helps: https://github.com/mnieber/salt/commit/b8b99148d437023829dae5c114bf48c600650326

[mnieber]

I would limit the scope of file.line to adding or removing one line in a file (replace mode can replace multiple lines). Also, to prevent surprises, I would by default fail loudly if the intented addition or removal could not be made. With this smaller scope and more strict expectation, it should be easier to design an intuitive behaviour.

BTW the "match" argument of the "replace" mode is currently used in a surprising way: the regex is matched once, and then all lines containing the exact same match are replaced.

[Da-Juan]

I ran into a problem similar to #39849
Regex cannot be used to match the beginning or the end of a line because the target file is parsed as a single string and not line by line.

Here modules/file.py#L1720 the file is loaded in the body variable and the regex is run in _regex_to_static function on this entire string.

The following example only works if the matching line is the first line in the file:

remove_install_mode:
  file.line:
    - name: /root/local_settings.py
    - match: "^INSTALL_MODE = .*"
    - mode: delete

[tremon015]

As other commenters (both here and in various other PRs) have said, I too find the implementation of file.line confusing. As an example, from reading the 2018.3.2 docs it is unclear to me how _insert_ mode differs from _ensure_ mode. Purely from the name of the mode, I might naively expect _insert_ to only add a line if no match was found, and _ensure_ mode to both add a line if no match was found, and replace a line if a match was found. But the documentation suggests otherwise:

_ensure_ If line does not exist, it will be added.

To me, this means that _ensure_ does nothing if the line already exists, even if it was a wildcard _match_ and _content_ differs from the matched line. The parameters _before_ and _after_ determine where the line is added if no match was found.

_replace_ If line already exists, it will be replaced.

This is clear to me. This means that _replace_ does nothing if the line doesn't exist, but in case of a match, the matched line will always be changed to match _content_.

_insert_ Insert a line.

To me, this means that the line will always be added (at _location_), regardless of whether a match occurs anywhere else in the file. If match is a regex, the line will not be changed to match _content_.

However, if I check what salt actually does, I find that for _ensure_ mode, the _match_ parameter seems to be completely ignored. Matching is done on _before_ and _after_ exclusively, and only _content_ determines whether a line is changed or added:

$ seq 5 > /tmp/test
$ salt-call state.single file.line name=/tmp/test mode=ensure content=\'5\' before=\'2\' match=\'1\'
local:
----------
          ID: /tmp/test
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 20:51:14.440107
    Duration: 50.389 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,4 +1,5 @@
                   1
                  +5
                   2
                   3
                   4

Similarly, for _insert_ mode, a line is added at the exact same location even if it already exists. The _match_ parameter seems ignored for this mode too. This is not an idempotent operation, so this operation would have been better suited for modules.file than for states.file:

$ salt-call state.single file.line name=/tmp/test mode=insert content=\'1\' location=start match=\'1\'
local:
----------
          ID: /tmp/test
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 21:01:39.542585
    Duration: 50.756 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,3 +1,4 @@
                  +1
                   1
                   5
                   4

Lastly, the modes _delete_ and _replace_ don't just operate on a single line, nor do they operate on all matching lines: they act on all copies of the first matching line. This is the least intuitive behaviour and I'm inclined to think this is a bug, but I can't be sure since it's underdocumented what happens when there are multiple similar lines in the file:

$ salt-call state.single file.line name=/tmp/test mode=replace content=\'5\' match=\'[0-3]+\'
local:
----------
          ID: /tmp/test
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 21:11:55.255290
    Duration: 50.609 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,5 +1,5 @@
                  -1
                  -1
                  +5
                  +5
                   5
                   2
                   3

Note how the regex would have matched the first four lines in the file, but it stopped on the first match found -- and then it replaced all exact copies of that line anyway. This behaviour also is not idempotent: a second run will find new changes. Even worse, I might expect the following delete operation to not result in any changes since the previous replace should have changed them all:

$ salt-call state.single file.line name=/tmp/test mode=delete match=\'[0-3]+\'
local:
----------
          ID: /tmp/test
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 21:21:56.948258
    Duration: 50.927 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,7 +1,6 @@
                   5
                   5
                   5
                  -2
                   3
                   4
                   5

Or maybe I might have expected the delete operation to delete at most one line:

$ salt-call state.single file.line name=/tmp/test mode=delete match=\'[4-5]+\'
local:
----------
          ID: /tmp/test
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 21:22:05.852593
    Duration: 50.433 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,6 +1,2 @@
                  -5
                  -5
                  -5
                   3
                   4
                  -5

All these operations might be by design, or they might be bugs. I can't tell, because the documentation doesn't really explain how file.line is expected to be used.

To fix these ambiguities (and, frankly, design errors), I would suggest to completely redefine the operation modes of this state function:

1. Drop all current usage of _mode_ and replace it with a simple _present_/_absent_ boolean value (or perhaps, deprecate this one and add two state functions: line_present and line_absent)
2. Make _match_ the primary search argument for all modes. It can still default to _content_ if not provided.
3. Use _before_ and _after_ to limit operation scope, for all modes. This means that only matches between _before_ and _after_ are considered, but also that _prepend_ and _append_ are relative to these boundaries, and that lines are only removed if between those bounds. The default scope is the entire file.
4. Replace the current operation modes with new parameters _action_if_found_ (keep, replace or drop) and _action_if_not_found_ (keep, prepend, append). Or, keep these flags internal and expose mode synonyms. I would advise against it, because it would only add unnecessary obfuscation and create possible documentation problems.
5. [edit to add] Add an _action_if_multiple_ (fail, change_first, change_all, merge) parameter. This would allow the user explicit control of matching behaviour
6. mode _delete_ would map to (drop, keep), as would new mode/function _absent_
7. new mode/function _present_ would default to (replace, append)
8. mode _replace_ would map to (replace, keep)
9. mode _ensure_ would not cleanly map to the new operation since it ignores _match_. But it could be translated to (replace, append) or (replace, prepend) based on the presence of _before_ and _after_.
10. mode _insert_ would not cleanly map to the new operation since it ignores _match_. The current implementation would match (prepend, prepend) or (append, append) based on the value of _location_.

It is probably much easier to introduce a new state function than to change the semantics of the existing one. But since this proposal introduces new operation modes, it should be possible to add the new behaviour to file.line without changing the existing behaviour.

[isbm]

@tremon015 well. In my defence, this was a port from CFEngine. :stuck_out_tongue_winking_eye: And this code isn't young, to be honest. Also not much feedback was in last two years, so... Yeah. Let's fix this beast.

If seriously, your insight is very helpful. As original author of this thing, I kindly accept the blame here and should consider to revamp all that thing again. To be honest, _personally_ I more prefer to keep current function but have it fixed and answer current problems with it.

I agree here that this function has a bit of inconsistencies and is not entirely clear. For that reason, I would propose the following (and ask your help here):

1. Let's agree to keep current function.
2. Based on the list above, please write an example of a YAML state and say what it should do. For each mode. I would expect 7-10 of those.
3. I can go again and fix this, then write tests based on your examples for _each_ use case.

Basically, give me an example how you would say "This way it makes more sense" or "I would like to write state this way". Again, file.line stays. The only changes are the mode of it and the expected result, as I see it (correct me if I missing something).

Would that work?

[grinapo]

I very much like @tremon015 's proposal, as file is not "a bit unclear" but very confusing, at least for me as a fresh user back then. Still it's a wrangling to get it done something properly. I was surprised back then to see that those use cases I needed look like extraordinary, since most of it cannot be done cleanly.
Also it has been noted along the way that file is the largest chunk of code around, which suggests caution when one try to extend it significantly.
But I long to see it extended as described above since it would make it clear what operation results what. Right now often easier to source the file from salt:// compared to managing it withfile operations.

[tremon015]

No defense needed, I wasn't trying to place blame :) -- just trying to make clear where the current function is lacking. The main problem that I see is that it's simply unclear what the function is meant to do. In many of the PRs against file.line I have seen "solutions" that simply avoid this function (i.e. recommend to use file.append or file.replace) because those functions have a better-defined operation.

So I think think we should start with your point 2. Let's first identify use cases to describe what the function should be doing. When we have identified the required operations, then we can determine if keeping it all in one function is the best solution, or we should split it out to separate functions, or even a separate state module (as proposed in #33751). I don't care either way, as long as the end result is usable line-editing state functions :D

In order to make it easy to link to use cases, I think it's best if we document one use case per comment. I'll start by adding some use cases of my own that I think are currently not covered (at least, not covered by the documentation), and I'll copy over some from other PRs that I think are valuable to have.

[tremon015]

So, use-case # 1 for me: augmenting a vendor-supplied config file.

I don't want to use file.managed because the file could be 2000 lines of default settings, and I only care about changing one or two, and I don't want to chase down all the vendor changes on every release. I also can't rely on the order in which the lines appear being constant, so file.line with _before_ or _after_ won't work. And if I change the configured value, it should not simply be added but the existing line should be replaced, so file.append won't work either.

Let's keep the vendor file short:

APP_USER=nobody
APP_GROUP=nobody

$ salt-call state.single file.line name=/tmp/appconfig mode=replace content=\'APP_GROUP=appgroup\' match=\'^APP_GROUP\s*=\'
local:
----------
          ID: /tmp/appconfig
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 17:57:44.630255
    Duration: 50.832 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,2 +1,2 @@
                   APP_USER=nobody
                  -APP_GROUP=nobody
                  +APP_GROUP=appgroup

So far, so good. But the app has an optional APP_KEYTAB argument that's not in the vendor config at all. What I want to happen is this:

$ salt-call state.single file.line name=/tmp/appconfig mode=ensure content=\'APP_KEYTAB=/etc/app/keytab\' match=\'^APP_KEYTAB\s*=\' location=end
local:
----------
          ID: /tmp/appconfig
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 18:01:11.799843
    Duration: 50.192 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,2 +1,3 @@
                   APP_USER=nobody
                   APP_GROUP=appgroup
                  +APP_KEYTAB=/etc/app/keytab

$ salt-call state.single file.line name=/tmp/appconfig mode=ensure content=\'APP_KEYTAB=/etc/app/keytab_new\' match=\'^APP_KEYTAB\s*=\' location=end
local:
----------
          ID: /tmp/appconfig
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 18:01:11.799843
    Duration: 50.192 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1,2 +1,3 @@
                   APP_USER=nobody
                   APP_GROUP=appgroup
                  -APP_KEYTAB=/etc/app/keytab
                  +APP_KEYTAB=/etc/app/keytab_new

And additionally, I should be able to manually move around the APP_KEYTAB line without file.line adding a duplicate at the end.

But I don't see how do I can make that happen currently: the _replace_ mode will not add the line, _insert_ will keep adding that same line over and over, and _ensure_ will add a second KEYTAB line instead of replacing the first.

[grinapo]

My usual use case: ensure that a config option exists and of a given value, and it's replacing (or next to) the place it should be. Like:

• the file contains "# option: 123" and that get a _following_ line as "option: 666"
• the file contains "option: 456" (and no comment) and it gets replaced with "option: 666"
• the file does not contain the pattern, so it gets inserted (preferable where I want to, like, following the first line matching a pattern, but appending at the end of the file may be a solution, inferior as it is) "option: 666"
• the file contains "# option: 123" AND "option: 456" so I want to keep the comment and replace the non-comment with "option: 666"

I'm not sure it's possible right now (apart from doing it in several steps).

[tremon015]

use-case # 2: section-based config files

Let's pick a specific example, the Debian inetd.conf has the following layout (actually, with colons instead of underscores, but salt's cmdline parsing tries a dict conversion when it sees colons):

#_STANDARD_ These are standard services.

#_BOOT_ services provided primarily for booting

#_OTHER_ Other services

Now I want to register a tftp server and a bootps server:

$ salt-call state.single file.line name=/tmp/inetd.conf mode=ensure content="tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp" match="^tftp\\s+" after="#_BOOT_"
local:
----------
          ID: /tmp/inetd.conf
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 18:30:48.876179
    Duration: 50.756 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -5,6 +5,7 @@
                   #_INFO_ Info services

                   #_BOOT_ services provided primarily for booting
                  +tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp

                   #_OTHER_ Other services

$ salt-call state.single file.line name=/tmp/inetd.conf mode=ensure content="bootps dgram udp4 wait root /usr/sbin/bootpd" match="^bootps\\s+" after="#_BOOT_"
local:
----------
          ID: /tmp/inetd.conf
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 18:58:29.670296
    Duration: 51.244 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -5,6 +5,7 @@
                   #_INFO_ Info services

                   #_BOOT_ services provided primarily for booting
                  +bootps dgram udp4 wait root /usr/sbin/bootpd
                   tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp

                   #_OTHER_ Other services

So far, so good. But a configuration like this (which can easily happen when each service state file manages its own inetd line) shouldn't result in both states competing for the same anchor spot. The next command should return no changes, but doesn't:

$ salt-call state.single file.line name=/tmp/inetd.conf mode=ensure content="tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp" match="^tftp\\s+" after="#_BOOT_"
local:
----------
          ID: /tmp/inetd.conf
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 18:59:09.474294
    Duration: 51.011 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -5,6 +5,7 @@
                   #_INFO_ Info services

                   #_BOOT_ services provided primarily for booting
                  +tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp
                   bootps dgram udp4 wait root /usr/sbin/bootpd
                   tftp dgram udp4 wait tftp:tftp /usr/sbin/in.tftpd in.tftpd /srv/tftp

I must also add that ensure did replace the tftp line when I modified it, so maybe _ensure_ does use the _match_ parameter in some cases? (edit: no, it's because of #37360 that similar-looking lines in the edited area are removed. It has to do with the first word of _content_, not with _match_)

[tremon015]

use-case # 3: context-sensitive _before_ markers (from #40821)

In case a file has multiple sections, it is useful to specify the start and end boundaries between which a statement should be occurring, not just the exact line. In the case of nginx.conf:

http {
   index index.html;
}

events {
   worker_connections 1024;
}

Specifying both _after_ and _before_ here doesn't work because the _before_ match isn't unique. Yet, from the nginx configuration syntax point of view, the line can be added anywhere between the _after_ match and the first occurring _before_ match following it, so it would be nice if the following worked:

$ salt-call state.single file.line name=/tmp/nginx.conf mode=ensure content="use epoll;" after="^events\\s*{" before="^}" indent=True
local:
----------
          ID: /tmp/nginx.conf
    Function: file.line
      Result: True
     Comment: Changes were made
     Started: 19:32:00.830687
    Duration: 51.196 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -4,5 +4,6 @@
                   }

                   events {
                  +   use epoll;
                      worker_connections 1024;
                   }

[Sxderp]

I think the parameter "mode" should be removed entirely. For the most part I like what @tremon015 suggested here, without point # 1 (I think it adds confusion). Unfortunately, this would break backward compatibility but I think it's for the better, as using single paramter that takes a single word to define functionality is too vague.

As such I suggest the parameters work as follow. I don't want to take too much credit for this, as a lot of it was outlined by @tremon015. Hopefully I provide enough information in order to avoid ambiguities. I tried to cover all the use cases in the parameter documenation.

One thing I do gloss over is how 'fuzzy matching' works. As I don't know how it currently works I haven't described it. But it should be described somewhere within this hypothetical documentation.

• name
  Filesystem path to the file to be edited.
• content
  Content of the line. Must be empty if if_present: drop and if_absent: keep.
• match
  Can be either a case sensitive string or regular expression. Will be used in order to determine if the line is present within the scoped range. If match is None then fuzzy matching based on content will be performed.
• strict_match
  Boolean. If set True then fuzzy matching WILL NOT be perform. In other words, match will be set to the exact string of content. The default is False. [TODO: Raise an error if both strict_match and match are set?]
• is_present
  Must be defined if is_absent is not defined! One of replace, drop, keep. Declare the action to perform when match is found within the scoped range. Defaults to keep.
  -- replace
  If match is found then that line will be replaced with content. This will only occur for the first occurance of match within the scoped range. Due to the default use of fuzzy matching it is recommended that match be explicitly defined else unintended lines be replace.
  -- drop
  If match is found then that line will be removed from the file. match must be defined and content cannot be defined if this action is used. This will only be applied to the first occurance of match within the scoped range.
  -- keep
  If match is found then no further action will be performed.
• is_absent
  Must be defined if is_present is not defined! One of append, prepend, keep. Declare the action to perform when match is not found within the scoped range. Defaults to keep. Additionally, content must be defined else an error is raised.
  -- append
  If match is not found within the scoped range then append content to the bottom of the scope.
  -- prepend
  If match is not found within the scoped range then prepend content to to the top of the scope.
  -- keep
  If match is not found within the scoped range then no further action will be performed.
• after
  Can be either a case sensitive string or regular expression. Limit the search scope to lines that occur after after, exclusively. If after does not occur then the scope is effectively zero lines. If after and before are both defined but after cannot be found then the scope is effectively zero lines. The default is the top of the file.
• before
  Can be either a case sensitive string or regular expression. Limit the search scope to lines that occur before before, exclusively. This is NOT greedy and will trigger on the first occurance. If before does not occur then the search scope is effectively the entire file. If after and before are both defined but before cannot be found after after the search scope is effectively the same as if before was not defined. The default is the end of the file.

TODO: DEFINE IF THERE CAN BE MULTIPLE BLOCKS! If a second after is found after the before do we perform checks again? I can think of a couple ways to handle this. And a lot of discussion could be done around it. However, for my below 'implementation' I assume that only the first such block will be examined.

I have left out the basic 'file creation' parameters. But they can, for the most part, be added in without issue. The only additional parameter that I think might add ambiguity is 'indent'.

Here's some Python pseudo code on how I envision the function would flow.

https://gist.github.com/Sxderp/065e762159638e16353d621482664ec8

[grinapo]

Use case:
I want to ensure that a line exists in the ssh/authorized_keys file. I do not care where, in fact: I do not have any known pattern present. The files are wildly different, I only care about this line being there.

(Which requires the pretty illogical file.append instead of file.line + mode: ensure.)

[gurubert]

The management of authorized_keys file can be done with the ssh_auth module: https://docs.saltstack.com/en/latest/ref/states/all/salt.states.ssh_auth.html

[grinapo]

Thanks @gurubert but that wasn't the point. :-) Replace "ssh" with "random_login_manager".

[strangedev]

I agree with @grinapo that the docs are pretty confusing here.
I am fairly new to salt, so I consult the documentation regularly when writing states.

What tripped me up was, that in the documentation of file.line under mode: ensure it is stated that

If line does not exist, it will be added.

Which, looking at the source, is only true if before and after are both defined, otherwise the line will be added before or after the match, regardless of existence.

An idea for a quick fix which would not require any hard code changes would be:

1. Update the documentation to explain that before and after are required for mode: ensure
2. Point readers to the more appropriate file.append state, which behaves as I would expect from file.line with mode: ensure and no before and after kwargs.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[strangedev]

Please reopen this issue.

[mzs114]

Please reopen this, I wasted like couple of hours till I realized there exists 'states.host' to manage /etc/hosts.