feat: config reloading #1764

cstockton · 2024-09-04T15:55:52Z

What kind of change does this PR introduce?

File based configuration reloading using fsnotify.

What is the current behavior?

Currently the Auth config is loaded once from the environment or file (-c flag) and persists until the service is restarted.

What is the new behavior?

The server may be configured with environment variables similar to today, but one or more additional configuration files may be provided as well as a flag to watch those files with fsnotify. When the files are updated the server will reload the configuration files and create a new *api.API object to be used in future requests.

cstockton · 2024-09-05T20:59:30Z

This latest commits contain the remainder of the initial draft of this feature. It's a little rough around the edges and I have some outstanding questions, but this will serve as a talking point.

Feature Details:

When watch-dir (-w <path-to-dir>) is provided as a command line flag a goroutine will be started in serve_cmd.go and begin blocking on a call to (*Reloader).Watch with a callback function that accepts a *conf.GlobalConfiguration object. Each time this function is called we create a new API object and store it within our reloader.AtomicHandler, previously given as the root handler to the *http.Server.

The Reloader uses some simple heuristics to deal with a few edge cases, an overview:

At most 1 configuration reload may happen per 10 seconds with a +-1s margin of error.
After a file within -watch-dir has changed the 10 second grace period begins. After that it will reload the config.
Config reloads first sort each file by name then processes them in sequence.
Directories within watch-dir are ignored during config reloading.
- Implementation quirk: directory changes can trigger a config reload, as I don't stat fsnotify events.
Files that do not end with a .env suffix are ignored.
It handles the removal or renaming of the -watch-dir during runtime, but an error message will be printed every 10 seconds as long as it's missing.

internal/api/cleanup.go

internal/api/listener.go

internal/reloader/handler.go

internal/reloader/handler_race_test.go

internal/api/api.go

cmd/serve_cmd.go

cstockton · 2024-09-06T16:22:09Z

Moving to ready for review, few things to be made aware of:
- Maximum shutdown time changed to 1min + 1min, though in practice it shouldn't happen. I can fix this, but would be more invasive to do correctly.
- Implementation quirk: directory changes can trigger a config reload, as I don't stat fsnotify events. I can add this now or we can wait to see if it's something worth fixing later.
- Logging preferences? In the event the directory was removed it would be a good bit of noise. I poked around some for duplicate log suppression but didn't see anything. I can easily add something to this area of the code though.

hf · 2024-09-06T16:24:40Z

Implementation quirk: directory changes can trigger a config reload, as I don't stat fsnotify events. I can add this now or we can wait to see if it's something worth fixing later.

Maybe at least check if the change resulted in a file that would affect the change? Like a .env or .conf extension on the file?

Logging preferences? In the event the directory was removed it would be a good bit of noise. I poked around some for duplicate log suppression but didn't see anything. I can easily add something to this area of the code though.

Actually that's a good point. Maybe if the dir gets removed or moved to another location it should just panic? That's not something you want to do to a running server. I'd rather it fail early than continue with some invalid state.

cstockton · 2024-09-06T16:50:34Z

Maybe at least check if the change resulted in a file that would affect the change? Like a .env or .conf extension on the file?

For directory changes I meant: changes within the directory that are a directory and not a file. For example:

mkdir auth.d
cp -a example.env /auth.d/example.env
auth -w auth.d &
mkdir auth.d/i-am-a-directory-but-still-trigger-fsnotify

I currently don't perform an os.Stat on each file change, thus I can't differentiate between dirs and files. If it becomes a common infra pattern to create another directory within the config dir it will result in superfluous reloads. I figured in practice this won't occur so avoiding the os.Stat is okay.

Actually that's a good point. Maybe if the dir gets removed or moved to another location it should just panic? That's not something you want to do to a running server. I'd rather it fail early than continue with some invalid state.

I do my best to make sure the auth server is only reloaded when the config file is valid, so the process will continue running in the last known good state. The main thing I was trying to mitigate with allowing it to recover from a temporarily missing config dir was someone doing:

mkdir /auth.d.deploy
cp -a new-configs/* /auth.d.deploy
mv /auth.d /auth.d.rollback
mv /auth.d.deploy /auth.d

I can panic if you would prefer once it's removed, just a little worried some infra tooling might end up doing an mv with staged config dir instead of modifying the existing one. If we decide not to panic, would you like me to implement log suppression? If so I can do something local to reloader if you don't have an existing pattern for that.

hf · 2024-09-06T16:57:11Z

cmd/root_cmd.go

@@ -23,7 +26,7 @@ var rootCmd = cobra.Command{
 func RootCommand() *cobra.Command {
 	rootCmd.AddCommand(&serveCmd, &migrateCmd, &versionCmd, adminCmd())
 	rootCmd.PersistentFlags().StringVarP(&configFile, "config", "c", "", "the config file to use")
-
+	rootCmd.PersistentFlags().StringVarP(&watchDir, "watch-dir", "w", "", "config directory to watch for changes")


Maybe to be more explicit:

Suggested change

rootCmd.PersistentFlags().StringVarP(&watchDir, "watch-dir", "w", "", "config directory to watch for changes")

rootCmd.PersistentFlags().StringVarP(&watchDir, "watch-config-dir", "w", "", "config directory to watch for changes")

hf · 2024-09-06T16:58:19Z

internal/conf/configuration.go


+func LoadGlobalFiles(filenames ...string) (*GlobalConfiguration, error) {


Just double making sure, this will load config like:

Environment

File A

File B

File C

Right? Environment goes first?

Correct, but the Environment is only used for startup. It is overwritten since godotenv writes to Go's copy of the process environment with os.Setenv. I didn't touch any of the logic for starting the process. I was thinking the purpose of keeping the -c flag was for the initialization and the config dir was for runtime changes that overwrote it. But maybe it shouldn't work this way?

How it works now:

# process export GOTRUE_base_env_var_01=... export GOTRUE_base_env_var_01=... auth -c '/base.conf' -w '/auth.d'

Before the first request:

os.Environ() already contains process values

conf.LoadGlobal("/base.conf")

/base.conf is read, overriding any env vars (by godotenv.Override)

OR

conf.LoadGlobal("") (-c not flag empty or missing)

.env is optionally loading, env vars have precedence (godotenv.Load)

The root handler is constructed and begins serving requests with this initialization configuration. Nothing is touched in the -watch-dir directory. If later on someone adds files there, they will start taking precedence. Since godotenv writes to the environment, they are lost if overwritten.

If auth.d should be part of initialization above we should probably make the two flags mutually exclusive, or define the ordering behavior concretely. That probably makes more sense come to think of it.

On both initialization and config.Reload, do the same exact behavior:

conf.LoadGlobalFiles(base, configDirFiles...)

first if base is "" (do as we do now)

check for ".env", don't override env vars

load base, override all env vars

second if configDirFiles is not empty, load each one in sequence