NE-2715: Update keepalived-ipfailover to replace iptables with nftables#235
NE-2715: Update keepalived-ipfailover to replace iptables with nftables#235Thealisyed wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@Thealisyed: This pull request references NE-2715 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR migrates the keepalived container from using iptables wrapper scripts to nftables for managing multicast firewall rules. The nftables package is installed, deprecated wrapper scripts are removed, configuration documentation is updated, and core failover setup/teardown logic switches to nftables table and rule management. Changesiptables to nftables migration for keepalived multicast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@README.md`:
- Line 14: The README.md contains a testing artifact line "//For my testing
purpose" that should not be published; remove that exact line from README.md (or
replace it with appropriate documentation text) so the README only contains
intended documentation content.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 522cf7a2-4f21-4b3a-966a-31dc5b172233
📒 Files selected for processing (10)
README.mdipfailover/keepalived/Dockerfileipfailover/keepalived/conf/settings.shipfailover/keepalived/iptables-scripts/ip6tablesipfailover/keepalived/iptables-scripts/ip6tables-restoreipfailover/keepalived/iptables-scripts/ip6tables-saveipfailover/keepalived/iptables-scripts/iptablesipfailover/keepalived/iptables-scripts/iptables-restoreipfailover/keepalived/iptables-scripts/iptables-saveipfailover/keepalived/lib/failover-functions.sh
💤 Files with no reviewable changes (6)
- ipfailover/keepalived/iptables-scripts/iptables
- ipfailover/keepalived/iptables-scripts/iptables-restore
- ipfailover/keepalived/iptables-scripts/ip6tables-restore
- ipfailover/keepalived/iptables-scripts/iptables-save
- ipfailover/keepalived/iptables-scripts/ip6tables
- ipfailover/keepalived/iptables-scripts/ip6tables-save
iptables is deprecated in RHEL 9 and the ip_tables kernel module is removed in RHEL 10. This replaces all iptables usage in the ipfailover component with native nft commands. Changes: - Remove iptables-scripts/ chroot wrappers (no longer needed since nft runs directly in the container) - Install nftables package in Dockerfile - Replace iptables calls in failover-functions.sh with nft equivalents that create a dedicated "inet keepalived" table for the multicast accept rule (224.0.0.18) - Preserve OPENSHIFT_HA_IPTABLES_CHAIN env var for backward compat Assisted with Claude
Thealisyed
force-pushed
the
NE-2715-ipfailover-nftables
branch
from
b3b419e to
bd18a29
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Thealisyed The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@coderabbitai can you do a quick review is all good? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== README.md diff area ==="
cat README.md | head -30
echo ""
echo "=== Dockerfile ==="
cat ipfailover/keepalived/Dockerfile
echo ""
echo "=== failover-functions.sh ==="
cat ipfailover/keepalived/lib/failover-functions.sh
echo ""
echo "=== settings.sh ==="
cat ipfailover/keepalived/conf/settings.sh
echo ""
echo "=== iptables-scripts dir (should be gone or empty) ==="
ls ipfailover/keepalived/iptables-scripts/ 2>/dev/null || echo "Directory removed or empty"Length of output: 8651 🏁 Script executed: #!/bin/bash
echo "=== Full README.md ==="
cat README.md
echo ""
echo "=== Check nft heredoc indentation (tabs vs spaces) ==="
cat -A ipfailover/keepalived/lib/failover-functions.sh | grep -A5 'NFT'
echo ""
echo "=== Check if nftables listed twice in Dockerfile ==="
grep -c 'nftables' ipfailover/keepalived/Dockerfile
echo ""
echo "=== Check any remaining references to iptables in shell scripts ==="
rg -n 'iptables' ipfailover/keepalived/ --type shLength of output: 1675
✅ What looks good
|
There was a problem hiding this comment.
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 `@ipfailover/keepalived/lib/failover-functions.sh`:
- Around line 62-75: The heredoc invocation of nft (-f -) inside the
HA_IPTABLES_CHAIN block may fail silently; update the block in
failover-functions.sh to capture the exit status of the nft -f - operation
(after invoking nft with the provided heredoc for table/chain creation), and if
it fails (non-zero exit code) log a clear error including the attempted rule
("ip daddr 224.0.0.18 accept" / "nft -f -") via echo or process logger and exit
non-zero (or return failure) so setup does not continue without the multicast
accept rule; also consider checking nft availability before attempting the
heredoc (e.g., via command -v nft) to produce a more specific error.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7499d2f7-b512-4081-bb8b-b9f59783b1d8
📒 Files selected for processing (9)
ipfailover/keepalived/Dockerfileipfailover/keepalived/conf/settings.shipfailover/keepalived/iptables-scripts/ip6tablesipfailover/keepalived/iptables-scripts/ip6tables-restoreipfailover/keepalived/iptables-scripts/ip6tables-saveipfailover/keepalived/iptables-scripts/iptablesipfailover/keepalived/iptables-scripts/iptables-restoreipfailover/keepalived/iptables-scripts/iptables-saveipfailover/keepalived/lib/failover-functions.sh
💤 Files with no reviewable changes (6)
- ipfailover/keepalived/iptables-scripts/ip6tables-save
- ipfailover/keepalived/iptables-scripts/ip6tables-restore
- ipfailover/keepalived/iptables-scripts/iptables-restore
- ipfailover/keepalived/iptables-scripts/iptables-save
- ipfailover/keepalived/iptables-scripts/iptables
- ipfailover/keepalived/iptables-scripts/ip6tables
✅ Files skipped from review due to trivial changes (1)
- ipfailover/keepalived/conf/settings.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ipfailover/keepalived/lib/failover-functions.sh (1)
62-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake nft setup idempotent for partial state and fail fast when nft apply fails.
If
inet keepalivedalready exists but is incomplete,nft -f -can fail on table redefinition and setup still proceeds without guaranteeingip daddr 224.0.0.18 accept.Suggested fix
if [[ -n "${HA_IPTABLES_CHAIN:-}" ]]; then echo " - Ensuring nft rule for keepalived multicast (224.0.0.18) ..." - if ! nft list chain inet keepalived filter 2>/dev/null | grep -q 'ip daddr 224.0.0.18 accept' ; then - echo " - Adding nft rule to accept multicast 224.0.0.18." - nft -f - <<-'NFT' - table inet keepalived { - chain filter { - type filter hook input priority 0; policy accept; - ip daddr 224.0.0.18 accept - } - } - NFT - fi + nft list table inet keepalived >/dev/null 2>&1 || \ + nft add table inet keepalived || { echo "ERROR: failed to create nft table inet keepalived"; return 1; } + + nft list chain inet keepalived filter >/dev/null 2>&1 || \ + nft add chain inet keepalived filter '{ type filter hook input priority 0; policy accept; }' || { echo "ERROR: failed to create nft chain inet keepalived filter"; return 1; } + + if ! nft list chain inet keepalived filter 2>/dev/null | grep -q 'ip daddr 224.0.0.18 accept' ; then + echo " - Adding nft rule to accept multicast 224.0.0.18." + nft add rule inet keepalived filter ip daddr 224.0.0.18 accept || { echo "ERROR: failed to add nft rule ip daddr 224.0.0.18 accept"; return 1; } + fi fi#!/bin/bash set -euo pipefail file="$(fd --full-path 'ipfailover/keepalived/lib/failover-functions.sh$' | head -n1)" python - <<'PY' import re, pathlib p = pathlib.Path("ipfailover/keepalived/lib/failover-functions.sh") t = p.read_text() m = re.search(r'if \[\[ -n "\$\{HA_IPTABLES_CHAIN:-\}" \]\]; then(.*?)\n\s*fi', t, re.S) block = m.group(1) if m else "" print("has_heredoc_apply:", "nft -f -" in block) print("has_table_guard:", "nft list table inet keepalived" in block) print("has_failfast_return:", "return 1" in block) PYExpected confirmation for current code: heredoc apply is present, while explicit table/chain guards and fail-fast return in this block are absent.
🤖 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 `@ipfailover/keepalived/lib/failover-functions.sh` around lines 62 - 74, The current HA_IPTABLES_CHAIN block uses a heredoc with "nft -f -" without guarding for partial existing state and doesn't fail fast; update the block to first probe the existing state with "nft list table inet keepalived" and/or "nft list chain inet keepalived filter" and only run the heredoc to create missing pieces (or run targeted "nft add table"/"nft add chain"/"nft add rule" commands) when those probes show resources are absent, and capture the exit status of the nft apply (the heredoc or the targeted commands) and immediately return 1 (or exit non-zero) on failure so the caller sees the error; keep these checks and the fail-fast return inside the same HA_IPTABLES_CHAIN conditional so the behavior is idempotent and errors are propagated.
🤖 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.
Duplicate comments:
In `@ipfailover/keepalived/lib/failover-functions.sh`:
- Around line 62-74: The current HA_IPTABLES_CHAIN block uses a heredoc with
"nft -f -" without guarding for partial existing state and doesn't fail fast;
update the block to first probe the existing state with "nft list table inet
keepalived" and/or "nft list chain inet keepalived filter" and only run the
heredoc to create missing pieces (or run targeted "nft add table"/"nft add
chain"/"nft add rule" commands) when those probes show resources are absent, and
capture the exit status of the nft apply (the heredoc or the targeted commands)
and immediately return 1 (or exit non-zero) on failure so the caller sees the
error; keep these checks and the fail-fast return inside the same
HA_IPTABLES_CHAIN conditional so the behavior is idempotent and errors are
propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33de2090-fe7b-49e1-a8ea-9918ab0cf1d6
📒 Files selected for processing (9)
ipfailover/keepalived/Dockerfileipfailover/keepalived/conf/settings.shipfailover/keepalived/iptables-scripts/ip6tablesipfailover/keepalived/iptables-scripts/ip6tables-restoreipfailover/keepalived/iptables-scripts/ip6tables-saveipfailover/keepalived/iptables-scripts/iptablesipfailover/keepalived/iptables-scripts/iptables-restoreipfailover/keepalived/iptables-scripts/iptables-saveipfailover/keepalived/lib/failover-functions.sh
💤 Files with no reviewable changes (6)
- ipfailover/keepalived/iptables-scripts/iptables-save
- ipfailover/keepalived/iptables-scripts/ip6tables
- ipfailover/keepalived/iptables-scripts/ip6tables-restore
- ipfailover/keepalived/iptables-scripts/iptables-restore
- ipfailover/keepalived/iptables-scripts/ip6tables-save
- ipfailover/keepalived/iptables-scripts/iptables
|
/retest |
|
@Thealisyed: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
iptables-scripts/chroot wrappers (no longer needed since nft runs directly in the container)nftablespackage in container imageinet keepalivednft table for the VRRP multicast accept rule (224.0.0.18)OPENSHIFT_HA_IPTABLES_CHAINenv var for backward compatibilityWhy
iptables is deprecated in RHEL 9 and the
ip_tableskernel module is removed in RHEL 10. Theiptables-nftuserspace shim remains but is deprecated. This migrates to native nft before the shim is removed entirely.What changed
ipfailover/keepalived/iptables-scripts/*ipfailover/keepalived/Dockerfilenftablespackage, removed COPY lines for wrapper scriptsipfailover/keepalived/conf/settings.shipfailover/keepalived/lib/failover-functions.shSummary by CodeRabbit