From 7b88b8b8102de6fbcee68a56a08755cec62f1e3a Mon Sep 17 00:00:00 2001 From: Pasha Sviderski Date: Wed, 10 Sep 2025 16:59:31 +1000 Subject: [PATCH] fix: write generated Caddyfile to disk only if succesfully loaded into local Caddy (means valid) --- internal/machine/caddyconfig/caddyfile.go | 7 +-- .../machine/caddyconfig/caddyfile_test.go | 6 ++- internal/machine/caddyconfig/controller.go | 45 +++++++++++-------- test/e2e/service_test.go | 42 +++++++++++++++++ 4 files changed, 77 insertions(+), 23 deletions(-) diff --git a/internal/machine/caddyconfig/caddyfile.go b/internal/machine/caddyconfig/caddyfile.go index 3fbe7c55..9394f801 100644 --- a/internal/machine/caddyconfig/caddyfile.go +++ b/internal/machine/caddyconfig/caddyfile.go @@ -54,6 +54,9 @@ https://{{$hostname}} { } log }{{end}} +` + caddyfileUnavailabeFooter = `# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine +# or the latest generated config is invalid. Please check the Caddy logs if it's running. ` ) @@ -118,9 +121,7 @@ func (g *CaddyfileGenerator) Generate( } if !includeCustom { - return fmt.Sprintf("%s\n%s\n"+ - "# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine.\n", - caddyfileHeader, caddyfile), nil + return fmt.Sprintf("%s\n%s\n%s", caddyfileHeader, caddyfile, caddyfileUnavailabeFooter), nil } upstreams := serviceUpstreams(containers) diff --git a/internal/machine/caddyconfig/caddyfile_test.go b/internal/machine/caddyconfig/caddyfile_test.go index 45f83bfd..6a53ea5a 100644 --- a/internal/machine/caddyconfig/caddyfile_test.go +++ b/internal/machine/caddyconfig/caddyfile_test.go @@ -926,7 +926,8 @@ http://api.example.com { log } -# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine. +# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine +# or the latest generated config is invalid. Please check the Caddy logs if it's running. `, }, { @@ -949,7 +950,8 @@ http://api.example.com { log } -# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine. +# NOTE: User-defined configs for services were skipped because Caddy is not running on this machine +# or the latest generated config is invalid. Please check the Caddy logs if it's running. `, }, } diff --git a/internal/machine/caddyconfig/controller.go b/internal/machine/caddyconfig/controller.go index 687261a9..0640b060 100644 --- a/internal/machine/caddyconfig/controller.go +++ b/internal/machine/caddyconfig/controller.go @@ -111,42 +111,51 @@ func filterHealthyContainers(containers []store.ContainerRecord) []store.Contain func (c *Controller) generateAndLoadCaddyfile(ctx context.Context, containers []store.ContainerRecord) { // Check if Caddy is available before attempting to generate and load config. caddyAvailable := c.client.IsAvailable(ctx) - - caddyfile, err := c.generateCaddyfile(ctx, containers, caddyAvailable) + caddyfile, err := c.generator.Generate(ctx, containers, caddyAvailable) if err != nil { c.log.Error("Failed to generate Caddyfile configuration.", "err", err) return } if !caddyAvailable { + // Caddy is not running so the generated Caddyfile should not include user-defined configs thus must be valid. + // It's safe to write the config to disk so that when Caddy is deployed on this machine, it can pick it up. + if err = c.writeCaddyfile(caddyfile); err != nil { + c.log.Error("Failed to write Caddyfile to disk.", "err", err) + return + } c.log.Debug("Caddy is not running on this machine, skipping configuration load.", "path", c.caddyfilePath) return } + // Caddy is available, try to load the config which may fail if the config is invalid. Generally, a config can + // pass the adaptation/validation step but still fail to load, for example, if it references resources that are + // not available. if err = c.client.Load(ctx, caddyfile); err != nil { c.log.Error("Failed to load new Caddy configuration into local Caddy instance.", "err", err, "path", c.caddyfilePath) - } else { - c.log.Info("New Caddy configuration loaded into local Caddy instance.", "path", c.caddyfilePath) + // Don't write invalid config to disk. + return } + + // Config loaded successfully, now write it to disk. + if err = c.writeCaddyfile(caddyfile); err != nil { + c.log.Error("Failed to write Caddyfile to disk after successful load.", "err", err) + // Config is already loaded in Caddy, so this is not critical. + } + + c.log.Info("New Caddy configuration loaded into local Caddy instance.", "path", c.caddyfilePath) } -func (c *Controller) generateCaddyfile( - ctx context.Context, containers []store.ContainerRecord, caddyAvailable bool, -) (string, error) { - caddyfile, err := c.generator.Generate(ctx, containers, caddyAvailable) - if err != nil { - return "", fmt.Errorf("generate Caddyfile: %w", err) +// writeCaddyfile writes the Caddyfile content to disk with proper permissions. +func (c *Controller) writeCaddyfile(caddyfile string) error { + if err := os.WriteFile(c.caddyfilePath, []byte(caddyfile), 0o640); err != nil { + return fmt.Errorf("write Caddyfile to file '%s': %w", c.caddyfilePath, err) } - - if err = os.WriteFile(c.caddyfilePath, []byte(caddyfile), 0o640); err != nil { - return "", fmt.Errorf("write Caddyfile to file '%s': %w", c.caddyfilePath, err) + if err := fs.Chown(c.caddyfilePath, "", CaddyGroup); err != nil { + return fmt.Errorf("change owner of Caddyfile '%s': %w", c.caddyfilePath, err) } - if err = fs.Chown(c.caddyfilePath, "", CaddyGroup); err != nil { - return "", fmt.Errorf("change owner of Caddyfile '%s': %w", c.caddyfilePath, err) - } - - return caddyfile, nil + return nil } func (c *Controller) generateJSONConfig(containers []store.ContainerRecord) error { diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 9e6bd448..93163ab1 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -438,6 +438,48 @@ myapp.example.com { assert.NotContains(t, config.Caddyfile, "invalid user-defined configs", "Should not have validation failure comments after caddy is deployed") + + // Store the current valid config for later comparison. + validConfig := config.Caddyfile + + // Now deploy a service with invalid Caddyfile that references missing cert files and check it isn't included. + invalidServiceName := "test-invalid-caddy-config" + t.Cleanup(func() { + err := cli.RemoveService(ctx, invalidServiceName) + if !errors.Is(err, api.ErrNotFound) { + require.NoError(t, err) + } + }) + + invalidCaddyfile := `test-invalid.example.com { + tls cert.pem key.pem +}` + invalidSpec := api.ServiceSpec{ + Name: invalidServiceName, + Container: api.ContainerSpec{ + Image: "portainer/pause:latest", + }, + Caddy: &api.CaddySpec{ + Config: invalidCaddyfile, + }, + } + + invalidDeployment := cli.NewDeployment(invalidSpec, nil) + _, err = invalidDeployment.Run(ctx) + require.NoError(t, err) + + invalidSvc, err := cli.InspectService(ctx, invalidServiceName) + require.NoError(t, err) + assertServiceMatchesSpec(t, invalidSvc, invalidSpec) + + // Wait a bit for any config updates to potentially happen. + time.Sleep(2 * time.Second) + + // Check that the Caddy config hasn't changed. + newConfig, err := cli.Caddy.GetConfig(ctx, nil) + require.NoError(t, err) + assert.Equal(t, validConfig, newConfig.Caddyfile, + "Caddy config should not change when an invalid user-defined Caddy config is deployed") }) t.Run("replicated", func(t *testing.T) {