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: in-db config values not loading for pgrst.server_trace_header and pgrst.server_cors_allowed_origins #3346

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

laurenceisla
Copy link
Member

Closes #3345

@laurenceisla
Copy link
Member Author

I tried using reReadConfig here, since it looks like the in-db configuration is not loaded:

run :: AppState -> (Observation -> IO ()) -> IO ()
run appState observer = do
observer $ AppServerStartObs prettyVersion
conf@AppConfig{..} <- AppState.getConfig appState

But after reloading the config the values will not update, since it will keep the old config values specified above.


Still, I'm not entirely sure if this is the best solution but it works in using the updated in-db configuration values.

@laurenceisla laurenceisla marked this pull request as ready for review March 25, 2024 23:08
Copy link
Member

@steve-chavez steve-chavez left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work 💯

To correctly test in-db override of config file values, the former must be different from the latter.
@laurenceisla laurenceisla merged commit 378c111 into PostgREST:main Mar 27, 2024
25 checks passed
@laurenceisla laurenceisla deleted the fix-dbconfig branch March 27, 2024 20:59
@wolfgangwalther
Copy link
Member

I still don't understand why we want to load server-xxx configs from the database at all. This still feels wrong to me, but I can't really articulate why.

@steve-chavez
Copy link
Member

server-timing-enabled makes a lot of sense for instance. If you'd want to quickly get timings for a request. Changing that through the db is much quicker than through the config.

server_cors_allowed_origins and server-trace-header might not be that useful as in-db config. But I don't see why not.

IMO the only things that shouldn't are server-host, port, etc.. things that could bring down the postgREST service.

@wolfgangwalther
Copy link
Member

Maybe it's also the other way around. Maybe I feel like the server- prefix is not correct for stuff that can actually be sensibly set via database configuration.

@steve-chavez
Copy link
Member

steve-chavez commented Apr 2, 2024

Maybe I feel

Perhaps we can settle this with a shared design principle.

I'd like to pursue extreme-database centric architecture (pdf), which goes a bit beyond "db as source of truth". Right now:

  • we're kinda db-centric (since we're two tier) but I'd like to get closer to the extreme part.
  • we cannot do the server-host, server-port and others but there could be a future where we do.

For now following this principle for PostgREST means that everything should be configurable through the db. WDYT?

@wolfgangwalther
Copy link
Member

For now following this principle for PostgREST means that everything should be configurable through the db. WDYT?

For the kubernetes-world, this would be a step backward. If you run PostgREST in a cluster, it should be configured via kubernetes, not the database.

@wolfgangwalther
Copy link
Member

I think the original idea of database config was to put the config for things to the same place where a change would occur, which would make the config need to change. Examples:

  • You need to change db-schemas, if you change your primary schema in the database. The change happens in the database, so the config option should be, too.
  • You might need to change server-port, if you change things around your networking setup. This happens outside the database and has zero relevance for it.

Or to reframe it: When configuring PostgREST, I would like to theoretically point multiple instances of PostgREST, with possibly very different "outside" environments, at the same database with the same postgrest user. In this case, all the config that is the same, because it depends on database stuff, should be in the database. All the config that is about the environment that PostgREST runs in should be outside the database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

pgrst.server_trace_header in-database config not working
3 participants