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: evtrigs ownership #1489

Merged
merged 2 commits into from
Mar 26, 2025
Merged

fix: evtrigs ownership #1489

merged 2 commits into from
Mar 26, 2025

Conversation

steve-chavez
Copy link
Member

Fixes #1437 (comment).

Moves migrations/db/init-scripts to migrations/db/migrations.

@steve-chavez steve-chavez force-pushed the fix-evtrigs-owner branch 4 times, most recently from 279b824 to 3ca8994 Compare March 22, 2025 00:07
@steve-chavez steve-chavez marked this pull request as ready for review March 22, 2025 00:39
@steve-chavez steve-chavez requested review from a team as code owners March 22, 2025 00:39
@samrose
Copy link
Collaborator

samrose commented Mar 22, 2025

@steve-chavez left a couple of comments, but looks good overall + thanks for tackling this.

@staaldraad might be worth reviewing this from security perspective in case you see some issue we don't here.

@samrose
Copy link
Collaborator

samrose commented Mar 23, 2025

@steve-chavez I guess the other review point on this PR is that we should definitely run through thorough testing in a local infra context to make sure everything works as expected. I'll follow up with you directly on it.

@nthtrung09it
Copy link

hi @steve-chavez, @samrose, I want to ask if this MR breaks the self-hosted version.
In the self-hosted version, there are scripts mounted into the init-scripts folder.

https://github.com/supabase/supabase/blob/884743ac306461b2ac6389bbf15e963679d911c4/docker/docker-compose.yml#L392
CleanShot 2025-03-25 at 12 57 33

And there is a 00-schema.sql which copied in ansible playbook, located inside Dockerfile-15 and Dockerfile-orioledb-17 files.
CleanShot 2025-03-25 at 13 01 28

@samrose
Copy link
Collaborator

samrose commented Mar 25, 2025

hi @steve-chavez, @samrose, I want to ask if this MR breaks the self-hosted version. In the self-hosted version, there are scripts mounted into the init-scripts folder.

https://github.com/supabase/supabase/blob/884743ac306461b2ac6389bbf15e963679d911c4/docker/docker-compose.yml#L392 CleanShot 2025-03-25 at 12 57 33

And there is a 00-schema.sql which copied in ansible playbook, located inside Dockerfile-15 and Dockerfile-orioledb-17 files. CleanShot 2025-03-25 at 13 01 28

Hi @nthtrung09it folks who self host need some form of self-created upgrade support. We don't provide self-hosted upgrade support across our versions, and don't have it on our roadmap. So it is up to people who self host to create a pathway to migrate between versions.

We do open source the fundamentals of our own upgrade path. You may be able to adapt those to your approach, and they are found here https://github.com/supabase/postgres/tree/develop/ansible/files/admin_api_scripts/pg_upgrade_scripts the tool being used is pg_upgrade between instances example https://github.com/supabase/postgres/blob/develop/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh#L406 but it's worth looking at the whole implementation

@samrose
Copy link
Collaborator

samrose commented Mar 25, 2025

hi @steve-chavez, @samrose, I want to ask if this MR breaks the self-hosted version. In the self-hosted version, there are scripts mounted into the init-scripts folder.
https://github.com/supabase/supabase/blob/884743ac306461b2ac6389bbf15e963679d911c4/docker/docker-compose.yml#L392 CleanShot 2025-03-25 at 12 57 33
And there is a 00-schema.sql which copied in ansible playbook, located inside Dockerfile-15 and Dockerfile-orioledb-17 files. CleanShot 2025-03-25 at 13 01 28

Hi @nthtrung09it folks who self host need some form of self-created upgrade support. We don't provide self-hosted upgrade support across our versions, and don't have it on our roadmap. So it is up to people who self host to create a pathway to migrate between versions.

We do open source the fundamentals of our own upgrade path. You may be able to adapt those to your approach, and they are found here https://github.com/supabase/postgres/tree/develop/ansible/files/admin_api_scripts/pg_upgrade_scripts the tool being used is pg_upgrade between instances example https://github.com/supabase/postgres/blob/develop/ansible/files/admin_api_scripts/pg_upgrade_scripts/initiate.sh#L406 but it's worth looking at the whole implementation

If you question is just about making the docker-compose.yml file in supabase/supabase work with these changes, we can update Dockerfiles and docker-compose.yml files sure. We just don't provide tooling for self-hosted upgrading, but we can update Dockerfile and compose files so that they will work

@nthtrung09it
Copy link

@samrose Yes, it's very cool to update Dockerfile and compose files for the self-hosted version. As I mentioned above, there are 3 files (98-webhooks.sql, 99-roles.sql, 99-jwt.sql) in docker-compose.yml in supabase/supabase and 1 file (00-schema.sql) in the Dockerfile.

@samrose
Copy link
Collaborator

samrose commented Mar 25, 2025

@samrose Yes, it's very cool to update Dockerfile and compose files for the self-hosted version. As I mentioned above, there are 3 files (98-webhooks.sql, 99-roles.sql, 99-jwt.sql) in docker-compose.yml in supabase/supabase and 1 file (00-schema.sql) in the Dockerfile.

We'll take a look thanks @nthtrung09it

@steve-chavez steve-chavez requested a review from a team as a code owner March 25, 2025 17:08
@steve-chavez steve-chavez marked this pull request as draft March 25, 2025 17:16
Fixes #1437

+ Moves migrations/db/init-scripts to migrations/db/migrations.
+ Make initial migrations idempotent.
+ Adds test for event triggers.
@steve-chavez
Copy link
Member Author

Previously the pgtap tests on

set role postgres;
create table test_priv();
SELECT table_owner_is('test_priv', 'postgres');
SELECT table_privs_are('test_priv', 'supabase_admin', array['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE']);
SELECT table_privs_are('test_priv', 'postgres', array['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE']);
SELECT table_privs_are('test_priv', 'anon', array['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE']);
SELECT table_privs_are('test_priv', 'authenticated', array['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE']);
SELECT table_privs_are('test_priv', 'service_role', array['DELETE', 'INSERT', 'REFERENCES', 'SELECT', 'TRIGGER', 'TRUNCATE', 'UPDATE']);
reset role;

Hid the reason of the failure, which was uncovered thanks to #1496:

default_privs.out       2025-03-26 01:15:37.820796798 +0000
2025-03-26T01:15:39.6288966Z postgres> @@ -5,8 +5,5 @@
2025-03-26T01:15:39.6289239Z postgres>   supabase_admin | public          | S
2025-03-26T01:15:39.6289567Z postgres>   supabase_admin | public          | f
2025-03-26T01:15:39.6290033Z postgres>   supabase_admin | public          | r
2025-03-26T01:15:39.6290355Z postgres> - postgres       | public          | S
2025-03-26T01:15:39.6290660Z postgres> - postgres       | public          | f
2025-03-26T01:15:39.6290965Z postgres> - postgres       | public          | r
2025-03-26T01:15:39.6291242Z postgres> -(6 rows)
2025-03-26T01:15:39.6291446Z postgres> +(3 rows)

See ci logs.

This means that the ALTER DEFAULT PRIVILEGES needs to run under supabase_admin and postgres, the latter is missing now.

@steve-chavez
Copy link
Member Author

hi @steve-chavez, @samrose, I want to ask if this MR breaks the self-hosted version.

@nthtrung09it It doesn't look like it does. Those init-scripts references come from a folder that's created inside the Dockerfile, they don't come from the source directory.

The migrations should be preserved given the following:

COPY migrations/db /docker-entrypoint-initdb.d/

COPY migrations/db /docker-entrypoint-initdb.d/

That being said, ideally we should build the Dockerfile directly from Nix, so tests passing would ensure the docker image builds.

Comment on lines 114 to 118
-- These are required so that the users receive grants whenever "postgres" creates tables/function
grant usage on schema public to postgres, anon, authenticated, service_role;
alter default privileges in schema public grant all on tables to postgres, anon, authenticated, service_role;
alter default privileges in schema public grant all on functions to postgres, anon, authenticated, service_role;
alter default privileges in schema public grant all on sequences to postgres, anon, authenticated, service_role;
alter default privileges for role postgres in schema public grant all on tables to postgres, anon, authenticated, service_role;
alter default privileges for role postgres in schema public grant all on functions to postgres, anon, authenticated, service_role;
alter default privileges for role postgres in schema public grant all on sequences to postgres, anon, authenticated, service_role;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing before, now it's much cleaner and explicit.

Comment on lines +825 to +826
echo "Running migrations tests"
pg_prove -p 5435 -U supabase_admin -h localhost -d postgres -v ${./migrations/tests}/test.sql
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this after the regression tests, which offer better error reporting.

@steve-chavez steve-chavez marked this pull request as ready for review March 26, 2025 02:06
2. Run all `db/migrations` with `supabase_admin` superuser role.
3. Finalize role passwords with `/etc/postgresql.schema.sql` if present.
1. Run all `db/migrations` with `supabase_admin` superuser role.
2. Finalize role passwords with `/etc/postgresql.schema.sql` if present.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a way to test that these ownerships won't regress toward the end of the AMI build too? That kind of testing has proven valuable over time/worth covering usually.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you covered this now

@steve-chavez steve-chavez merged commit ef851d1 into develop Mar 26, 2025
12 checks passed
@steve-chavez steve-chavez deleted the fix-evtrigs-owner branch March 26, 2025 03:49
pcnc added a commit that referenced this pull request Mar 27, 2025
pcnc added a commit that referenced this pull request Mar 27, 2025
* Revert "fix: evtrigs ownership (#1489)"

This reverts commit ef851d1.

* chore: improve migration workflows
@soedirgo soedirgo mentioned this pull request Apr 7, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong ownership for some built-in event triggers
3 participants