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
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp