Mapping features offered by the formula not sufficient to mirror postfix reqs

[ixs]

Turns out, the mapping features offered by the formula are not sufficient to mirror real-life postfix deployments.

This issue serves as a discussion point to consider how to improve the current situation. It would probably also make a lot of sense to have a chat on IRC and discuss our options...

(NB: When I say mapping this can be read as lookup tables as well)

Postfix offers basically three ways of using mapping tables:

• Static, in-memory or network based lookup tables which are set in main.cf:
  • transport_maps = static:relay_example.com:25
  • sender_canonical_maps = tcp:127.0.0.1:10001
• External files which can optionally be hashed and generally follow a newline separated "key: value" format.
  transport_maps = hash:/etc/postfix/transports
• Databases backends, e.g. ldap, mysql etc.
  • main.cf config item points at an external configuration file:
    transport_maps = mysql:/etc/postfix/transports.cf
  • external configuration file contains e.g. mysql query and connection parameters:

    dbname = mail
    hosts = db.example.com
    user = dbuser
    password = dbsecret
    query = SELECT "relay_example.com:25";
    

Detailed descriptions of the various types can be found at http://www.postfix.org/DATABASE_README.html.

The formula currently has a handful of different ways of configuring maps:

• Configured as a static or in-memory lookup table in main.cf:
  virtual_uid_maps: static:5000
  This is handled as a regular parameter in main.cf
• External hash-table:
  • Lookup table configured in main.cf:
    relay_recipient_maps: hash:/etc/postfix/relay_domains
  • Content configured via the pillar:

     postfix:
      mapping:
        relay_recipient_maps:
           - example.com: OK
    

• VirtualUser support introduced in 65f86e6 which adds MySQL support to the lookup tables for virtual_alias_maps, virtual_mailbox_domains and virtual_mailbox_maps configured in the vmail pillar as well as a regular hashed transport lookup table configured in the transport pillar. The resulting configuration file uses a syntax that is found under the heading "OBSOLETE QUERY INTERFACE" in http://www.postfix.org/mysql_table.5.html.
• An additional change layered on top in f8514b3 that does not really add new functionality but just adds a new pillar named mysql and uses the data included in that pillar to build a mysql query using the newer syntax described in http://www.postfix.org/mysql_table.5.html.

There are multiple shortcomings of the current design.

1. Only a single parameter is supported per configuration item. Postfix has no trouble consulting multiple files per map item: e.g. transport_maps = hash:/etc/postfix/transport, mysql:/etc/postfix/transports.cf configured in main.cf would have postfix first try a lookup in the on-disk hashed file and if nothing found try a mysql query to a database. This is not supported by the formula.

2. The VirtualUser related commits should probably never have been merged as is in the first place.
   The first commit adding VirtualUser support is written in a very specific way that is probably only usable for a single site as it encodes too many assumptions about the SQL database layout in the formula. Furthermore, the transport pillar should not have been added at all but instead the regular mapping pillar should have been used.
   The second commit repeats the mistake of the first commit by encoding site-specifics db schema assumptions in the SQL query and thus the formula.
   Furthermore, creating a new pillar entry to replicate the same functionality as already exists is kinda useless as well.

3. A generic mapping configuration is not provided.

   • We'd need something that is able to do multiple entries per lookup table configuration item.
   • We'd need to both support traditional hashed key:value files as well as lookup configuration files for database backends.

I am not sure however, how to do these changes without breaking backwards compatibility.
I am not really worried about the virtualuser changes to be honest as I consider them broken already: They are that limited that most likely nobody other than the original contributors are using them.
I am unclear however, how to best allow for multiple entries per lookup table without breaking existing configurations as shown in the pillar.example file:

Currently supported:

postfix:
  config:
    smtp_sasl_password_maps: hash:/etc/postfix/sasl_passwd
  mapping:
    smtp_sasl_password_maps:
      - smtp.example.com: myaccount:somepassword

Future version, which might break backwards compatibility in case we cannot convert internally using jinja:

postfix:
  config:
    smtp_sasl_password_maps:
      - hash:/etc/postfix/sasl_passwd
      - mysql:/etc/postfix/sasl_passwd.cf
  mapping:
    smtp_sasl_password_maps:
      /etc/postfix/sasl_passwd:
        - smtp.example.com: myaccount:somepassword
      /etc/postfix/sasl_passwd.cf:
        dbname = mail
        hosts = db.example.com
        user = dbuser
        password = dbsecret
        query = SELECT password FROM sasl_passwd WHERE user == '%s';

This would basically move the existing mapping entries one level down and add a dict keyed off the filename to separate these. Alternatively, instead of a dict we could use a list and key it off the position, mapping the main.cf entry with the mapping entry.
It might be possible to offer backwards compatibility for existing setups but I am not 100% sure.

Before I start off and actually do the work, I'd like some feedback about the viability. What do other committers think? Worth it? Not mergeable because it breaks backwards compat?

[ixs]

@aboe76 @javierbertoli @EvaSDK @davidkarlsen @0xf10e @alxwr This might be relevant for you as you've done commits in the last two years to this formula.

[0xf10e]

Turns out, the mapping features offered by the formula are not sufficient to mirror real-life postfix deployments.

[...]
There are multiple shortcomings of the current design.

1. Only a single parameter is supported per configuration item. Postfix has no trouble [...]

It worked for my small setup, so it's a starter, right? ; )

2. The VirtualUser related commits should probably never have been merged as is in the first place. [...]

No idea about that one.

3. A generic mapping configuration is not provided.
   
   * We'd need something that is able to do multiple entries per lookup table configuration item.
   * We'd need to both support traditional hashed key:value files as well as lookup configuration files for database backends.

This may get difficult with the database backends. You could try prefixing the filename in your version of pillar[postfix:mapping:smtp_sasl_password_maps] below with something like mysql: so you can handle the contained data as configuration for a DB-query.

[...]
I am unclear however, how to best allow for multiple entries per lookup table without breaking existing configurations as shown in the pillar.example file:

Currently supported:

postfix:
  config:
    smtp_sasl_password_maps: hash:/etc/postfix/sasl_passwd
  mapping:
    smtp_sasl_password_maps:
      - smtp.example.com: myaccount:somepassword

Future version, which might break backwards compatibility in case we cannot convert internally using jinja:

postfix:
  config:
    smtp_sasl_password_maps:
      - hash:/etc/postfix/sasl_passwd
      - mysql:/etc/postfix/sasl_passwd.cf
  mapping:
    smtp_sasl_password_maps:
      /etc/postfix/sasl_passwd:
        - smtp.example.com: myaccount:somepassword
      /etc/postfix/sasl_passwd.cf:
        dbname = mail
        hosts = db.example.com
        user = dbuser
        password = dbsecret
        query = SELECT password FROM sasl_passwd WHERE user == '%s';

This would basically move the existing mapping entries one level down and add a dict keyed off the filename to separate these. Alternatively, instead of a dict we could use a list and key it off the position, mapping the main.cf entry with the mapping entry.
It might be possible to offer backwards compatibility for existing setups but I am not 100% sure.

Don't try too hard to adjust data structures in jinja. Some things are just not possible
(in pure jinja) and you might hurt your brain when trying anyway.

Before I start off and actually do the work, I'd like some feedback about the viability. What do other committers think? Worth it? Not mergeable because it breaks backwards compat?

I'd say if you can use test.check_pillar to catch someone trying to run the states with their old data in the old format it should be mergeable.

I wasn't too active lately, is there a formal way to handle breaking changes in formulas...?

[ixs]

There are multiple shortcomings of the current design.

1. Only a single parameter is supported per configuration item. Postfix has no trouble [...]

It worked for my small setup, so it's a starter, right? ; )

Pffft, it works for my regular nodes that only relay to a central machine as well.
But for a central mailstore, potentially serving hundreds of users? Not so much I fear. :-)

2. The VirtualUser related commits should probably never have been merged as is in the first place. [...]

No idea about that one.

I think backing them out is the right thing to do. They replicate existing functionality and are way too limited to have actually ever been used by anyone. Furthermore: Future postfix versions will not support the code anymore as written...

3. A generic mapping configuration is not provided.
   
   * We'd need something that is able to do multiple entries per lookup table configuration item.
   * We'd need to both support traditional hashed key:value files as well as lookup configuration files for database backends.

This may get difficult with the database backends. You could try prefixing the filename in your version of pillar[postfix:mapping:smtp_sasl_password_maps] below with something like mysql: so you can handle the contained data as configuration for a DB-query.

Ohh, I was looking at the filename of the map-file as it is written on disk, not the full string. That has the benefit of not needing any change if people move from e.g. proxy:mysql:/whatevs to mysql:/whatevs if we only key it off the /whatevs part.

Don't try too hard to adjust data structures in jinja. Some things are just not possible
(in pure jinja) and you might hurt your brain when trying anyway.

I am all for that, but how do we wanna handle existing users at that point? A big note in the README that stuff is now different and you need to change your pillars?
Works for me. But in that case, we should probably also think about adding an official deprecation policy: e.g. we strive to add backwards compatibility to new changes if needed and we will keep this in place for 6 months. In case this is not possible, a clear description of the problem and how to convert data will be added to the CHANGES file.

Before I start off and actually do the work, I'd like some feedback about the viability. What do other committers think? Worth it? Not mergeable because it breaks backwards compat?

I'd say if you can use test.check_pillar to catch someone trying to run the states with their old data in the old format it should be mergeable.

Interesting. You suggest to just abort the formula in case we detect old data and notify the user? Interesting concept.

I wasn't too active lately, is there a formal way to handle breaking changes in formulas...?

Nothing really official that I have found. There seems to be attempts like php.ng where they develop a new formula in parallel to the old formula and then let the old one languish full of bugs incentivizing people to move to the newer one. 😝
The other option I saw was a note about using a specific release/checkout of the formula in case one is using a specific software version. e.g. if you're still on software X.Y you need an older version of this formula. Not really applicable here though.

[0xf10e]

(Answering to parts of this separately, makes it easier to read - at leas for me ;))
[...]

Don't try too hard to adjust data structures in jinja. Some things are just not possible
(in pure jinja) and you might hurt your brain when trying anyway.

I am all for that, but how do we wanna handle existing users at that point? A big note in the README that stuff is now different and you need to change your pillars?
Works for me. But in that case, we should probably also think about adding an official deprecation policy: e.g. we strive to add backwards compatibility to new changes if needed and we will keep this in place for 6 months. In case this is not possible, a clear description of the problem and how to convert data will be added to the CHANGES file.

We can add states causing warnings first and keep them around while working on the new version.

Before I start off and actually do the work, I'd like some feedback about the viability. What do other committers think? Worth it? Not mergeable because it breaks backwards compat?

I'd say if you can use test.check_pillar to catch someone trying to run the states with their old data in the old format it should be mergeable.

Interesting. You suggest to just abort the formula in case we detect old data and notify the user? Interesting concept.

Somewhat like a service not starting with an invalid configuration, right?
Should be the way to go when you detect borky input data anyway 😏

I wasn't too active lately, is there a formal way to handle breaking changes in formulas...?

Nothing really official that I have found. There seems to be attempts like php.ng where they develop a new formula in parallel to the old formula and then let the old one languish full of bugs incentivizing people to move to the newer one. stuck_out_tongue_closed_eyes
The other option I saw was a note about using a specific release/checkout of the formula in case one is using a specific software version. e.g. if you're still on software X.Y you need an older version of this formula. Not really applicable here though.

Yeah... I really don't like those annoying "ng" formulas/states.
I'd rather work with branches and tags. We could implement those potentially breaking changes in a branch "pillar_v2" or something and tell users about it in the README and said warnings before making it the default.

[daks]

I wasn't too active lately, is there a formal way to handle breaking changes in formulas...?

Nothing really official that I have found. There seems to be attempts like php.ng where they develop a new formula in parallel to the old formula and then let the old one languish full of bugs incentivizing people to move to the newer one. stuck_out_tongue_closed_eyes
The other option I saw was a note about using a specific release/checkout of the formula in case one is using a specific software version. e.g. if you're still on software X.Y you need an older version of this formula. Not really applicable here though.

Yeah... I really don't like those annoying "ng" formulas/states.
I'd rather work with branches and tags. We could implement those potentially breaking changes in a branch "pillar_v2" or something and tell users about it in the README and said warnings before making it the default.

This is not really the better place to discuss it but I really think this is a big problem in the way salt formulas are handled now: no versions (only git master), mandatory ascendent compatibility, etc.

The 'ng' of handling it is a poor solution.

[alxwr]

@ixs I agree with with most of your mentioned shortcomings and proposed changes.
I've just got some questions left:

2. The VirtualUser related commits should probably never have been merged as is in the first place. [...]

No idea about that one.

I think backing them out is the right thing to do. They replicate existing functionality and are way too limited to have actually ever been used by anyone. Furthermore: Future postfix versions will not support the code anymore as written...

Well, what do you mean by "backing them out" exactly? I'm using virtual_* in my setup, and I'd like to continue to do so. :-)
I fully agree with your remarks that (redundant) Pillars transport and vmail as well as site-specific queries need to go.

To give a concrete example (an to avoid misunderstandings): would this Pillar data still be supported?

mx.example.test:
    ----------
    postfix:
        ----------
        config:
            ----------
            # [...]
            virtual_alias_maps:
                hash:/etc/postfix/virtual_alias_maps
            virtual_mailbox_domains:
                - example.test
                - example2.test
            virtual_mailbox_maps:
                None
            virtual_transport:
                lmtp:unix:private/dovecot-lmtp
        mapping:
            ----------
            virtual_alias_maps:
                |_
                  ----------
                  [email protected]:
                      [email protected]

I'd say if you can use test.check_pillar to catch someone trying to run the states with their old data in the old format it should be mergeable.

Interesting. You suggest to just abort the formula in case we detect old data and notify the user? Interesting concept.

2¢:

• .ng is a poor solution, which should only be temporary.
• Branches are only a temporary solution. (They need to be merged ASAP, otherwise a fork inside the same Git repo will emerge.)
• Breaking changes should cause the formula to fail. (i.e. Pillars vmail or transport present? => abort!)
• Version numbers (tags) are be great, if they convey meaning. (https://semver.org)
• People ought to use a clone of the formula's repository anyways. Therefore nobody is forced to upgrade, which reduces the impact of a 'breaking' change immensely.

My preferred approach would be to start the refactoring in an extra branch, put warnings into master, and merge when the code is ready and tested. If someone is still using conflicting pillar data, let the formula fail.

[ixs]

@alxwr Thanks for your reply.

Let's go through it step by step.

Well, what do you mean by "backing them out" exactly? I'm using virtual_* in my setup, and I'd like to continue to do so. :-)

Ahhh. So you are the one user who is using that feature. Glad we found you. ;-)

What I mean by backing out that change: Get rid of the whole code that sets up the virtual_* handling. It's a duplication of the more general code that configures tables/maps with the only difference that a few assumptions are hardcoded.
My aim would be to see if it is possible to still support the virtual_ pillars but converting their contents into the more general variables.
I have a branch locally where I removed the virtual_ special code and added a generic sql/ldap handling. I believe I can still support the old virtual_ stuff with it but I am not sure.

In case backwards compatibility is not possible we can drop a note about that into the README and explain how to use the existing pillars to achieve the same results as the virtual_ special case.

To give a concrete example (an to avoid misunderstandings): would this Pillar data still be supported?

I believe the example you gave is easy to support in the future as it is not using any mysql handling.

I'd say if you can use test.check_pillar to catch someone trying to run the states with their old data in the old format it should be mergeable.

* .ng is a poor solution, which should only be temporary.

I agree. I think .ng is a terrible thing.

* Branches are only a temporary solution. (They need to be merged ASAP, otherwise a fork inside the same Git repo will emerge.)

Agreed with that as well. I would have suggested using the branch more in the way of:

• If you want the old functionality, use the branch XXX which has the old code.
• If you stay on master, you're getting the new stuff and might need to change your pillars.
  This is assuming we cannot guarantee backwards compatibility.

* Breaking changes should cause the formula to fail. (i.e. Pillars `vmail` or `transport` present? => abort!)

Right.

* Version numbers (tags) are be great, if they convey meaning. (https://semver.org)

Has not been done before though.

* People ought to use a clone of the formula's repository anyways. Therefore nobody is forced to upgrade, which reduces the impact of a 'breaking' change immensely.

True but I suspect reality will look a bit different.

My preferred approach would be to start the refactoring in an extra branch, put warnings into master, and merge when the code is ready and tested. If someone is still using conflicting pillar data, let the formula fail.

Okay, that is something we could do... I think in this case, it might make sense to handle the branch within the repo and then do a PR against master later on. Or do we wanna keep doing PRs against said branch?

[daks]

* Breaking changes should cause the formula to fail. (i.e. Pillars `vmail` or `transport` present? => abort!)

Right.

I think too it's the best solution to make a backward-incompatible change

* People ought to use a clone of the formula's repository anyways. Therefore nobody is forced to upgrade, which reduces the impact of a 'breaking' change immensely.

True but I suspect reality will look a bit different.

I suspect too it's false for a large part of salt users, which don't make a fork of the formula, and test it on themselves.

[alxwr]

* People ought to use a clone of the formula's repository anyways. Therefore nobody is forced to upgrade, which reduces the impact of a 'breaking' change immensely.

True but I suspect reality will look a bit different.

I suspect too it's false for a large part of salt users, which don't make a fork of the formula, and test it on themselves.

@daks I didn't think of forks (as in GitHub forks), but of cloned repositories (i.e. submodules) on the salt master. Those should only change on git pull (which hopefully isn't done automatically in most setups).

[alxwr]

@ixs Thanks for taking the time to answer my questions!

Well, what do you mean by "backing them out" exactly? I'm using virtual_* in my setup, and I'd like to continue to do so. :-)

Ahhh. So you are the one user who is using that feature. Glad we found you. ;-)

Yay, I'm special! :-)

My objective are simple (no dependencies), tiny mailserver setups. I just don't know how to do that without virtual domains. If there's another (simpler) way, I'm all ears. :-)

What I mean by backing out that change: Get rid of the whole code that sets up the virtual_* handling. It's a duplication of the more general code that configures tables/maps with the only difference that a few assumptions are hardcoded.
My aim would be to see if it is possible to still support the virtual_ pillars but converting their contents into the more general variables.
I have a branch locally where I removed the virtual_ special code and added a generic sql/ldap handling. I believe I can still support the old virtual_ stuff with it but I am not sure.

Agreed. I prefer generalized, DRY code too. (In fact I found this peculiar aspect of the formula rather confusing.) I'm ok with your proposed changes as long as they still allow me to use a multi-domain setup without the need for a DB backend.

In case backwards compatibility is not possible we can drop a note about that into the README and explain how to use the existing pillars to achieve the same results as the virtual_ special case.

If the virtual_ special case is code nobody needs to use or wants to maintain, we should just drop it and add the explanation to the README.

To give a concrete example (an to avoid misunderstandings): would this Pillar data still be supported?

I believe the example you gave is easy to support in the future as it is not using any mysql handling.

\o/

* Branches are only a temporary solution. (They need to be merged ASAP, otherwise a fork inside the same Git repo will emerge.)

Agreed with that as well. I would have suggested using the branch more in the way of:

• If you want the old functionality, use the branch XXX which has the old code.
• If you stay on master, you're getting the new stuff and might need to change your pillars.
  This is assuming we cannot guarantee backwards compatibility.

I like that idea. This could work with other formulas too.

* Version numbers (tags) are be great, if they convey meaning. (https://semver.org)

Has not been done before though.

That's true. AFAIK most formulas don't use multiple branches either.
My idea is to use annotated tags instead of branches. (A branch is only needed, if someones work diverts from master. Tags would answer the question "Where do I divert?".)

* People ought to use a clone of the formula's repository anyways. Therefore nobody is forced to upgrade, which reduces the impact of a 'breaking' change immensely.

True but I suspect reality will look a bit different.

Well, it's in the official documentation:

We strongly recommend forking a formula repository […]

IMO it's therefore reasonable to decline responsibility for unexpected changes in other people's infrastructure. (I mean it's clear that one must test upgraded software before putting it into production.)

My preferred approach would be to start the refactoring in an extra branch, put warnings into master, and merge when the code is ready and tested. If someone is still using conflicting pillar data, let the formula fail.

Okay, that is something we could do... I think in this case, it might make sense to handle the branch within the repo and then do a PR against master later on. Or do we wanna keep doing PRs against said branch?

This reminds me of an Email I received from @gtmanfred (I suppose):

When working on formula repositories, please fork them and then work on your own repository, instead of directly on a branch of the formulas repository.
[image with many of my commits in his timeline]
Not making your own fork, and proposing PRs from that, causes a lot of noise on other peoples github.com feed.

Suggestion: Let's start with a dev branch in your GitHub repository. When you consider it to be stable enough, I'd be glad to test it in my setup. If needed, PRs can still be submitted to that dev branch.
As soon as the branch is stable and passed the first round of testing, open a PR to this formula. We'll wait for feedback and merge the PR if the reviewers give their OK.

[roskens]

I have a comment on this formula not supporting lists for the maps. I was trying to figure out how to update the formula to support that, then realized that Postfix allows variables to be created on demand, so the following should work without any changes to the formula:

postfix:
  config:
    smtp_sasl_password_maps: $smtp_sasl_password_1_maps $smtp_sasl_password_2_maps
    smtp_sasl_password_1_maps: hash:/etc/postfix/sasl_passwd
    smtp_sasl_password_2_maps: mysql:/etc/postfix/sasl_passwd.cf
  mapping:
    smtp_sasl_password_1_maps:
      - smtp.example.com: myaccount:somepassword
    smtp_sasl_password_2_maps:
      dbname = mail
      hosts = db.example.com
      user = dbuser
      password = dbsecret
      query = SELECT password FROM sasl_passwd WHERE user == '%s';

I came up with this because I was trying come up with a way to us two files for a virtual_alias_maps and have the mapping data also be in the pillar data.

[alxwr]

@ixs Would #83 (comment) in combination with #90 solve the discussed shortcomings for you? (If not I'd be happy to create a dev branch.)
There's still room for improvement of this formula [1], but we'd get multiple mappings "The Postfix Way" without major refactoring.

[1] If we do a refactoring we should adhere to the new approach outlined in the template-formula.
See also saltstack-formulas/template-formula#21 and https://github.com/saltstack-formulas/template-formula as well as https://github.com/saltstack-formulas/template-formula/blob/master/docs/TOFS_pattern.rst

[alxwr]

@roskens Thanks for your suggestion of using "The Postfix Way". :-)

[ixs]

@alxwr Hmmm... This is certainly a clever way of solving the issue...

But I am not sure how I feel about "vadalizing" the postfix config namespace as a workaround. It is certainly clever though. :-)

Give me a few days, I just put the mailserver I installed using the formula (with my local mods) into production, so I need to spend a few days babysitting everything...

Then I'll have a look at details but my first impression is: Go ahead with the dev branch. It's a good idea.

[ixs]

Alright. Did tons of testing and found one missing feature but I believe I have something worth looking at now:

https://github.com/bawuenet/postfix-formula/tree/local_changes

Have a look and share what you think.

[alxwr]

@ixs Thanks for your work!
I created a branch development in this repository where we can merge and test this PR, and make use of https://github.com/saltstack-formulas/template-formula.

https://github.com/bawuenet/postfix-formula/tree/local_changes looks good to me at first glance. (It's late where I live, so I'm not capable of a meaningful review right now. :-)
I'll test your changes on my (non-representative) setup. Feel free to ping me if it takes me more than a few days to do so.

[ixs]

Quick note, this would also fix the issue described in #89.

[ixs]

@alxwr Thanks.

Another thing I noticed the other day: The description of the formula parts and the action do not really match.

It says the state "postfix" installs and starts the service while "postfix.config" configures main.cf and optionally master.cf.

Turns out the basic postfix state will also configure stuff like aliases etc.

Does that seem sensible to you? Or should those parts go into postfix.config?
In my reading, it would make more sense in postfix.config.

[alxwr]

@ixs Config should go into postfix.config.