From 82f07453f81f39f83248526e4445eb12ee58362d Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 24 Sep 2026 02:24:14 +0100 Subject: [PATCH 1/3] fix(cloudcp): recover provider MSP clients after support recreates Reconcile PR #2216 with current main and retain the paid-plan contracts. Reattach support containers to active tenant networks at monitor startup and before checks, and restart unhealthy clients rather than stopping them under Docker unless-stopped. Require exact tenant-network ownership labels and select support containers on the configured provider ingress network to preserve isolation. Cover startup recovery, restart behaviour, missing networks and wrong-owner networks. Change-source: pulse-maintainer --- .../v6/internal/subsystems/cloud-paid.md | 15 ++ .../subsystems/deployment-installability.md | 10 + internal/cloudcp/docker/manager.go | 47 ++++ internal/cloudcp/docker/manager_test.go | 71 ++++++ internal/cloudcp/health_monitor.go | 27 ++- internal/cloudcp/health_monitor_test.go | 206 ++++++++++++++++++ 6 files changed, 373 insertions(+), 3 deletions(-) create mode 100644 internal/cloudcp/health_monitor_test.go diff --git a/docs/release-control/v6/internal/subsystems/cloud-paid.md b/docs/release-control/v6/internal/subsystems/cloud-paid.md index 79323bf518..83e0c2c3e7 100644 --- a/docs/release-control/v6/internal/subsystems/cloud-paid.md +++ b/docs/release-control/v6/internal/subsystems/cloud-paid.md @@ -3482,6 +3482,21 @@ coverage: `TestProviderMSPWorkspaceLadderRisesFromTheEvaluation` in `pkg/licensing/features_test.go`. +### Provider MSP health monitor recovers clients across upgrades + +The health monitor rejoins Traefik and the control plane to each active +client's labelled isolated network on startup and before every health check. +Recreating either support container drops its tenant-network attachments; +without reconnection, routes fail and a healthy client can appear unreachable. +The monitor now restarts an unhealthy client with Docker's restart operation, +rather than stopping it: `unless-stopped` does not revive a container stopped +through the API. Missing isolated networks are skipped; a network whose labels +do not identify that tenant is refused rather than connected. Support containers +must also have the role label and belong to the configured provider ingress +network, avoiding a same-host provider stack with the same role label. Regression +coverage is in `internal/cloudcp/health_monitor_test.go` and +`internal/cloudcp/docker/manager_test.go`. + ### Provider MSP status reads client health that any caller can observe `provider-msp status`, which `upgrade.sh` gates on before every provider diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index 6f958d029c..7f5d487a1f 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -2416,6 +2416,16 @@ artifact-selection behaviour. ## Current State +### Provider MSP clients survive a support-container recreate + +Recreating the control plane or Traefik detaches it from each client's +isolated network. The health monitor now reattaches both support containers +on startup and each pass, and restarts rather than stops an unhealthy client. +The original PR reports a two-client v6.4.1 lab reproduction and recovery on +the same installation on 2026-09-23; that is not installed acceptance of this +integrated source. The network reconnection requires exact tenant ownership +labels before mutating an existing network. + ### Provider MSP operations accept a renewed licence `provider-msp preflight`, `proof`, `recover` and `backup` treat a licence diff --git a/internal/cloudcp/docker/manager.go b/internal/cloudcp/docker/manager.go index d579159919..9495da4300 100644 --- a/internal/cloudcp/docker/manager.go +++ b/internal/cloudcp/docker/manager.go @@ -409,8 +409,15 @@ func (m *Manager) connectSupportContainersToTenantNetwork(ctx context.Context, n } func (m *Manager) connectSupportContainersByLabel(ctx context.Context, networkName, label string) error { + providerNetwork := strings.TrimSpace(m.cfg.Network) + if providerNetwork == "" { + return fmt.Errorf("provider ingress network is required to select support containers") + } filters := client.Filters{} filters = filters.Add("label", label) + // Role labels are shared by every provider MSP installation on a host. + // Never attach another installation's support container to this tenant. + filters = filters.Add("network", providerNetwork) result, err := m.cli.ContainerList(ctx, client.ContainerListOptions{ Filters: filters, }) @@ -863,6 +870,46 @@ func (m *Manager) Stop(ctx context.Context, containerID string) error { return err } +// Restart stops and starts a container in one daemon call. The health +// monitor relies on it to recover an unhealthy tenant: a container stopped +// through the API is never brought back by the unless-stopped restart +// policy, so stopping it left the client workspace down for good. +func (m *Manager) Restart(ctx context.Context, containerID string) error { + timeout := 30 + _, err := m.cli.ContainerRestart(ctx, containerID, client.ContainerRestartOptions{Timeout: &timeout}) + return err +} + +// EnsureSupportContainersOnTenantNetwork reattaches the provider support +// containers (Traefik and the control plane) to a tenant's isolated network. +// They join it when the tenant is created, but recreating either one, as an +// upgrade or any compose change does, drops the attachment: Traefik then has +// no route to the client and the control plane cannot reach it. A tenant +// without an isolated network has nothing to reattach. +func (m *Manager) EnsureSupportContainersOnTenantNetwork(ctx context.Context, tenantID string) error { + if m == nil || m.cli == nil || !m.cfg.IsolateTenantNetworks || strings.TrimSpace(tenantID) == "" { + return nil + } + networkName := m.tenantNetworkName(tenantID) + if networkName == "" { + return nil + } + inspected, err := m.cli.NetworkInspect(ctx, networkName, client.NetworkInspectOptions{}) + if err != nil { + if errdefs.IsNotFound(err) { + return nil + } + return fmt.Errorf("inspect tenant network %q: %w", networkName, err) + } + // A name alone does not establish ownership. Never attach provider support + // containers to an unrelated network that happens to have the derived name. + labels := inspected.Network.Labels + if labels["pulse.tenant.id"] != tenantID || labels[tenantRuntimeNetworkLabel] != tenantRuntimeNetworkLabelValue { + return fmt.Errorf("network %q is not the isolated network for tenant %q", networkName, tenantID) + } + return m.connectSupportContainersToTenantNetwork(ctx, networkName) +} + // Start starts a stopped tenant container. func (m *Manager) Start(ctx context.Context, containerID string) error { _, err := m.cli.ContainerStart(ctx, containerID, client.ContainerStartOptions{}) diff --git a/internal/cloudcp/docker/manager_test.go b/internal/cloudcp/docker/manager_test.go index 40d4726c2b..822adc93d8 100644 --- a/internal/cloudcp/docker/manager_test.go +++ b/internal/cloudcp/docker/manager_test.go @@ -2,6 +2,8 @@ package docker import ( "context" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -603,3 +605,72 @@ func TestDockerRecordedHealthDecidesWhenTheImageDeclaresOne(t *testing.T) { } } } + +// A client created before per-client networks has no isolated network to +// rejoin. Reconciling it must be a quiet no-op, not a warning every minute. +func TestEnsureSupportContainersOnTenantNetworkSkipsMissingNetwork(t *testing.T) { + var mutations []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Api-Version", "1.47") + if strings.HasSuffix(r.URL.Path, "/_ping") { + _, _ = w.Write([]byte("OK")) + return + } + if r.Method != http.MethodGet { + mutations = append(mutations, r.Method+" "+r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"network not found"}`)) + })) + t.Cleanup(srv.Close) + t.Setenv("DOCKER_HOST", "tcp://"+strings.TrimPrefix(srv.URL, "http://")) + t.Setenv("DOCKER_TLS_VERIFY", "") + t.Setenv("DOCKER_CERT_PATH", "") + + mgr, err := NewManager(ManagerConfig{Image: "pulse:test", Network: "pulse-provider-msp", IsolateTenantNetworks: true}) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + t.Cleanup(func() { _ = mgr.Close() }) + + if err := mgr.EnsureSupportContainersOnTenantNetwork(context.Background(), "t-legacy"); err != nil { + t.Fatalf("EnsureSupportContainersOnTenantNetwork = %v, want nil for a client without an isolated network", err) + } + if len(mutations) != 0 { + t.Fatalf("reconcile changed the host for a missing network: %v", mutations) + } +} + +func TestEnsureSupportContainersOnTenantNetworkRejectsWrongOwner(t *testing.T) { + var mutations []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Api-Version", "1.47") + if strings.HasSuffix(r.URL.Path, "/_ping") { + _, _ = w.Write([]byte("OK")) + return + } + if r.Method != http.MethodGet { + mutations = append(mutations, r.Method+" "+r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"Name":"pulse-provider-msp-tenant-t-acme","Id":"net-1","Labels":{"pulse.tenant.id":"t-other","pulse.provider-msp.network":"tenant"}}`)) + })) + t.Cleanup(srv.Close) + t.Setenv("DOCKER_HOST", "tcp://"+strings.TrimPrefix(srv.URL, "http://")) + t.Setenv("DOCKER_TLS_VERIFY", "") + t.Setenv("DOCKER_CERT_PATH", "") + + mgr, err := NewManager(ManagerConfig{Image: "pulse:test", Network: "pulse-provider-msp", IsolateTenantNetworks: true}) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + t.Cleanup(func() { _ = mgr.Close() }) + + if err := mgr.EnsureSupportContainersOnTenantNetwork(context.Background(), "t-acme"); err == nil { + t.Fatal("expected wrong-owner network to be rejected") + } + if len(mutations) != 0 { + t.Fatalf("reconcile changed wrong-owner network: %v", mutations) + } +} diff --git a/internal/cloudcp/health_monitor.go b/internal/cloudcp/health_monitor.go index 157db3da31..a249d1fdc2 100644 --- a/internal/cloudcp/health_monitor.go +++ b/internal/cloudcp/health_monitor.go @@ -49,6 +49,12 @@ func (m *Monitor) Run(ctx context.Context) { Bool("restart_on_fail", m.cfg.RestartOnFail). Msg("Health monitor started") + // Check once straight away. The control plane starts fresh after every + // upgrade, and the first pass is what puts the recreated support + // containers back on each client's network; waiting a full interval + // leaves every client route broken for that long. + m.checkAll(ctx) + ticker := time.NewTicker(m.cfg.Interval) defer ticker.Stop() @@ -64,6 +70,9 @@ func (m *Monitor) Run(ctx context.Context) { } func (m *Monitor) checkAll(ctx context.Context) { + if ctx.Err() != nil { + return + } activeContainerIDs := make(map[string]struct{}) tenants, err := m.registry.ListByState(registry.TenantStateActive) if err != nil { @@ -80,6 +89,16 @@ func (m *Monitor) checkAll(ctx context.Context) { } activeContainerIDs[tenant.ContainerID] = struct{}{} + // Rejoin Traefik and the control plane to this client's network + // first. A recreated support container (every upgrade recreates the + // control plane) has lost it, which cuts the client's route and + // would make a healthy client look unreachable. + if err := m.docker.EnsureSupportContainersOnTenantNetwork(ctx, tenant.ID); err != nil { + log.Warn().Err(err). + Str("tenant_id", tenant.ID). + Msg("Health monitor: could not reattach support containers to tenant network") + } + healthy, err := m.docker.HealthCheck(ctx, tenant.ContainerID) if err != nil { log.Warn().Err(err). @@ -117,14 +136,16 @@ func (m *Monitor) checkAll(ctx context.Context) { Int("fail_threshold", m.cfg.FailThreshold). Msg("Container unhealthy, attempting restart") - if err := m.docker.Stop(ctx, tenant.ContainerID); err != nil { + // Restart, never Stop: the unless-stopped policy does not bring + // back a container stopped through the API, so a stop took the + // client workspace down until someone started it by hand. + if err := m.docker.Restart(ctx, tenant.ContainerID); err != nil { log.Error(). Err(err). Str("tenant_id", tenant.ID). Str("container_id", tenant.ContainerID). - Msg("Failed to stop unhealthy container") + Msg("Failed to restart unhealthy container") } - // Docker restart policy (unless-stopped) will restart the container m.failures[tenant.ContainerID] = 0 } } diff --git a/internal/cloudcp/health_monitor_test.go b/internal/cloudcp/health_monitor_test.go new file mode 100644 index 0000000000..0a62e2d047 --- /dev/null +++ b/internal/cloudcp/health_monitor_test.go @@ -0,0 +1,206 @@ +package cloudcp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "regexp" + "strings" + "sync" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/cloudcp/docker" + "github.com/rcourtman/pulse-go-rewrite/internal/cloudcp/registry" +) + +// fakeProviderDaemon answers the few Docker Engine API calls the health +// monitor makes for one provider-hosted client workspace, and records every +// mutating call so a test can assert what the monitor did to the host. +type fakeProviderDaemon struct { + mu sync.Mutex + tenantNetwork string + attached map[string]bool // container IDs on the tenant network + support map[string]string + health string + calls []string + healthChecked chan struct{} +} + +var dockerAPIVersionPrefix = regexp.MustCompile(`^/v[0-9.]+`) + +func (d *fakeProviderDaemon) ServeHTTP(w http.ResponseWriter, r *http.Request) { + d.mu.Lock() + defer d.mu.Unlock() + + path := dockerAPIVersionPrefix.ReplaceAllString(r.URL.Path, "") + w.Header().Set("Api-Version", "1.47") + w.Header().Set("Content-Type", "application/json") + switch { + case path == "/_ping": + _, _ = w.Write([]byte("OK")) + case r.Method == http.MethodGet && path == "/networks/"+d.tenantNetwork: + containers := map[string]any{} + for id := range d.attached { + containers[id] = map[string]any{"Name": id} + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "Name": d.tenantNetwork, "Id": "net-1", "Containers": containers, + "Labels": map[string]string{"pulse.tenant.id": "t-acme", "pulse.provider-msp.network": "tenant"}, + }) + case r.Method == http.MethodPost && path == "/networks/"+d.tenantNetwork+"/connect": + var body struct{ Container string } + _ = json.NewDecoder(r.Body).Decode(&body) + d.attached[body.Container] = true + d.calls = append(d.calls, "connect "+body.Container) + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodGet && path == "/containers/json": + filters := r.URL.Query().Get("filters") + if !strings.Contains(filters, `"network"`) || !strings.Contains(filters, "pulse-provider-msp") { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"support containers must be selected from the provider ingress network"}`)) + return + } + var items []map[string]any + for label, id := range d.support { + if strings.Contains(filters, label) { + items = append(items, map[string]any{"Id": id, "State": "running"}) + } + } + _ = json.NewEncoder(w).Encode(items) + case r.Method == http.MethodGet && strings.HasPrefix(path, "/containers/") && strings.HasSuffix(path, "/json"): + if d.healthChecked != nil { + select { + case d.healthChecked <- struct{}{}: + default: + } + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "Id": strings.TrimSuffix(strings.TrimPrefix(path, "/containers/"), "/json"), + "State": map[string]any{"Status": "running", "Running": true, "Health": map[string]any{"Status": d.health}}, + "Config": map[string]any{"Labels": map[string]string{}}, + }) + case r.Method == http.MethodPost && strings.HasPrefix(path, "/containers/"): + d.calls = append(d.calls, strings.TrimPrefix(path, "/containers/")) + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]string{"message": "no such object: " + path}) + } +} + +func (d *fakeProviderDaemon) recorded() []string { + d.mu.Lock() + defer d.mu.Unlock() + return append([]string(nil), d.calls...) +} + +// An upgrade recreates the control plane (and often Traefik), and the new +// containers are not on any client's isolated network. The monitor used to +// read that as a failing client and, three minutes later, Stop it; Docker's +// unless-stopped policy never restarts a container stopped through the API, +// so every client workspace stayed down. The monitor must put the support +// containers back on the network, and when a client really is unhealthy it +// must restart it rather than stop it. +func TestHealthMonitorReattachesSupportContainersAndRestartsInsteadOfStopping(t *testing.T) { + daemon := &fakeProviderDaemon{ + tenantNetwork: "pulse-provider-msp-tenant-t-acme", + attached: map[string]bool{"tenant-container": true}, + support: map[string]string{ + "pulse.provider-msp.role=traefik": "traefik-recreated", + "pulse.provider-msp.role=control-plane": "control-plane-recreated", + }, + health: "healthy", + healthChecked: make(chan struct{}, 1), + } + srv := httptest.NewServer(daemon) + t.Cleanup(srv.Close) + t.Setenv("DOCKER_HOST", "tcp://"+strings.TrimPrefix(srv.URL, "http://")) + t.Setenv("DOCKER_TLS_VERIFY", "") + t.Setenv("DOCKER_CERT_PATH", "") + + mgr, err := docker.NewManager(docker.ManagerConfig{ + Image: "pulse:test", + Network: "pulse-provider-msp", + IsolateTenantNetworks: true, + TenantNetworkPrefix: "pulse-provider-msp-tenant", + BaseDomain: "msp.example.com", + }) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + t.Cleanup(func() { _ = mgr.Close() }) + + reg, err := registry.NewTenantRegistry(t.TempDir()) + if err != nil { + t.Fatalf("NewTenantRegistry: %v", err) + } + t.Cleanup(func() { _ = reg.Close() }) + if err := reg.Create(®istry.Tenant{ID: "t-acme", AccountID: "a-1", State: registry.TenantStateActive, ContainerID: "tenant-container"}); err != nil { + t.Fatalf("Create tenant: %v", err) + } + + monitor := NewMonitor(reg, mgr, MonitorConfig{Interval: time.Hour, RestartOnFail: true, FailThreshold: 3}) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + monitor.Run(ctx) + close(done) + }() + select { + case <-daemon.healthChecked: + case <-time.After(5 * time.Second): + cancel() + t.Fatal("monitor did not check the tenant on startup") + } + deadline := time.Now().Add(5 * time.Second) + for { + observed, err := reg.Get("t-acme") + if err == nil && observed != nil && observed.HealthCheckOK { + break + } + if time.Now().After(deadline) { + cancel() + t.Fatalf("monitor did not persist healthy result on startup: tenant=%+v err=%v", observed, err) + } + time.Sleep(5 * time.Millisecond) + } + cancel() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("monitor did not stop after cancellation") + } + + got := strings.Join(daemon.recorded(), ", ") + for _, want := range []string{"connect traefik-recreated", "connect control-plane-recreated"} { + if !strings.Contains(got, want) { + t.Fatalf("after one pass the daemon saw %q, want %q", got, want) + } + } + tenant, err := reg.Get("t-acme") + if err != nil || tenant == nil || !tenant.HealthCheckOK { + t.Fatalf("tenant after healthy pass = %+v, err %v; want HealthCheckOK", tenant, err) + } + + // Already attached: later passes change nothing on the network. + monitor.checkAll(context.Background()) + if again := daemon.recorded(); len(again) != 2 { + t.Fatalf("second pass reconnected attached containers: %v", again) + } + + daemon.mu.Lock() + daemon.health = "unhealthy" + daemon.mu.Unlock() + for range 3 { + monitor.checkAll(context.Background()) + } + got = strings.Join(daemon.recorded(), ", ") + if !strings.Contains(got, "tenant-container/restart") { + t.Fatalf("unhealthy client was not restarted; daemon saw %q", got) + } + if strings.Contains(got, "tenant-container/stop") { + t.Fatalf("unhealthy client was stopped, which unless-stopped never undoes; daemon saw %q", got) + } +} From d7bf38a647e3ef360525bede98187d00eacbddd4 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 24 Sep 2026 03:06:17 +0100 Subject: [PATCH 2/3] test(cloudcp): isolate provider MSP recovery by ingress network Exercise two provider installations sharing a Docker daemon and role labels. Verify that each recreated support pair only rejoins its own client network and that an unhealthy client is restarted, never stopped. Change-source: pulse-maintainer --- internal/cloudcp/health_monitor_test.go | 179 ++++++++++++++++++++++++ 1 file changed, 179 insertions(+) diff --git a/internal/cloudcp/health_monitor_test.go b/internal/cloudcp/health_monitor_test.go index 0a62e2d047..30e66a1bc8 100644 --- a/internal/cloudcp/health_monitor_test.go +++ b/internal/cloudcp/health_monitor_test.go @@ -204,3 +204,182 @@ func TestHealthMonitorReattachesSupportContainersAndRestartsInsteadOfStopping(t t.Fatalf("unhealthy client was stopped, which unless-stopped never undoes; daemon saw %q", got) } } + +// Two provider installations can share a Docker daemon and the same support +// role labels. A recreated container must only join the tenant network of the +// installation whose ingress network already contains it. +type multiProviderDaemon struct { + mu sync.Mutex + networks map[string]*multiProviderNetwork + support map[string]multiProviderSupport + health map[string]string + calls []string +} + +type multiProviderNetwork struct { + tenantID string + attached map[string]bool +} + +type multiProviderSupport struct { + role string + ingress string +} + +func (d *multiProviderDaemon) ServeHTTP(w http.ResponseWriter, r *http.Request) { + d.mu.Lock() + defer d.mu.Unlock() + + path := dockerAPIVersionPrefix.ReplaceAllString(r.URL.Path, "") + w.Header().Set("Api-Version", "1.47") + w.Header().Set("Content-Type", "application/json") + switch { + case path == "/_ping": + _, _ = w.Write([]byte("OK")) + case r.Method == http.MethodGet && strings.HasPrefix(path, "/networks/"): + name := strings.TrimPrefix(path, "/networks/") + net, ok := d.networks[name] + if !ok { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"network not found"}`)) + return + } + containers := make(map[string]any, len(net.attached)) + for id := range net.attached { + containers[id] = map[string]any{"Name": id} + } + labels := map[string]string{} + if net.tenantID != "" { + labels = map[string]string{"pulse.tenant.id": net.tenantID, "pulse.provider-msp.network": "tenant"} + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "Name": name, "Id": name, "Containers": containers, "Labels": labels, + }) + case r.Method == http.MethodPost && strings.HasPrefix(path, "/networks/") && strings.HasSuffix(path, "/connect"): + name := strings.TrimSuffix(strings.TrimPrefix(path, "/networks/"), "/connect") + var body struct{ Container string } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + if _, ok := d.networks[name]; !ok { + w.WriteHeader(http.StatusNotFound) + return + } + d.networks[name].attached[body.Container] = true + d.calls = append(d.calls, "connect "+body.Container+" "+name) + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodGet && path == "/containers/json": + var filters map[string]map[string]bool + if err := json.Unmarshal([]byte(r.URL.Query().Get("filters")), &filters); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + var items []map[string]any + for id, support := range d.support { + if matchesDockerFilter(filters["label"], support.role) && matchesDockerFilter(filters["network"], support.ingress) { + items = append(items, map[string]any{"Id": id, "State": "running"}) + } + } + _ = json.NewEncoder(w).Encode(items) + case r.Method == http.MethodGet && strings.HasPrefix(path, "/containers/") && strings.HasSuffix(path, "/json"): + id := strings.TrimSuffix(strings.TrimPrefix(path, "/containers/"), "/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "Id": id, + "State": map[string]any{"Status": "running", "Running": true, "Health": map[string]any{"Status": d.health[id]}}, + "Config": map[string]any{"Labels": map[string]string{}}, + }) + case r.Method == http.MethodPost && strings.HasPrefix(path, "/containers/"): + d.calls = append(d.calls, strings.TrimPrefix(path, "/containers/")) + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"no such object"}`)) + } +} + +func matchesDockerFilter(values map[string]bool, candidate string) bool { + if len(values) == 0 { + return true + } + return values[candidate] +} + +func (d *multiProviderDaemon) recorded() []string { + d.mu.Lock() + defer d.mu.Unlock() + return append([]string(nil), d.calls...) +} + +func TestHealthMonitorKeepsProviderInstallationsIsolatedOnRecovery(t *testing.T) { + daemon := &multiProviderDaemon{ + networks: map[string]*multiProviderNetwork{ + "provider-a-tenant-t-alpha": {tenantID: "t-alpha", attached: map[string]bool{"client-alpha": true}}, + "provider-b-tenant-t-beta": {tenantID: "t-beta", attached: map[string]bool{"client-beta": true}}, + }, + support: map[string]multiProviderSupport{ + "traefik-a": {role: "pulse.provider-msp.role=traefik", ingress: "provider-a"}, + "control-a": {role: "pulse.provider-msp.role=control-plane", ingress: "provider-a"}, + "traefik-b": {role: "pulse.provider-msp.role=traefik", ingress: "provider-b"}, + "control-b": {role: "pulse.provider-msp.role=control-plane", ingress: "provider-b"}, + }, + health: map[string]string{"client-alpha": "healthy", "client-beta": "unhealthy"}, + } + srv := httptest.NewServer(daemon) + t.Cleanup(srv.Close) + t.Setenv("DOCKER_HOST", "tcp://"+strings.TrimPrefix(srv.URL, "http://")) + t.Setenv("DOCKER_TLS_VERIFY", "") + t.Setenv("DOCKER_CERT_PATH", "") + + for _, client := range []struct { + provider, tenantID, containerID string + passes int + }{ + {provider: "provider-a", tenantID: "t-alpha", containerID: "client-alpha", passes: 1}, + {provider: "provider-b", tenantID: "t-beta", containerID: "client-beta", passes: 3}, + } { + mgr, err := docker.NewManager(docker.ManagerConfig{ + Image: "pulse:test", Network: client.provider, IsolateTenantNetworks: true, + TenantNetworkPrefix: client.provider + "-tenant", BaseDomain: "msp.example.com", + }) + if err != nil { + t.Fatalf("NewManager(%s): %v", client.provider, err) + } + t.Cleanup(func() { _ = mgr.Close() }) + reg, err := registry.NewTenantRegistry(t.TempDir()) + if err != nil { + t.Fatalf("NewTenantRegistry(%s): %v", client.provider, err) + } + t.Cleanup(func() { _ = reg.Close() }) + if err := reg.Create(®istry.Tenant{ + ID: client.tenantID, AccountID: client.provider, State: registry.TenantStateActive, + ContainerID: client.containerID, + }); err != nil { + t.Fatalf("Create(%s): %v", client.tenantID, err) + } + monitor := NewMonitor(reg, mgr, MonitorConfig{RestartOnFail: true, FailThreshold: 3}) + for range client.passes { + monitor.checkAll(context.Background()) + } + } + + got := daemon.recorded() + want := map[string]int{ + "connect traefik-a provider-a-tenant-t-alpha": 1, + "connect control-a provider-a-tenant-t-alpha": 1, + "connect traefik-b provider-b-tenant-t-beta": 1, + "connect control-b provider-b-tenant-t-beta": 1, + "client-beta/restart": 1, + } + for _, call := range got { + if want[call] == 0 { + t.Fatalf("unexpected Docker mutation %q (including cross-client attachment or stop); all=%v", call, got) + } + want[call]-- + } + for call, count := range want { + if count != 0 { + t.Fatalf("missing Docker mutation %q; all=%v", call, got) + } + } +} From 4f7718a27fe8e013406c68aa98307c6f7f723e94 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Thu, 24 Sep 2026 03:50:21 +0100 Subject: [PATCH 3/3] fix(cloudcp): reject unowned tenant networks during provisioning Require exact tenant and runtime labels before adopting an existing network, share that check with support-container recovery, and cover wrong or absent labels. Update the cloud-paid and deployment contracts to state the provisioning boundary. Change-source: pulse-maintainer --- .../v6/internal/subsystems/cloud-paid.md | 10 ++- .../subsystems/deployment-installability.md | 7 ++- internal/cloudcp/docker/manager.go | 16 +++-- internal/cloudcp/docker/manager_test.go | 63 +++++++++++++++++++ 4 files changed, 88 insertions(+), 8 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/cloud-paid.md b/docs/release-control/v6/internal/subsystems/cloud-paid.md index b27ef1afc2..86f0329229 100644 --- a/docs/release-control/v6/internal/subsystems/cloud-paid.md +++ b/docs/release-control/v6/internal/subsystems/cloud-paid.md @@ -3443,10 +3443,16 @@ client's isolated tenant network. Recreating either one, which every `upgrade.sh` run does to the control plane, drops those attachments, cutting the client's Traefik route and, with the old stop behaviour, taking every client offline three minutes later. A client without an isolated network is -skipped quietly. Regression coverage: +skipped quietly during recovery. Provisioning may create a missing isolated +network, but it must not adopt a same-named pre-existing network unless the +network has the exact tenant ID and provider-MSP tenant-runtime labels; an +empty tenant ID is invalid. This prevents a foreign or unlabelled network from +becoming a client's runtime route before support containers are attached. +Regression coverage: `TestHealthMonitorReattachesSupportContainersAndRestartsInsteadOfStopping` in `internal/cloudcp/health_monitor_test.go` and -`TestEnsureSupportContainersOnTenantNetworkSkipsMissingNetwork` in +`TestEnsureSupportContainersOnTenantNetworkSkipsMissingNetwork` and +`TestEnsureTenantNetworkRejectsUnownedExistingNetwork` in `internal/cloudcp/docker/manager_test.go`. ### Provider-hosted MSP platforms buy and renew their own licence diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index abff57d80c..d2b6414441 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -2438,8 +2438,11 @@ on startup and each pass, and restarts rather than stops an unhealthy client. The original PR reports a two-client v6.4.1 lab reproduction and recovery on the same installation on 2026-09-23; that is not installed acceptance of this integrated source. Reconnection selects support containers from the configured -provider ingress network and requires exact tenant ownership labels before -mutating an existing network. +provider ingress network. Both reconnection and new workspace provisioning +require exact tenant ownership and tenant-runtime labels on an existing +isolated network before attaching containers; provisioning refuses an +unlabelled same-named network instead of adopting it. A legacy client without +an isolated network remains a reconnection no-op. ### Provider MSP operations accept a renewed licence diff --git a/internal/cloudcp/docker/manager.go b/internal/cloudcp/docker/manager.go index 9495da4300..8c1bbf669b 100644 --- a/internal/cloudcp/docker/manager.go +++ b/internal/cloudcp/docker/manager.go @@ -365,12 +365,17 @@ func (m *Manager) ensureTenantNetwork(ctx context.Context, tenantID string) (str } return networkName, nil } + if strings.TrimSpace(tenantID) == "" { + return "", fmt.Errorf("tenant ID is required for an isolated network") + } networkName := m.tenantNetworkName(tenantID) inspect, err := m.cli.NetworkInspect(ctx, networkName, client.NetworkInspectOptions{}) if err == nil { - if got := strings.TrimSpace(inspect.Network.Labels["pulse.tenant.id"]); got != "" && got != tenantID { - return "", fmt.Errorf("tenant network %q belongs to tenant %q, not %q", networkName, got, tenantID) + // A matching name is not evidence that this is our isolated network. + // In particular, never adopt a pre-existing unlabelled Docker network. + if !isOwnedTenantNetwork(inspect.Network.Labels, tenantID) { + return "", fmt.Errorf("network %q is not the isolated network for tenant %q", networkName, tenantID) } return networkName, nil } @@ -392,6 +397,10 @@ func (m *Manager) ensureTenantNetwork(ctx context.Context, tenantID string) (str return networkName, nil } +func isOwnedTenantNetwork(labels map[string]string, tenantID string) bool { + return tenantID != "" && labels["pulse.tenant.id"] == tenantID && labels[tenantRuntimeNetworkLabel] == tenantRuntimeNetworkLabelValue +} + func (m *Manager) connectSupportContainersToTenantNetwork(ctx context.Context, networkName string) error { if m == nil || !m.cfg.IsolateTenantNetworks { return nil @@ -903,8 +912,7 @@ func (m *Manager) EnsureSupportContainersOnTenantNetwork(ctx context.Context, te } // A name alone does not establish ownership. Never attach provider support // containers to an unrelated network that happens to have the derived name. - labels := inspected.Network.Labels - if labels["pulse.tenant.id"] != tenantID || labels[tenantRuntimeNetworkLabel] != tenantRuntimeNetworkLabelValue { + if !isOwnedTenantNetwork(inspected.Network.Labels, tenantID) { return fmt.Errorf("network %q is not the isolated network for tenant %q", networkName, tenantID) } return m.connectSupportContainersToTenantNetwork(ctx, networkName) diff --git a/internal/cloudcp/docker/manager_test.go b/internal/cloudcp/docker/manager_test.go index 822adc93d8..ad238d2cb0 100644 --- a/internal/cloudcp/docker/manager_test.go +++ b/internal/cloudcp/docker/manager_test.go @@ -2,6 +2,7 @@ package docker import ( "context" + "encoding/json" "net/http" "net/http/httptest" "os" @@ -674,3 +675,65 @@ func TestEnsureSupportContainersOnTenantNetworkRejectsWrongOwner(t *testing.T) { t.Fatalf("reconcile changed wrong-owner network: %v", mutations) } } + +func TestEnsureTenantNetworkRejectsUnownedExistingNetwork(t *testing.T) { + var labels map[string]string + var mutations []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Api-Version", "1.47") + if strings.HasSuffix(r.URL.Path, "/_ping") { + _, _ = w.Write([]byte("OK")) + return + } + if r.Method != http.MethodGet { + mutations = append(mutations, r.Method+" "+r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "Name": "pulse-provider-msp-tenant-t-acme", + "Id": "net-1", + "Labels": labels, + }) + })) + t.Cleanup(srv.Close) + t.Setenv("DOCKER_HOST", "tcp://"+strings.TrimPrefix(srv.URL, "http://")) + t.Setenv("DOCKER_TLS_VERIFY", "") + t.Setenv("DOCKER_CERT_PATH", "") + + mgr, err := NewManager(ManagerConfig{Image: "pulse:test", Network: "pulse-provider-msp", IsolateTenantNetworks: true}) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + t.Cleanup(func() { _ = mgr.Close() }) + + for _, tc := range []struct { + name string + labels map[string]string + wantOK bool + }{ + {name: "unlabelled"}, + {name: "wrong tenant", labels: map[string]string{"pulse.tenant.id": "t-other", tenantRuntimeNetworkLabel: tenantRuntimeNetworkLabelValue}}, + {name: "missing runtime label", labels: map[string]string{"pulse.tenant.id": "t-acme"}}, + {name: "wrong runtime label", labels: map[string]string{"pulse.tenant.id": "t-acme", tenantRuntimeNetworkLabel: "ingress"}}, + {name: "owned tenant network", labels: map[string]string{"pulse.tenant.id": "t-acme", tenantRuntimeNetworkLabel: tenantRuntimeNetworkLabelValue}, wantOK: true}, + } { + t.Run(tc.name, func(t *testing.T) { + labels = tc.labels + mutations = nil + got, err := mgr.ensureTenantNetwork(context.Background(), "t-acme") + if tc.wantOK { + if err != nil || got != "pulse-provider-msp-tenant-t-acme" { + t.Fatalf("ensureTenantNetwork = (%q, %v), want owned network", got, err) + } + } else if err == nil || got != "" { + t.Fatalf("ensureTenantNetwork = (%q, %v), want fail closed", got, err) + } + if len(mutations) != 0 { + t.Fatalf("ensureTenantNetwork mutated existing network: %v", mutations) + } + }) + } + if got, err := mgr.ensureTenantNetwork(context.Background(), ""); err == nil || got != "" { + t.Fatalf("ensureTenantNetwork(empty) = (%q, %v), want rejection", got, err) + } +}