Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

alxwr · 2019-04-09T18:25:22Z

With this change the following becomes feasible:

postfix:
  config:
    virtual_alias_maps: "$my_va_map"
    my_va_map: hash:{{ pillar['userland_etc'] }}/postfix/virtual_alias_maps
  mapping:
    # previously: 'virtual_alias_maps:'
    my_va_map:
      - '@example.test': [email protected]

resulting in

----------                                                                                                
          ID: /usr/local/etc/postfix/main.cf                                                              
    Function: file.managed                                                                                
      Result: True                                                                                        
     Comment: File /usr/local/etc/postfix/main.cf updated                                                 
     Started: 20:22:32.857905                                                                             
    Duration: 157.56 ms                                                                                   
     Changes:                                                                                             
              ----------                                                                                  
              diff:                                                                                       
                  ---                                                                                     
                  +++                                                                                     
                  @@ -70,7 +70,7 @@                                                                       
                                                                                                          
                                                                                                          
                                                                                                          
                  -virtual_alias_maps = hash:/usr/local/etc/postfix/virtual_alias_maps
                  +my_va_map = hash:/usr/local/etc/postfix/virtual_alias_maps
                   
                   
                   tls_high_cipherlist = EDH+CAMELLIA:EDH+aRSA:EECDH+aRSA+AESGCM:EECDH+aRSA+SHA256:EECDH:+
CAMELLIA128:+AES128:+SSLv3:!aNULL:!eNULL:!LOW:!3DES:!MD5:!EXP:!PSK:!DSS:!RC4:!SEED:!IDEA:!ECDSA:kEDH
                  @@ -89,6 +89,7 @@
                   smtpd_tls_protocols = !SSLv2, !SSLv3
                   smtpd_tls_session_cache_timeout = 3600s
                   strict_rfc821_envelopes = yes
                  +virtual_alias_maps = $my_va_map
                   virtual_mailbox_maps = 
                   virtual_transport = lmtp:unix:private/dovecot-lmtp
                   virtual_mailbox_domains = dmz.tty1.eu

This allows also for multiple maps within virtual_alias_maps as mentioned in #89 and #83 (comment) by @roskens.

I also fixed mapping.j2 along the way. It didn't work with Python 3.6.

Tested on FreeBSD 11.2.

vutny · 2019-04-10T08:26:21Z

postfix/files/main.cf

-        'sender_canonical_maps',
-    ] %}
-{%- endif -%}
+{%- set processed_parameters = salt['pillar.get']('postfix:mapping', {}).keys() | list %}


@alxwr I don't think you need additional list filter. The keys() method will always return a new list.

In Python 3.6 keys() returns a dict_keys object, so the list is mandatory.

Without | list:

jinja2.exceptions.UndefinedError: 'dict_keys object' has no attribute 'append'

As far as I understand it: In Python 3.6 keys() of an OrderedDict object returns a view (which may change) on the keys of the object.

Oh, thanks for pointing me out to Python 3.6 specifics, I wasn't aware of that.

However, still think it looks weird. As I can tell from the code, the processed_parameters used only once to verify the key exists. No need for such complexity.
We could avoid making the list at all:

{%- set processed_parameters = salt['pillar.get']('postfix:mapping', {}) %}

Then update the key instead of appending at line 14:

{%- do processed_parameters.update({parameter: value}) %}

Finally, at line 130 the if statement will work the same way.

@vutny Thanks for your suggestions regarding the elegance of the code. I appreciate that.

processed_parameters was a list, therefore I kept it that way. In my mind it makes more sense to manage a list of parameter names than to needlessly copy their values. postfix:mapping's contents may be quite large. Therefore I suggest we stick with the list.

I disagree with using a dict instead of a list, but if this is non-negotiable, I'll give way. :-)

@alxwr No worries, I would just like to make things here as portable as possible because of two reasons:

Generally using Python methods directly is undesirable, unless there's no other clear way.
As we've already learned, different Python version have "smaaaall" differences. And the Jinja code is not really Python, it is a layer on top. So it is better to go with Jinja semantics as much as possible in common situations.

The most heavy call here is pillar.get. It needs to fork minion process, talk to the master via message bus and network layer, receive probably megabytes back, cache it, deserialize and finally return back to the caller.
Lots of stuff is going on under the hood. Comparing to that, storing even a big dict in temporary memory is nothing.

If your concern is that holding the values is unnecessary, we could use pillar.keys function! Nothing else need to be changed then.

@vutny Then we're on the same page. :-) Thanks for the thorough explanation!

Why didn't I think of pillar.keys in the first place? O.o
Thanks for the review and the suggestion! I updated the PR.

The | list is needed because of Py3.6: "jinja2.exceptions.UndefinedError: 'dict_keys object' has no attribute 'append'".

vutny · 2019-04-11T09:10:10Z

postfix/files/mapping.j2

@@ -21,6 +21,7 @@
 {%- else %}
 {#- Some settings need order, handle OrderedDict #}
 {% for item in data %}


You can simply unpack data in the for loop right away:

{% for k, v in data %} {{ format_value(k, v) }} {%- endfor -%}

You can simply unpack data in the for loop right away:

Just tried that. It won't fly in Python 3.6:

ValueError: not enough values to unpack (expected 2, got 1)

Yeah, I have mixed the list of dicts with the list of tuples. Your changes definitely looks great.

vutny · 2019-04-24T09:26:03Z

postfix/files/main.cf

-        'sender_canonical_maps',
-    ] %}
-{%- endif -%}
+{%- set processed_parameters = salt['pillar.keys']('postfix:mapping') | list %}


@alxwr Would you mind to add a comment stating why we need list filter here?
We know, because of Python 3.6, but others would appreciate that greatly. 😃

@alxwr Would you mind to add a comment stating why we need list filter here?
We know, because of Python 3.6, but others would appreciate that greatly. smiley

Added comment. :-)

Looks like you also need to prepend the |default([], true) filter before the list, since if the Pillar does not exist, the renderer returns KeyError: u'Pillar key not found: postfix:mapping'.

@vutny good catch! :-) Updated the PR.

@vutny We still get that error even with |default([], true):

KeyError: 'Pillar key not found: postfix:mapping'

I'd switch back to

{%- set processed_parameters = salt['pillar.get']('postfix:mapping', {}).keys() | list %}

Looks like having pillar.keys here is not an option here. The exception is raised on the Salt module level, so the default filter is no use. I guess we need to revert back to the get method and either continue with dict to avoid dict_keys object error, or convert it to a list.

Updated the PR.

I'd stick with the list. After all its usable in Py2.7 and Py3.x without jumping through loops.

…altstack-formulas#89

vutny

Thanks for trying my suggestions, @alxwr 😄

alxwr · 2019-04-26T22:38:28Z

@vutny Thanks for making me re-think my code and learn new details. :-)

mapping.j2: fixed handling of OrderedDict in Python 3

c432ae2

alxwr requested a review from 0xf10e April 9, 2019 18:25

alxwr self-assigned this Apr 9, 2019

alxwr requested a review from aboe76 April 9, 2019 18:25

This was referenced Apr 9, 2019

virtual_alias_maps parameter is ignored. #89

Closed

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

Open

vutny reviewed Apr 10, 2019

View reviewed changes

vutny reviewed Apr 11, 2019

View reviewed changes

alxwr force-pushed the fix-89 branch from eaecc1c to 4add60f Compare April 24, 2019 09:18

vutny reviewed Apr 24, 2019

View reviewed changes

alxwr force-pushed the fix-89 branch 2 times, most recently from 599f688 to cdc4c5e Compare April 24, 2019 10:11

alxwr added 2 commits April 24, 2019 12:38

Explain multiple entries in virtual_alias_maps via pillar.example

bb6746c

main.cf: ignore only actually used keys of 'postfix:mapping'; fixes s…

fbaa2dc

…altstack-formulas#89

alxwr force-pushed the fix-89 branch from cdc4c5e to fbaa2dc Compare April 24, 2019 10:39

vutny approved these changes Apr 26, 2019

View reviewed changes

alxwr merged commit 19f5de8 into saltstack-formulas:master Apr 26, 2019

alxwr deleted the fix-89 branch April 26, 2019 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

alxwr commented Apr 9, 2019

vutny Apr 10, 2019

alxwr Apr 10, 2019

vutny Apr 11, 2019

alxwr Apr 24, 2019

vutny Apr 24, 2019 •

edited

Loading

alxwr Apr 24, 2019

vutny Apr 11, 2019

alxwr Apr 24, 2019

vutny Apr 24, 2019

vutny Apr 24, 2019

alxwr Apr 24, 2019

vutny Apr 24, 2019

alxwr Apr 24, 2019

alxwr Apr 24, 2019

vutny Apr 24, 2019

alxwr Apr 24, 2019

alxwr Apr 24, 2019

vutny left a comment

alxwr commented Apr 26, 2019

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

Fix #89 by ignoring only keys which are actually used in Pillar 'postfix:mapping' #90

Conversation

alxwr commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vutny Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vutny left a comment

Choose a reason for hiding this comment

alxwr commented Apr 26, 2019

vutny Apr 24, 2019 •

edited

Loading