Issue 56430 (#57147)

* Adding integration test for 127.0.0.1/32 and ::1/128.

* Making sure file is not corrupted when render fails

* Fixes #56430

* Adding changelog for MR 57147/Issue 56430

changelogs/fragments/57147_Fix_postgresql_pg_hba_TypeError_pg_hba.conf_wiped.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- "postgresql_pg_hba - Fix TypeError after which pg_hba.conf is wiped (https://github.com/ansible/ansible/issues/56430)"

lib/ansible/modules/database/postgresql/postgresql_pg_hba.py

@@ -82,8 +82,10 @@ options:
   order:
     description:
       - The entries will be written out in a specific order.
-      - With this option you can control by which field they are ordered first, second and last.
+        With this option you can control by which field they are ordered first, second and last.
-      - s=source, d=databases, u=users.
+        s=source, d=databases, u=users.
+        This option is deprecated since 2.9 and will be removed in 2.11.
+        Sortorder is now hardcoded to sdu.
     default: sdu
     choices: [ sdu, sud, dsu, dus, usd, uds ]
   state:
@@ -301,6 +303,7 @@ class PgHba(object):
         if not self.changed():
             return False
 
+        contents = self.render()
         if self.pg_hba_file:
             if not (os.path.isfile(self.pg_hba_file) or self.create):
                 raise PgHbaError("pg_hba file '{0}' doesn't exist. "
@@ -316,7 +319,7 @@ class PgHba(object):
             filed, __path = tempfile.mkstemp(prefix='pg_hba')
             fileh = os.fdopen(filed, 'w')
 
-        fileh.write(self.render())
+        fileh.write(contents)
         self.unchanged()
         fileh.close()
         return True
@@ -364,11 +367,7 @@ class PgHba(object):
         '''
         This method returns all the rules of the PgHba object
         '''
-        rules = sorted(self.rules.values(),
+        rules = sorted(self.rules.values())
-                       key=lambda rule: rule.weight(self.order,
-                                                    len(self.users) + 1,
-                                                    len(self.databases) + 1),
-                       reverse=True)
         for rule in rules:
             ret = {}
             for key, value in rule.items():
@@ -544,8 +543,12 @@ class PgHbaRule(dict):
         except ValueError:
             return self['src']
 
-    def weight(self, order, numusers, numdbs):
+    def __lt__(self, other):
-        '''
+        """This function helps sorted to decide how to sort.
+
+        It just checks itself against the other and decides on some key values
+        if it should be sorted higher or lower in the list.
+        The way it works:
         For networks, every 1 in 'netmask in binary' makes the subnet more specific.
         Therefore I chose to use prefix as the weight.
         So a single IP (/32) should have twice the weight of a /16 network.
@@ -554,69 +557,100 @@ class PgHbaRule(dict):
         - for ipv4, we use a weight scale of 0 (all possible ipv4 addresses) to 128 (single ip)
         Therefore for ipv4, we use prefixlen (0-32) * 4 for weight,
         which corresponds to ipv6 (0-128).
-        '''
+        """
-        if order not in PG_HBA_ORDERS:
+        myweight = self.source_weight()
-            raise PgHbaRuleError('{0} is not a valid order'.format(order))
+        hisweight = other.source_weight()
+        if myweight != hisweight:
+            return myweight > hisweight
 
+        myweight = self.db_weight()
+        hisweight = other.db_weight()
+        if myweight != hisweight:
+            return myweight < hisweight
+
+        myweight = self.user_weight()
+        hisweight = other.user_weight()
+        if myweight != hisweight:
+            return myweight < hisweight
+        try:
+            return self['src'] < other['src']
+        except TypeError:
+            return self.source_type_weight() < other.source_type_weight()
+        errormessage = 'We have two rules ({1}, {2})'.format(self, other)
+        errormessage += ' with exact same weight. Please file a bug.'
+        raise PgHbaValueError(errormessage)
+
+    def source_weight(self):
+        """Report the weight of this source net.
+
+        Basically this is the netmask, where IPv4 is normalized to IPv6
+        (IPv4/32 has the same weight as IPv6/128).
+        """
         if self['type'] == 'local':
-            sourceobj = ''
+            return 130
-            # local is always 'this server' and therefore considered /32
+
-            srcweight = 130  # (Sort local on top of all)
-        else:
         sourceobj = self.source()
         if isinstance(sourceobj, ipaddress.IPv4Network):
-                srcweight = sourceobj.prefixlen * 4
+            return sourceobj.prefixlen * 4
-            elif isinstance(sourceobj, ipaddress.IPv6Network):
+        if isinstance(sourceobj, ipaddress.IPv6Network):
-                srcweight = sourceobj.prefixlen
+            return sourceobj.prefixlen
-            elif isinstance(sourceobj, str):
+        if isinstance(sourceobj, str):
             # You can also write all to match any IP address,
             # samehost to match any of the server's own IP addresses,
             # or samenet to match any address in any subnet that the server is connected to.
             if sourceobj == 'all':
                 # (all is considered the full range of all ips, which has a weight of 0)
-                    srcweight = 0
+                return 0
-                elif sourceobj == 'samehost':
+            if sourceobj == 'samehost':
                 # (sort samehost second after local)
-                    srcweight = 129
+                return 129
-                elif sourceobj == 'samenet':
+            if sourceobj == 'samenet':
                 # Might write some fancy code to determine all prefix's
                 # from all interfaces and find a sane value for this one.
                 # For now, let's assume IPv4/24 or IPv6/96 (both have weight 96).
-                    srcweight = 96
+                return 96
-                elif sourceobj[0] == '.':
+            if sourceobj[0] == '.':
                 # suffix matching (domain name), let's asume a very large scale
                 # and therefore a very low weight IPv4/16 or IPv6/64 (both have weight 64).
-                    srcweight = 64
+                return 64
-                else:
             # hostname, let's asume only one host matches, which is
             # IPv4/32 or IPv6/128 (both have weight 128)
-                    srcweight = 128
+            return 128
+        raise PgHbaValueError('Cannot deduct the source weight of this source {1}'.format(sourceobj))
 
+    def source_type_weight(self):
+        """Give a weight on the type of this source.
+
+        Basically make sure that IPv6Networks are sorted higher than IPv4Networks.
+        This is a 'when all else fails' solution in __lt__.
+        """
+        sourceobj = self.source()
+        if isinstance(sourceobj, ipaddress.IPv4Network):
+            return 2
+        if isinstance(sourceobj, ipaddress.IPv6Network):
+            return 1
+        if isinstance(sourceobj, str):
+            return 0
+        raise PgHbaValueError('This source {1} is of an unknown type...'.format(sourceobj))
+
+    def db_weight(self):
+        """Report the weight of the database.
+
+        Normally, just 1, but for replication this is 0, and for 'all', this is more than 2.
+        """
         if self['db'] == 'all':
-            dbweight = numdbs
+            return 100000
-        elif self['db'] == 'replication':
+        if self['db'] == 'replication':
-            dbweight = 0
+            return 0
-        elif self['db'] in ['samerole', 'samegroup']:
+        if self['db'] in ['samerole', 'samegroup']:
-            dbweight = 1
+            return 1
-        else:
+        return 1 + self['db'].count(',')
-            dbweight = 1 + self['db'].count(',')
 
+    def user_weight(self):
+        """Report weight when comparing users."""
         if self['usr'] == 'all':
-            uweight = numusers
+            return 1000000
-        else:
+        return 1
-            uweight = 1
-
-        ret = []
-        for character in order:
-            if character == 'u':
-                ret.append(uweight)
-            elif character == 's':
-                ret.append(srcweight)
-            elif character == 'd':
-                ret.append(dbweight)
-        ret.append(sourceobj)
-
-        return tuple(ret)
 
 
 def main():

test/integration/targets/postgresql/defaults/main.yml

@@ -18,10 +18,20 @@ pg_hba_test_ips:
 - source: '192.168.0.0/24'
   netmask: ''
   databases: 'all,replication'
+- source: '192.168.1.0/24'
+  netmask: ''
+  databases: 'all'
+  method: reject
+- source: '127.0.0.1/32'
+  netmask: ''
+- source: '::1/128'
+  netmask: ''
 - source: '0000:ff00::'
   netmask: 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ff00'
+  method: scram-sha-256
 - source: '172.16.0.0'
   netmask: '255.255.0.0'
+  method: trust
 
 # defaults for test SSL
 ssl_db: 'ssl_db'

test/integration/targets/postgresql/tasks/postgresql_pg_hba.yml

@@ -10,7 +10,7 @@
     source: '0000:ffff::'
     netmask: 'ffff:fff0::'
     method: md5
-    backup: true
+    backup: 'True'
     order: sud
     state: "{{item}}"
   check_mode: yes
@@ -28,7 +28,7 @@
     contype: "{{item.contype|default('host')}}"
     databases: "{{item.databases|default('all')}}"
     dest: /tmp/pg_hba.conf
-    method: md5
+    method: "{{item.method|default('md5')}}"
     netmask: "{{item.netmask|default('')}}"
     order: sud
     source: "{{item.source|default('')}}"
@@ -44,12 +44,12 @@
 
 - name: Add several ip addresses
   postgresql_pg_hba:
-    backup: true
+    backup: 'True'
     contype: "{{item.contype|default('host')}}"
-    create: true
+    create: 'True'
     databases: "{{item.databases|default('all')}}"
     dest: /tmp/pg_hba.conf
-    method: md5
+    method: "{{item.method|default('md5')}}"
     netmask: "{{item.netmask|default('')}}"
     order: sud
     source: "{{item.source|default('')}}"
@@ -68,7 +68,7 @@
     contype: "{{item.contype|default('host')}}"
     databases: "{{item.databases|default('all')}}"
     dest: /tmp/pg_hba.conf
-    method: md5
+    method: "{{item.method|default('md5')}}"
     netmask: "{{item.netmask|default('')}}"
     order: sud
     source: "{{item.source|default('')}}"
@@ -84,7 +84,7 @@
 
 - name: Add new ip address for backup check and netmask_sameas_prefix check
   postgresql_pg_hba:
-    backup: true
+    backup: 'True'
     contype: host
     dest: /tmp/pg_hba.conf
     method: md5
@@ -96,7 +96,7 @@
 
 - name: Add new ip address for netmask_sameas_prefix check
   postgresql_pg_hba:
-    backup: true
+    backup: 'True'
     contype: host
     dest: /tmp/pg_hba.conf
     method: md5
@@ -122,15 +122,20 @@
   register: pg_hba_fail_src_all_with_netmask
   ignore_errors: yes
 
+- debug:
+    var: pg_hba.pg_hba
 - assert:
     that:
     - 'pg_hba.pg_hba == [
-           { "db": "all", "method": "md5", "type": "local", "usr": "all" },
            { "db": "all", "method": "md5", "type": "local", "usr": "postgres" },
-           { "db": "all", "method": "md5", "src": "0:ff00::/120", "type": "host", "usr": "all" },
+           { "db": "all", "method": "md5", "type": "local", "usr": "all" },
-           { "db": "all", "method": "md5", "src": "192.168.0.0/24", "type": "host", "usr": "all" },
+           { "db": "all", "method": "md5", "src": "127.0.0.1/32", "type": "host", "usr": "all" },
+           { "db": "all", "method": "md5", "src": "::1/128", "type": "host", "usr": "all" },
+           { "db": "all", "method": "scram-sha-256", "src": "0:ff00::/120", "type": "host", "usr": "all" },
            { "db": "replication", "method": "md5", "src": "192.168.0.0/24", "type": "host", "usr": "all" },
-           { "db": "all", "method": "md5", "src": "172.16.0.0/16", "type": "host", "usr": "all" },
+           { "db": "all", "method": "md5", "src": "192.168.0.0/24", "type": "host", "usr": "all" },
+           { "db": "all", "method": "reject", "src": "192.168.1.0/24", "type": "host", "usr": "all" },
+           { "db": "all", "method": "trust", "src": "172.16.0.0/16", "type": "host", "usr": "all" },
            { "db": "all", "method": "md5", "src": "0:fff0::/28", "type": "host", "usr": "all" }
       ]'
     - 'pg_hba_change is changed'