feat: validate a configuration before it replaces the running one (#2661)

Author: nicolas-grekas

caddy/admin_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -418,3 +419,141 @@ func TestRegisteredModuleWorkerPoolsMustBeCorrect(t *testing.T) {
 	assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker2Path, "expected module worker with \"match\" directive to be present")
 	assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker3Path, "expected module worker without \"match\" directive to be present")
 }
+
+// Validate() must reject incorrect configs before Start() calls frankenphp.Shutdown()
+func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	initServer(t, tester, `
+		{
+			skip_install_trust
+			admin localhost:2999
+			http_port `+testPort+`
+
+			frankenphp {
+				worker ../testdata/worker-with-counter.php 1
+			}
+		}
+
+		localhost:`+testPort+` {
+			route {
+				root ../testdata
+				rewrite worker-with-counter.php
+				php
+			}
+		}
+		`, "caddyfile")
+
+	workerURL := "http://localhost:" + testPort + "/worker-with-counter.php"
+	servedBefore := countedRequests(t, workerURL)
+
+	// the worker file does not exist, which Init() only reports once the
+	// running configuration is gone
+	rejected := `
+	{
+		skip_install_trust
+		admin localhost:2999
+		http_port ` + testPort + `
+
+		frankenphp {
+			worker ../testdata/not-a-worker.php 1
+		}
+	}
+
+	localhost:` + testPort + ` {
+		route {
+			root ../testdata
+			rewrite worker-with-counter.php
+			php
+		}
+	}
+	`
+
+	r, err := http.NewRequest("POST", "http://localhost:2999/load", bytes.NewBufferString(rejected))
+	require.NoError(t, err)
+	r.Header.Set("Content-Type", "text/caddyfile")
+	resp, err := http.DefaultClient.Do(r)
+	require.NoError(t, err)
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	// /load writes the adaptation warnings before loading, so the status is
+	// already 200 when the error follows them in the body
+	require.Contains(t, string(body), "invalid configuration: worker filename is invalid")
+
+	// the runtime that served before the rejected reload still serves, and
+	// it is the same one: its worker kept counting
+	require.Equal(t, servedBefore+1, countedRequests(t, workerURL))
+}
+
+// Validate() cannot see a worker declared in a php_server block: Caddy calls it
+// before the modules provision, so Start() must reject those before shutting down
+func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	initServer(t, tester, `
+		{
+			skip_install_trust
+			admin localhost:2999
+			http_port `+testPort+`
+		}
+
+		localhost:`+testPort+` {
+			route {
+				root ../testdata
+				rewrite worker-with-counter.php
+				php {
+					worker ../testdata/worker-with-counter.php 1
+				}
+			}
+		}
+		`, "caddyfile")
+
+	workerURL := "http://localhost:" + testPort + "/worker-with-counter.php"
+	servedBefore := countedRequests(t, workerURL)
+
+	rejected := `
+	{
+		skip_install_trust
+		admin localhost:2999
+		http_port ` + testPort + `
+	}
+
+	localhost:` + testPort + ` {
+		route {
+			root ../testdata
+			rewrite worker-with-counter.php
+			php {
+				worker ../testdata/not-a-worker.php 1
+			}
+		}
+	}
+	`
+
+	r, err := http.NewRequest("POST", "http://localhost:2999/load", bytes.NewBufferString(rejected))
+	require.NoError(t, err)
+	r.Header.Set("Content-Type", "text/caddyfile")
+	resp, err := http.DefaultClient.Do(r)
+	require.NoError(t, err)
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	require.Contains(t, string(body), "worker filename is invalid")
+	require.Equal(t, servedBefore+1, countedRequests(t, workerURL))
+}
+
+// the number of requests testdata/worker-with-counter.php has served
+func countedRequests(t *testing.T, workerURL string) int {
+	t.Helper()
+
+	resp, err := http.Get(workerURL)
+	require.NoError(t, err)
+	defer func() { require.NoError(t, resp.Body.Close()) }()
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+
+	count, err := strconv.Atoi(strings.TrimPrefix(string(body), "requests:"))
+	require.NoError(t, err, "unexpected worker response %q", body)
+
+	return count
+}

caddy/app.go

@@ -60,15 +60,15 @@ type FrankenPHPApp struct {
 	// EXPERIMENTAL: MaxRequests sets the maximum number of requests a PHP thread handles before restarting (0 = unlimited)
 	MaxRequests int `json:"max_requests,omitempty"`
 
-	opts            []frankenphp.Option
-	metrics         frankenphp.Metrics
-	ctx             context.Context
-	logger          *slog.Logger
-	modules         []*FrankenPHPModule
-	usedWorkerNames map[string]bool
-	httpApp         *caddyhttp.App
-	hasStarted      atomic.Bool
-	started         chan any
+	opts          []frankenphp.Option
+	provisionOpts []frankenphp.Option
+	metrics       frankenphp.Metrics
+	ctx           context.Context
+	logger        *slog.Logger
+	modules       []*FrankenPHPModule
+	httpApp       *caddyhttp.App
+	hasStarted    atomic.Bool
+	started       chan any
 }
 
 var errIni = errors.New(`"php_ini" must be in the format: php_ini "<key>" "<value>"`)
@@ -107,18 +107,29 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error {
 	return nil
 }
 
-func (f *FrankenPHPApp) Start() error {
-	defer func() {
-		close(f.started)
-	}()
+// Validate is called by Caddy before Start() to validate before shutting down the currently running config
+func (f *FrankenPHPApp) Validate() error {
+	opts, err := f.collectOptions(caddy.NewReplacer(), false)
+	if err != nil {
+		return err
+	}
 
-	repl := caddy.NewReplacer()
+	return frankenphp.Validate(opts...)
+}
 
+// collectOptions turns the configuration into the options Init() takes.
+// keep is for the configuration that starts, whose servers the modules
+// serve from; Validate() collects those of one that may never start.
+func (f *FrankenPHPApp) collectOptions(repl *caddy.Replacer, keep bool) ([]frankenphp.Option, error) {
 	optionsMU.RLock()
-	f.opts = append(f.opts, options...)
+	// We have at least 9 hardcoded options
+	opts := make([]frankenphp.Option, 0, 9+len(options)+len(f.provisionOpts))
+	opts = append(opts, options...)
 	optionsMU.RUnlock()
 
-	f.opts = append(f.opts,
+	opts = append(opts, f.provisionOpts...)
+
+	opts = append(opts,
 		frankenphp.WithContext(f.ctx),
 		frankenphp.WithLogger(f.logger),
 		frankenphp.WithNumThreads(f.NumThreads),
@@ -130,18 +141,42 @@ func (f *FrankenPHPApp) Start() error {
 		frankenphp.WithMaxRequests(f.MaxRequests),
 	)
 
+	usedWorkerNames := make(map[string]bool, len(f.Workers))
+
 	// register global workers
 	for _, w := range f.Workers {
 		w.FileName = repl.ReplaceKnown(w.FileName, "")
-		w.Name = f.createUniqueWorkerName(w, "")
-		opts, err := w.toWorkerOptions()
+		w.Name = createUniqueWorkerName(usedWorkerNames, w, "")
+		workerOptions, err := w.toWorkerOptions()
 		if err != nil {
-			return err
+			return nil, err
 		}
-		f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, opts...))
+		opts = append(opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...))
+	}
+
+	moduleOpts, err := f.collectModuleOptions(repl, usedWorkerNames, keep)
+	if err != nil {
+		return nil, err
+	}
+
+	return append(opts, moduleOpts...), nil
+}
+
+func (f *FrankenPHPApp) Start() error {
+	defer func() {
+		close(f.started)
+	}()
+
+	opts, err := f.collectOptions(caddy.NewReplacer(), true)
+	if err != nil {
+		return err
 	}
+	f.opts = opts
 
-	if err := f.registerModules(repl); err != nil {
+	// Validate() ran before the modules provisioned, so it saw the global
+	// block alone: what a php_server declares is checked here, in time for
+	// the configuration in place to survive a refusal
+	if err := frankenphp.Validate(f.opts...); err != nil {
 		return err
 	}
 
@@ -178,35 +213,42 @@ func (f *FrankenPHPApp) Stop() error {
 }
 
 // register workers and servers for "php" and "php_server" modules
-func (f *FrankenPHPApp) registerModules(repl *caddy.Replacer) error {
-	modulesByIndex := make(map[int]*FrankenPHPModule, len(f.modules))
-	for _, module := range f.modules {
-		if module.ServerIndex == 0 {
-			if err := f.registerModule(repl, module); err != nil {
-				return err
-			}
-			continue
-		}
+func (f *FrankenPHPApp) collectModuleOptions(repl *caddy.Replacer, usedWorkerNames map[string]bool, keep bool) ([]frankenphp.Option, error) {
+	opts := make([]frankenphp.Option, 0, len(f.modules))
+	serversByIndex := make(map[int]*frankenphp.Server, len(f.modules))
 
+	for _, module := range f.modules {
 		// modules with the same server_idx should share the same server instance
 		// example: the worker { match * } rule adds 2 "php" subroutes to the caddy handler
 		// the 2 handlers belong to the same "php_server" and must therefore share workers
-		if existingModule, ok := modulesByIndex[module.ServerIndex]; ok {
-			module.server = existingModule.server
-			continue
+		if module.ServerIndex != 0 {
+			if server, ok := serversByIndex[module.ServerIndex]; ok {
+				if keep {
+					module.server = server
+				}
+
+				continue
+			}
+		}
+
+		server, moduleOpts, err := f.collectModule(repl, module, usedWorkerNames)
+		if err != nil {
+			return nil, err
 		}
 
-		modulesByIndex[module.ServerIndex] = module
-		if err := f.registerModule(repl, module); err != nil {
-			return err
+		if keep {
+			module.server = server
+		}
+		if module.ServerIndex != 0 {
+			serversByIndex[module.ServerIndex] = server
 		}
+		opts = append(opts, moduleOpts...)
 	}
 
-	return nil
+	return opts, nil
 }
 
-// register a server instance and its workers for a single Caddy module
-func (f *FrankenPHPApp) registerModule(repl *caddy.Replacer, module *FrankenPHPModule) error {
+func (f *FrankenPHPApp) collectModule(repl *caddy.Replacer, module *FrankenPHPModule, usedWorkerNames map[string]bool) (*frankenphp.Server, []frankenphp.Option, error) {
 	serverName := f.resolveServerName(module)
 	server, err := frankenphp.NewServer(
 		module.resolvedDocumentRoot,
@@ -216,43 +258,38 @@ func (f *FrankenPHPApp) registerModule(repl *caddy.Replacer, module *FrankenPHPM
 		frankenphp.WithServerLogger(module.logger),
 	)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
-	module.server = server
-	f.opts = append(f.opts, frankenphp.WithServer(server))
+	opts := []frankenphp.Option{frankenphp.WithServer(server)}
 
 	for _, w := range module.Workers {
 		w.FileName = repl.ReplaceKnown(w.FileName, "")
-		w.Name = f.createUniqueWorkerName(w, serverName)
+		w.Name = createUniqueWorkerName(usedWorkerNames, w, serverName)
 		workerOptions, err := w.toWorkerOptions()
 		if err != nil {
-			return err
+			return nil, nil, err
 		}
 		workerOptions = append(workerOptions, frankenphp.WithWorkerServerScope(server))
-		f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...))
+		opts = append(opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...))
 	}
 
-	return nil
+	return server, opts, nil
 }
 
 // avoid name collisions for workers
 // on collision, a name is first qualified with the server name
 // ("<serverName>:<name>") before falling back to a numeric postfix
-func (f *FrankenPHPApp) createUniqueWorkerName(wc workerConfig, serverName string) string {
-	if f.usedWorkerNames == nil {
-		f.usedWorkerNames = make(map[string]bool)
-	}
-
+func createUniqueWorkerName(usedWorkerNames map[string]bool, wc workerConfig, serverName string) string {
 	if wc.Name == "" {
 		wc.Name, _ = fastabs.FastAbs(wc.FileName)
 	}
 
 	name := wc.Name
 	suffix := 0
 	for {
-		if _, ok := f.usedWorkerNames[name]; !ok {
-			f.usedWorkerNames[name] = true
+		if _, ok := usedWorkerNames[name]; !ok {
+			usedWorkerNames[name] = true
 			break
 		}
 		if serverName != "" {