Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix installer, CREATE instead of ALL for public schema (Postgres 15) #9903

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Sep 12, 2023

What this PR does / why we need it:

Unfortunately, the installer we shipped in Dataverse 6.0 is broken. As of this writing, here is the expected error:

performing database setup
Admin database connectivity succeeds.
PostgreSQL version: 13
Looks like the user already exists. Continuing.
Couldn't grant privileges on dvndb to dvnap

(Jenkins is also failing.)

This pull request fixes the installer for Postgres 13 and 15 by restoring this statement we've always had:

conn_cmd = "GRANT ALL PRIVILEGES on DATABASE "+pgDb+" to "+pgUser+";"

(In the 6.0 installer it changed to GRANT CREATE.)

This pull request also adjusts permissions for Postgres 15, granting only CREATE instead of ALL to the public schema (which was the intention of the change in 6.0):

if int(pg_major_version) >= 15:
   conn_cmd = "GRANT CREATE ON SCHEMA public TO "+pgUser+";"

This should be sufficient permission to handle the change in Postgres 15. See our docs for details about the change:

You are welcome to experiment with newer versions of PostgreSQL, but please note that as of PostgreSQL 15, permissions have been restricted on the public schema (release notes, EDB blog post, Crunchy Data blog post). The Dataverse installer has been updated to restore the old permissions, but this may not be a long term solution.

I wrote "old permissions" but to be specific, we want to GRANT CREATE (rather than ALL) for Postgres 15.

Note that for the Jenkins test of Postgres 15 below, the GRANT is made by Ansible prior to the installer running. This is because while the installer assumes it can be a Postgres admin, this is not the case in a dataverse-ansible environment (where we do our automated testing). That is, in the dataverse-ansible environment, rather than trust being granted, more granular permissions are set by Ansible, like this:

- name: create dataverse postgres user, set permissions
  postgresql_user:
    db: '{{ db.postgres.name }}'
    name: '{{ db.postgres.user }}'
    password: '{{ db.postgres.pass }}'
    role_attr_flags: 'NOSUPERUSER,CREATEDB,CREATEROLE,INHERIT,LOGIN'
  when: db.use_rds == false

- name: postgresql 15 requires explicit permissions on public schema
  community.postgresql.postgresql_privs:
    db: '{{ db.postgres.name }}'
    privs: CREATE
    type: schema
    objs: public
    role: '{{ db.postgres.user }}'

Per Tuesday standup, we need to fix dvninstall.zip for 6.0... something like this:

  • unzip
  • replace scripts/installer/install.py
  • rezip
  • reupload

Which issue(s) this PR closes:

Closes NONE

Special notes for your reviewer:

Suggestions on how to test this:

  • Test installer with Postgres 13
  • Test installer with Postgres 15
  • Reminder to update dvinstall.zip, per above, if all is well!

Is there a release notes update needed for this change?:

No, and I don't think it's worth updating the 6.0 release notes or announcing we re-uploaded dvinstall.zip. If anyone runs into this, we can link them here.

Additional documentation:

No.

@pdurbin pdurbin added this to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) via automation Sep 12, 2023
@pdurbin pdurbin self-assigned this Sep 12, 2023
@pdurbin pdurbin moved this from Ready for Review ⏩ to IQSS Team - In Progress 💻 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Sep 12, 2023
@pdurbin pdurbin changed the title CREATE instead of ALL for public schema CREATE instead of ALL for public schema (Postgres 15) Sep 12, 2023
@pdurbin pdurbin changed the title CREATE instead of ALL for public schema (Postgres 15) fix installer, CREATE instead of ALL for public schema (Postgres 15) Sep 12, 2023
@pdurbin pdurbin moved this from IQSS Team - In Progress 💻 to Ready for Review ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Sep 12, 2023
@pdurbin pdurbin removed their assignment Sep 12, 2023
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Ready for Review ⏩ to Ready for QA ⏩ Sep 12, 2023
@kcondon kcondon moved this from QA ✅ to IQSS Team - In Progress 💻 in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Sep 13, 2023
@pdurbin pdurbin self-assigned this Sep 13, 2023
@pdurbin
Copy link
Member Author

pdurbin commented Sep 13, 2023

As discussed at standup...

  • We plan to merge this PR because we've already updated dvninstall.zip with the updated install.py (details below). Merging this PR fixes the installer for Postgres 13 and fixes our automated testing.
  • The installer likely needs more work to support Postgres 15. (I started to test this but I'm putting it down.)
  • We have already modified the 6.0 release notes to remove any mention of Postgres 15
  • We are aware that the guides mention Postgres 15 at https://guides.dataverse.org/en/6.0/installation/prerequisites.html#postgresql but in the context of experimentation. This part seems to be inaccurate but we'll leave it alone: "The Dataverse installer has been updated to restore the old permissions"

The md5s match for install.py in this PR and the version in dvninstall.zip, which I just downloaded.

$ md5sum scripts/installer/install.py /tmp/foo/dvinstall/install.py
f99ca354d46def0f23e61b0d74f9759e  scripts/installer/install.py
f99ca354d46def0f23e61b0d74f9759e  /tmp/foo/dvinstall/install.py
$ git log --oneline | head -1
d6727c0bb7 CREATE instead of ALL for public schema
$ git status
On branch lessperm
nothing to commit, working tree clean

@kcondon and others if I missed anything, please add it. Thanks! Off to QA.

@pdurbin pdurbin moved this from IQSS Team - In Progress 💻 to Ready for QA ⏩ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Sep 13, 2023
@pdurbin pdurbin removed their assignment Sep 13, 2023
@pdurbin pdurbin added the Size: 10 A percentage of a sprint. 7 hours. label Sep 13, 2023
@kcondon kcondon self-assigned this Sep 13, 2023
@kcondon kcondon merged commit ca3da56 into develop Sep 13, 2023
2 of 3 checks passed
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Sep 13, 2023
@kcondon kcondon deleted the lessperm branch September 13, 2023 20:55
@pdurbin pdurbin added this to the 6.1 milestone Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 10 A percentage of a sprint. 7 hours.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants