Fix: Select iptables backend by kernel module, fail loud on misprogram

[huang195]

Problem

detect_iptables_cmd() in proxy-init/init-iptables.sh prefers
iptables-legacy whenever the legacy nat table is merely listable
(iptables-legacy -t nat -L -n). On nft-only nodes (OpenShift/ROSA, EKS-nft)
the empty legacy table still lists fine, so legacy gets chosen and our chains
land in a backend the live datapath never consults — a silent fail-open for
a control whose entire job is to enforce egress. A write-probe is no better:
individual legacy ops succeed via nft_compat even where the full nat pipeline
later fails, so probing legacy can't distinguish "works" from "works until the
real pipeline."

Fixes the detection half of #502.

Change

Detect the functional backend via /proc/modules instead of a readability
probe:

/proc/modules	backend	platforms
iptable_nat present	iptables-legacy	Kind, kubeadm (kube-proxy uses it)
absent	iptables (nft)	OpenShift/ROSA, EKS-nft

/proc/modules is host-kernel-wide, readable from the pod netns
(verified from a pod running proxy-init), timing-independent (unlike
matching Istio's not-yet-installed rules), and not foolable by nft_compat
(unlike a write-probe). IPTABLES_CMD still overrides — the operator can set it
per-platform. PROC_MODULES is overridable for tests.

Fail loud, never silent — new require_jump() helper: after each chain is
hooked into OUTPUT/PREROUTING (both redirect and enforce-redirect
modes), assert the jump actually landed in the chosen backend and exit 1 with
an actionable message otherwise. set -e already aborts on hard errors; this
turns the subtler "command returned 0 but the rule isn't there" case from a
silent bypass into a visible crash.

Not a ROSA enablement (important)

This does not make egress enforcement work on ROSA HCP and does nothing
about SELinux. On ROSA it changes the failure: proxy-init now correctly
selects nft and fails loud on the real container_t → nf_tables SELinux
denial (Could not fetch rule set generation id: Permission denied), instead of
silently mis-selecting legacy. Making nft actually program on ROSA
(SELinux/SCC seLinuxOptions, or node-level/ambient redirection) is a separate
track tracked in #502.

Testing

• sh -n / bash -n clean; detection function exercised against fixture module
  tables (nft → iptables, override → iptables-legacy).
• test-enforce-redirect.sh extended with a /proc/modules-seam detection unit
  test (no real kernel needed); existing nft structural/capture/drop assertions
  unchanged.
• Kind (iptable_nat loaded) selects iptables-legacy as before — no
  regression for the local/dev path.

Reviewer note

The module check picks legacy whenever iptable_nat is loaded. The rare edge —
a node with iptable_nat loaded but Istio/kube-proxy actually on nft — would
split backends; IPTABLES_CMD covers it, and it doesn't occur on Kind (both
legacy) or nft-only nodes (iptable_nat absent).

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

Summary by CodeRabbit

• Bug Fixes

  • Improved proxy initialization reliability by enhancing firewall backend detection and adding validation to ensure that firewall rules are properly applied, preventing silent failures in rule installation.

• Tests

  • Added unit tests for firewall backend detection logic across different system configurations.

[coderabbitai]

Warning

Review limit reached

@huang195, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 44 minutes and 33 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9d5126f3-3f61-4bb6-ba38-f5b83b2c9c89

📥 Commits

Reviewing files that changed from the base of the PR and between c609254 and d6e1f91.

📒 Files selected for processing (1)

• authbridge/proxy-init/init-iptables.sh

📝 Walkthrough

Walkthrough

init-iptables.sh replaces the iptables-legacy readability probe in detect_iptables_cmd() with a /proc/modules check for iptable_nat. A new require_jump() helper hard-verifies inserted jump rules via iptables -C, replacing four prior idempotency checks. Docs and tests are updated to match.

Changes

iptables Backend Detection and Rule Verification

Layer / File(s)	Summary
Backend detection logic and docs authbridge/proxy-init/Dockerfile.init, authbridge/proxy-init/init-iptables.sh	detect_iptables_cmd() now reads PROC_MODULES (default /proc/modules) to check for iptable_nat instead of probing iptables-legacy; Dockerfile comment and inline script docs updated to describe the new /proc/modules-based logic.
require_jump() helper and call sites authbridge/proxy-init/init-iptables.sh	New require_jump() function runs iptables -C after insertion and exits with error details on failure. Four -C/-I idempotency patterns in setup_enforce_redirect() and the PROXY_OUTPUT/PROXY_INBOUND jump insertions are replaced with require_jump calls.
Backend detection unit tests authbridge/proxy-init/test-enforce-redirect.sh	New test block evals extracted detection code against legacy and nft /proc/modules fixture files, conditionally checks iptables-legacy output, always validates the nft case, and verifies IPTABLES_CMD override.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Bug: init-iptables.sh picks wrong backend on nft-only nodes (readability probe ≠ liveness) #502: This PR directly implements the fix described in the issue — replacing the unreliable iptables-legacy -t nat -L probe with /proc/modules-based detection and adding require_jump() to prevent silent rule insertion failures on nft-only nodes.

Possibly related PRs

• kagenti/kagenti-extensions#484: Modifies the same init-iptables.sh backend detection and jump-rule enforcement mechanisms that this PR updates, introducing MODE=enforce-drop chains that depend on the correct backend being selected.

Suggested reviewers

• mrsabath

Poem

🐇 Hoppity-hop through the kernel's domain,
No more probing legacy in vain!
/proc/modules tells the tale —
iptable_nat tips the scale.
require_jump won't let rules slip by,
This rabbit checks before saying goodbye! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly summarizes the main change: fixing iptables backend selection by kernel module detection and adding validation to fail loudly on misconfiguration, which aligns with the primary objectives.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@authbridge/proxy-init/init-iptables.sh`:
- Around line 121-125: The debugging tip comment that states both components use
iptables-legacy contradicts the newly documented backend auto-detection
behavior. Update the outdated comment to reflect that the script now
intelligently auto-detects and supports both the legacy nat backend (using
iptable_nat kernel module on legacy clusters like Kind and kubeadm) and the
nft-only backend (on platforms like OpenShift/ROSA), as described in the
adjacent documentation. This ensures the debugging guidance aligns with the
actual auto-detection capability explained in the kernel module detection logic.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c1ff3af-61b3-40dd-8c0c-0f682cf80abf

📥 Commits

Reviewing files that changed from the base of the PR and between 1cd2a12 and c609254.

📒 Files selected for processing (3)

• authbridge/proxy-init/Dockerfile.init
• authbridge/proxy-init/init-iptables.sh
• authbridge/proxy-init/test-enforce-redirect.sh

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align the debugging tip with backend auto-detection behavior.

Line 109 still says both components use iptables-legacy, but this file now supports nft backend selection. That contradiction can mislead runtime triage on nft-only clusters.

📝 Proposed doc fix

-#   iptables-legacy-save       — both Istio and AuthProxy use iptables-legacy
+#   iptables-save / iptables-legacy-save
+#                               — inspect whichever backend was selected at startup

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@authbridge/proxy-init/init-iptables.sh` around lines 121 - 125, The debugging
tip comment that states both components use iptables-legacy contradicts the
newly documented backend auto-detection behavior. Update the outdated comment to
reflect that the script now intelligently auto-detects and supports both the
legacy nat backend (using iptable_nat kernel module on legacy clusters like Kind
and kubeadm) and the nft-only backend (on platforms like OpenShift/ROSA), as
described in the adjacent documentation. This ensures the debugging guidance
aligns with the actual auto-detection capability explained in the kernel module
detection logic.

[akram]

Looks good — the /proc/modules approach is the right call. I independently hit this exact problem on ROSA HCP and tried three increasingly aggressive write-probes (list, chain creation, REDIRECT rule creation) — all passed via nft_compat even though the full nat pipeline fails at runtime. /proc/modules is the only reliable signal.

require_jump() is a great addition — we had silent fail-open on our cluster for hours before realizing legacy was selected but non-functional.

This pairs well with the operator-side egressEnforcement: none opt-out in kagenti-operator#436 — on ROSA HCP where neither backend works (SELinux blocks nf_tables too), the admin sets egressEnforcement: none and proxy-init is never injected. If it is injected by mistake, this PR ensures it crashes visibly instead of silently bypassing.

One minor note: the PR body says "does nothing about SELinux" — worth linking to the Red Hat KB 5990311 and kagenti-extensions#502 for the full picture, since someone debugging the nft Permission denied on ROSA will land here first.

LGTM.

[mrsabath]

Solid, well-reasoned fix. The /proc/modules signal is the right call — host-kernel-wide, timing-independent, and not foolable by nft_compat the way a write-probe is. require_jump is cleanly wired into every insert site: the -C args match each -I exactly, across both redirect and enforce-redirect paths and IPv4+IPv6. The comments are genuinely excellent.

One real defect holds it back: the new detection unit test is neutralized by the harness's own inherited IPTABLES_CMD, so it doesn't actually validate detection — see the inline comment. It's green in CI only because no workflow runs this harness (CI runs sh -n/shellcheck + the proxy-init Docker build, not the runtime test). 一字之差 (a single var makes all the difference) — clear the inherited override in the detection subshells and the test will exercise what it's meant to.

Areas reviewed: Shell (init-iptables.sh, test harness), Dockerfile, commit/PR conventions, CI wiring, security.
Commits: 2, both signed-off (DCO passing).
CI: all checks green — but note the runtime test harness is not part of CI.

[mrsabath]

must-fix — this detection unit test is short-circuited by the harness's own IPTABLES_CMD, so it doesn't test what it claims to.

The re-exec at L39–40 injects IPTABLES_CMD="${IPT}" (= iptables-nft) into the environment for the entire netns run, and detect_iptables_cmd checks IPTABLES_CMD first and returns it immediately. So:

• L172 (mods_legacy) returns iptables-nft, expected iptables-legacy → FAIL
• L178 (mods_nft) returns iptables-nft, expected iptables → FAIL

Reproduced by extracting the function and exporting IPTABLES_CMD=iptables-nft: both cases return iptables-nft. This stays green in CI only because no workflow invokes this harness (CI = sh -n/shellcheck + the proxy-init Docker build). The override case (L181) passes for the wrong reason — it sets its own override, so it'd pass even if detection were completely broken.

Fix: clear the inherited var for the non-override subshells, e.g. wrap the block with env -u IPTABLES_CMD or unset IPTABLES_CMD before L172/L178 (the L181 override case re-sets it explicitly, so it's unaffected).

[mrsabath]

nit — the parenthetical “(e.g. rules written into a backend that is not the live datapath)” slightly overstates what require_jump catches. The -C check runs against the chosen backend (${_rj_cmd}), so if detection mis-selects a backend, the rule is both written to and found in that same backend and require_jump passes — it can't detect mis-selection. What it genuinely guards is the “insert returned 0 but the rule didn't persist” case. Consider trimming the e.g. so the comment doesn't read as datapath-vs-backend validation. Mechanism is sound and strictly additive — no functional concern.