nebula icon indicating copy to clipboard operation
nebula copied to clipboard

Break apart file loading from Config struct

Open kriive opened this issue 3 years ago • 3 comments

I'm embedding nebula in my binary and I'm passing the configuration through LoadString(), so I'm not passing through the usual config file. When using LoadString() there's no way to Reload a certificate, this PR tries to correct that separating the actual Reload() from ReloadConfig(). Function names may not be the clearest, so I'm super open to suggestions.

kriive avatar Oct 21 '21 14:10 kriive

I'm not quite sure the best way to solve it but this way will cause some unexpected behavior. I completely agree that this path should be supported but I think we need to move some more bits around.

2 things come to mind.

  1. Anything that relies on HasChanged will always reload (unless the caller managed c.oldSettings prior, which is super awkward). This is the main area that needs thought, how best to poke the change and set them up for success without being overly disruptive to the process.
  2. Doesn't need to occur in this PR: In the same vein as #550, the signal notifier should likely be passed to config or handled entirely seperately. This would allow the implementing process to control how/when reload occurs.

A possible solution could be breaking apart the file loading from Config and have Reload expect the config string. This would speak to both points above. The signal notifier and file loading could be handled by the cmd/ services.

nbrownus avatar Oct 27 '21 15:10 nbrownus

I finally found some time (I got the flu, so plenty of time haha) to implement this. I broke apart file reading and loading from the Config struct, so Load and Reload are now variadic and take one or more config as strings.

I added some tests (which I now see I'll have to fix, since they're failing) to cover sorting and some simple cases.

To handle SIGHUP I implemented what I believe is an ugly hack I'm not really fond of. I believe we could implement this better if Main accepted a ctx object initialized in the various cmd/ main()s.

For example:

func Main(ctx context.Context, c *config.C, configTest bool, buildVersion string, logger *logrus.Logger, tunFd *int) (retcon *Control, reterr error) {
	ctx, cancel := context.WithCancel(ctx)
	// Automatically cancel the context if Main returns an error, to signal all created goroutines to quit.
	defer func() {
		if reterr != nil {
			cancel()
		}
	}()
	// ...

At this point we could handle SIGHUPs directly in the package main. What do you think?

kriive avatar Dec 22 '21 15:12 kriive

Any news on this?

kriive avatar Feb 17 '22 08:02 kriive