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: db test on SELinux enabled systems #2683

Conversation

avallete
Copy link
Member

@avallete avallete commented Sep 14, 2024

What kind of change does this PR introduce?

This PR adds the :z flag to the volume binding for SELinux systems when using the supabase db test command.

Fixes #2659

What is the current behavior?

When running supabase db test on a system with SELinux enabled, pg_tap fails to execute because the /tmp volume does not have the appropriate permissions, causing issues with the bind mount.

What is the new behavior?

With this change, the :z flag ensures the /tmp volume is properly relabeled to allow pg_tap to run successfully on SELinux-enabled systems. There are no expected impacts on systems where SELinux is not enabled.

Additional context

I'm hesitant about applying this change across all volume bindings in the CLI codebase. While this should fix the issue for SELinux users without affecting other systems, there might be security concerns, especially for "dynamic" bindings where the exact directory being mounted is unpredictable. Since this change is intended for local development, the security impact should be minimal, but I’m open to feedback on whether this is the best approach.

Related to supabase#2659
SELinux will require docker to pass the z option to label the
directory as usable inside the container
@avallete avallete requested a review from a team as a code owner September 14, 2024 12:12
@coveralls
Copy link

coveralls commented Sep 14, 2024

Pull Request Test Coverage Report for Build 10906052517

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage remained the same at 60.091%

Files with Coverage Reduction New Missed Lines %
internal/storage/rm/rm.go 2 89.53%
Totals Coverage Status
Change from base Build 10897542975: 0.0%
Covered Lines: 6443
Relevant Lines: 10722

💛 - Coveralls

@avallete avallete force-pushed the avallete/fix-db-test-selinux-binding-container branch from efd4bd8 to 7e1776d Compare September 14, 2024 13:39
@avallete
Copy link
Member Author

Made the changes to scope the z flags to Linux with SELinux installed environments only.

Changed the volume binding option across the codebase instead of just for the db test command.

@sweatybridge
Copy link
Contributor

sweatybridge commented Sep 17, 2024

I'm not fully convinced about the scope of this change. We use different docker volume mount types, including named, anonymous, and bind mounts. Perhaps it's only bind mount that's breaking in selinux rather than every mount type.

Therefore, I think it's worth setting up a selinux test to reproduce this error, and to find out if other commands like functions deploy also need to be fixed.

There's also the alternative to disable selinux for a specific container which should be fine for running db tests https://jaosorior.dev/2018/selinux-and-docker-notes/

P.S. We probably need to use bitbucket which supports fedora runner https://bitbucket.org/supabase-cli/setup-cli/src/master/bitbucket-pipelines.yml

@avallete
Copy link
Member Author

avallete commented Sep 17, 2024

Setting up testing before making changes indeed sounds like a better plan. Plus it'll add coverage for one more platform, I'll do that, thank's for pointing in the right direction.

Edit:

I've tried to setup a fedora pipeline with SELinux enabled on bitbucket, sadly it seems that since the host doesn't have it enabled I can't do such things:
Screenshot 2024-09-17 at 13 38 59

I've looked around and it seems like except self-hosted runners there is no easy ways to get this setup under CI/CD. But that's a whole other scope level.

In the meantime I'll try to setup a VM with fedora and SELinux enabled, and see if disabling selinux via volume flag seems to do the trick for db test.

@avallete
Copy link
Member Author

I was able to reproduce the issue locally on a virtual machine. The SecurityOpt option mentioned in the article you linked, @sweatybridge, successfully resolves the problem with the db test run.

However, I wasn't able to find a way to test this in a CI environment. Interestingly, the tests passed in my reproduction environment as well, likely due to our use of mocked filesystems and Docker.

I encountered a separate issue related to an invalid RLIMIT_NOFILE when running both the storage and realtime containers. This seems like a distinct problem that should be addressed independently.

@sweatybridge
Copy link
Contributor

Amazing, thank you for going the extra mile to reproduce this.

Is the rlimit issue a warning or straight error starting storage container? Either way, I'm happy to deal with that separately.

@sweatybridge sweatybridge merged commit 9c1b27a into supabase:develop Sep 17, 2024
13 checks passed
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.

Cannot run db tests (Permission denied on /tmp)
3 participants