[BUG] Python3 incompatibilities

[ixs]

When running this formula under python3, multiple issues crop up:

• dictsort is used extensively and has changed functionality between py2 and py3. Python2 would happily sort a dictionary containing both strings and integers. Python3 however will return a TypeError saying that int and str are incompatible types. This is a known change in the sorted() function.

• {{ zone_records | tojson }} seems to break on mixed data as well. I did not dig into the reason yet.

There are two possible ways of resolving the issue:

1. Document that zone data must be all strings. Especially when configuring PTR RRs. Instead of using an int as key, this needs to be wrapped in quotes to ensure there are only strings. This ill work but has the drawback that e.g. '110' is now seen as smaller than e.g. '40' and the order of entries in the zone files appear incorrectly sorted.

2. Remove all use of dictsort and instead rely on the python3 behavior that dicts are now returned in deterministic (but not sorted) order.

What is the preferred solution?

For 2. an example patch might look like this:

diff --git a/bind/config.sls b/bind/config.sls
index 0d200e7..d9a62b8 100644
--- a/bind/config.sls
+++ b/bind/config.sls
@@ -223,9 +223,9 @@ bind_rndc_client_config:

 {%- set views = {False: salt['pillar.get']('bind', {})} %}{# process non-view zones in the same loop #}
 {%- do views.update(salt['pillar.get']('bind:configured_views', {})) %}
-{%- for view, view_data in views|dictsort %}
+{%- for view, view_data in views.items() %}
 {%-   set dash_view = '-' + view if view else '' %}
-{%-   for zone, zone_data in view_data.get('configured_zones', {})|dictsort -%}
+{%-   for zone, zone_data in view_data.get('configured_zones', {}).items() -%}
 {%-     if 'file' in zone_data %}
 {%-       set file = zone_data.file %}
 {%-       set zone = file|replace(".txt", "") %}
@@ -260,7 +260,7 @@ zones{{ dash_view }}-{{ zone }}{{ '.include' if serial_auto else '' }}:
         zone: zones{{ dash_view }}-{{ zone }}
         soa: {{ salt['pillar.get']("bind:available_zones:" + zone + ":soa") | json }}
         includes: {{ salt['pillar.get']("bind:available_zones:" + zone + ":includes") | json }}
-        records: {{ zone_records | json }}
+        records: {{ zone_records }}
         include: False
     {% endif %}
     - user: {{ salt['pillar.get']('bind:config:user', map.user) }}
diff --git a/bind/files/zone.jinja b/bind/files/zone.jinja
index 9583197..ac77c62 100644
--- a/bind/files/zone.jinja
+++ b/bind/files/zone.jinja
@@ -28,11 +28,11 @@ $TTL {{ soa['ttl'] }}
 {% if include %}
 $INCLUDE {{ include }}
 {% else %}
-{% for type, rrs in records|dictsort %}
+{% for type, rrs in records.items() %}
 ;
 ; {{ type }} RRs
 ;
-{%- for host, data in rrs|dictsort %}
+{%- for host, data in rrs.items() %}
 {%- if data is mapping %}
 {%- if 'ttl' in data %}
 {{ host }} {{ data['ttl'] }} {{ data['type'] }} {{ data['record'] }}

[myii]

@ixs Thanks for raising this issue. We've got well-structured automated testing available on this formula and I believe the way to tackle this is to formally define the problem with failing tests. If you're not familiar with InSpec, that's not a problem -- if you can provide me with the inputs (pillar/config) and outputs (rendered files), py2 versus py3, I can put those tests together so that we can be clear in what we're attempting to fix here. I mean something like the feedback during https://github.com/saltstack-formulas/firewalld-formula/pull/40, which led to tests that helped ensure the new implementation was doing exactly what it was supposed to.

If you look at our current matrix, we're actually testing py3 mainly:

https://travis-ci.com/github/saltstack-formulas/bind-formula/builds/154561467

We've still got the pre-salted py2 images available, it should be relatively simple to put this all together.

[ixs]

The underlying problem is the following:

[andreas@herbert tmp]$ cat test.py
data = {0: 0, 1: 1, 'a': 'a'}

print(data)
print(sorted(data))
[root@herbert tmp]# python2 test.py
{0: 0, 1: 1, 'a': 'a'}
[0, 1, 'a']
[root@herbert tmp]# python3 test.py
{0: 0, 1: 1, 'a': 'a'}
Traceback (most recent call last):
  File "test.py", line 4, in <module>
    print(sorted(data))
TypeError: '<' not supported between instances of 'str' and 'int'

This would happen for example if you define the following data in your pillar (soa etc. omitted for brevity):

bind:
  available_zones:
    0.0.172.in-addr.arpa:
      records:
          PTR:
            1: ns1.example.net.
            $GENERATE 1-253 $: ip127-0-0-$.example.net.

The first record in the yaml file is cast as an integer, the second one as a string. At this point, dictsort breaks. If the first record key is wrapped in quotes, it is read as a string and dictsort works fine under py3.

[myii]

@ixs Thanks, I'll rustle up a failing test or two and then get back to you.

[ixs]

@myii thanks, it's been a while since I last worked with inspec and you're probably much quicker in whipping up a test-case. :-)

[myii]

@ixs First bit of feedback.

---

This was enough to trigger the error: https://github.com/myii/bind-formula/commit/485880a638c2b605e56113e142362b5e7e735815.

https://travis-ci.org/github/myii/bind-formula/builds/670517157:

All of the tracebacks in the failing job returned back to this line, which isn't highlighting the dictsort problem itself just yet:

records: {{ zone_records | json }}    <======================

---

Added another tiny commit to get through to the dictsort error: https://github.com/myii/bind-formula/commit/f2a923a9442792f8313dc5ea8fc6515ffa63d5b9.

https://travis-ci.org/github/myii/bind-formula/builds/670520245:

The failing job contains the traceback returning back to the dictsort:

              Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 394, in render_jinja_tmpl
                  output = template.render(**decoded_context)
                File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1008, in render
                  return self.environment.handle_exception(exc_info, True)
                File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 780, in handle_exception
                  reraise(exc_type, exc_value, tb)
                File "/usr/lib/python3/dist-packages/jinja2/_compat.py", line 37, in reraise
                  raise value.with_traceback(tb)
                File "<template>", line 35, in top-level template code
                File "/usr/lib/python3/dist-packages/jinja2/filters.py", line 222, in do_dictsort
                  return sorted(value.items(), key=sort_func)
              TypeError: unorderable types: str() < int()

              ; line 35

              ---
              [...]
              {% else %}
              {% for type, rrs in records|dictsort %}
              ;
              ; {{ type }} RRs
              ;
              {%- for host, data in rrs|dictsort %}    <======================

---

There are two possible ways of resolving the issue:

1. Document that zone data must be all strings. Especially when configuring PTR RRs. Instead of using an int as key, this needs to be wrapped in quotes to ensure there are only strings. This ill work but has the drawback that e.g. '110' is now seen as smaller than e.g. '40' and the order of entries in the zone files appear incorrectly sorted.

2. Remove all use of dictsort and instead rely on the python3 behavior that dicts are now returned in deterministic (but not sorted) order.

If the sorting is of great significance, then there's always the option of using lists but that's heavy-handed and a lot of hassle for everyone in terms of having to modify their pillars. If necessary, I can add another test to check for the ordering as well.

Otherwise, at this stage, number 2 seems like the better option, since that avoids errors and pillar modification.

CC: @daks, your feedback would be useful here.

[ixs]

@myii yeah, the | json error I spotted too but it never was clear to me why it is breaking. But I also did not dig deep enough.

I agree that option 2, removing dictsort is the best approach. Since python3.6 (IIRC) dicts are preserving the order of insertion of keys and are not random anymore. That is actually a nice feature which means that dictsort is not really that important anymore IMHO.

[myii]

https://github.com/saltstack-formulas/users-formula/pull/219#issuecomment-629973311

Relevant here as well, that .items()|sort results in the same error as |dictsort.

[remichristiaan]

I ran into the following on Py 3.6.9. (salt 3001) using a pillar with 2 views defined:

Data failed to compile:
----------
    Rendering SLS 'base:bind.config' failed: Jinja error: '<' not supported between instances of 'str' and 'bool'
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 400, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/lib/python3/dist-packages/jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/lib/python3/dist-packages/jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 206, in top-level template code
  File "/usr/lib/python3/dist-packages/jinja2/filters.py", line 242, in do_dictsort
    return sorted(value.items(), key=sort_func, reverse=reverse)
TypeError: '<' not supported between instances of 'str' and 'bool'

; line 206

{%- do views.update(salt['pillar.get']('bind:configured_views', {})) %}
{%- for view, view_data in views|dictsort %}    <======================
{%-   set dash_view = '-' + view if view else '' %}
{%-   for zone, zone_data in view_data.get('configured_zones', {})|dictsort -%}
{%-     if 'file' in zone_data %}
{%-       set file = zone_data.file %}
{%-       set zone = file|replace(".txt", "") %}
[...]

Removing |dictsort from the {%- for view, view_data in views|dictsort %} line results in:

Rendering SLS 'base:bind.config' failed: Jinja error: 'bool' object is not iterable
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 400, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/lib/python3/dist-packages/jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/lib/python3/dist-packages/jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 206, in top-level template code
TypeError: 'bool' object is not iterable

; line 206

Changing line 204 to the following resolves the 'bool' issue with |dictsort set):

{%- set views = {'': salt['pillar.get']('bind', {})} %}{# process non-view zones in the same loop #}

If you also remove dictsort (line 206), the following error is given:

    Rendering SLS 'base:bind.config' failed: Jinja error: not enough values to unpack (expected 2, got 0)
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 400, in render_jinja_tmpl
    output = template.render(**decoded_context)
  File "/usr/lib/python3/dist-packages/jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/lib/python3/dist-packages/jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 206, in top-level template code
ValueError: not enough values to unpack (expected 2, got 0)

; line 206

---
[...]
{%- endif %}
{% endif %}

{%- set views = {'': salt['pillar.get']('bind', {})} %}{# process non-view zones in the same loop #}
{%- do views.update(salt['pillar.get']('bind:configured_views', {})) %}
{%- for view, view_data in views %}    <======================
{%-   set dash_view = '-' + view if view else '' %}
{%-   for zone, zone_data in view_data.get('configured_zones', {})|dictsort -%}
{%-     if 'file' in zone_data %}
{%-       set file = zone_data.file %}
{%-       set zone = file|replace(".txt", "") %}
[...]

[bcrisp4]

With a quick "find and replace" I've replaced all uses of dictsort with .items(). https://github.com/bcrisp4/bind-formula/commit/bebf12fcf568a4b0e00703e3dcee371b894e174c

This seems to have resolved this issue for me.

If removing dictsort is the preferred solution, I can look at putting together a PR for this. Is there anything else that needs to be done to resolve this issue?

Edit:

I missed also removing the json filter in the config state per the originally proposed patch

[daks]

CC: @daks, your feedback would be useful here.

@myii just saw now that you pinged me on this issue! Do you still need me to look at something? (FYI we don't use this formula, just an old version with custom patches)

[myii]

@myii just saw now that you pinged me on this issue! Do you still need me to look at something? (FYI we don't use this formula, just an old version with custom patches)

@daks That was probably because you provided #129 so I must have thought you used the formula back then. No worries, the general situation is understood, we just need to finalise how we resolve this.

With a quick "find and replace" I've replaced all uses of dictsort with .items(). bcrisp4@bebf12f

This seems to have resolved this issue for me.

If removing dictsort is the preferred solution, I can look at putting together a PR for this. Is there anything else that needs to be done to resolve this issue?

@bcrisp4 Thanks for the offer, I think that's the right way forward. @ixs Are you good with that?