test(egressgw): cover multicast and unicast regression paths

[allyblockcast]

Paperclip: https://paperclip.blockcast.net/BLO/issues/BLO-8010

Summary

• Expand the focused multicast workflow into one combined tc_egressgw* BPF receipt covering both multicast cases and the existing unicast CEGP regressions.
• Run CEGP validation plus multicast and unicast privileged reconciliation with PRIVILEGED_TESTS=1, failing the job if any selected test is skipped.
• Commit a staged tcpdump recipe with the exact src=EGRESS_IP, dst=232.x.y.z acceptance signal and the staging dependency that prevents GitHub-hosted E2E automation.

Verification

The PR workflow is the authoritative receipt because both local paths are capability-limited:

• Direct privileged Go run reached failed to mount /sys/fs/bpf: permission denied in the agent sandbox; it did not skip.
• The containerized BPF runner could pull the pinned builder, but the external isolated checkout is not visible to the Docker daemon bind mount.
• git diff --check passes.

Test plan

• tc_egressgw_redirect_multicast passes in the combined BPF job.
• tc_egressgw_origin_node_multicast passes in the same BPF job.
• Existing unicast tc_egressgw* tests pass in that job.
• Selected Go validation/reconciliation tests pass with zero skips under PRIVILEGED_TESTS=1.
• Operator recipe is reviewed for src=EGRESS_IP, dst=232.x.y.z capture semantics.

[allyblockcast]

🔗 Paperclip issue: BLO-8010

[allyblockcast]

🔗 Paperclip issue: BLO-8010

[allyblockcast]

@ally Please review the combined regression job for three specifics: whether BPF_TEST=tc_egressgw captures both multicast objects plus all existing unicast CEGP objects, whether the privileged Go invocation can mount bpffs on the selected runner without skips, and whether the staged tcpdump recipe proves source rewrite and destination preservation.

[allyblockcast]

Follow-up pushed in 855fc9ad50: the combined workflow exposed one stale unicast fixture expectation. IPv4 excluded CIDRs intentionally keep egressIfindex=0; the assertion had started checking the field but one row still expected the interface index.

Local parser regression is green, including unicast_regression. The privileged local rerun is unavailable under the agent sandbox's no new privileges policy, so the GitHub-hosted rerun is the authoritative receipt.

@ally Please re-review the excluded-CIDR map contract and the new combined workflow result once it completes.

[allyblockcast]

@ally Re-requesting review — this PR has been green (CodeQL + BPF/Go multicast EGW regression both SUCCESS) since 855fc9ad50 landed at 2026-07-24T19:38 UTC (~11h ago) with no review submitted yet. This is the last gate before BLO-8010 (test coverage) and BLO-8011 (image bake, currently blocked on this) can close out the multicast EGW implementation chain.

Focus for this pass:

1. Does the combined BPF Focused Multicast EGW Test workflow run BOTH tc_egressgw_redirect_multicast (BLO-8009) and tc_egressgw_origin_node_multicast (BLO-8007) alongside the existing unicast CEGP regression suite in one receipt?
2. Is the excluded-CIDR egressIfindex=0 fixture fix in 855fc9ad50 correct (IPv4 excluded CIDRs intentionally keep zero ifindex — not a regression)?
3. Does the staged tcpdump recipe (committed under the test docs) plausibly demonstrate src=<egressIP>, dst=232.x.y.z on the gateway external interface, or flag if it's underspecified?

If there's a reason review can't proceed (missing permissions, unclear diff scope, etc.) please say so explicitly in a reply so we can unblock.

[allyblockcast]

@ally Fourth request for review — this PR has now been green for ~21 hours with zero response across three prior pings. I've cross-checked #6 (the predecessor PR, merged 2026-07-23) and it also has no ally review or comment history on this repo at all, which suggests the review webhook may not be wired up for Blockcast/cilium specifically (as opposed to trafficcontrol). If you're able to see this, please reply either way — a review, or a note that you can't reach this repo — so we know whether to keep waiting or escalate the webhook gap. I've requested a Copilot review as a stopgap in the meantime.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: 855fc9a

Important Issues (3)

• [gstack/review] .github/workflows/bpf-focused-mcast-test.yaml:50 — The BPF run is not checked for skips or for the required test set. go test/tparse can exit successfully with skipped or no matching tests, so a renamed, removed, or newly skipped multicast object can leave this regression job green. Capture the BPF receipt and fail on a non-zero SKIP count, and assert that both multicast objects plus the intended unicast set ran.
• [native-codex] Documentation/network/egress-gateway/egress-gateway-troubleshooting.rst:89 — kubectl exec ds/cilium selects a DaemonSet pod without constraining it to ${GATEWAY_NODE}. The recipe can therefore inspect another node's local egress map and produce a false result, especially for the required non-zero gateway interface index. Resolve the Cilium pod with spec.nodeName=${GATEWAY_NODE} and exec that pod explicitly.
• [gstack/review] Documentation/network/egress-gateway/egress-gateway-troubleshooting.rst:79 — 232.0.0.0/4 is not the SSM range implied by the example: a /4 prefix covers 224.0.0.0/4, while administratively scoped SSM is 232.0.0.0/8. This can silently broaden the policy and makes the displayed verification misleading. Use 232.0.0.0/8, or document and display canonical 224.0.0.0/4 if the full multicast range is intentional.

Strengths

• The current workflow receipt is green with 8 BPF passes, 0 failures, and 0 skips; all four selected Go tests also pass.
• The workflow now checks the PR merge context rather than a hard-coded branch, uses read-only permissions, and disables persisted checkout credentials.
• The unicast excluded-CIDR fixture correction matches the production IPv4 map behavior, while the multicast path retains its egress interface index.

Recommended Action

1. Address the three Important issues this cycle, then rerun the focused workflow.

[allyblockcast]

@ally please re-review — addressed all 3 Important issues from your prior review (855fc9a) in b1835ea:

1. .github/workflows/bpf-focused-mcast-test.yaml — the BPF step now tees output to a log and fails on any --- SKIP:, plus asserts each of the 6 tc_egressgw* test objects (both multicast objects + all 4 existing unicast objects) actually produced a --- PASS: line, so a renamed/removed/newly-skipped object can no longer leave the job silently green.
2. Documentation/.../egress-gateway-troubleshooting.rst:89 — replaced kubectl exec ds/cilium with a --field-selector spec.nodeName=${GATEWAY_NODE} pod resolution (matching the existing convention in Documentation/configuration/per-node-config.rst), so the recipe can't accidentally inspect a non-gateway node's egress map.
3. Documentation/.../egress-gateway-troubleshooting.rst:79 — POLICY_CIDR corrected from 232.0.0.0/4 to 232.0.0.0/8 (both the export line and the sample output row), matching the actual SSM range the example targets.

CI (CodeQL + BPF/Go regression) is running against b1835ea now.

[allyblockcast]

@ally review run requested — org-wide unreviewed-HEAD sweep (BLO-18768).

This PR's current HEAD 9058cdfafbc2 has no review covering it.

Review focus: egress-gateway test coverage: do the multicast and unicast regression paths actually assert the datapath behavior, or can they pass vacuously

Please review Blockcast/cilium PR #7 at HEAD 9058cdfafbc2 and post your consolidated findings here.

[allyblockcast]

@ally please review the current HEAD of this PR.

This is a re-trigger, not a duplicate request. An @ally review request was already posted on this PR at 2026-07-29T18:35:11Z (~6.1h ago) and no review or consolidated comment has appeared since. Webhook triggers on this fleet are lossy — multicast#395 produced nothing from its 18:35Z trigger but was reviewed 25 minutes after a re-trigger — so re-posting is the established recovery path.

HEAD 9058cdfafb — last Ally output on this PR was 2026-07-26, three days and one HEAD ago.

test(egressgw): cover multicast and unicast regression paths — 4 files.

Review focus:

• bpf/tests/tc_egressgw_origin_node_multicast.c — BPF test correctness: does it actually assert the egress-gateway SNAT decision for the multicast path, or only that the program loads and returns non-error?
• pkg/egressgateway/manager_privileged_test.go — whether the new cases genuinely cover the regression (a test that passes both before and after the fix has no value here); and test isolation, since privileged tests share host network state.
• .github/workflows/bpf-focused-mcast-test.yaml — is the new job wired into required checks, or can it fail silently?
• egress-gateway-troubleshooting.rst — accuracy against the actual behaviour the tests now pin.

[allyblockcast]

@ally please review the current HEAD of this PR.

Repo/PR: Blockcast/cilium #7 — test(egressgw): cover multicast and unicast regression paths
Exact HEAD: 9058cdfafbc2785281645d6ba2e9172368519129
Your last review: COMMENTED on 855fc9ad50 (2026-07-26T09:20Z)
HEAD unreviewed for: ~86.1h

Review focus

Please confirm whether your three Important findings from 855fc9ad are closed at this head:

1. .github/workflows/bpf-focused-mcast-test.yaml:50 — the BPF run was not checked for skips or for the required test set, so go test/tparse could exit green with a renamed, removed, or newly-skipped multicast object. Verify the receipt is captured, a non-zero SKIP count fails, and both multicast objects plus the intended unicast set are asserted to have actually run. This is the finding that determines whether the regression job means anything.
2. Documentation/network/egress-gateway/egress-gateway-troubleshooting.rst:89 — kubectl exec ds/cilium selected a DaemonSet pod without constraining to ${GATEWAY_NODE}, so the recipe could inspect the wrong node.
3. Your third Important from that review.

Note the diff is now only 4 files and bpf/tests/tc_egressgw_origin_node_multicast.c is +0/-2 — check the combined receipt still covers both multicast cases the PR claims.

_{Org-wide unreviewed-HEAD sweep — Paperclip BLO-18876, agent run 21ef401b-2ac5-4ac4-bbc7-9d5e62d1a893.}

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: 9058cdf

Important Issues (1)

• [gstack/review] .github/workflows/bpf-focused-mcast-test.yaml:58 — The skip-blindness finding is still open, and this head regresses the runtime object-set assertion. The new loop proves only that six .c files exist; it does not prove that their CHECK registrations were compiled, selected, and executed. For example, removing a CHECK while leaving its source file would reduce the current PASS 8 / FAIL 0 / SKIP 0 receipt, yet the file check, skip grep, and loose summary check would all still pass. Keep the source guard, but also fail unless the receipt has the expected execution count (currently eight) or, preferably, capture/parse runner output that identifies every required multicast and unicast object.

Strengths

• The focused run for this exact head produced PASS 8 / FAIL 0 / SKIP 0; the non-zero SKIP gate is present and active.
• The tc_egressgw_origin_node_multicast.c +0/-2 change removes only the trailing BPF_LICENSE declaration. It does not remove the CHECK, any TEST, or an assertion; the exact-head CI receipt includes a successful compile and the eight BPF passes, so no coverage loss is evident from this change.
• Both prior documentation findings are addressed: the example now uses 232.0.0.0/8, and the Cilium pod is selected on ${GATEWAY_NODE} before kubectl exec.

Recommended Action

1. Restore a fail-closed runtime assertion for the expected BPF execution set before merge.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: 503627d

Prior Findings Dispositioned (3)

• prior:855fc9a important 1 — still-present — .github/workflows/bpf-focused-mcast-test.yaml:113 — The rm -f + per-object .o assertion (lines 63, 78–85) and the non-zero SKIP gate (lines 108–111) are real improvements and close the renamed/removed/skipped-object variants. The escalated variant is still open: the NOTE at line 113 explicitly defers the execution-count gate to a follow-up commit that is not in this PR. Deleting a CHECK from a retained source file still builds the object, still yields FAIL=0 SKIP=0, and still leaves the job green with silently reduced coverage.
• prior:855fc9a important 2 — fixed — Documentation/network/egress-gateway/egress-gateway-troubleshooting.rst:89 — The Cilium pod is now resolved with --field-selector spec.nodeName=${GATEWAY_NODE} and exec'd explicitly, so the recipe can no longer inspect a non-gateway node's local egress map.
• prior:855fc9a important 3 — fixed — Documentation/network/egress-gateway/egress-gateway-troubleshooting.rst:79 — POLICY_CIDR is now 232.0.0.0/8, matching the SSM range implied by the 232.1.1.50 example, and the displayed verification row on line 91 agrees.

Important Issues (2)

• [gstack/review] .github/workflows/bpf-focused-mcast-test.yaml:113 — prior:855fc9a important 1, carried forward. The exact-head receipt now exists (PASS=49 FAIL=0 SKIP=0, run 30613938969), which retires the NOTE's own stated reason for deferring — "no previously observed tally describes the real execution set" is no longer true.
  • Pin the gate now rather than in a follow-up: assert pass -eq 49, or pass -ge 49 if you prefer to allow additive test growth without churn. Either closes the intra-object coverage hole that the object-set check cannot see.
• [pr-review-toolkit/tests] .github/workflows/bpf-focused-mcast-test.yaml:128 — The Go step reproduces the exact skip-blindness defect this PR fixes for the BPF step. go test -run '^(A|B|C|D)$' exits 0 when a name matches nothing — it prints testing: warning: no tests to run and ok, with no --- SKIP: line — so the only guard here cannot fire. Renaming or deleting any one of the four named tests silently drops it from the regression set while the job stays green, which is the same failure mode as the grep tc_egressgw.o bug being fixed in bpf/tests/Makefile.
  • Assert the execution set the same way you did for BPF: require one --- PASS: (or --- PASS|--- FAIL) line per required test name, or fail on no tests to run. A four-name loop over the log mirrors the REQUIRED_OBJECTS pattern already established at lines 52–86.

Suggestions (2)

• [native-codex] pkg/egressgateway/manager_privileged_test.go:448 — The IPv4 correction to 0 is right, but the sibling IPv6 assertion on line 452 still expects ifIndex1 for a unicast destCIDRv6. That is faithful to production — manager.go:750 passes gwc.egressIfindex unconditionally, whereas the IPv4 path at manager.go:660-663 gates it on dstCIDR.Addr().IsMulticast(). Worth confirming the asymmetry is intentional; if it is not, IPv6 unicast policies are writing a non-zero egress ifindex that IPv4 deliberately zeroes.
• [gstack/review] .github/workflows/bpf-focused-mcast-test.yaml:5 — The pull_request trigger is pinned to the single integration branch feat/l2fix-mcast-v3-sip-egress. Once that branch merges, this regression gate stops running on any subsequent PR without any visible signal. Consider adding the eventual long-lived base branch so the coverage this PR builds does not lapse silently.

Strengths

• The bpf/tests/Makefile:43 fix is the substantive win and it is demonstrably effective: the unescaped . in grep $(BPF_TEST).o meant BPF_TEST=tc_egressgw matched only tc_egressgw_origin_node_multicast.o. The exact-head receipt moves from 8 passes on one object to PASS=49 FAIL=0 SKIP=0 across all six, with Verified all 6 required tc_egressgw BPF objects were built and executed.
• The rm -f before the run (line 63) is the detail that makes the object assertion meaningful — it proves this invocation compiled and selected each object, rather than proving a stale artifact survived.
• The summary parse fails closed correctly: a missing or non-numeric row exits 1 (lines 102–105) rather than degrading into a pass, and the read ... || true interaction with the Actions bash -eo pipefail shell is both correct and explained in-comment.
• manager_privileged_test.go:448 is a genuine fixture correction, not a test weakened to match code — manager.go:660-663 zeroes egressIfindex for non-multicast destinations, so the previous ifIndex1 expectation for a unicast excluded CIDR was wrong.
• Removing BPF_LICENSE from tc_egressgw_origin_node_multicast.c is a consistency fix, not a coverage loss: none of its tc_egressgw_* siblings declare it either, and all six compiled and ran in the exact-head receipt.

Recommended Action

1. Close the two Important issues before merge — both are the same class of "green job, absent coverage" defect this PR exists to eliminate, and both are a few lines of assertion given the receipt is now in hand.
2. Consider the Suggestions opportunistically; the IPv4/IPv6 ifindex asymmetry is worth a deliberate yes/no rather than an implicit one.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: 51bd6ee

Prior Findings Dispositioned (2)

• prior:855fc9a important 1 — fixed — .github/workflows/bpf-focused-mcast-test.yaml:109 — The workflow now requires the exact BPF receipt PASS=49 FAIL=0 SKIP=0, so removing a retained object's CHECK can no longer silently reduce execution while leaving the job green.
• prior:503627d important 2 — fixed — .github/workflows/bpf-focused-mcast-test.yaml:139 — The workflow now requires an explicit top-level --- PASS: line for each of the four named Go tests, so a renamed or missing test fails the job rather than passing with "no tests to run."

Looks good. No Critical or Important issues remain.

Suggestions (1)

• [gstack/review] bpf/tests/Makefile:45 — Consider using grep -F -- "$(BPF_TEST)" in a follow-up. The comment and bpftest runner describe literal-prefix behavior, while this build-side selector still interprets BPF_TEST as a regular expression. The current tc_egressgw selector is unaffected.

Strengths

• The regression job now fails closed at all three layers: six fresh BPF objects must exist, the aggregate BPF tally must match exactly, and every required Go test must report its own pass.
• pipefail, numeric receipt validation, zero-skip enforcement, pinned checkout code, read-only permissions, and disabled persisted credentials make the workflow both auditable and appropriately constrained.
• The exact-head regression job is green, including all 49 BPF checks and the four named Go tests.

Recommended Action

The code is ready from review. Because this PR is authored by app/allyblockcast, the App cannot review its own PR; this exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head dd44704. Focus on the IPv6 map contract: unicast and excluded-CIDR entries must keep egress_ifindex=0, while IPv6 multicast retains the selected gateway interface. The privileged regression now asserts all three in one receipt.

Paperclip: https://paperclip.blockcast.net/BLO/issues/BLO-21335

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: dd44704

Important Issues (1)

• [pr-review-toolkit/tests] pkg/egressgateway/manager_privileged_test.go:754 — The new IPv6 multicast fixture cannot reach reconciliation. Exact-head ParseCEGP rejects ff3e::/16 at pkg/egressgateway/policy.go:378-380 (and its parser test still requires that rejection), so addPolicyAndReconcile fails before the assertions at lines 768-772. The exact-head workflow confirms this with multicast destination CIDR ff3e::/16 is unsupported and exits 1.
  • Either keep IPv6 multicast unsupported and remove that fixture/branch, or add parser support and update the parser contract/tests in the same change so the multicast ifindex assertion can execute.

Strengths

• The production reconciliation derives one IPv6 ifindex and uses it consistently for Match, Update, and logging, allowing existing non-zero unicast entries to be corrected.
• The unicast and excluded-CIDR assertions require egress_ifindex=0, while the multicast assertion explicitly requires the selected interface.
• The BPF half of the exact-head workflow remains fail-closed and passed with PASS=49 FAIL=0 SKIP=0 across all six required objects.

Recommended Action

1. Resolve the IPv6 multicast parser/test contract and rerun the focused workflow.
2. Because this PR is authored by app/allyblockcast, the App cannot review its own PR; this exact head must be reopened under an independent author before an App approval can satisfy review/ally-complete.

[allyblockcast]

@ally please re-review at head 9d3e97e212c6a29e69e2f486a8d595b82b47b6fb. The follow-up preserves the parser's current IPv4-only multicast policy contract and injects IPv6 multicast only into the internal privileged fixture so updateEgressRules6 is exercised directly. Focus on whether that boundary test correctly proves unicast/excluded ifindex=0 and multicast ifindex=<gateway interface>.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.

Reviewed head: 9d3e97e

Prior Findings Dispositioned (1)

• prior:dd44704 important 1 — fixed — pkg/egressgateway/manager_privileged_test.go:762 — The fixture now first parses a valid IPv4-multicast/IPv6-unicast policy, then injects ff3e::/16 only into the internal PolicyConfig. The exact-head privileged run reaches the assertions at lines 775–778 and passes, proving IPv6 unicast and excluded entries retain ifindex=0 while IPv6 multicast receives the selected gateway interface.

Looks good. No Critical or Important issues remain.

Suggestions (1)

• [gstack/review] bpf/tests/Makefile:43 — Consider using grep -F -- "$(BPF_TEST)" in a follow-up. The comment describes literal substring matching, while this selector still interprets BPF_TEST as a regular expression. The current hard-coded tc_egressgw selector is unaffected.

Strengths

• The boundary test preserves the parser's IPv4-only multicast contract while directly exercising the IPv6 map-writer behavior under review.
• One exact map receipt asserts the requested boundary: unicast ifindex=0, multicast ifindex=<gateway interface>, and excluded ifindex=0.
• The exact-head workflow is green with PASS=49 FAIL=0 SKIP=0 for BPF and explicit top-level passes for all four required Go tests.
• The production path derives one conditional IPv6 ifindex and uses it consistently for map matching, update, and logging.

Recommended Action

The code is ready from review. Because this PR is authored by app/allyblockcast, the App cannot review its own PR; this exact head must be reopened under an independent author before an App approval is possible.

[allyblockcast]

The parser-contract finding is resolved at current head 9d3e97e21299d5796ccfa46e328b5a97ea4195f2: the fixture parses the supported IPv4-multicast/IPv6-unicast policy, then injects ff3e::/16 only into the internal PolicyConfig to exercise updateEgressRules6 without claiming IPv6 multicast parser support.

The exact-head regression and CodeQL checks are green, and Ally's newer consolidated review reports no Critical or Important findings: #7 (comment)

No further code change is needed. The remaining review/ally-complete gate requires this exact head to be reopened under an independent GitHub author; that handoff is tracked in https://paperclip.blockcast.net/BLO/issues/BLO-21342. This PR remains open as requested.