Resolve #148: workaround prereq causing infinite recursion

[vutny]

This PR is a fix for the issue #148, which caused by PR #138. It appears that recent (?) versions of Salt are unable to process the prereq requisite correctly.

So, instead of having separate state to make a backup of the pg_hba.conf file, which is going to be modified during the postgres.server SLS run, the file copy will be created right before the modification by facilitating file.managed state-specific requisite parameter: check_cmd. This is the only option I found so far to be working reliably.

There is only one special case where its behavior is not optimal, i.e. when pg_hba.conf does not exist at all. Salt cannot copy the file which is absent and return and error code which causes the state failure. That's why I used onlyif requisite to create the file ahead if it is missing, which is rare, because the formula is designed to modify distribution-provided configuration.
Anyhow, the side effects of managing previously absent pg_hba.conf file are the empty backup file (nothing to backup really) and the salt state call does report full diff (which is nice I think).

[javierbertoli]

I'd only make a little change here: I'd move backup logic one level up (at the same level of the ACLs conditional), so they're 'independent' and make things clearer.

In this PR, backup logic is inside the acls condition. And while I understand that currently, if no ACLs are declared, probably no change will happen to the file and no backup is needed, I think there's no need to force the dependency between both declarations.

What do you think?

[vutny]

Thanks for quick response @javierbertoli !

Yes, indeed, there will be no change to the pg_hba.conf file if ACL list is empty. Because there is no point in having PostgreSQL running without an ability to use it, obviously 😄

And that's why I've put "backup" condition statement inside "ACL" condition.
The algorithm is the following:

• If there are ACLs configured, we will provision the file and its contents
  • If there is backup extension configured, then make a backup of previous version
• Else if there are no ACLs, we will just set the correct ownership and permission for the file
  • In this case there is no need to backup a file, because it's contents wouldn't be touched at all

So, making the decision about "to backup" or "not to backup" depends on the ACL list configuration.
If we will put the backup logic at the higher level, the state run would do unnecessary job to copy the file with the same contents each time when some attributes of the file have changed, because check_cmd parameter reacts on any change of the file state. The case is rather rare, but that's the thing I intended to avoid. This behavior also is consistent with backups which the file.blockreplace state makes: it does not honor file attributes at all.

I hope I explained all this stuff clearly.

[vutny]

@javierbertoli Do you have any concerns about what I wrote above?

[javierbertoli]

Yes, indeed, there will be no change to the pg_hba.conf file if ACL list is empty. Because there is no point in having PostgreSQL running without an ability to use it, obviously 😄

Most of the time, as soon as you install the package you'll have a pg_hba.conf file in place, even if you don't have ACLs in the pillar (most distros usually deploy a default one). Backing it up when it changes (even if you don't touch the default ACLs) seems a nice feature. That's why I like more the idea of not having that hard dependency between having ACLs and doing backups: I'd rather have a backup on any change to the file (even if somebody change something in the file from outside salt) than only when the ACLs change.

But it's your call, I can live with both behaviours 😄 . Do you want me to merge it as is?

[vutny]

@javierbertoli I completely agree with you. My aim is to resolve the issue, but bringing random stuff to the repo is not we wanna do here 😄 So, I really would like to have my changes not only to be accepted, but also understood.

Let me clarify the backup strategy case by case. It really doesn't change much from the previous revision.

• You have empty ACL in Pillar (which means you don't want to modify it now), but distribution provided config exists. It means that the file will not be touched, and no backup would be taken.
• You do have ACL in Pillar, and distribution provided config exists or will be created during package installation. Here we will do the backup.
• You do have ACL in Pillar, and the file was modified locally. Also we will do backup.
• You do have ACL in Pillar, but the file matches the configuration. No contents change, no backup.
• You don't have ACL in Pillar (which means we want defaults). If the file contents differs, no matter how it was created, we will do the backup.

Basically, the backup will always be taken when the ACL change is detected. There is no changes from previous behavior in this scenario.

About this statement: {%- if postgres.acls %}.
The postgres.acl variable doesn't contain a Pillar value exclusively. It falls back to the defaults provided in defaults.yaml when Pillar item does not exist. That's what the logic in the map.jinja file does.
All this stuff means that we actually do not have hard dependency only on ACL in Pillar to make a decision about backup.
We depend on generic ACL config, which may come from Pillar or formula provided default. And that's why the backup will be taken any time the pg_hba.conf file contents are going to change.

Did I explain it better this time?

[javierbertoli]

Hi @vutny, sorry for the delay, I got sidetracked by work 😄.

I understand your description on how this work just, as I said, I like the idea better of backups being done when file change, independently of ACLs. I consider conf file backup ( {%- if postgres.config_backup %}) and conf file generation ({% if postgres.acl %}) two different things, the latter requiring the former if the former is set. But postgres.config_backup should be possible without depending on {% if postgres.acl %}, just on the fact of whether pg_hba.conf file changed or not, either because of the formula or by external intervention.

As the PR is right now, you're counting with the fact that ACLs are always there (because defaults.yaml provide values for postgres.acl) and therefore, postgres.config_backup dependency on postgres.acl is always satisfied in the formula. If this ever changes, backups will be automatically broken. With my proposed modification, they would not.

As I said, it's a minor thing atm, not that critical. I'll merge as is, in order to fix #148. We can get to this back later I guess.

Thanks for all your work! 👍

[vutny]

Awesome, @javierbertoli

I totally understand your concern and I personally don't like to have such complex "workarounds" at all. I strongly believe this is caused by Salt bug, but still I had no chance to dive deeper into it. I think later we will be able to get rid of this questionable stuff.