Feature: Add ability to periodically pull config

[YourTechBud]

This is a follow up request to #3994 - Dynamic config loading at startup.

Currently the config loading is a startup activity. As suggested in the PR, it would be great if we could pull this config periodically at a configurable time interval.

[YourTechBud]

Wanted to quickly follow up on this since I needed this feature for the project I've been working on. I can spare some time to submit a PR over the weekend. I had two proposals in mind

1] Change the function signature of the LoadConfig method

This one is a breaking change but seems to make the most sense. Currently the signature of the LoadConfig method is

func (cl ConfigLoader) LoadConfig(ctx caddy.Context) ([]byte, error) {
  var config []byte
  // Load the marshaled json config in the `config` variable
  return config, nil
}

I suggest we change it to

func (cl ConfigLoader) LoadConfig(ctx caddy.Context) (chan []byte, error) {
  configC := make(chan []byte, 0) // It's important for the channel buffer size to be 0
  // Write to the configC channel as many times as necessary in a separate goroutine
  return configC, nil
}

In the admin module we can change the caller of this method to look something like this

	// if dynamic config is requested, set that up and run it
	if cfg != nil && cfg.Admin != nil && cfg.Admin.Config != nil && cfg.Admin.Config.LoadRaw != nil {
		val, err := ctx.LoadModule(cfg.Admin.Config, "LoadRaw")
		if err != nil {
			return fmt.Errorf("loading config loader module: %s", err)
		}
		loadedConfigC, err := val.(ConfigLoader).LoadConfig(ctx)
		if err != nil {
			return fmt.Errorf("loading dynamic config from %T: %v", val, err)
		}

		// do this in a goroutine so current config can finish being loaded; otherwise deadlock
		go func() {
                        for loadedConfig := range loadedConfigC {
			        Log().Info("applying dynamically-loaded config", zap.String("loader_module", val.(Module).CaddyModule().ID.Name()))
			        currentCfgMu.Lock()
			        err := unsyncedDecodeAndRun(loadedConfig, false)
			        currentCfgMu.Unlock()
			        if err == nil {
				        Log().Info("dynamically-loaded config applied successfully")
			        } else {
				        Log().Error("running dynamically-loaded config failed", zap.Error(err))
			        }
                        }
		}()
	}

This change will give maximum flexibility to the developer. The developer can go on to implement their own pull/push based config sync solution. Closing the channel after writing to it once preserves existing behavior.

2] Add another optional method to the caddy.config_loaders namespace

Here we add another LoadConfigC function which returns a chan []byte instead of []byte. The admin module can check if this optional method is implemented and will use this to load the config. Otherwise it will default to the existing method and implementation.

[mholt]

Thanks for the proposals. I don't especially love either of them yet (need more time to think about it) -- but first, I have a question:

Change the function signature of the LoadModule method

Did you mean the LoadConfig method?

I'll need to spend some time deciding how the receiving side of the configs would be best architected.

[YourTechBud]

Yes. I meant LoadConfig. My bad.

I just feel using a channel would provide maximum flexibility here. The only downside is that we won't be able to handle errors if any.

To overcome that we can use a callback instead of a channel.

[mholt]

I'll circle back around to this sometime after after the 2.4 release. 👍

[YourTechBud]

Hi @mholt. I was wondering if we could circle back to this issue now? I don't mind contributing this feature since I will be needing it

[mholt]

I don't think we should change the exported API.

Currently, recursive config loads are disallowed to prevent accidental (and tight) infinite loops. But this could be a useful way to implement periodic pulling: simply pull a config that is itself configured to pull a config also. This way, pulling could be terminated simply by receiving a config that isn't configured to pull again. Of course, we'd have to avoid tight looping by implementing a timer. We could then allow recursive config pulling if there's also a timer configured.

If you need this feature, want to try implementing this and let us know if it works? I think the right place for the timer duration is a new field in ConfigSettings:

caddy/admin.go

Lines 97 to 112 in 2de7e14

	// ConfigSettings configures the management of configuration.
	type ConfigSettings struct {
	// Whether to keep a copy of the active config on disk. Default is true.
	// Note that "pulled" dynamic configs (using the neighboring "load" module)
	// are not persisted; only configs that are pushed to Caddy get persisted.
	Persist *bool `json:"persist,omitempty"`
	// Loads a configuration to use. This is helpful if your configs are
	// managed elsewhere, and you want Caddy to pull its config dynamically
	// when it starts. The pulled config completely replaces the current
	// one, just like any other config load. It is an error if a pulled
	// config is configured to pull another config.
	//
	// EXPERIMENTAL: Subject to change.
	LoadRaw json.RawMessage `json:"load,omitempty" caddy:"namespace=caddy.config_loaders inline_key=module"`
	}

Aside from implementing the timer, you'll need to adjust these lines:

caddy/caddy.go

Lines 262 to 273 in 2de7e14

	// prevent recursive config loads; that is a user error, and
	// although frequent config loads should be safe, we cannot
	// guarantee that in the presence of third party plugins, nor
	// do we want this error to go unnoticed (we assume it was a
	// pulled config if we're not allowed to persist it)
	if !allowPersist &&
	newCfg != nil &&
	newCfg.Admin != nil &&
	newCfg.Admin.Config != nil &&
	newCfg.Admin.Config.LoadRaw != nil {
	return fmt.Errorf("recursive config loading detected: pulled configs cannot pull other configs")
	}

to allow recursive pulling if a duration greater than zero is configured. That should probably be all that is needed.

[YourTechBud]

I see what you are suggesting. I do have some questions however. When will the subsequent call to this API happen?

Are we suggesting a goroutine will go off if we encounter a timer value greater than zero? We'll call the exported API once the timer expires? Or should we simply block the current routine.

Also? What if a call to the admin API is made while the timer is active? Do we want to account for that? Cause that could potentially lead to two active timers ticking simultaneously in separate routines.

[mholt]

Good questions. Yeah, if a timer duration is specified, then the loading of the config will need to be done in a goroutine so that the timer can block the config load without blocking the rest of the server.

At a glance, I imagine the timer will be inserted in here:

caddy/caddy.go

Lines 477 to 500 in 2de7e14

	// if dynamic config is requested, set that up and run it
	if cfg != nil && cfg.Admin != nil && cfg.Admin.Config != nil && cfg.Admin.Config.LoadRaw != nil {
	val, err := ctx.LoadModule(cfg.Admin.Config, "LoadRaw")
	if err != nil {
	return fmt.Errorf("loading config loader module: %s", err)
	}
	loadedConfig, err := val.(ConfigLoader).LoadConfig(ctx)
	if err != nil {
	return fmt.Errorf("loading dynamic config from %T: %v", val, err)
	}
	// do this in a goroutine so current config can finish being loaded; otherwise deadlock
	go func() {
	Log().Info("applying dynamically-loaded config", zap.String("loader_module", val.(Module).CaddyModule().ID.Name()))
	currentCfgMu.Lock()
	err := unsyncedDecodeAndRun(loadedConfig, false)
	currentCfgMu.Unlock()
	if err == nil {
	Log().Info("dynamically-loaded config applied successfully")
	} else {
	Log().Error("running dynamically-loaded config failed", zap.Error(err))
	}
	}()
	}

probably right before L483 (LoadConfig()). And then, since we'd already be in a goroutine, the goroutine started at L489 could just happen in the same goroutine (i.e. remove the go func() { ... } wrapping).

Also? What if a call to the admin API is made while the timer is active? Do we want to account for that? Cause that could potentially lead to to active timers ticking simultaneously in separate routines.

Also a great question - a goroutine should never be started without knowing how it will stop. Fortunately, we have a Context in that function, so while we wait for the timer, we will also wait for the context to be cancelled. If it is, we can stop the timer and exit the goroutine. Similar to what we do in the reverse proxy:

caddy/modules/caddyhttp/reverseproxy/reverseproxy.go

Lines 764 to 769 in 7c68809

	select {
	case <-time.After(time.Duration(lb.TryInterval)):
	return true
	case <-ctx.Done():
	return false
	}

[YourTechBud]

Got it. Will try to submit a PR over the weekend.

[colinaaa]

Any progress on this? We need this feature too, and I would like to help

[francislavoie]

I think we can close this, it's been implemented in #4246. This will land in v2.4.4!