Merge lp:~stub/charms/precise/postgresql/cleanups into lp:charms/postgresql

[Stuart Bishop]

Remove an unused helper and fix some edge cases in the relation-broken hooks.

[Charles Butler (lazypower)]

Greetings Stuart,

I'm reviewing the merge details and from what I understand is going on here, you're handling some cases where access to the data would happen due to another hook stripping access from the proposed user correct?

I've deployed this branch and poked about in postgres. The block was reached and defaulted away from removing access from the user, which was nice to see that its idempotent in caring about data access and integrity after the relationship departs.

You get my +1, and if I was off base in the review please correct me.

Thanks for your continued effort on maintaining a high quality charm. Another member of the charmers team will be along shortly to continue the review and potentially promulgate your changes.

review: Approve

[lp:~stub/charms/precise/postgresql/cleanups]

105. By Stuart Bishop on 2014-02-04

Merged charm-helpers into cleanups.

106. By Stuart Bishop on 2014-02-05

Require clients to wait for database to acknowledge requested database name, and add example client relationship hooks to demonstrate the protocol more clearly

107. By Stuart Bishop on 2014-02-05

Fix bug in example hooks (relation_get() may return None)

[Stuart Bishop (stub)]

@lazypower Almost, yes. I'm handling the cases where the user or database was blown away completely. This could happen by an administrator doing it manually, or pathological situations in the hooks. For the insanely curious, I can't quite remember the pathalogical situation now, but I think it was something like adding a client relation to a replicated PostgreSQL service and then immediately destroying the PostgreSQL service. A hot standbys could have ended up as a standalone database before the created users and databases had replicated across, and then attempting to remove privileges from objects that never got around to existing. The sort of error only the test suite will ever see.

I managed to pump new documentation into README.md after Charles' review sorry. I can move it elsewhere if it is a problem.

[Marco Ceppi (marcoceppi)]

More READMEs are not a problem at all! We appreciate the effort completely!

[Marco Ceppi (marcoceppi)]

LGTM +1

review: Approve

=== modified file 'README.md'
--- README.md	2014-01-15 10:28:22 +0000
+++ README.md	2014-02-05 12:31:00 +0000
@@ -94,7 +94,9 @@
 ### the client service provides:
 
 - `database`: Optional. The name of the database to use. The postgresql service
-  will create it if necessary.
+  will create it if necessary. If your charm sets this, then it must wait
+  until a matching `database` value is presented on the PostgreSQL side of
+  the relation (ie. `relation-get database` returns the value you set).
 - `roles`: Optional. A comma separated list of database roles to grant the
   database user. Typically these roles will have been granted permissions to
 access the tables and other database objects.  Do not grant permissions
@@ -154,6 +156,71 @@
 even having the service revert to a single 'standalone' unit.
 
 
+## Example client hooks
+
+Python::
+
+    import sys
+    from charmhelpers.core.hookenv import (
+        Hooks, config, relation_set, relation_get,
+        local_unit, related_units, remote_unit)
+
+    hooks = Hooks()
+    hook = hooks.hook
+
+    @hook
+    def db_relation_joined():
+        relation_set('database', config('database'))  # Explicit database name
+        relation_set('roles', 'reporting,standard')  # DB roles required
+
+    @hook
+    def db_relation_changed():
+        db_changed(related_units())
+
+    @hook
+    def db_relation_departed():
+        # Note we currently have to explicitly filter the dying unit
+        # from our list of database units due to LP Bug #1192433
+        db_changed(
+            unit for unit in related_units() if unit != remote_unit())
+
+    @hook
+    def db_relation_broken():
+        db_changed([])
+
+    def db_changed(active_db_units):
+        # Rather than try to merge in just this particular database
+        # connection that triggered the hook into our existing connections,
+        # it is easier to iterate over all active related databases and
+        # reset the entire list of connections.
+        conn_str_tmpl = "dbname={dbname} user={user} host={host} port={port}"
+        master_conn_str = None
+        slave_conn_strs = []
+        for db_unit in active_db_units:
+            if relation_get('database', db_unit) != config('database'):
+                continue  # Not yet acknowledged requested database name.
+
+            allowed_units = relation_get('allowed-units') or ''  # May be None
+            if local_unit() not in allowed_units.split():
+                continue  # Not yet authorized.
+
+            conn_str = conn_str_tmpl.format(**relation_get(unit=db_unit)
+            remote_state = relation_get('state', db_unit)
+
+            if remote_state == 'standalone' and len(active_db_units) == 1:
+                master_conn_str = conn_str
+            elif relation_state == 'master':
+                master_conn_str = conn_str
+            elif relation_state == 'hot standby':
+                slave_conn_strs.append(conn_str)
+
+        update_my_db_config(master=master_conn_str, slaves=slave_conn_strs)
+
+    if __name__ == '__main__':
+        hooks.execute(sys.argv)
+
+
+
 # Contact Information
 
 ## PostgreSQL 

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-02-03 20:56:40 +0000
+++ hooks/hooks.py	2014-02-05 12:31:00 +0000
@@ -1345,11 +1345,16 @@
         user = unit_relation_data.get('user', None)
         database = unit_relation_data['database']
 
-        sql = "REVOKE ALL PRIVILEGES ON DATABASE %s FROM %s"
-        run_sql_as_postgres(sql, AsIs(quote_identifier(database)),
-                            AsIs(quote_identifier(user)))
-        run_sql_as_postgres(sql, AsIs(quote_identifier(database)),
-                            AsIs(quote_identifier(user + "_schema")))
+        # We need to check that the database still exists before
+        # attempting to revoke privileges because the local PostgreSQL
+        # cluster may have been rebuilt by another hook.
+        sql = "SELECT datname FROM pg_database WHERE datname = %s"
+        if run_select_as_postgres(sql, database)[0] != 0:
+            sql = "REVOKE ALL PRIVILEGES ON DATABASE %s FROM %s"
+            run_sql_as_postgres(sql, AsIs(quote_identifier(database)),
+                                AsIs(quote_identifier(user)))
+            run_sql_as_postgres(sql, AsIs(quote_identifier(database)),
+                                AsIs(quote_identifier(user + "_schema")))
 
     postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba)
@@ -1365,8 +1370,13 @@
     if local_state['state'] in ('master', 'standalone'):
         user = hookenv.relation_get('user', unit=hookenv.local_unit())
         if user:
-            sql = "ALTER USER %s NOSUPERUSER"
-            run_sql_as_postgres(sql, AsIs(quote_identifier(user)))
+            # We need to check that the user still exists before
+            # attempting to revoke privileges because the local PostgreSQL
+            # cluster may have been rebuilt by another hook.
+            sql = "SELECT usename FROM pg_user WHERE usename = %s"
+            if run_select_as_postgres(sql, user)[0] != 0:
+                sql = "ALTER USER %s NOSUPERUSER"
+                run_sql_as_postgres(sql, AsIs(quote_identifier(user)))
 
     postgresql_hba = os.path.join(_get_postgresql_config_dir(), "pg_hba.conf")
     generate_postgresql_hba(postgresql_hba)
@@ -1434,25 +1444,6 @@
             os.environ['PGPASSFILE'] = org_pgpassfile
 
 
-def drop_database(dbname, warn=True):
-    import psycopg2
-    timeout = 120
-    now = time.time()
-    while True:
-        try:
-            db_cursor(autocommit=True).execute(
-                'DROP DATABASE IF EXISTS "{}"'.format(dbname))
-        except psycopg2.Error:
-            if time.time() > now + timeout:
-                if warn:
-                    log("Unable to drop database {}".format(dbname), WARNING)
-                else:
-                    raise
-            time.sleep(0.5)
-        else:
-            break
-
-
 def authorized_by(unit):
     '''Return True if the peer has authorized our database connections.'''
     relation = hookenv.relation_get(unit=unit)