Details

Reviewers

douardda

Group Reviewers

Reviewers

Maniphest Tasks

T4228: scrubber: Investigate the apparent lock (staging)

Commits

rDCORE47d9e8e22fa6: db: Grant read access to guest user on all schema tables

Summary

This now automatically grant read-only access to the guest user at db initialization or
upgrade time. This way, sysadm no longer have to manually alter the schema after
initialization or upgrade.

Related to T4228

Test Plan

tox should still be happy

Diff Detail

Repository

rDCORE Foundations and core functionalities

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 29615
Build 46281: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 46280: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	1,332 ms	Jenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_create_and_init_db cli_runner = <click.testing.CliRunner object at 0x7fa6ea3f4240> postgresql = <connection object at 0x7fa6ebd01908; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=26870 options=''', closed: 0> mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7fa6e86a8378>
	509 ms	Jenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_create_and_init_db_new_api cli_runner = <click.testing.CliRunner object at 0x7fa6e887dc18> postgresql = <connection object at 0x7fa6eb24c2c8; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=26870 options=''', closed: 0> mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7fa6e86e67b8>
	207 ms	Jenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_initialization_idempotent swh_db_cli = (<click.testing.CliRunner object at 0x7fa6e883e198>, {'channel_binding': 'prefer', 'dbname': 'tests', 'gssencmode': 'prefer', 'host': '127.0.0.1', ...}) mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7fa6e8644b70> postgresql = <connection object at 0x7fa6eb2d1048; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=26870 options=''', closed: 0>
	509 ms	Jenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_initialization_with_env swh_db_cli = (<click.testing.CliRunner object at 0x7fa6ea427588>, {'channel_binding': 'prefer', 'dbname': 'tests', 'gssencmode': 'prefer', 'host': '127.0.0.1', ...}) mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7fa6e86449d8> postgresql = <connection object at 0x7fa6ebd01908; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=26870 options=''', closed: 0>
	400 ms	Jenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_initialization_works_with_flags cli_runner = <click.testing.CliRunner object at 0x7fa6e882b5c0> postgresql = <connection object at 0x7fa6eb108cc8; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=26870 options=''', closed: 0> mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7fa6e8985048>
		View Full Test Results (10 Failed · 288 Passed · 1 Skipped)

Event Timeline

ardumont created this revision.May 31 2022, 9:19 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 31 2022, 9:19 AM

ardumont edited the summary of this revision. (Show Details)May 31 2022, 9:21 AM

Build has FAILED

Patch application report for D7913 (id=28524)

Rebasing onto 749ac571a7...

Current branch diff-target is up to date.

Changes applied before test

commit 0c2ff429ef9b5b5d2a5bab4e92944227a6b531fb
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue May 31 09:18:03 2022 +0200

    db: Grant read access to guest user on all tables of the schema
    
    Whether at db initialization or upgrade time.
    
    Related to T4228

Link to build: https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/434/
See console output for more information: https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/434/console

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 9:21 AM

Harbormaster failed remote builds in B29615: Diff 28524!

Adapt tests to define the role guest in their db

Build is green

Patch application report for D7913 (id=28525)

Rebasing onto 749ac571a7...

Current branch diff-target is up to date.

Changes applied before test

commit a5918c1b25ce8819172683ec841bfeab73efe564
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue May 31 09:18:03 2022 +0200

    db: Grant read access to guest user on all tables of the schema
    
    Whether at db initialization or upgrade time.
    
    Related to T4228

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/435/ for more details.

Harbormaster completed remote builds in B29616: Diff 28525.May 31 2022, 9:48 AM

ardumont requested review of this revision.May 31 2022, 9:48 AM

ardumont edited the summary of this revision. (Show Details)May 31 2022, 9:50 AM

ardumont edited the summary of this revision. (Show Details)May 31 2022, 9:57 AM

Tests for the cli are using again the pytest-plugin db tool (i think it got removed at some point for some reason).

I believe this comes from f8a07dfa0bf6

The question being, is it required to use this version of the postgresql factory instead of the stock one?
I was hoping to rather get rid of this custom DbJanitor back then: it generates more complexity than it saves time. And now that we support recent-ish versions of pytest-postgresql, we do benefit from the "test db creation from template" feature, making the startup time for each test pretty similar than using this custom janitor.

With @douardda, we agreed on irc to postpone this diff a bit the time david found back how to properly update the postgres template (to integrate the guest user).
So the part spawning back the pytest-plugin postgresql_fact becomes unnecessary [1]

[1] That's getting slowly retired from other swh modules.

ardumont planned changes to this revision.May 31 2022, 11:28 AM

@douardda Any news on how to modify a db template for the tests?

@douardda Any news on how to modify a db template for the tests?

15:32 douardda ben normalement, l'argument load du postgresql_proc() va etre utilisé pour creer le template de base utilisé ensuite pour creer la base pour chaque test
15:33 douardda c'est ce que j'utilise dans https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/pytest_plugin.py$20-28 par ex
15:34 douardda la le 'load' va de facto executer la mecanique swh.core d'init d'une base
15:34 ardumont ok dc je devrais pouvoir utiliser ca et ajouter une fonction qui ajoute un user guest ds la liste passee a 'load'
15:35 douardda tu peux ajouter un chemin de script sql directement comme element dans cette list

ardumont edited the summary of this revision. (Show Details)Jun 3 2022, 5:51 PM

Adapt according to exchange

ardumont edited the summary of this revision. (Show Details)Jun 3 2022, 5:53 PM

Build is green

Patch application report for D7913 (id=28683)

Rebasing onto e1a1d84eb4...

Current branch diff-target is up to date.

Changes applied before test

commit cc5e2957e739e31b2b448f5fb90a97af415ace35
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue May 31 09:18:03 2022 +0200

    db: Grant read access to guest user on all schema tables
    
    This now automatically grant read-only access to the guest user at db initialization or
    upgrade time. This way, sysadm no longer have to manually alter the schema after
    initialization or upgrade.
    
    Related to T4228

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/444/ for more details.

Harbormaster completed remote builds in B29765: Diff 28683.Jun 3 2022, 5:56 PM

One remark from @vsellier got my attention, it will break the docker setup.
Maybe @douardda hinted at it as well...

He is not wrong there, see for example [1].
So some adaptations will be needed there.

[1]

swh-scheduler_1                     | swh-scheduler database setup
swh-scheduler_1                     | Creating extensions...
swh-scheduler_1                     | psql:/srv/softwareheritage/venv/lib/python3.7/site-packages/swh/scheduler/sql/10-superuser-init.sql:1: NOTICE:  extension "uuid-ossp" already exists, skipping
swh-scheduler_1                     | Traceback (most recent call last):
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/bin/swh", line 33, in <module>
swh-scheduler_1                     |     sys.exit(load_entry_point('swh.core', 'console_scripts', 'swh')())
swh-scheduler_1                     |   File "/src/swh-core/swh/core/cli/__init__.py", line 184, in main
swh-scheduler_1                     |     return swh(auto_envvar_prefix="SWH")
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 1137, in __call__
swh-scheduler_1                     |     return self.main(*args, **kwargs)
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 1062, in main
swh-scheduler_1                     |     rv = self.invoke(ctx)
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 1668, in invoke
swh-scheduler_1                     |     return _process_result(sub_ctx.command.invoke(sub_ctx))
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 1668, in invoke
swh-scheduler_1                     |     return _process_result(sub_ctx.command.invoke(sub_ctx))
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 1404, in invoke
swh-scheduler_1                     |     return ctx.invoke(self.callback, **ctx.params)
swh-scheduler_1                     |   File "/srv/softwareheritage/venv/lib/python3.7/site-packages/click/core.py", line 763, in invoke
swh-scheduler_1                     |     return __callback(*args, **kwargs)
swh-scheduler_1                     |   File "/src/swh-core/swh/core/cli/db.py", line 123, in db_init_admin
swh-scheduler_1                     |     init_admin_extensions(module, dbname)
swh-scheduler_1                     |   File "/src/swh-core/swh/core/db/db_utils.py", line 607, in init_admin_extensions
swh-scheduler_1                     |     execute_sqlfiles(sqlfiles, conninfo)
swh-scheduler_1                     |   File "/src/swh-core/swh/core/db/db_utils.py", line 700, in execute_sqlfiles
swh-scheduler_1                     |     c.execute(query)
swh-scheduler_1                     | psycopg2.errors.UndefinedObject: role "guest" does not exist
swh-scheduler_1                     |

One remark from @vsellier got my attention, it will break the docker setup.
Maybe @douardda hinted at it as well...

He is not wrong there, see for example [1].
So some adaptations will be needed there.

[1]

Taken care of with D7965.

ardumont mentioned this in D7965: postgres db: Create guest user at db initialization time.Jun 7 2022, 2:36 PM

maybe only show a warning if the grant query fails (rather than crashing)?

This revision is now accepted and ready to land.Jun 8 2022, 10:02 AM

maybe only show a warning if the grant query fails (rather than crashing)?

good point, i'll check how to properly do that.

@vsellier suggested to check if the guest user exist but i guess all this are implementation detail.

What we want is indeed not to crash if this grant does not work for whatever reasons.

Thanks.

Warn if failing to grant read-only access to guest user

Build is green

Patch application report for D7913 (id=28709)

Rebasing onto e1a1d84eb4...

Current branch diff-target is up to date.

Changes applied before test

commit 47d9e8e22fa6c88eadbd552eebfbc42e51f72813
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue May 31 09:18:03 2022 +0200

    db: Grant read access to guest user on all schema tables
    
    This now automatically grant read-only access to the guest user at db initialization or
    upgrade time. This way, sysadm no longer have to manually alter the schema after
    initialization or upgrade.
    
    Related to T4228

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/445/ for more details.

Harbormaster completed remote builds in B29791: Diff 28709.Jun 8 2022, 10:41 AM

Closed by commit rDCORE47d9e8e22fa6: db: Grant read access to guest user on all schema tables (authored by ardumont). · Explain WhyJun 8 2022, 10:55 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDCORE47d9e8e22fa6: db: Grant read access to guest user on all schema tables.

db: Grant read access to guest user on all tables of the schema
ClosedPublic
Actions

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Patch application report for D7913 (id=28524)

Changes applied before test

Patch application report for D7913 (id=28525)

Changes applied before test

Patch application report for D7913 (id=28683)

Changes applied before test

Patch application report for D7913 (id=28709)

Changes applied before test

Revision Contents
Changeset List

Diff 28524

swh/core/db/db_utils.py

db: Grant read access to guest user on all tables of the schemaClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Patch application report for D7913 (id=28524)

Changes applied before test

Patch application report for D7913 (id=28525)

Changes applied before test

Patch application report for D7913 (id=28683)

Changes applied before test

Patch application report for D7913 (id=28709)

Changes applied before test

Revision ContentsChangeset List

Diff 28524

swh/core/db/db_utils.py

db: Grant read access to guest user on all tables of the schema
ClosedPublic
Actions

Unit TestsFailed
View All

Revision Contents
Changeset List