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

feat: enable runtime configuration reloads for auth #1229

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cstockton
Copy link

As part of a longer term goal to support runtime configuration loading the auth team would like to make use of the --config-dir flag and ensure the /etc/auth.d folder is created. (Related: auth - feat: config reloading)

This is a small diff doing three things:

  • Create the /etc/auth.d directory.
  • Copies the gotrue-optimizations.service.j2 to also copy the gotrue.generated.env file to the /etc/auth.d directory.
  • Change the gotrue.service.j2 to use the --config-dir flag set to the newly created /etc/auth.d directory.

What kind of change does this PR introduce?

Feature

What is the current behavior?

Right now gotrue.service is started without the --config-dir flag.

What is the new behavior?

The gotrue.service will be started with the --config-dir flag, causing the service to reload configuration changes at runtime.

Additional context

I tested this change and did notice the machines I have access to do not have the /etc/gotrue folder created, I also did not see that folder in the ansible configuration files. I chose to be safe and copy the same invalid path. If the path was an oversight and we want to also update the path of the generated config file to something that exists, I'm open to including that change here.

This is a small diff doing three things:

- Create the `/etc/auth.d` directory.
- Copies the `gotrue-optimizations.service.j2` to also copy the
  `gotrue.generated.env` file to the `/etc/auth.d` directory.
- Change the `gotrue.service.j2` to use the `--config-dir` flag
  set to the newly created `/etc/auth.d` directory.
@@ -5,6 +5,7 @@ Description=GoTrue (Auth) optimizations
Type=oneshot
# we don't want failures from this command to cause PG startup to fail
ExecStart=/bin/bash -c "/opt/supabase-admin-api optimize auth --destination-config-file-path /etc/gotrue/gotrue.generated.env ; exit 0"
ExecStartPost=/bin/bash -c "cp -a /etc/gotrue/gotrue.generated.env /etc/auth.d/20_generated.env ; exit 0"
Copy link
Member

@kangmingtay kangmingtay Sep 23, 2024

Choose a reason for hiding this comment

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

hmm some projects also have a /etc/gotrue.overrides.env file which we use to overwrite configs temporarily - should those be copied over as well?

Copy link
Author

Choose a reason for hiding this comment

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

I made sure to only make the changes requested in this pull request. @hf might be able to expand long term thoughts here. I did make some notes to bring up later, I'll post them here for transparency:

--

Some options:
So we now copy generated.env over in the gotrue-optimizations.service - but we don't copy "overrides" anywhere. I can't find references to it in all the source code I have access to except for the service file. I also haven't seen it in use on the ec2 instances.

One or more of the follow options if /etc/gotrue.* files should overwrite auth.d files:

ExecStartPre=-/bin/bash -c "cp -a /etc/gotrue.generated.env /etc/auth.d/20_generated.env; exit 0"
ExecStartPre=-/bin/bash -c "cp -a /etc/gotrue.env /etc/auth.d/50_auth.env; exit 0"
ExecStartPre=-/bin/bash -c "cp -a /etc/gotrue.overrides.env /etc/auth.d/70_overrides.env; exit 0"

Or one or more of the following if we only want to copy them if they don't exist already in the auth.d dir:

ExecStartPre=-/bin/bash -c "test -r /etc/auth.d/20_generated.env || cp -a /etc/gotrue.generated.env /etc/auth.d/20_generated.env; exit 0"
ExecStartPre=-/bin/bash -c "test -r /etc/auth.d/50_auth.env || cp -a /etc/gotrue.env /etc/auth.d/50_auth.env; exit 0"
ExecStartPre=-/bin/bash -c test -r /etc/auth.d/70_overrides.env || cp -a /etc/gotrue.overrides.env /etc/auth.d/70_overrides.env; exit 0"

Copy link

@samrose
Copy link
Contributor

samrose commented Sep 24, 2024

@cstockton I'll try to take a look today, the change in gotrue service seems to have broken the integration test that builds an AMI corresponding to this commit, and tests the auth/gotrue and other services

https://github.com/supabase/postgres/actions/runs/11003641978/job/30553062346#step:11:235

Probably going to be something simple we can do to adjust and I will look at it soon.

@cstockton
Copy link
Author

@samrose Thanks sam, apologies, I should have mentioned this set of changes has a direct dependency on the recently merged but currently unreleased changes in supabase/auth#1771. If a flag provided to auth doesn't exist, the server will exit.

@samrose
Copy link
Contributor

samrose commented Sep 24, 2024

@cstockton ah ok. Maybe it was the intention just to keep this in draft until the the auth changes release then?

@cstockton
Copy link
Author

@samrose You're right Sam, I should have been more clear, the intent is to leave this as a draft. I was hoping to get ahead of any feedback you might have on the changes themselves.

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.

3 participants