From 562bb2596e89c2c769188d3d6eb4dfa5d5dfeb56 Mon Sep 17 00:00:00 2001 From: rshoemaker Date: Fri, 3 Apr 2026 17:10:14 -0400 Subject: [PATCH 1/2] fix: attach extra_networks to service containers ServiceContainerSpec was silently dropping extra_networks from orchestrator_opts. Services were only attached to bridge and the database overlay, even when extra networks were specified. Mirror the existing Postgres spec.go pattern to append ExtraNetworks with target and aliases. --- .../orchestrator/swarm/service_spec.go | 27 ++++++- .../orchestrator/swarm/service_spec_test.go | 80 +++++++++++++++++++ server/internal/orchestrator/swarm/spec.go | 2 + 3 files changed, 106 insertions(+), 3 deletions(-) diff --git a/server/internal/orchestrator/swarm/service_spec.go b/server/internal/orchestrator/swarm/service_spec.go index 9901620d..635a9226 100644 --- a/server/internal/orchestrator/swarm/service_spec.go +++ b/server/internal/orchestrator/swarm/service_spec.go @@ -67,9 +67,18 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, "pgedge.host.id": opts.HostID, } - // Merge user-provided extra labels (matches Postgres ExtraLabels behavior) - if opts.ServiceSpec.OrchestratorOpts != nil && opts.ServiceSpec.OrchestratorOpts.Swarm != nil { - for k, v := range opts.ServiceSpec.OrchestratorOpts.Swarm.ExtraLabels { + // Extract swarm orchestrator options (matches Postgres pattern in spec.go). + // NOTE: ExtraVolumes is not supported for service containers — services get + // their bind mounts configured per service type below. Add support here if a + // use case arises. + var swarmOpts *database.SwarmOpts + if opts.ServiceSpec.OrchestratorOpts != nil { + swarmOpts = opts.ServiceSpec.OrchestratorOpts.Swarm + } + + // Merge user-provided extra labels + if swarmOpts != nil { + for k, v := range swarmOpts.ExtraLabels { labels[k] = v } } @@ -90,6 +99,18 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, }, } + // Append user-requested extra networks (e.g. Traefik, reverse proxy). + // NOTE: DriverOpts on ExtraNetworkSpec is accepted by the API but not + // passed through here or in Postgres spec.go — add if needed. + if swarmOpts != nil { + for _, net := range swarmOpts.ExtraNetworks { + networks = append(networks, swarm.NetworkAttachmentConfig{ + Target: net.ID, + Aliases: net.Aliases, + }) + } + } + // Get container image (already resolved in ServiceImage) image := opts.ServiceImage.Tag diff --git a/server/internal/orchestrator/swarm/service_spec_test.go b/server/internal/orchestrator/swarm/service_spec_test.go index 83c97573..798a7596 100644 --- a/server/internal/orchestrator/swarm/service_spec_test.go +++ b/server/internal/orchestrator/swarm/service_spec_test.go @@ -309,6 +309,86 @@ func intPtr(i int) *int { return &i } +func TestServiceContainerSpec_ExtraNetworks(t *testing.T) { + opts := &ServiceContainerSpecOptions{ + ServiceSpec: &database.ServiceSpec{ + ServiceID: "mcp-server", + ServiceType: "mcp", + OrchestratorOpts: &database.OrchestratorOpts{ + Swarm: &database.SwarmOpts{ + ExtraNetworks: []database.ExtraNetworkSpec{ + {ID: "traefik_us-west-2a", Aliases: []string{"mcp"}}, + {ID: "monitoring"}, + }, + }, + }, + }, + ServiceInstanceID: "db1-mcp-host1", + DatabaseID: "db1", + DatabaseName: "testdb", + HostID: "host1", + ServiceName: "db1-mcp-host1", + Hostname: "mcp-host1", + CohortMemberID: "node-123", + ServiceImage: &ServiceImage{Tag: "ghcr.io/pgedge/postgres-mcp:latest"}, + Credentials: &database.ServiceUser{Username: "svc_mcp", Password: "pw"}, + DatabaseNetworkID: "db1-database", + Port: intPtr(8080), + DataPath: "/var/lib/pgedge/services/db1-mcp-host1", + } + + spec, err := ServiceContainerSpec(opts) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + + networks := spec.TaskTemplate.Networks + // [0]=bridge, [1]=database overlay, [2..]=extra networks + if len(networks) != 4 { + t.Fatalf("got %d networks, want 4", len(networks)) + } + if networks[2].Target != "traefik_us-west-2a" { + t.Errorf("networks[2].Target = %q, want %q", networks[2].Target, "traefik_us-west-2a") + } + if len(networks[2].Aliases) != 1 || networks[2].Aliases[0] != "mcp" { + t.Errorf("networks[2].Aliases = %v, want [mcp]", networks[2].Aliases) + } + if networks[3].Target != "monitoring" { + t.Errorf("networks[3].Target = %q, want %q", networks[3].Target, "monitoring") + } +} + +func TestServiceContainerSpec_NoExtraNetworks(t *testing.T) { + opts := &ServiceContainerSpecOptions{ + ServiceSpec: &database.ServiceSpec{ + ServiceID: "mcp-server", + ServiceType: "mcp", + }, + ServiceInstanceID: "db1-mcp-host1", + DatabaseID: "db1", + DatabaseName: "testdb", + HostID: "host1", + ServiceName: "db1-mcp-host1", + Hostname: "mcp-host1", + CohortMemberID: "node-123", + ServiceImage: &ServiceImage{Tag: "ghcr.io/pgedge/postgres-mcp:latest"}, + Credentials: &database.ServiceUser{Username: "svc_mcp", Password: "pw"}, + DatabaseNetworkID: "db1-database", + Port: intPtr(8080), + DataPath: "/var/lib/pgedge/services/db1-mcp-host1", + } + + spec, err := ServiceContainerSpec(opts) + if err != nil { + t.Fatalf("ServiceContainerSpec() error = %v", err) + } + + networks := spec.TaskTemplate.Networks + if len(networks) != 2 { + t.Fatalf("got %d networks, want 2", len(networks)) + } +} + // --- PostgREST container spec tests --- func makePostgRESTSpecOpts() *ServiceContainerSpecOptions { diff --git a/server/internal/orchestrator/swarm/spec.go b/server/internal/orchestrator/swarm/spec.go index 765e7a42..6ffa7640 100644 --- a/server/internal/orchestrator/swarm/spec.go +++ b/server/internal/orchestrator/swarm/spec.go @@ -71,6 +71,8 @@ func DatabaseServiceSpec( mounts = append(mounts, docker.BuildMount(vol.HostPath, vol.DestinationPath, false)) } + // NOTE: DriverOpts on ExtraNetworkSpec is accepted by the API but not + // passed through here — add if needed. for _, net := range instance.OrchestratorOpts.Swarm.ExtraNetworks { networks = append(networks, swarm.NetworkAttachmentConfig{ Target: net.ID, From 26b5dbffe53a81e16daa707858ba666de3c355b7 Mon Sep 17 00:00:00 2001 From: rshoemaker Date: Tue, 7 Apr 2026 13:31:02 -0400 Subject: [PATCH 2/2] fix: reject unsupported orchestrator_opts for services Return API validation errors when a service spec includes extra_volumes or driver_opts on extra_networks, rather than silently ignoring them at deploy time. --- server/internal/api/apiv1/validate.go | 30 +++++++++- server/internal/api/apiv1/validate_test.go | 57 +++++++++++++++++++ .../orchestrator/swarm/service_spec.go | 7 +-- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/server/internal/api/apiv1/validate.go b/server/internal/api/apiv1/validate.go index 20a3c7d6..2f6e278a 100644 --- a/server/internal/api/apiv1/validate.go +++ b/server/internal/api/apiv1/validate.go @@ -343,8 +343,8 @@ func validateServiceSpec(svc *api.ServiceSpec, path []string, isUpdate bool, nod errs = append(errs, validateMemory(svc.Memory, appendPath(path, "memory"))...) } - // Validate orchestrator_opts - errs = append(errs, validateOrchestratorOpts(svc.OrchestratorOpts, appendPath(path, "orchestrator_opts"))...) + // Validate orchestrator_opts (service-specific restrictions on top of shared checks) + errs = append(errs, validateServiceOrchestratorOpts(svc.OrchestratorOpts, appendPath(path, "orchestrator_opts"))...) return errs } @@ -662,6 +662,32 @@ func validateOrchestratorOpts(opts *api.OrchestratorOpts, path []string) []error return errs } +// validateServiceOrchestratorOpts runs the shared orchestrator_opts checks and +// adds service-specific restrictions. Services do not support extra_volumes +// (bind mounts are configured per service type) or driver_opts on extra_networks. +func validateServiceOrchestratorOpts(opts *api.OrchestratorOpts, path []string) []error { + errs := validateOrchestratorOpts(opts, path) + + if opts == nil || opts.Swarm == nil { + return errs + } + + if len(opts.Swarm.ExtraVolumes) > 0 { + err := errors.New("extra_volumes is not supported for services") + errs = append(errs, newValidationError(err, appendPath(path, "swarm", "extra_volumes"))) + } + + for i, net := range opts.Swarm.ExtraNetworks { + if len(net.DriverOpts) > 0 { + netPath := appendPath(path, "swarm", "extra_networks", arrayIndexPath(i), "driver_opts") + err := errors.New("driver_opts is not supported for services") + errs = append(errs, newValidationError(err, netPath)) + } + } + + return errs +} + func validatePgBackRestOptions(opts map[string]string, path []string) []error { var errs []error diff --git a/server/internal/api/apiv1/validate_test.go b/server/internal/api/apiv1/validate_test.go index e3980c3c..4b43c0a2 100644 --- a/server/internal/api/apiv1/validate_test.go +++ b/server/internal/api/apiv1/validate_test.go @@ -1264,6 +1264,63 @@ func TestValidateServiceSpec(t *testing.T) { "cpus: failed to parse CPUs", }, }, + { + name: "extra_volumes rejected for services", + svc: &api.ServiceSpec{ + ServiceID: "mcp-server", + ServiceType: "mcp", + Version: "latest", + HostIds: []api.Identifier{"host-1"}, + Config: map[string]any{}, + OrchestratorOpts: &api.OrchestratorOpts{ + Swarm: &api.SwarmOpts{ + ExtraVolumes: []*api.ExtraVolumesSpec{ + {HostPath: "/data", DestinationPath: "/mnt/data"}, + }, + }, + }, + }, + expected: []string{ + "orchestrator_opts.swarm.extra_volumes: extra_volumes is not supported for services", + }, + }, + { + name: "driver_opts rejected for service extra_networks", + svc: &api.ServiceSpec{ + ServiceID: "mcp-server", + ServiceType: "mcp", + Version: "latest", + HostIds: []api.Identifier{"host-1"}, + Config: map[string]any{}, + OrchestratorOpts: &api.OrchestratorOpts{ + Swarm: &api.SwarmOpts{ + ExtraNetworks: []*api.ExtraNetworkSpec{ + {ID: "traefik", DriverOpts: map[string]string{"com.docker.network.driver.mtu": "1500"}}, + }, + }, + }, + }, + expected: []string{ + "orchestrator_opts.swarm.extra_networks[0].driver_opts: driver_opts is not supported for services", + }, + }, + { + name: "valid service with extra_networks and no driver_opts", + svc: &api.ServiceSpec{ + ServiceID: "mcp-server", + ServiceType: "mcp", + Version: "latest", + HostIds: []api.Identifier{"host-1"}, + Config: map[string]any{}, + OrchestratorOpts: &api.OrchestratorOpts{ + Swarm: &api.SwarmOpts{ + ExtraNetworks: []*api.ExtraNetworkSpec{ + {ID: "traefik"}, + }, + }, + }, + }, + }, } { t.Run(tc.name, func(t *testing.T) { err := errors.Join(validateServiceSpec(tc.svc, nil, false)...) diff --git a/server/internal/orchestrator/swarm/service_spec.go b/server/internal/orchestrator/swarm/service_spec.go index 635a9226..3f32084c 100644 --- a/server/internal/orchestrator/swarm/service_spec.go +++ b/server/internal/orchestrator/swarm/service_spec.go @@ -68,9 +68,8 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, } // Extract swarm orchestrator options (matches Postgres pattern in spec.go). - // NOTE: ExtraVolumes is not supported for service containers — services get - // their bind mounts configured per service type below. Add support here if a - // use case arises. + // ExtraVolumes and DriverOpts are rejected at the API validation layer + // (validateServiceOrchestratorOpts). var swarmOpts *database.SwarmOpts if opts.ServiceSpec.OrchestratorOpts != nil { swarmOpts = opts.ServiceSpec.OrchestratorOpts.Swarm @@ -100,8 +99,6 @@ func ServiceContainerSpec(opts *ServiceContainerSpecOptions) (swarm.ServiceSpec, } // Append user-requested extra networks (e.g. Traefik, reverse proxy). - // NOTE: DriverOpts on ExtraNetworkSpec is accepted by the API but not - // passed through here or in Postgres spec.go — add if needed. if swarmOpts != nil { for _, net := range swarmOpts.ExtraNetworks { networks = append(networks, swarm.NetworkAttachmentConfig{