Page MenuHomeSoftware Heritage

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

Authored by ardumont on May 31 2022, 9:19 AM.

Details

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

Event Timeline

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.

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.

@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

Adapt according to exchange

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.

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.

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.