fix(vhosts): better cleanup solution

[Yoda-BZH]

Sometimes, the cleanup process happend AFTER vhosts are installed, which resulted in no vhost declared.

Parsing declared vhosts in pillar and comparing to present files on the server allows to remove only unwanted files. Plus, files are kept in sites-available, and removed only in sites-enabled.

PR progress checklist (to be filled in by reviewers)

• [ ] Changes to documentation are appropriate (or tick if not required)
• [x] 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
• [x] [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

263

Describe the changes you're proposing

Instead of purging sites-available / sites-enabled, remove only non-declared files.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Before (with cleanup after installing vhosts files) :

local:
----------
          ID: server_conf_2
    Function: file.managed
        Name: /etc/nginx/sites-available/anotherapp.example.com-443.conf
      Result: True
     Comment: File /etc/nginx/sites-available/anotherapp.example.com-443.conf updated
     Started: 10:21:32.888803
    Duration: 96.286 ms
     Changes:   
              ----------
              diff:
                  New file
              mode:
                  0644
----------
          ID: server_conf_3
    Function: file.managed
        Name: /etc/nginx/sites-available/anotherapp.example.com-80.conf
      Result: True
     Comment: File /etc/nginx/sites-available/anotherapp.example.com-80.conf updated
     Started: 10:21:32.985477
    Duration: 90.509 ms
     Changes:   
              ----------
              diff:
                  New file
              mode:
                  0644
----------
          ID: nginx_server_enabled_dir
    Function: file.directory
        Name: /etc/nginx/sites-enabled
      Result: True
     Comment: Files cleaned from directory /etc/nginx/sites-enabled
     Started: 10:21:33.581762
    Duration: 8.854 ms
     Changes:   
              ----------
              /etc/nginx/sites-enabled/anotherapp.example.com-443.conf:
                  ----------
                  removed:
                      Removed due to clean
              /etc/nginx/sites-enabled/anotherapp.example.com-80.conf:
                  ----------
                  removed:
                      Removed due to clean
              removed:
                  - /etc/nginx/sites-enabled/anotherapp.example.com-443.conf
                  - /etc/nginx/sites-enabled/anotherapp.example.com-80.conf
----------
          ID: nginx_server_available_dir
    Function: file.directory
        Name: /etc/nginx/sites-available
      Result: True
     Comment: Files cleaned from directory /etc/nginx/sites-available
     Started: 10:21:33.590973
    Duration: 5.703 ms
     Changes:   
              ----------
              /etc/nginx/sites-available/anotherapp.example.com-443.conf:
                  ----------
                  removed:
                      Removed due to clean
              /etc/nginx/sites-available/anotherapp.example.com-80.conf:
                  ----------
                  removed:
                      Removed due to clean
              removed:
                  - /etc/nginx/sites-available/anotherapp.example.com-443.conf
                  - /etc/nginx/sites-available/anotherapp.example.com-80.conf
----------
          ID: server_state_2
    Function: file.symlink
        Name: /etc/nginx/sites-enabled/anotherapp.example.com-443.conf
      Result: True
     Comment: Created new symlink /etc/nginx/sites-enabled/anotherapp.example.com-443.conf -> /etc/nginx/sites-available/anotherapp.example.com-443.conf
     Started: 10:21:33.619386
    Duration: 4.783 ms
     Changes:   
              ----------
              new:
                  /etc/nginx/sites-enabled/anotherapp.example.com-443.conf
----------
          ID: server_state_3
    Function: file.symlink
        Name: /etc/nginx/sites-enabled/anotherapp.example.com-80.conf
      Result: True
     Comment: Created new symlink /etc/nginx/sites-enabled/anotherapp.example.com-80.conf -> /etc/nginx/sites-available/anotherapp.example.com-80.conf
     Started: 10:21:33.636314
    Duration: 4.584 ms
     Changes:   
              ----------
              new:
                  /etc/nginx/sites-enabled/anotherapp.example.com-80.conf

after :

local:
----------
          ID: /etc/nginx/sites-enabled/tests1.conf
    Function: file.absent
      Result: None
     Comment: File /etc/nginx/sites-enabled/tests1.conf is set for removal
     Started: 11:12:25.806465
    Duration: 0.625 ms
     Changes:   
              ----------
              removed:
                  /etc/nginx/sites-enabled/tests1.conf

Previous vhosts are not purged, only unwanted file tests1.conf is removed

The code is inspired by apache.vhosts.cleanup https://github.com/saltstack-formulas/apache-formula/blob/master/apache/vhosts/cleanup.sls#L10-L40

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

[pull-assistant[bot]]

Best reviewed: commit by commit

---

Optimal code review plan

     feat(vhosts): Better cleanup solution

Powered by Pull Assistant. Last update ff9a5b3 ... ff9a5b3. Read the comment docs.

[daks]

Hello, thanks for you report.

Sometimes, the cleanup process happend AFTER vhosts are installed, which resulted in no vhost declared.

Could you give us more details about those problems? because I run the states in the following order and don't have this problem

  - nginx.pkg
  - nginx.config
  - nginx.servers_config
  - nginx.snippets
  - nginx.servers
  - nginx.certificates
  - nginx.service

Plus, files are kept in sites-available, and removed only in sites-enabled.

In fact, I saw that too. It's not really a problem because Nginx don't use them but in fact it could be improved.

Generally, I think this formula needs some work to make it better, in a more standardized, logical and simpler way (see my comment here for example https://github.com/saltstack-formulas/nginx-formula/issues/194#issuecomment-400951143) but I'm not sure about the change you propose: the clean argument to file.directory seems/is the correct way to do it, and I'm not sure a more complex solution is a good idea.

[Yoda-BZH]

My setup is :

• pillarstack
• salt 3001

with the configuration :

states:
  list:
    - nginx

nginx:
  servers:
    purge_servers_config: true

With this setup, new vhosts are created, then the cleanup is ran, which deletes all vhosts.

Plus, cleaning & recreating all files generetes IO. All files have the creation / modification time updated.

Not modified files should not be touched.

[sticky-note]

@Yoda-BZH Thanks for this contribution Several steps needs to be completed before this can be merged: 1) Write a correct semantic-release compatible commit message: ex: fix(vhosts): better cleanup solution see https://github.com/semantic-release/semantic-release for further documentation

2) Check up why tests are failing: https://travis-ci.com/github/saltstack-formulas/nginx-formula/jobs/353869985

[daks]

There are actually two problems with this formula:

1. sometimes/always configuration is not applied correctly and user ends with a wrong Nginx configuration for declared vhosts
2. sites-* directories are always purged

I finally managed to reproduce these problems. Let's try to solve them separately because 1. is directly related to this formula but for me 2. is either a salt giant bug (with clean on file.directory) or a misusage in this formula

As I said previously, at work we don't call directly nginx but a wrapper which calls (nearly) this list of states:

include:
  {%- if nginx.ng is defined %}
  - nginx.deprecated
  {%- endif %}
  - nginx.config
  {%- if nginx.snippets is defined %}
  - nginx.snippets
  {%- endif %}
  - nginx.servers_config
  - nginx.servers
  - nginx.certificates
  - nginx.service

Do you think you can try to edit nginx/init.sls to put this list in this order and tell me if it solves problem 1? Thanks

[Yoda-BZH]

Hi,

Using this list of state fixes the problem, yes.

But each time salt runs, all files are deleted and recreated, even if no modification has to be made.

This causes unecessary salt steps to be made, and unecessary I/Os on disk.

Checks based on file age (for example) cannot be made.

File that are not modified should not be touched/removed&recreated.

This PR removes only no-longer-needed files.

[Yoda-BZH]

Hello,

any news ?

Thank you

[tacerus]

Hi,

I'm interested in this patch as well, but it seems to be missing an if conditional honoring the nginx.servers.purge_servers_config pillar setting.