Salt: The `apache.configfile` state does not allow multiple same sections

[vutny]

Description of Issue/Question

Looks like the apache.configfile state function has limited capabilities in setting multiple sections of the same type at a single level of config hierarchy.

Setup

Taking the example from the docs:

/etc/httpd/conf.d/website.com.conf:
  apache.configfile:
    - config:
      - VirtualHost:
          this: '*:80'
          ServerName:
            - website.com
          ServerAlias:
            - www.website.com
            - dev.website.com
          ErrorLog: logs/website.com-error_log
          CustomLog: logs/website.com-access_log combined
          DocumentRoot: /var/www/vhosts/website.com
          Directory:
            this: /var/www/vhosts/website.com
            ...

It is unclear how to add additional config options for other sub-directories to this virtual host.
To achive resulting config look like this:

<VirtualHost *:80>
ServerName website.com
ServerAlias www.website.com dev.website.com
ErrorLog logs/website.com-error_log
CustomLog logs/website.com-access_log combined
DocumentRoot /var/www/vhosts/website.com
<Directory /var/www/vhosts/website.com>
Require all granted
Options Indexes
</Directory>
<Directory /var/www/vhosts/website.com/admin>
AllowOverride None
Order Deny,Allow
Allow from 192.168.100.0/24
</Directory>
</VirtualHost>

[Ch3LL]

@vutny i am able to replicate this behavior And when adding another Directory like below:

          Directory:
            this: /var/www/vhosts/website.com
          Directory:
            this: /var/www/vhosts/website2.com

or this:

          Directory:
            this: /var/www/vhosts/website.com
            this: /var/www/vhosts/website2.com

I receive a conflicting id error. Looks like we need to add the ability to add more then one directory. Thanks

Also here is a docker container for anyone that wants to quickly replicate this:

1. docker run -it -v /home/ch3ll/git/salt/:/testing ch3ll/issues:38306 salt-call --local state.sls apache

[aaronlauterer]

Any news regarding this? I am still experiencing it with 2017.7.5 (Nitrogen).

[boltronics]

I too would like to express interest in seeing this issue solved.

It seems to me that the syntax should be changed so that the items in the VirtualHost dict (and elsewhere) are also presented as a list to avoid duplicate keys when rendering the YAML. ie. Instead of

/etc/httpd/conf.d/website.com.conf:
  apache.configfile:
    - config:
      - VirtualHost:
          this: '*:80'
          ServerName:
            - website.com
          DocumentRoot: /var/www/vhosts/website.com
          Directory:
            this: /var/www/vhosts/website.com
            Order: Deny,Allow
            Deny from: all
            Allow from:
              - 192.168.100.0/24
          # Key error:
          Directory:
            this: /var/www/vhosts/another-website.com
            Order: Deny,Allow
            Deny from: all
            Allow from:
              - 192.168.101.0/24

where in this example only one Directory key is available (since it's effectively presented as a YAML-formatted dict), the required syntax could be changed to the following:

/etc/httpd/conf.d/website.com.conf:
  apache.configfile:
    - config:
      - VirtualHost:
        - this: '*:80'
        - ServerName:
          - website.com
        - DocumentRoot: /var/www/vhosts/website.com
        - Directory:
          - this: /var/www/vhosts/website.com
          - Order: Deny,Allow
          - Deny from: all
          - Allow from:
            - 192.168.100.0/24
        - Directory:
          - this: /var/www/vhosts/another-website.com
          - Order: Deny,Allow
          - Deny from: all
          - Allow from:
            - 192.168.101.0/24

Obviously this is a change that breaks backwards compatibility with user sls files. As such, if someone were to submit a PR with the above adjustments, would it be considered for inclusion?

[Ch3LL]

We never want to break backwards compatibility unless it is properly deprecated as documented here: https://docs.saltstack.com/en/latest/topics/development/deprecations.html

Essentially you would add a deprecation notice that it will change in whichever release (2 feature releases out) and that behavior would not change the default until then, but the new behavior would also be available in the current release.

[boltronics]

@Ch3LL Thanks for clarifying.

Looking over the above again, I think it should be possible to keep backwards compatibility while also supporting the new style. It would just be a matter of checking the value type under each key to see if it's a dict or list before deciding which code path to use. So I guess there's no rush to deprecate the existing code.

[slivik]

I have just created pull request fixing this. Hopefully it will be merged soon.

[boltronics]

Nice work @slivik!

[slivik]

Thanks :)

[vutny]

Seems to be resolved with merging #48244 @rallytime

[rallytime]

Awesome! Thanks for confirming, @vutny!