Skip to content

fix: attach extra_networks to service containers#328

Merged
rshoemaker merged 2 commits intomainfrom
fix/PLAT-535/svc-extra-networks
Apr 9, 2026
Merged

fix: attach extra_networks to service containers#328
rshoemaker merged 2 commits intomainfrom
fix/PLAT-535/svc-extra-networks

Conversation

@rshoemaker
Copy link
Copy Markdown
Contributor

@rshoemaker rshoemaker commented Apr 3, 2026

Summary

  • Wire extra_networks from orchestrator_opts.swarm into the service container spec, matching the existing Postgres container behavior
  • Extract swarmOpts local variable to consolidate nil-guard (consistent with spec.go)
  • Add comments noting ExtraVolumes (not supported for services) and DriverOpts (not passed through) are intentionally omitted

Test plan

  • make test passes
  • Deploy a service with orchestrator_opts.swarm.extra_networksdocker service inspect should show the extra network(s)
  • Deploy a service without orchestrator_opts — only bridge and database overlay attached (no regression)

PLAT-535

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8eaa861c-5b76-453a-82ce-11ea2be3e9d2

📥 Commits

Reviewing files that changed from the base of the PR and between 562bb25 and 26b5dbf.

📒 Files selected for processing (3)
  • server/internal/api/apiv1/validate.go
  • server/internal/api/apiv1/validate_test.go
  • server/internal/orchestrator/swarm/service_spec.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/internal/orchestrator/swarm/service_spec.go

📝 Walkthrough

Walkthrough

Refactors Swarm orchestrator option handling, forwards user-configured extra Swarm networks into service task network attachments, clarifies in-code behavior for extra volumes/driver options, and adds validation and unit tests to reject/accept specific Swarm orchestrator fields at API validation.

Changes

Cohort / File(s) Summary
Service Container Networking
server/internal/orchestrator/swarm/service_spec.go, server/internal/orchestrator/swarm/service_spec_test.go
Extracts local swarmOpts, uses swarmOpts.ExtraLabels for label merging, appends swarmOpts.ExtraNetworks entries to TaskTemplate.Networks mapping net.IDTarget and net.AliasesAliases. Tests added to verify default vs. extra networks and alias handling.
API Validation for Service Orchestrator Options
server/internal/api/apiv1/validate.go, server/internal/api/apiv1/validate_test.go
Introduces validateServiceOrchestratorOpts layering service-specific checks over shared validation. Rejects swarm.extra_volumes and swarm.extra_networks[*].driver_opts for services; tests added for rejection and acceptance cases.
Database Service Spec Comment
server/internal/orchestrator/swarm/spec.go
Adds a clarifying comment that ExtraNetworkSpec.DriverOpts is accepted by the API but not forwarded into the created swarm.NetworkAttachmentConfig.

Poem

I’m a rabbit in the code, ears tuned to nets, 🐇
I hop in extra networks and place their sets,
Two defaults first, then aliases align,
Tests hop by to prove each binding fine,
Labels stitched, validations keep us in line. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: attaching extra_networks to service containers in Swarm orchestration.
Description check ✅ Passed The description includes Summary, Changes, Test plan, and Issue reference but lacks Testing section details and explicit Changelog/Breaking changes checklist items from the template.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/PLAT-535/svc-extra-networks

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codacy-production
Copy link
Copy Markdown

codacy-production bot commented Apr 3, 2026

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 7 complexity · 0 duplication

Metric Results
Complexity 7
Duplication 0

View in Codacy

TIP This summary will be updated as you push new changes. Give us feedback

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/spec.go (1)

74-81: Inconsistent variable usage: use swarmOpts.ExtraNetworks for consistency.

Line 76 accesses instance.OrchestratorOpts.Swarm.ExtraNetworks directly, while lines 66-72 use the extracted swarmOpts variable for ExtraLabels and ExtraVolumes. The new service_spec.go code consistently uses swarmOpts.ExtraNetworks.

♻️ Suggested fix for consistency
 		// NOTE: DriverOpts on ExtraNetworkSpec is accepted by the API but not
 		// passed through here — add if needed.
-		for _, net := range instance.OrchestratorOpts.Swarm.ExtraNetworks {
+		for _, net := range swarmOpts.ExtraNetworks {
 			networks = append(networks, swarm.NetworkAttachmentConfig{
 				Target:  net.ID,
 				Aliases: net.Aliases,
 			})
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/internal/orchestrator/swarm/spec.go` around lines 74 - 81, The loop
building NetworkAttachmentConfig uses
instance.OrchestratorOpts.Swarm.ExtraNetworks directly while earlier code uses
the extracted swarmOpts variable; change the loop to iterate over
swarmOpts.ExtraNetworks instead to be consistent with how ExtraLabels and
ExtraVolumes are accessed (look for the variable swarmOpts and the loop that
appends to networks with swarm.NetworkAttachmentConfig and replace the source
collection with swarmOpts.ExtraNetworks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/internal/orchestrator/swarm/spec.go`:
- Around line 74-81: The loop building NetworkAttachmentConfig uses
instance.OrchestratorOpts.Swarm.ExtraNetworks directly while earlier code uses
the extracted swarmOpts variable; change the loop to iterate over
swarmOpts.ExtraNetworks instead to be consistent with how ExtraLabels and
ExtraVolumes are accessed (look for the variable swarmOpts and the loop that
appends to networks with swarm.NetworkAttachmentConfig and replace the source
collection with swarmOpts.ExtraNetworks).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8ef1eb1c-b5c0-408a-824d-3752ca728ade

📥 Commits

Reviewing files that changed from the base of the PR and between 0724783 and 562bb25.

📒 Files selected for processing (3)
  • server/internal/orchestrator/swarm/service_spec.go
  • server/internal/orchestrator/swarm/service_spec_test.go
  • server/internal/orchestrator/swarm/spec.go

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.
@rshoemaker rshoemaker requested a review from jason-lynch April 8, 2026 20:04
@rshoemaker rshoemaker merged commit 5109a1d into main Apr 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants