Multi-VAN support - #2564
Conversation
* No new CRDs needed * Network ID can be set through site.spec.networkId * Router Access and Link now accept inter-network role * Router Access and Link now accept spec.routingKeys (inter-network role) * Dynamic port allocation for Router Access (only supported with updated CRD) * CRDs have been updated, but all updates are optional if not using multi-van * Unit and integration tests added
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds multi-VAN fields to Skupper resources, synchronizes router networks and auto-links, supports inter-network roles and routing keys, shares router-access port allocation, and detects CRD capabilities for dynamic ports. ChangesMulti-VAN routing and router access
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The PR adds dynamic RouterAccess port allocation and multi-network synchronization, but failed updates can leave released ports recorded as active, causing duplicate assignments, while certain network-management entities can stop later router configuration synchronization and valid allocated ports can be rejected during cleanup. These correctness and availability risks should be fixed before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 73 functions across 31 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5e06ede-299f-4f21-89a9-0b358c270f1f
📒 Files selected for processing (32)
config/crd/bases/skupper_link_crd.yamlconfig/crd/bases/skupper_router_access_crd.yamlconfig/crd/bases/skupper_site_crd.yamlconfig/rbac/cluster/clusterrole.yamlinternal/cmd/skupper/debug/sweeper/ports_test.gointernal/cmd/skupper/link/kube/link_generate.gointernal/fixtures/skupper_resources.gointernal/kube/adaptor/config_sync.gointernal/kube/client/client.gointernal/kube/client/crds.gointernal/kube/client/crds_test.gointernal/kube/client/fake/fake_client.gointernal/kube/controller/controller.gointernal/kube/grants/tokens.gointernal/kube/site/site.gointernal/kube/site/site_test.gointernal/kube/watchers/watchers.gointernal/ports/ports.gointernal/ports/ports_test.gointernal/qdr/amqp_mgmt.gointernal/qdr/qdr.gointernal/qdr/qdr_test.gointernal/qdr/sync_router_ops.gointernal/site/link.gointernal/site/link_test.gointernal/site/routeraccess.gointernal/site/routeraccess_test.gopkg/apis/skupper/v2alpha1/link_types_test.gopkg/apis/skupper/v2alpha1/types.gopkg/apis/skupper/v2alpha1/zz_generated.deepcopy.gotests/integration/kube/controller/site_test.gotests/integration/kube/controller/suite_test.go
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
c-kruse
left a comment
c-kruse
left a comment
There was a problem hiding this comment.
Curious if you've got a plan for status for multi-van Links. IIUC these would stay Pending right now.
@c-kruse not yet. I believe it is better to handle it separately. |
* RouterAccess description improved * Debug Sweeper now uses its own constant * Validate port collisions during RouterAccess reconciliation
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 82420edb-7eef-457a-a19b-a9cdbbafdb62
📒 Files selected for processing (7)
config/crd/bases/skupper_router_access_crd.yamlinternal/cmd/skupper/debug/sweeper/ports.gointernal/kube/site/site.gointernal/site/link.gointernal/site/routeraccess.gointernal/site/routeraccess_test.gopkg/apis/skupper/v2alpha1/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/site/link.go
- config/crd/bases/skupper_router_access_crd.yaml
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 6d96efc0-5cbe-41f9-bc1b-7fb150389ac2
📒 Files selected for processing (5)
internal/kube/site/extended_bindings.gointernal/kube/site/site.gointernal/ports/ports.gointernal/ports/ports_test.gointernal/qdr/port_mapping.go
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| port, err = s.getPool().NextFreePort() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back partial allocations when a later role fails.
If a RouterAccess has multiple dynamic roles, NextFreePort() can allocate an earlier role and fail on a later role. The return at Line 1826 exits without releasing allocatedPorts or undoing the matching la.AllocatePort mutation. Each retry can leak another port from the shared pool and eventually block allocations for other RouterAccesses and listeners.
Release the partial allocations before returning from this loop.
Proposed fix
port, err = s.getPool().NextFreePort()
if err != nil {
+ la.ReleaseUnusedPorts(allocatedPorts...)
+ s.getPool().ReleaseAll(allocatedPorts...)
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| port, err = s.getPool().NextFreePort() | |
| if err != nil { | |
| return err | |
| port, err = s.getPool().NextFreePort() | |
| if err != nil { | |
| la.ReleaseUnusedPorts(allocatedPorts...) | |
| s.getPool().ReleaseAll(allocatedPorts...) | |
| return err |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: bc711717-56aa-494a-a20c-064a8121a915
📒 Files selected for processing (5)
internal/kube/site/extended_bindings.gointernal/kube/site/site.gointernal/qdr/port_mapping.gointernal/site/routeraccess.gointernal/site/routeraccess_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/kube/site/site.go
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| continue | ||
| } | ||
| if name, ok := usedPorts[int32(role.Port)]; ok && name != ra.Name { | ||
| return true, fmt.Sprintf("router access: %s", name), role.Port |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Release allocated ports on the conflict path.
When this check reports a conflict, internal/kube/site/site.go at Lines [1714-1740] removes the existing entry but does not release its GetAllocatedPorts() from the shared pool. A dynamic port then remains unavailable after the RouterAccess is removed. Repeated conflicts can exhaust the pool and make later allocation return No available ports. Release the previous entry’s allocated ports before deleting it. Apply the same cleanup to the mixed static/dynamic rejection path. (raw.githubusercontent.com)
Source: MCP tools
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/skupperproject-skupper-1bf01da4 -maxdepth 2 -type f -name '*.md' -print \
| sort \
| while read -r f; do
case "$f" in
*/learnings/*|*/architecture/*) ;;
*) printf '\n### %s\n' "$f"; sed -n '1,220p' "$f" ;;
esac
done
printf '%s\n' '--- target file outline ---'
ast-grep outline internal/site/routeraccess.go
printf '%s\n' '--- target implementation and nearby definitions ---'
sed -n '1,230p' internal/site/routeraccess.go
printf '%s\n' '--- direct callers and conflict handling ---'
rg -n -C 8 'HasPortConflict|GetPortForRole|desiredListeners|RouterAccessConfig\.Apply|Apply\(.*RouterAccess' internal internal 2>/dev/nullRepository: skupperproject/skupper
Length of output: 30926
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- RouterAccess port semantics ---'
rg -n -C 12 'func \(.*RouterAccess.*GetPortForRole|type RouterAccessRole|type RouterAccess struct|func \(.*RouterAccess.*FindRole' pkg internal
printf '%s\n' '--- conflict caller and update path ---'
sed -n '1746,1865p' internal/kube/site/site.go
printf '%s\n' '--- listener contract and endpoint handling ---'
rg -n -C 12 'func .*AddListener|type Listener|ListenersDifference|GetMatchingListeners|RemoveListener' internal/qdrRepository: skupperproject/skupper
Length of output: 28230
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- allocated-port and role definitions ---'
sed -n '977,1025p' pkg/apis/skupper/v2alpha1/types.go
rg -n -C 8 'MixesDynamicAndStaticPorts|RouterAccess.*(Validate|validate)|roles:|port:' pkg internal | head -240
printf '%s\n' '--- HasPortConflict test cases ---'
sed -n '586,915p' internal/site/routeraccess_test.go
printf '%s\n' '--- RouterAccess map population ---'
rg -n -C 10 'linkAccess\s*=|linkAccess\[|CheckRouterAccess\(' internal/kube/site/site.goRepository: skupperproject/skupper
Length of output: 36169
Reject duplicate ports within one RouterAccess.
HasPortConflict does not compare ports between candidate roles. Two roles on port 55671 can pass the check, while desiredListeners creates two listeners on the same bind endpoint. Add an intra-resource duplicate-port check and a table case.
Source: MCP tools
There was a problem hiding this comment.
I am going to write a CEL expression to handle it. Since RouterAccess CRD is changing anyway.
There was a problem hiding this comment.
@fgiorgetti, that is appropriate for duplicate ports within one RouterAccess. A CEL expression can reject the invalid resource before reconciliation.
The allocated-port release path is separate from this validation. It still needs cleanup when reconciliation rejects or removes an existing RouterAccess.
🧠 Learnings used
Learnt from: fgiorgetti
Repo: skupperproject/skupper PR: 2564
File: pkg/apis/skupper/v2alpha1/types.go:982-991
Timestamp: 2026-08-31T15:10:14.858Z
Learning: In `pkg/apis/skupper/v2alpha1/types.go`, RouterAccess did not previously prevent conflicts between manually assigned role ports. Manual port-conflict validation is required when allocating dynamic router-access ports.
You are interacting with an AI system.
* Do not allow duplicated role names * Do not allow duplicated static ports
2 participants
Summary by CodeRabbit