Skip to content

userspace: add port mirror snapshot foundation#1392

Open
psaab wants to merge 5 commits into
masterfrom
codex/1376-userspace-port-mirror-snapshot
Open

userspace: add port mirror snapshot foundation#1392
psaab wants to merge 5 commits into
masterfrom
codex/1376-userspace-port-mirror-snapshot

Conversation

@psaab
Copy link
Copy Markdown
Owner

@psaab psaab commented May 17, 2026

Summary

  • add userspace mirror snapshot DTO with ingress ifindex, output ifindex, and rate
  • build mirror snapshots from forwarding-options port-mirroring while rejecting duplicate ingress ifindexes and skipping unresolved outputs
  • keep userspace capability admission fail-closed until runtime cross-binding clone/inject delivery is wired

References #1376.

Tests

  • go test ./pkg/dataplane/userspace -run 'TestConfigSnapshotMirrorConfigsRoundTrip|TestDeriveUserspaceCapabilitiesKeepsPortMirroringFailClosed|TestBuildMirrorConfigSnapshots'
  • git diff --check

Remaining gaps

  • runtime mirror clone/inject path and mirror drop counters are not implemented in this foundation
  • no TUN-based mirror sink is added; cross-binding inject remains the required runtime path

Copilot AI review requested due to automatic review settings May 17, 2026 01:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds the initial Go-side “port mirroring” snapshot representation for the userspace dataplane, deriving per-ingress mirror mappings from forwarding-options port-mirroring while keeping capability admission fail-closed until runtime clone/inject support exists.

Changes:

  • Extend the userspace control-plane snapshot protocol with mirror_configs entries (ingress ifindex, output ifindex, sampling rate).
  • Build mirror config snapshots from config + resolved interface ifindexes, rejecting duplicate ingress ifindexes and skipping unresolved outputs.
  • Add Go tests for JSON wire stability and mirror snapshot building; keep the userspace capability gate rejecting port mirroring.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
pkg/dataplane/userspace/snapshot.go Adds mirror snapshot derivation and wires it into buildSnapshot().
pkg/dataplane/userspace/protocol.go Adds MirrorConfigs to ConfigSnapshot and defines MirrorConfigSnapshot.
pkg/dataplane/userspace/protocol_test.go Adds JSON round-trip / wire-key pin test for mirror_configs.
pkg/dataplane/userspace/manager_test.go Adds tests for fail-closed capability gating and mirror snapshot builder behavior.

Comment on lines +145 to +149
out = append(out, MirrorConfigSnapshot{
IngressIfindex: ingressIfindex,
OutputIfindex: outputIfindex,
Rate: uint32(inst.InputRate),
})
Comment thread pkg/dataplane/userspace/snapshot.go Outdated
Comment on lines +78 to +79
func mustBuildMirrorConfigSnapshots(cfg *config.Config, interfaces []InterfaceSnapshot) []MirrorConfigSnapshot {
mirrors, err := buildMirrorConfigSnapshots(cfg, interfaces)

func TestConfigSnapshotMirrorConfigsRoundTrip(t *testing.T) {
in := ConfigSnapshot{
Version: 1,
@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Claude round-1 review on ac0a04e0

Verdict: MERGE-READY (foundation only — Go-side scaffolding for #1376)

Snapshot + admission plumbing only. No Rust dataplane impl. The userspace capability gate for port mirroring remains closed.

What's in scope

4 files +236/-0:

  • pkg/dataplane/userspace/protocol.go — wire-format additions for port-mirror snapshot
  • pkg/dataplane/userspace/snapshot.go — Go-side builder
  • pkg/dataplane/userspace/protocol_test.go + manager_test.go — round-trip tests

Hostile checks

Recommendation

Block on: nothing structural for the foundation slice.

Strongly consider: verify missing-instance/interface fail-closed pattern (Codex will).

Defer to follow-up PRs: Rust mirror egress path, buffer-doubling UMEM accounting, backpressure handling, failover semantics (no clone-state sync).

Awaiting Codex (task-mp9559w2-zumig4).

@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Round-1 review consolidated on ac0a04e0

Reviewer Verdict
Claude MERGE-READY (foundation)
Codex MERGE-NEEDS-MINOR
Copilot (no review yet on this HEAD)

Codex MINOR — Test coverage thinner than scope

"protocol_test.go:63 only proves Go self round-trip for mirror_configs. Current Rust ConfigSnapshot has no mirror_configs field and no deny_unknown_fields, so older/current xpfd should ignore the added JSON field, but this is not pinned by a Rust compatibility test."

And:

"manager_test.go:1903 tests buildMirrorConfigSnapshots directly, not the real buildSnapshot integration point."

Same "production publish path coverage" gap pattern as #1396. The mirror_configs field is built by the helper but not necessarily wired into the publishing path. A regression that stops wiring MirrorConfigs into the published snapshot wouldn't be caught.

What Codex verified clean

  • Capability gate stays closed: manager.go:775 rejects port-mirroring configs ✓
  • Missing-reference fail-closed: missing output/input ifindexes produce warn + skip, duplicate ingress errors. ✓
  • Diff scope: exactly 4 files, +236/-0. ✓

Recommendation

Block on: nothing structural.

Strongly consider: add a Rust-side decode test confirming mirror_configs is tolerated by current ConfigSnapshot (forward-compat), and an integration test through buildSnapshot (not just buildMirrorConfigSnapshots helper).

Defer to follow-up PRs: Rust mirror egress path, UMEM buffer-doubling accounting, backpressure semantics, failover (no clone-state sync), capability gate removal.

Codex task: task-mp9559w2-zumig4. Not merging — author's decision.

@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Round-2 update pushed in ec4e9cb. Closed the review coverage gap with:

  • Rust snapshot DTO + round-trip coverage for mirror_configs in userspace-dp/src/protocol.rs and userspace-dp/src/main_tests.rs
  • Real Go buildSnapshot integration coverage that resolves loopback ifindex and verifies MirrorConfigs is emitted from the full snapshot path

Validation:

  • go test ./pkg/dataplane/userspace -run 'TestConfigSnapshotMirrorConfigsRoundTrip|TestDeriveUserspaceCapabilitiesKeepsPortMirroringFailClosed|TestBuildMirrorConfigSnapshots|TestBuildSnapshotIncludesMirrorConfigsFromRealInterfaceSnapshot'
  • cargo test --manifest-path userspace-dp/Cargo.toml config_snapshot_mirror_configs_roundtrip -- --nocapture
  • git diff --check

@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Claude round-2 review on ec4e9cbd

Verdict: MERGE-READY (foundation slice, test coverage gap closed)

Round-1 MINOR addressed. 3 files, +90/-0:

  • pkg/dataplane/userspace/manager_test.go (+33): real buildSnapshot integration test, not just helper
  • userspace-dp/src/main_tests.rs (+45): Rust round-trip / forward-compat test for mirror_configs field
  • userspace-dp/src/protocol.rs (+12): minor field plumbing for the test

The two specific gaps Codex flagged:

  1. ✓ Rust compat test added (forward-compat — current ConfigSnapshot tolerates mirror_configs)
  2. ✓ Go integration test exercises real buildSnapshot, not just the helper

Capability gate at manager.go:775 should still reject port-mirroring configs per #1376 contract — let Codex verify it's unchanged.

Awaiting Codex (task-mp96mlkx-0s1hx5).

@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Round-2 review consolidated on ec4e9cbd

Reviewer Verdict
Claude MERGE-READY
Codex MERGE-READY

Round-1 MINOR (test coverage gap) closed cleanly. 3 files, +90/-0:

  • Rust round-trip + forward-compat test for mirror_configs
  • Real buildSnapshot integration test ✓
  • Capability gate unchanged at manager.go:775

Codex finding: no new MAJORs.

Recommendation: MERGE-READY. Defer to follow-up PRs: Rust mirror egress path, UMEM buffer-doubling accounting, backpressure semantics, failover, capability gate removal.

Codex task: task-mp96mlkx-0s1hx5. Not merging — author's decision.

Copilot AI review requested due to automatic review settings May 17, 2026 06:31
@psaab psaab force-pushed the codex/1376-userspace-port-mirror-snapshot branch from ec4e9cb to 4586233 Compare May 17, 2026 06:31
@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

Rebased after #1395 merge

Rebased the port-mirror snapshot PR onto current master after the three-color policer snapshot merge.

What changed in the conflict resolution:

  • Kept both snapshot surfaces in the Go DTO/build path:
    • three_color_policers
    • mirror_configs
  • Kept both Rust DTO wire fields and split the Rust round-trip coverage into independent tests.
  • Preserved the existing Go port-mirror snapshot tests and the merged three-color policer snapshot test.

Validation run on 4586233e:

  • go test ./pkg/dataplane/userspace
  • cargo test --manifest-path userspace-dp/Cargo.toml config_snapshot

Both passed. Cargo still emits the existing warning set.

@psaab
Copy link
Copy Markdown
Owner Author

psaab commented May 17, 2026

@copilot review\n\nPlease re-review current HEAD after the latest fix-forward/rebase push.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comment thread pkg/dataplane/userspace/snapshot.go Outdated
Comment on lines +79 to +89
func mustBuildMirrorConfigSnapshots(cfg *config.Config, interfaces []InterfaceSnapshot) []MirrorConfigSnapshot {
mirrors, err := buildMirrorConfigSnapshots(cfg, interfaces)
if err != nil {
// The mirror table contract is one output per ingress ifindex. Keep
// snapshot publication fail-closed by omitting an ambiguous mirror table
// if this helper is called on an invalid config.
slog.Warn("userspace snapshot: invalid port-mirroring config skipped", "err", err)
return nil
}
return mirrors
}
Comment on lines +146 to +151
out = append(out, MirrorConfigSnapshot{
IngressIfindex: ingressIfindex,
OutputIfindex: outputIfindex,
Rate: uint32(inst.InputRate),
})
}
Copilot AI and others added 2 commits May 17, 2026 06:36
Copilot AI review requested due to automatic review settings May 17, 2026 06:37
@psaab psaab review requested due to automatic review settings May 17, 2026 06:37
Copilot AI review requested due to automatic review settings May 17, 2026 06:38
@psaab psaab review requested due to automatic review settings May 17, 2026 06:38
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.

3 participants