Merge lp:~bloodearnest/charms/trusty/postgresql/trunk into lp:charms/trusty/postgresql

[Simon Davy]

* Add tests for pg_hba.conf generation
     - found/fixed bug in extra_pg_auth handiling
     - found/fixed 2 bugs in master relation handling
     - found/fixed bug in admin_addresses handling

    * Slight refactor of update_pg_hba_conf to make more testable, moving the main
    logic into a separate function with params, rather than embedding stateful
    functions in amongst the logic

Add test coverage of the pg_hba.conf generation, and fix several bugs in the process.

[lp:~bloodearnest/charms/trusty/postgresql/trunk]

133. By Simon Davy on 2015-11-19

add test file

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1470/

review: Approve (automated testing)

[Stuart Bishop (stub)]

I approve of this branch.

review: Approve

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1455/

review: Approve (automated testing)

=== modified file 'hooks/service.py'
--- hooks/service.py	2015-10-01 01:36:42 +0000
+++ hooks/service.py	2015-11-19 21:24:52 +0000
@@ -323,6 +323,22 @@
 
 @data_ready_action
 def update_pg_hba_conf():
+
+    # grab the needed current state
+    config = hookenv.config()
+    rels = context.Relations()
+    path = postgresql.pg_hba_conf_path()
+    with open(path, 'r') as f:
+        pg_hba = f.read()
+
+    # generate the new state
+    pg_hba_content = generate_pg_hba_conf(pg_hba, config, rels)
+
+    # write out the new state
+    helpers.rewrite(path, pg_hba_content)
+
+
+def generate_pg_hba_conf(pg_hba, config, rels):
     '''Update the pg_hba.conf file (host based authentication).'''
     rules = []  # The ordered list, as tuples.
 
@@ -344,8 +360,6 @@
     # user connect to their matching PostgreSQL user, if it exists.
     add('local', 'all', 'all', 'peer')
 
-    rels = context.Relations()
-
     # Peers need replication access as the charm replication user.
     if rels.peer:
         for peer, relinfo in rels.peer.items():
@@ -390,14 +404,14 @@
     # as the relation gets its own user to avoid sharing credentials,
     # and logical replication connections will want to specify the
     # database name.
-    for rel in rels['master']:
+    for rel in rels['master'].values():
         for relinfo in rel.values():
             addr = postgresql.addr_to_range(relinfo['private-address'])
             add('host', 'replication',
                 postgresql.quote_identifier(rel.local['user']),
                 postgresql.quote_identifier(addr),
                 'md5', '# {}'.format(relinfo))
-            if 'database' is rel.local:
+            if 'database' in rel.local:
                 add('host',
                     postgresql.quote_identifier(rel.local['database']),
                     postgresql.quote_identifier(rel.local['user']),
@@ -405,25 +419,20 @@
                     'md5', '# {}'.format(relinfo))
 
     # External administrative addresses, if specified by the operator.
-    config = hookenv.config()
     for addr in config['admin_addresses'].split(','):
         if addr:
-            add('host', 'all', 'all', postgresql.addr_to_range(addr),
+            add('host', 'all', 'all',
+                postgresql.quote_identifier(postgresql.addr_to_range(addr)),
                 'md5', '# admin_addresses config')
 
     # And anything-goes rules, if specified by the operator.
-    for line in config['extra_pg_auth'].splitlines():
-        add((line, '# extra_pg_auth config'))
+    for line in config['extra_pg_auth'].split(','):
+        add(line + '# extra_pg_auth config')
 
     # Deny everything else
     add('local', 'all', 'all', 'reject', '# Refuse by default')
     add('host', 'all', 'all', 'all', 'reject', '# Refuse by default')
 
-    # Load the existing file
-    path = postgresql.pg_hba_conf_path()
-    with open(path, 'r') as f:
-        pg_hba = f.read()
-
     # Strip out the existing juju managed section
     start_mark = '### BEGIN JUJU SETTINGS ###'
     end_mark = '### END JUJU SETTINGS ###'
@@ -438,7 +447,7 @@
     rules.insert(0, (start_mark,))
     rules.append((end_mark,))
     pg_hba += '\n' + '\n'.join(' '.join(rule) for rule in rules)
-    helpers.rewrite(path, pg_hba)
+    return pg_hba
 
 
 def assemble_postgresql_conf():

=== added file 'tests/test_pg_hba_conf.py'
--- tests/test_pg_hba_conf.py	1970-01-01 00:00:00 +0000
+++ tests/test_pg_hba_conf.py	2015-11-19 21:24:52 +0000
@@ -0,0 +1,112 @@
+# Copyright 2015 Canonical Ltd.
+#
+# This file is part of the PostgreSQL Charm for Juju.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3, as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+import os
+import sys
+import unittest
+
+sys.path.append(os.path.join(os.path.dirname(__file__), os.pardir, 'hooks'))
+
+from service import generate_pg_hba_conf
+from collections import defaultdict
+
+
+# set of classes to stub out the context relation interface
+class Relations(defaultdict):
+    def __init__(self, *args, **kwargs):
+        # set RelationSet as default factory
+        super(Relations, self).__init__(RelationSet, *args, **kwargs)
+        self.peer = {}
+
+
+class RelationSet(dict):
+    def add_unit(self, unit, data=None, local=None):
+        rel = RelationData()
+        rel[unit] = {'private-address': '1.2.3.4'}
+        if local is not None:
+            rel.local.update(**local)
+        if data is not None:
+            rel[unit].update(**data)
+        self[unit] = rel
+
+
+class RelationData(dict):
+    def __init__(self, *args, **kwargs):
+        super(RelationData, self).__init__(*args, **kwargs)
+        self.local = defaultdict(str)
+
+
+class TestPgHbaConf(unittest.TestCase):
+
+    def test_no_relations_or_config(self):
+        content = generate_pg_hba_conf('', defaultdict(str), Relations())
+        self.assertIn('local all postgres peer map=juju_charm', content)
+        self.assertIn('local all all peer', content)
+        self.assertIn('local all all reject', content)
+        self.assertIn('host all all all reject', content)
+
+    def test_peer_relation(self):
+        rels = Relations()
+        rels.peer = {
+            'unit/1': {'private-address': '1.2.3.4'},
+        }
+        content = generate_pg_hba_conf('', defaultdict(str), rels)
+        self.assertIn('host replication _juju_repl "1.2.3.4/32" md5', content)
+        self.assertIn('host postgres _juju_repl "1.2.3.4/32" md5', content)
+
+    def test_db_relation(self):
+        rels = Relations()
+        rels['db'].add_unit('unit/1', local={
+            'user': 'user',
+            'database': 'database',
+            'schema_user': 'schema_user',
+        })
+        content = generate_pg_hba_conf('', defaultdict(str), rels)
+        self.assertIn('host "database" "user" "1.2.3.4/32" md5', content)
+        self.assertIn(
+            'host "database" "schema_user" "1.2.3.4/32" md5', content)
+
+    def test_db_admin_relation(self):
+        rels = Relations()
+        rels['db-admin'].add_unit('unit/1', local=({'user': 'user'}))
+        content = generate_pg_hba_conf('', defaultdict(str), rels)
+        self.assertIn('host all "user" "1.2.3.4/32" md5', content)
+
+    def test_master_relation(self):
+        rels = Relations()
+        rels['master'].add_unit('unit/1', local=({
+            'user': 'user',
+            'database': 'database',
+        }))
+        content = generate_pg_hba_conf('', defaultdict(str), rels)
+        self.assertIn('host replication "user" "1.2.3.4/32" md5', content)
+        self.assertIn('host "database" "user" "1.2.3.4/32" md5', content)
+
+    def test_admin_addresses_config(self):
+        rels = Relations()
+        config = defaultdict(str)
+        config['admin_addresses'] = '192.168.1.0/24,10.0.0.0/8,1.2.3.4'
+        content = generate_pg_hba_conf('', config, rels)
+        self.assertIn('host all all "192.168.1.0/24" md5', content)
+        self.assertIn('host all all "10.0.0.0/8" md5', content)
+        self.assertIn('host all all "1.2.3.4/32" md5', content)
+
+    def test_extra_pg_auth(self):
+        rels = Relations()
+        config = defaultdict(str)
+        config['extra_pg_auth'] = 'local all sso md5,local all ssoadmin md5'
+        content = generate_pg_hba_conf('', config, rels)
+        self.assertIn('\nlocal all sso md5', content)
+        self.assertIn('\nlocal all ssoadmin md5', content)