Allow merging of acls from multiple pillar files

[jerrykan]

It would be useful to be able to define acls in multiple different pillar files. This is not possible using a list because lists can not be merged. If we use a dict then salt can merge all the acls together. The key name for the lists is only used for sorting the groupings of acls.

For backwards compatibility we check to see if postgres:acls is a list and handle it properly.

[myoung34]

can you show a run of it doing it?

Also maybe it's just me but I dont think of pillars as being mergeable things.
Wouldn't you separate them into specific pillars and not expect them to merge?
Do you have an example of any other salt formulas where merging across pillars to a state like this is used?

I'm asking to play devils advocate, I'm relatively new to salt. Not new enough to not understand it at its core but I haven't used it in enough environments to come across what you're describing. I haven't seen this behavior in other CF tools either like puppet or chef.

[myoung34]

Pinging @blbradley just out of curiosity if this type of pillar behavior is common or a community standard

[jerrykan]

@myoung34 it depends on how you structure your pillar data. In some cases it may make sense to define all the ACLs in one file, but in other cases it may make sense to define them in different pillar files.

I personally like to structure things so that everything related to a specific application/system is defined in the same pillar file, and I include that file per host as needed. An example might be the following:

# pillar/top.sls
base:
  '*':
    - postgres

  'host1':
    - app1
    - app2

  'host2':
    - app2
    - app3
  
  'host3':
    - app1
    - app3

# pillar/postgres.sls
postgres:
  user_upstream_repo: False

# pillar/app1.sls
apache:
  # ... configure the apache virtual host for the app ...

app1:
  # ... configure the application ...

postgres:
  acls:
    app1:
      - ['host', 'app1', 'app1', '127.0.0.1/32', 'md5']

# pillar/app2.sls
apache:
  # ... configure the apache virtual host for the app ...

app2:
  # ... configure the application ...

postgres:
  acls:
    app2:
      - ['host', 'app2', 'app2', '127.0.0.1/32', 'md5']

# pillar/app3.sls
apache:
  # ... configure the apache virtual host for the app ...

app3:
  # ... configure the application ...

postgres:
  acls:
    app3:
      - ['host', 'app3', 'app3', '127.0.0.1/32', 'md5']

Doing this I can easily install a different set of apps on each host without clobbering the ACLs of other apps.

Everytime you include a pillar file in the pillar/top.sls it is merged into the "global" pillar data, so the merging stuff is already being done, you just may not notice if you decide to structure you pillar files in a different way.

[vutny]

I really like this stuff.

Although sorting by ACL group name doesn't look like a perfect solution, it's fine since we have the postgres local user entry coming first in the pg_hba.conf template.

I think this PR is good to go.

[jerrykan]

@vutny if ordering is important then I would suggest using a groups naming scheme where names start with numbers. Something like:

postgres:
  acls:
    10_app2:
      - ['host', 'app2', 'app2', '127.0.0.1/32', 'md5']

    20_app1:
      - ['host', 'app1', 'app1', '127.0.0.1/32', 'md5']

    30_app3:
      - ['host', 'app3', 'app3', '127.0.0.1/32', 'md5']

[vutny]

Nice! Would you mind to update the pillar.example contents accordingly? Thank you.

[myoung34]

Im gonna let someone else be the one to merge this. I've not seen this functionality before, and having data get merged in from multiple places is unpredictable. I conversed with another maintainer and we both think that the config should come from one file. I'm going to go with your statements of I personally like to structure and . In some cases and go with my gut of abstaining. I'm not arguing, I'm just saying I'm not the guy to use personal use-cases of other people as a reason to merge something I've not seen before.

[blbradley]

@jerrykan Thank you for your contribution.

I am the other maintainer @myoung34 talked to. We both agree that most users would use a single pillar file to configure postgres via Salt.

I can see a use case where someone would want to use the postgres pillar key to configure their application's database connection parameters. Your change could help this. However, I believe that use case can be easily covered by using pillar.get('postgres.acls') and conditional logic in the states for your application.

Edit: inserted 'pillar' in second paragraph

[blbradley]

postgres.acls is not the correct pillar key for what I said. But the details still apply.

[vutny]

@blbradley @myoung34 I understand your concerns.
But I see (and actually have) a very common use case for the feature developed by @jerrykan: role-based environments.

I have numerous hosts with PostgreSQL serving different purposes, i.e. has different roles. Let's say something like web-backend, datawarehouse, cmdb, and many others. All are within the same infrastructure perimeter and share a significant part of their configuration, including ACL, for administrative and automation purposes. Despite that, each PostgreSQL installation is unique and differs a little bit from any other one.

This is where having a single large Pillar config for each instance does not scale at all. From time to time I need to update common configuration. And instead of modifying a common Pillar for all minions which is applied via top.sls (so easy!), I need to look carefully at each SLS file, make lots of similar changes and hope that I didn't miss anything. What if I want to use something more convenient than plain text YAML file to store my environment-specific Pillar data - AWS, Consul, even some SQL?

The ability to merge Pillars from different sources really helps a lot and widely used. It enables flexibility and makes maintenance easier.
The Salt documentation encourages to mix Pillars from different places:
https://docs.saltstack.com/en/latest/topics/pillar/#pillar-namespace-flattening
Your pillar data may come from external sources and Pillars even could be read from other Pillars!

Limiting the formula to support just a basic cases which most users want discourages scalability and prevents application of the formula in complex real-world environments.

We already merge Pillar data with *.yaml lookup maps and use that structured data across the states to abstract differences in operating systems and packages. Why can't we get the same behavior for ACL configuration too? It also could be distribution-specific.

I think that the improvement submitted by @jerrykan is awesome and extremely useful. The implementation is small, simple and even backwards-compatible. And I would really appreciate to have it in the upstream.

Thank you all.

[blbradley]

@vutny Thanks for the link to pillar namespace flattening.

@myoung34 Give that link a read. I think that would be the common pillar behavior you were looking for. Then, look at the new pillar example, and tell me what you think. FYI, (in general) all pillar data gets merged together, and the rules can be a bit strange. I think this change could be good based on the link.

@jerrykan Please give us a state run as per @myoung34's request.

[jerrykan]

I just noticed that this pull request seems to have been neglected by me. Are these changes still of interest? If so I can rebase them off the current master and try to get them upstream again. What other work would be needed?