fix(sudoers/included): fix idempotence with purge_includedir=True

stasjok commented 3 years ago

PR progress checklist (to be filled in by reviewers)

[ ] Changes to documentation are appropriate (or tick if not required)
[ ] Changes to tests are appropriate (or tick if not required)
[ ] Reviews completed

What type of PR is this?

Primary type

[ ] [build] Changes related to the build system
[ ] [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ ] [ci] Changes to the continuous integration configuration
[ ] [feat] A new feature
[x] [fix] A bug fix
[ ] [perf] A code change that improves performance
[ ] [refactor] A code change that neither fixes a bug nor adds a feature
[ ] [revert] A change used to revert a previous commit
[ ] [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[ ] [docs] Documentation changes
[ ] [test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Describe the changes you're proposing

clean: True should be used with require requisite in order to clean only those files that are not managed by salt. Without this change the formula would clean and recreate all files every time. If /etc/sudoers.d directory doesn't exist, it will be created with makedirs: True with default permissions, then permissions will be fixed at the end.

Pillar / config required to test the proposed changes

sudoers:
  purge_includedir: yes
  included_files:
    test:
      users:
        foo:
          - 'ALL=(ALL) ALL'

Debug log showing how the proposed changes work

Documentation checklist

[ ] Updated the README (e.g. Available states).
[ ] Updated pillar.example.

Testing checklist

[ ] Included in Kitchen (i.e. under state_top).
[ ] Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
[ ] Updated the relevant test pillar.

Additional context

daks commented 3 years ago

Hello, thanks for this PR.

Using provided Kitchen tests, I simply added purge_includedir: yes to test/salt/pillar/default.sls and ran kitchen converge several times.

Its output seems to indicate that it removes files to recreate them later:

[...]
       ----------
                 ID: /etc/sudoers.d
           Function: file.directory
[...]
            Changes:   
              ----------
              /etc/sudoers.d/extra-file:
                  ----------
                  removed:
                      Removed due to clean
              /etc/sudoers.d/extra-file-2:
                  ----------
                  removed:
                      Removed due to clean
              /etc/sudoers.d/extra-file-3:
                  ----------
                  removed:
                      Removed due to clean
              removed:
                  - /etc/sudoers.d/extra-file
                  - /etc/sudoers.d/extra-file-3
                  - /etc/sudoers.d/extra-file-2
       ----------
                 ID: sudoers include /etc/sudoers.d/extra-file
           Function: file.managed
               Name: /etc/sudoers.d/extra-file
             Result: True
            Comment: File /etc/sudoers.d/extra-file updated
            Started: 12:00:02.341704
            Changes:   
              ----------
              diff:
                  New file
              mode:
                  0440
       ----------
                 ID: sudoers include extra-file-2
           Function: file.managed
               Name: /etc/sudoers.d/extra-file-2
             Result: True
            Comment: File /etc/sudoers.d/extra-file-2 updated
            Started: 12:00:02.382879
           Duration: 41.585 ms
            Changes:   
              ----------
              diff:
                  New file
              mode:
                  0440
       ----------
                 ID: sudoers include extra-file-3
           Function: file.managed
               Name: /etc/sudoers.d/extra-file-3
             Result: True
            Comment: File /etc/sudoers.d/extra-file-3 updated
            Started: 12:00:02.424920
           Duration: 43.308 ms
            Changes:   
              ----------
              diff:
                  New file
              mode:
                  0440

       Summary for local
       ------------
       Succeeded: 7 (changed=4)
       Failed:    0
       ------------
       Total states run:     7
       Total run time: 230.110 ms

But I'm not sure it really remove and recreate the files, because inodes stay the same

# after first converge
kitchen@0e03fd9419ee:~$ sudo ls -li /etc/sudoers.d/
total 12
9772507 -r--r----- 1 root root 296 Apr 13 12:00 extra-file
9772509 -r--r----- 1 root root 312 Apr 13 12:00 extra-file-2
9772510 -r--r----- 1 root root 308 Apr 13 12:00 extra-file-3

# after second one
kitchen@0e03fd9419ee:~$ sudo ls -li /etc/sudoers.d/
total 12
9772507 -r--r----- 1 root root 296 Apr 13 12:05 extra-file
9772509 -r--r----- 1 root root 312 Apr 13 12:05 extra-file-2
9772510 -r--r----- 1 root root 308 Apr 13 12:05 extra-file-3

So inodes are the same which means files are the same, but still modification time has been changed.

I also monitored the inodes to understand what happens, an excerpt of the output

$ sudo inotifywait -r -m /etc/sudoers.d/
[...]
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ DELETE extra_file
/etc/sudoers.d/ CREATE extra_file7y_5c9_h
/etc/sudoers.d/ OPEN extra_file7y_5c9_h
/etc/sudoers.d/ CLOSE_WRITE,CLOSE extra_file7y_5c9_h
/etc/sudoers.d/ MODIFY extra_file7y_5c9_h
/etc/sudoers.d/ OPEN extra_file7y_5c9_h
/etc/sudoers.d/ MODIFY extra_file7y_5c9_h
/etc/sudoers.d/ CLOSE_WRITE,CLOSE extra_file7y_5c9_h
/etc/sudoers.d/ MOVED_FROM extra_file7y_5c9_h
/etc/sudoers.d/ MOVED_TO extra_file
/etc/sudoers.d/ ATTRIB extra_file

Without the option to purge the directory

$ sudo inotifywait -r -m /etc/sudoers.d/
[...]
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN,ISDIR 
/etc/sudoers.d/ ACCESS,ISDIR 
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE,ISDIR 
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file
/etc/sudoers.d/ OPEN extra_file
/etc/sudoers.d/ ACCESS extra_file
/etc/sudoers.d/ CLOSE_NOWRITE,CLOSE extra_file

stasjok commented 3 years ago

I'm not sure why in your case inodes stay the same (maybe some docker specifics?), but in my case on real server they changes after every state.apply:

root@debian10-test ~# ls -il /etc/sudoers.d/
total 5
113274 -r--r----- 1 root root 296 Apr 13 17:55 extra-file
root@debian10-test ~# ls -il /etc/sudoers.d/
total 1
113366 -r--r----- 1 root root 296 Apr 13 17:55 extra-file
root@debian10-test ~# ls -il /etc/sudoers.d/
total 1
113438 -r--r----- 1 root root 296 Apr 13 17:55 extra-file

In your inotifywait

/etc/sudoers.d/ DELETE extra_file
/etc/sudoers.d/ CREATE extra_file7y_5c9_h
/etc/sudoers.d/ OPEN extra_file7y_5c9_h
/etc/sudoers.d/ CLOSE_WRITE,CLOSE extra_file7y_5c9_h
/etc/sudoers.d/ MODIFY extra_file7y_5c9_h
/etc/sudoers.d/ OPEN extra_file7y_5c9_h
/etc/sudoers.d/ MODIFY extra_file7y_5c9_h
/etc/sudoers.d/ CLOSE_WRITE,CLOSE extra_file7y_5c9_h
/etc/sudoers.d/ MOVED_FROM extra_file7y_5c9_h
/etc/sudoers.d/ MOVED_TO extra_file
/etc/sudoers.d/ ATTRIB extra_file

it removes the file, creates temporary file with random name, writes to it, renames it to destination, then changes permissions isn't it?

Also a quote from documentation:

Make sure that only files that are set up by salt and required by this function are kept. If this option is set then everything in this directory will be deleted unless it is required.

AFAIK, it's by design. It looks in requisites to decide what files to exclude from deletion.

daks commented 3 years ago

I haven't been able to reproduce the inode change, even on a 'real VM', but I see differences in the salt output with your patch: the following part does not appear!

----------
          ID: /etc/sudoers.d
    Function: file.directory
      Result: True
     Comment: Files cleaned from directory /etc/sudoers.d
     Started: 17:42:53.131283
    Duration: 3.479 ms
     Changes:
              ----------
              /etc/sudoers.d/whatever:
                  ----------
                  removed:
                      Removed due to clean
              removed:
                  - /etc/sudoers.d/whatever

Congrats for having found this information on the documentation! I think it could also be used for https://github.com/saltstack-formulas/nginx-formula/pull/266.

I'll merge this code now, thanks for your contribution.

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 0.23.4 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

stasjok commented 3 years ago

I haven't been able to reproduce the inode change, even on a 'real VM'

I was curious about that. And the explanation turns out to be simple: filesystems reuse inodes. Salt removes and recreates the same amount of files and these files get the same inodes, because salt is doing the same thing during every run.

Simple code to test on xfs or ext4 filesystem:

stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 test
stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 test
stas@server ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
31492857 test
removed 'test'
renamed '.temp_test' -> 'test'
31492857 test

But the same code on btrfs or zfs (copy-on-write) filesystem:

stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282467 test
removed 'test'
renamed '.temp_test' -> 'test'
282468 test
stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282468 test
removed 'test'
renamed '.temp_test' -> 'test'
282469 test
stas@server2 ~> touch test && ls -i test && rm -v test && touch .temp_test && mv -v .temp_test test && ls -i test
282469 test
removed 'test'
renamed '.temp_test' -> 'test'
282470 test

When I was testing inodes in the earlier post I was using a LXC container on ZFS filesystem with pretty active IO. But even without IO new files get new inodes on COW filesystems based on my testing.

myii commented 3 years ago

@stasjok I was just chatting to @javierbertoli and there appears to be an edge case which still fails. I think it was related to this comment:

https://github.com/saltstack/salt/issues/26605#issuecomment-742092590

stasjok commented 3 years ago

@stasjok I was just chatting to @javierbertoli and there appears to be an edge case which still fails. I think it was related to this comment:

saltstack/salt#26605 (comment)

It should work as long as required state function is file (as in this case). If salt team can improve clean: True to work with more various states, like pkgrepo in mentioned comment, it would be great (although I don't know how it can be done generic enough; parse all parameters to find everything that looks like filename?). As I understand now it considers only requires with file function: https://github.com/saltstack/salt/blob/e3c52e5fba7e2cbb1126ea784ce60f65fcb59254/salt/states/file.py#L601

saltstack-formulas / sudoers-formula