diff --git a/internal/machine/caddyconfig/caddyfile.go b/internal/machine/caddyconfig/caddyfile.go index 979a2307..292e8a75 100644 --- a/internal/machine/caddyconfig/caddyfile.go +++ b/internal/machine/caddyconfig/caddyfile.go @@ -19,7 +19,7 @@ import ( ) const ( - caddyfileHeaderFmt = `# Caddyfile autogenerated by Uncloud (DO NOT EDIT): %s + caddyfileHeaderFmt = `# Caddyfile autogenerated by Uncloud on machine '%s' (DO NOT EDIT): %s # Automatically updated on service or health status changes. # Docs: https://uncloud.run/docs/concepts/ingress/overview ` @@ -68,8 +68,10 @@ https://{{$hostname}} { type CaddyfileGenerator struct { // machineID is the unique identifier of the machine where the controller is running. machineID string - validator CaddyfileValidator - log *slog.Logger + // machineName is the human-friendly name of the machine. + machineName string + validator CaddyfileValidator + log *slog.Logger } // CaddyfileValidator is an interface for validating Caddyfile configurations. @@ -77,14 +79,17 @@ type CaddyfileValidator interface { Validate(ctx context.Context, caddyfile string) error } -func NewCaddyfileGenerator(machineID string, validator CaddyfileValidator, log *slog.Logger) *CaddyfileGenerator { +func NewCaddyfileGenerator( + machineID, machineName string, validator CaddyfileValidator, log *slog.Logger, +) *CaddyfileGenerator { if log == nil { log = slog.Default() } return &CaddyfileGenerator{ - machineID: machineID, - validator: validator, - log: log, + machineID: machineID, + machineName: machineName, + validator: validator, + log: log, } } @@ -124,7 +129,7 @@ func (g *CaddyfileGenerator) Generate( return "", fmt.Errorf("generate base Caddyfile from service ports: %w", err) } - caddyfileHeader := fmt.Sprintf(caddyfileHeaderFmt, time.Now().UTC().Format(time.RFC3339)) + caddyfileHeader := fmt.Sprintf(caddyfileHeaderFmt, g.machineName, time.Now().UTC().Format(time.RFC3339)) if !includeCustom { return fmt.Sprintf("%s\n%s\n%s", caddyfileHeader, caddyfile, caddyfileUnavailabeFooter), nil } diff --git a/internal/machine/caddyconfig/caddyfile_test.go b/internal/machine/caddyconfig/caddyfile_test.go index 2f97fa72..5b89534e 100644 --- a/internal/machine/caddyconfig/caddyfile_test.go +++ b/internal/machine/caddyconfig/caddyfile_test.go @@ -26,7 +26,7 @@ func normaliseGeneratedTimestamp(caddyfile string) string { return generatedTimestampRegex.ReplaceAllString(caddyfile, "(DO NOT EDIT): TIMESTAMP_PLACEHOLDER") } -const testCaddyfileHeader = `# Caddyfile autogenerated by Uncloud (DO NOT EDIT): TIMESTAMP_PLACEHOLDER +const testCaddyfileHeader = `# Caddyfile autogenerated by Uncloud on machine 'test-machine' (DO NOT EDIT): TIMESTAMP_PLACEHOLDER # Automatically updated on service or health status changes. # Docs: https://uncloud.run/docs/concepts/ingress/overview @@ -190,7 +190,7 @@ https://secure.example.com { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Validator is not expected to be called in these tests. - generator := NewCaddyfileGenerator("test-machine-id", nil, nil) + generator := NewCaddyfileGenerator("test-machine-id", "test-machine", nil, nil) config, err := generator.Generate(ctx, tt.containers, true) @@ -226,7 +226,7 @@ func TestCaddyfileGeneratorWithCustomConfigs(t *testing.T) { time.Now(), ), }, - want: `# Caddyfile autogenerated by Uncloud (DO NOT EDIT): TIMESTAMP_PLACEHOLDER + want: `# Caddyfile autogenerated by Uncloud on machine 'test-machine' (DO NOT EDIT): TIMESTAMP_PLACEHOLDER # Automatically updated on service or health status changes. # Docs: https://uncloud.run/docs/concepts/ingress/overview @@ -423,7 +423,7 @@ web.example.com { time.Now(), ), }, - want: `# Caddyfile autogenerated by Uncloud (DO NOT EDIT): TIMESTAMP_PLACEHOLDER + want: `# Caddyfile autogenerated by Uncloud on machine 'test-machine' (DO NOT EDIT): TIMESTAMP_PLACEHOLDER # Automatically updated on service or health status changes. # Docs: https://uncloud.run/docs/concepts/ingress/overview @@ -661,7 +661,7 @@ badconfig.com { time.Now(), ), }, - want: `# Caddyfile autogenerated by Uncloud (DO NOT EDIT): TIMESTAMP_PLACEHOLDER + want: `# Caddyfile autogenerated by Uncloud on machine 'test-machine' (DO NOT EDIT): TIMESTAMP_PLACEHOLDER # Automatically updated on service or health status changes. # Docs: https://uncloud.run/docs/concepts/ingress/overview @@ -834,7 +834,7 @@ valid.example.com { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - generator := NewCaddyfileGenerator("test-machine-id", validator, nil) + generator := NewCaddyfileGenerator("test-machine-id", "test-machine", validator, nil) config, err := generator.Generate(ctx, tt.containers, true) @@ -976,7 +976,7 @@ http://api.example.com { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Validator is not expected to be called in these tests. - generator := NewCaddyfileGenerator("test-machine-id", nil, nil) + generator := NewCaddyfileGenerator("test-machine-id", "test-machine", nil, nil) config, err := generator.Generate(ctx, tt.containers, false) require.NoError(t, err) diff --git a/internal/machine/caddyconfig/client.go b/internal/machine/caddyconfig/client.go index c988e8f1..2901a632 100644 --- a/internal/machine/caddyconfig/client.go +++ b/internal/machine/caddyconfig/client.go @@ -34,21 +34,16 @@ func NewCaddyAdminClient(socketPath string) *CaddyAdminClient { } } -// IsAvailable checks if the local Caddy instance is running and responding to admin API requests. -func (c *CaddyAdminClient) IsAvailable(ctx context.Context) bool { - // Caddy doesn't serve a /ping endpoint. It's a random endpoint we can use to check if Caddy is running. - req, err := http.NewRequestWithContext(ctx, "GET", "http://localhost/ping", nil) +// IsAvailable checks if the local Caddy instance is listening on the admin socket. +func (c *CaddyAdminClient) IsAvailable() bool { + conn, err := net.DialTimeout("unix", c.socketPath, 1*time.Second) + // A stale socket file left over from a crashed Caddy container returns ECONNREFUSED so this is correctly handled + // as unavailable. if err != nil { return false } + conn.Close() - resp, err := c.client.Do(req) - if err != nil { - return false - } - defer resp.Body.Close() - - // Any HTTP response means Caddy is running and accessible. return true } diff --git a/internal/machine/caddyconfig/controller.go b/internal/machine/caddyconfig/controller.go index a4afe754..5f5a65bd 100644 --- a/internal/machine/caddyconfig/controller.go +++ b/internal/machine/caddyconfig/controller.go @@ -5,8 +5,11 @@ import ( "encoding/json" "fmt" "log/slog" + "net/netip" "os" "path/filepath" + "slices" + "strings" "github.com/psviderski/uncloud/internal/fs" "github.com/psviderski/uncloud/internal/machine/store" @@ -29,6 +32,28 @@ type Controller struct { client *CaddyAdminClient store *store.Store log *slog.Logger + // lastFingerprint caches the fingerprint of the containers used to generate the latest successfully loaded + // Caddyfile. nil means it hasn't been loaded yet or the last load failed. + lastFingerprint []containerFingerprint + // lastCaddyfile caches the last generated Caddyfile. + lastCaddyfile string +} + +// containerFingerprint is the subset of container data that the Caddyfile generator depends on. +// Comparing fingerprints lets the controller skip no-op regenerations. +type containerFingerprint struct { + ID string + IP netip.Addr + Ports []api.PortSpec + CaddyConfig string +} + +// Equal returns whether two fingerprints describe the same container input to the Caddyfile generator. +func (f containerFingerprint) Equal(other containerFingerprint) bool { + return f.ID == other.ID && + f.IP == other.IP && + api.PortsEqual(f.Ports, other.Ports) && + f.CaddyConfig == other.CaddyConfig } func NewController(machineID, configDir, adminSock string, store *store.Store) (*Controller, error) { @@ -41,12 +66,11 @@ func NewController(machineID, configDir, adminSock string, store *store.Store) ( log := slog.With("component", "caddy-controller") client := NewCaddyAdminClient(adminSock) - generator := NewCaddyfileGenerator(machineID, client, log) + // generator is initialised by Run() once the machine name is resolved from the store. return &Controller{ machineID: machineID, caddyfilePath: filepath.Join(configDir, "Caddyfile"), - generator: generator, client: client, store: store, log: log, @@ -54,6 +78,17 @@ func NewController(machineID, configDir, adminSock string, store *store.Store) ( } func (c *Controller) Run(ctx context.Context) error { + // Default the machine name to the machine ID so the Caddyfile header still carries a stable identifier if + // the store lookup fails. + machineName := c.machineID + if m, err := c.store.GetMachine(ctx, c.machineID); err != nil { + c.log.Error("Failed to get machine from store, Caddy configuration will use machine ID as the name.", + "machine_id", c.machineID, "err", err) + } else { + machineName = m.Name + } + c.generator = NewCaddyfileGenerator(c.machineID, machineName, c.client, c.log) + containers, changes, err := c.store.SubscribeContainers(ctx) if err != nil { return fmt.Errorf("subscribe to container changes: %w", err) @@ -74,7 +109,7 @@ func (c *Controller) Run(ctx context.Context) error { if !ok { return fmt.Errorf("containers subscription failed") } - c.log.Info("Cluster containers changed, updating Caddy configuration.") + c.log.Debug("Cluster containers changed, regenerating Caddy configuration.") containers, err = c.store.ListContainers(ctx, store.ListOptions{}) if err != nil { @@ -111,9 +146,20 @@ func filterHealthyContainers(containers []store.ContainerRecord) []store.Contain return healthy } +// generateAndLoadCaddyfile regenerates the Caddyfile from the given containers and loads it into the local Caddy +// if available. 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) + caddyAvailable := c.client.IsAvailable() + + // Skip regeneration when Caddy is available and the containers since the last successful load haven't changed. + // When Caddy is unavailable we still regenerate to keep the Caddyfile on disk updated. + fingerprint := fingerprintContainers(containers) + if caddyAvailable && slices.EqualFunc(fingerprint, c.lastFingerprint, containerFingerprint.Equal) { + c.log.Debug("Caddy configuration is unchanged.", "path", c.caddyfilePath) + return + } + caddyfile, err := c.generator.Generate(ctx, containers, caddyAvailable) if err != nil { c.log.Error("Failed to generate Caddyfile configuration.", "err", err) @@ -123,7 +169,7 @@ func (c *Controller) generateAndLoadCaddyfile(ctx context.Context, containers [] 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 { + if err = c.writeCaddyfileIfChanged(caddyfile); err != nil { c.log.Error("Failed to write Caddyfile to disk.", "err", err) return } @@ -137,30 +183,71 @@ func (c *Controller) generateAndLoadCaddyfile(ctx context.Context, containers [] 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) + // Mark the cache stale so the next container change retries the load even if the container set is unchanged. + c.lastFingerprint = nil // Don't write invalid config to disk. return } + c.lastFingerprint = fingerprint // Config loaded successfully, now write it to disk. - if err = c.writeCaddyfile(caddyfile); err != nil { + if err = c.writeCaddyfileIfChanged(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. + // Config is already loaded in Caddy, so this is not critical. The next regeneration retries the disk write. + return } c.log.Info("New Caddy configuration loaded into local Caddy instance.", "path", c.caddyfilePath) } -// writeCaddyfile writes the Caddyfile content to disk with proper permissions. -func (c *Controller) writeCaddyfile(caddyfile string) error { +// fingerprintContainers returns a fingerprint of containers that the Caddyfile generator depends on. +func fingerprintContainers(containers []store.ContainerRecord) []containerFingerprint { + fingerprints := make([]containerFingerprint, len(containers)) + for i, cr := range containers { + // Ignore ports parsing error as not much we can do about it. The generator just logs them and continues. + ports, _ := cr.Container.ServicePorts() + fingerprints[i] = containerFingerprint{ + ID: cr.Container.ID, + IP: cr.Container.UncloudNetworkIP(), + Ports: ports, + CaddyConfig: cr.Container.ServiceSpec.CaddyConfig(), + } + } + slices.SortFunc(fingerprints, func(a, b containerFingerprint) int { + return strings.Compare(a.ID, b.ID) + }) + + return fingerprints +} + +// writeCaddyfileIfChanged writes the Caddyfile content to disk with proper permissions only if its body differs +// from the last successfully written content. The first line of the Caddyfile carries a generation timestamp that +// changes on every regeneration, so it's excluded from the comparison to avoid redundant writes. +func (c *Controller) writeCaddyfileIfChanged(caddyfile string) error { + if caddyfileBody(caddyfile) == caddyfileBody(c.lastCaddyfile) { + return nil + } + 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) } + c.lastCaddyfile = caddyfile + return nil } +// caddyfileBody returns the Caddyfile content without its first line, which carries a generation timestamp that +// rotates on every regeneration. +func caddyfileBody(caddyfile string) string { + if i := strings.IndexByte(caddyfile, '\n'); i >= 0 { + return caddyfile[i+1:] + } + return caddyfile +} + func (c *Controller) generateJSONConfig(containers []store.ContainerRecord) error { serviceContainers := make([]api.ServiceContainer, len(containers)) for i, cr := range containers { diff --git a/internal/machine/caddyconfig/controller_test.go b/internal/machine/caddyconfig/controller_test.go new file mode 100644 index 00000000..a394a82f --- /dev/null +++ b/internal/machine/caddyconfig/controller_test.go @@ -0,0 +1,64 @@ +package caddyconfig + +import ( + "net/netip" + "reflect" + "testing" + + "github.com/psviderski/uncloud/pkg/api" + "github.com/stretchr/testify/assert" +) + +// TestContainerFingerprint_EqualCoversAllFields is a guard: when a field is added to containerFingerprint, +// Equal must also compare it. A mutation of any single field should flip equality to false. If this test fails +// after adding a field, update Equal to include it. +func TestContainerFingerprint_EqualCoversAllFields(t *testing.T) { + t.Parallel() + + base := containerFingerprint{ + ID: "container-1", + IP: netip.MustParseAddr("10.210.0.2"), + Ports: []api.PortSpec{{ + Hostname: "app.example.com", + ContainerPort: 8080, + Protocol: api.ProtocolHTTP, + Mode: api.PortModeIngress, + }}, + CaddyConfig: "caddy-config", + } + + assert.True(t, base.Equal(base), "base fingerprint must be equal to itself") + + rt := reflect.TypeOf(base) + for i := 0; i < rt.NumField(); i++ { + field := rt.Field(i) + t.Run(field.Name, func(t *testing.T) { + t.Parallel() + + mutated := base + mutated.Ports = append([]api.PortSpec(nil), base.Ports...) + v := reflect.ValueOf(&mutated).Elem().FieldByName(field.Name) + + switch field.Name { + case "ID", "CaddyConfig": + v.SetString(v.String() + "-changed") + case "IP": + v.Set(reflect.ValueOf(netip.MustParseAddr("10.210.0.99"))) + case "Ports": + mutated.Ports = []api.PortSpec{{ + Hostname: "different.example.com", + ContainerPort: 9090, + Protocol: api.ProtocolHTTP, + Mode: api.PortModeIngress, + }} + default: + t.Fatalf("containerFingerprint has a new field %q without a mutation case in this test. "+ + "Add a case here and make sure Equal() compares it.", field.Name) + } + + assert.False(t, base.Equal(mutated), + "changing %q must flip Equal to false. Update containerFingerprint.Equal to compare it.", + field.Name) + }) + } +} diff --git a/internal/machine/dns/resolver.go b/internal/machine/dns/resolver.go index f022a0c5..2e2bbe0d 100644 --- a/internal/machine/dns/resolver.go +++ b/internal/machine/dns/resolver.go @@ -4,7 +4,9 @@ import ( "context" "fmt" "log/slog" + "maps" "net/netip" + "slices" "sync" "time" @@ -102,12 +104,21 @@ func (r *ClusterResolver) updateServiceIPs(containers []store.ContainerRecord) { containersCount++ } + // Sort each service's IPs so they have a deterministic order for comparison. + for _, ips := range newServiceIPs { + slices.SortFunc(ips, func(a, b netip.Addr) int { return a.Compare(b) }) + } + // Skip the swap when the services or their container IPs haven't changed. + if maps.EqualFunc(r.serviceIPs, newServiceIPs, slices.Equal[[]netip.Addr]) { + return + } + // Update the serviceIPs map atomically. r.mu.Lock() r.serviceIPs = newServiceIPs r.mu.Unlock() - r.log.Debug("DNS records updated.", "services", len(newServiceIPs)/3, "containers", containersCount) + r.log.Info("DNS records updated.", "services", len(newServiceIPs)/3, "containers", containersCount) } // Resolve returns IP addresses of the service containers. diff --git a/internal/machine/dns/resolver_test.go b/internal/machine/dns/resolver_test.go new file mode 100644 index 00000000..a0dd4802 --- /dev/null +++ b/internal/machine/dns/resolver_test.go @@ -0,0 +1,77 @@ +package dns + +import ( + "reflect" + "testing" + + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/network" + "github.com/psviderski/uncloud/internal/machine/store" + "github.com/psviderski/uncloud/pkg/api" + "github.com/stretchr/testify/assert" +) + +func TestClusterResolver_UpdateServiceIPs(t *testing.T) { + t.Parallel() + + containers := []store.ContainerRecord{ + newRecord("svc-id-1", "web", "10.210.0.2", "mach-1"), + newRecord("svc-id-1", "web", "10.210.0.3", "mach-2"), + newRecord("svc-id-2", "api", "10.210.1.2", "mach-1"), + } + + r := NewClusterResolver(nil) + r.updateServiceIPs(containers) + + firstMapPtr := reflect.ValueOf(r.serviceIPs).Pointer() + assert.NotZero(t, firstMapPtr, "first call should populate the map") + assert.NotEmpty(t, r.Resolve("web")) + assert.NotEmpty(t, r.Resolve("api")) + + // Second call with the same input must not swap the map. + r.updateServiceIPs(containers) + assert.Equal(t, firstMapPtr, reflect.ValueOf(r.serviceIPs).Pointer(), + "second call with identical input must not rewrite the map") + + // Reordering the input also must not rewrite the map. + reordered := []store.ContainerRecord{containers[2], containers[0], containers[1]} + r.updateServiceIPs(reordered) + assert.Equal(t, firstMapPtr, reflect.ValueOf(r.serviceIPs).Pointer(), + "reordered input with same containers must not rewrite the map") + + // A real change must rewrite the map. + changed := append([]store.ContainerRecord{}, containers...) + changed = append(changed, newRecord("svc-id-3", "db", "10.210.2.2", "mach-1")) + r.updateServiceIPs(changed) + assert.NotEqual(t, firstMapPtr, reflect.ValueOf(r.serviceIPs).Pointer(), + "adding a new service should rewrite the map") + assert.NotEmpty(t, r.Resolve("db")) +} + +func newRecord(serviceID, serviceName, ip, machineID string) store.ContainerRecord { + return store.ContainerRecord{ + Container: api.ServiceContainer{ + Container: api.Container{ + InspectResponse: container.InspectResponse{ + ContainerJSONBase: &container.ContainerJSONBase{ + ID: serviceName + "-" + ip, + State: &container.State{Running: true}, + }, + NetworkSettings: &container.NetworkSettings{ + Networks: map[string]*network.EndpointSettings{ + // Hardcoded to avoid an import cycle via internal/machine/docker. + "uncloud": {IPAddress: ip}, + }, + }, + Config: &container.Config{ + Labels: map[string]string{ + api.LabelServiceID: serviceID, + api.LabelServiceName: serviceName, + }, + }, + }, + }, + }, + MachineID: machineID, + } +} diff --git a/internal/machine/store/container.go b/internal/machine/store/container.go index a3772051..024998da 100644 --- a/internal/machine/store/container.go +++ b/internal/machine/store/container.go @@ -1,14 +1,17 @@ package store import ( + "cmp" "context" "encoding/json" "fmt" "log/slog" + "slices" "strings" "time" sq "github.com/Masterminds/squirrel" + "github.com/docker/docker/api/types/container" "github.com/psviderski/uncloud/pkg/api" ) @@ -49,10 +52,9 @@ type DeleteOptions struct { // CreateOrUpdateContainer creates a new container record or updates an existing one in the store database. // The container is associated with the given machine ID that indicates which machine the container is running on. func (s *Store) CreateOrUpdateContainer(ctx context.Context, ctr api.ServiceContainer, machineID string) error { - // Remove the environment variables from the container record before storing it in the database - // to avoid leaking secrets. - ctr.Config.Env = nil - ctr.ServiceSpec.Container.Env = nil + // Stabilise the order of slices that Docker returns non-deterministically, so that byte-level + // comparison of the serialised container does not flag spurious changes. + normaliseContainerForStore(&ctr) cJSON, err := json.Marshal(ctr) if err != nil { @@ -80,6 +82,23 @@ func (s *Store) CreateOrUpdateContainer(ctx context.Context, ctr api.ServiceCont return nil } +// normaliseContainerForStore removes potentially sensitive data and normalises the container fields that Docker may +// return in non-deterministic order so that byte-level comparison of the serialised container does not flag spurious +// changes. +func normaliseContainerForStore(ctr *api.ServiceContainer) { + // Remove the environment variables to avoid leaking secrets. + ctr.Config.Env = nil + ctr.ServiceSpec.Container.Env = nil + + // Docker returns Mounts in a non-deterministic order so sort them. + slices.SortFunc(ctr.Mounts, func(a, b container.MountPoint) int { + return cmp.Or( + strings.Compare(a.Destination, b.Destination), + strings.Compare(a.Source, b.Source), + ) + }) +} + // ListContainers returns a list of container records from the store database that match the given options. func (s *Store) ListContainers(ctx context.Context, opts ListOptions) ([]ContainerRecord, error) { q := sq.Select("id", "container", "machine_id", "sync_status", "updated_at").From("containers").