launchpad-reviewers team mailing list archive

[Colin Watson <mp+448506@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:set-role-commit into launchpad:master.

Commit message:
Commit after running SET ROLE

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/448506

In charmed deployments configured with `set_role_after_connecting: True`, we execute a `SET ROLE` command immediately after connecting to the database.  However, according to https://www.postgresql.org/docs/10/sql-set.html which is referenced by https://www.postgresql.org/docs/10/sql-set-role.html, the effects of the `SET` command disappear if the transaction is rolled back.  We must therefore commit the transaction to ensure that the effects persist until the end of the session.

I strongly suspect that this is why we've been seeing `buildd-manager` sometimes running as the wrong database role: it's to be expected that Launchpad code will roll back transactions sometimes, for example as part of handling an exception, and if that happened to be one of the first things that `buildd-manager` did then we'd see this effect.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:set-role-commit into launchpad:master.

diff --git a/lib/lp/services/database/sqlbase.py b/lib/lp/services/database/sqlbase.py
index c144f35..75bfe73 100644
--- a/lib/lp/services/database/sqlbase.py
+++ b/lib/lp/services/database/sqlbase.py
@@ -602,6 +602,7 @@ def connect(user=None, dbname=None, isolation=ISOLATION_LEVEL_DEFAULT):
         and user != parsed_dsn["user"]
     ):
         con.cursor().execute("SET ROLE %s", (user,))
+        con.commit()
     return con
 
 
diff --git a/lib/lp/services/database/tests/test_sqlbase.py b/lib/lp/services/database/tests/test_sqlbase.py
index 8d79948..44a17db 100644
--- a/lib/lp/services/database/tests/test_sqlbase.py
+++ b/lib/lp/services/database/tests/test_sqlbase.py
@@ -51,10 +51,16 @@ class TestConnect(TestCase):
         who, _, how = self.examineConnection(con)
         self.assertEqual(("launchpad_main", "serializable"), (who, how))
 
-    def getCurrentUser(self, con: connection) -> None:
+    def assertCurrentUser(self, con: connection, user: str) -> None:
         with con.cursor() as cur:
             cur.execute("SELECT current_user")
-            return cur.fetchone()[0]
+            self.assertEqual(user, cur.fetchone()[0])
+        # Ensure that the role is set for the whole session, not just the
+        # current transaction.
+        con.rollback()
+        with con.cursor() as cur:
+            cur.execute("SELECT current_user")
+            self.assertEqual(user, cur.fetchone()[0])
 
     def test_refuses_connstring_with_user(self):
         connstr = "dbname=%s user=foo" % DatabaseLayer._db_fixture.dbname
@@ -84,7 +90,7 @@ class TestConnect(TestCase):
             {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
             parse_dsn(con.dsn),
         )
-        self.assertEqual("ro", self.getCurrentUser(con))
+        self.assertCurrentUser(con, "ro")
 
     def test_accepts_connstring_uri_without_user(self):
         connstr = "postgresql:///%s" % DatabaseLayer._db_fixture.dbname
@@ -94,7 +100,7 @@ class TestConnect(TestCase):
             {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
             parse_dsn(con.dsn),
         )
-        self.assertEqual("ro", self.getCurrentUser(con))
+        self.assertCurrentUser(con, "ro")
 
     def test_set_role_after_connecting_refuses_connstring_without_user(self):
         connstr = "dbname=%s" % DatabaseLayer._db_fixture.dbname
@@ -134,7 +140,7 @@ class TestConnect(TestCase):
             {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
             parse_dsn(con.dsn),
         )
-        self.assertEqual("read", self.getCurrentUser(con))
+        self.assertCurrentUser(con, "read")
 
     def test_set_role_after_connecting_accepts_connstring_uri_with_user(self):
         connstr = "postgresql://ro@/%s" % DatabaseLayer._db_fixture.dbname
@@ -146,7 +152,7 @@ class TestConnect(TestCase):
             {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
             parse_dsn(con.dsn),
         )
-        self.assertEqual("read", self.getCurrentUser(con))
+        self.assertCurrentUser(con, "read")
 
     def test_set_role_after_connecting_not_member(self):
         connstr = "dbname=%s user=ro" % DatabaseLayer._db_fixture.dbname
diff --git a/lib/lp/services/webapp/adapter.py b/lib/lp/services/webapp/adapter.py
index 755d7d7..4879e27 100644
--- a/lib/lp/services/webapp/adapter.py
+++ b/lib/lp/services/webapp/adapter.py
@@ -470,6 +470,7 @@ class LaunchpadDatabase(Postgres):
 
         if dbconfig.set_role_after_connecting and dbuser != parsed_dsn["user"]:
             raw_connection.cursor().execute("SET ROLE %s", (dbuser,))
+            raw_connection.commit()
 
         # Set read only mode for the session.
         # An alternative would be to use the _ro users generated by
diff --git a/lib/lp/services/webapp/tests/test_adapter.py b/lib/lp/services/webapp/tests/test_adapter.py
index 42cdb2a..fce1642 100644
--- a/lib/lp/services/webapp/tests/test_adapter.py
+++ b/lib/lp/services/webapp/tests/test_adapter.py
@@ -1,6 +1,7 @@
 # Copyright 2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import transaction
 from psycopg2.errors import InsufficientPrivilege
 from psycopg2.extensions import parse_dsn
 from zope.component import getUtility
@@ -27,6 +28,12 @@ class TestLaunchpadDatabase(TestCase):
         self.assertEqual(
             user, store.execute("SELECT current_user").get_one()[0]
         )
+        # Ensure that the role is set for the whole session, not just the
+        # current transaction.
+        transaction.abort()
+        self.assertEqual(
+            user, store.execute("SELECT current_user").get_one()[0]
+        )
 
     def test_refuses_connstring_with_user(self):
         connstr = "dbname=%s user=foo" % DatabaseLayer._db_fixture.dbname