NE-2716: Replace iptables with nftables in origin-egress-router#242
NE-2716: Replace iptables with nftables in origin-egress-router#242Thealisyed wants to merge 1 commit into
Conversation
|
@Thealisyed: This pull request references NE-2716 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe egress router switches from iptables to nftables. The Dockerfile installs ChangesEgress router nftables migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Thealisyed
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Thealisyed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@egress/router/egress-router.sh`:
- Around line 112-126: The nft reload in setup_nft is not idempotent or atomic
because it recreates table ip nat and then applies cleanup and generated rules
in separate steps, which can fail on restart or leave a partial NAT state.
Update setup_nft in egress-router.sh to load the table, cleanup actions, chain
declarations, and gen_nft_rules output together through a single nft -f -
transaction so the whole reload succeeds or fails as one unit.
🪄 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: 2ab70d76-28db-4323-b945-086f20c71a0c
📒 Files selected for processing (3)
egress/router/Dockerfileegress/router/egress-router.shegress/router/egress_router_test.go
Migrate the egress router from the deprecated iptables-nft shim to native nftables. The rule generation and setup functions are renamed and updated to emit nft syntax, with the nat table and chains created atomically via a heredoc. Unit test expected outputs updated accordingly. Assisted with Claude
Thealisyed
force-pushed
the
NE-2716-egress-router-nftables
branch
from
b4c8e8e to
45f2641
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
egress/router/egress-router.sh (1)
112-126: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the nft reload restart-safe before declaring the table.
Line 115 recreates
table ip natbefore Line 123 flushes it, so a container restart can fail before cleanup if the table/chains already exist. Prefer an idempotent delete/destroy before recreating the table and loading generated rules in the samenft -f -batch.Potential direction, if supported by the target nftables version
function setup_nft() { { cat <<-'NFT' + destroy table ip nat table ip nat { chain prerouting { type nat hook prerouting priority dstnat; policy accept; } @@ - } - flush table ip nat + } NFT gen_nft_rules } | nft -f - }Please verify the RHEL 9 image’s nftables supports
destroy table; otherwise use a guarded delete before the batch.🤖 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 `@egress/router/egress-router.sh` around lines 112 - 126, The nft reload in setup_nft is not restart-safe because it declares table ip nat before cleaning up any existing state. Update the nft -f - batch to remove or destroy the existing nat table first, then recreate it and load gen_nft_rules, keeping the whole flow idempotent. Use setup_nft as the locator and prefer destroy table if supported by the target nftables version; otherwise use a guarded delete before the new table declaration.
🤖 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 `@egress/router/egress-router.sh`:
- Around line 112-126: The nft reload in setup_nft is not restart-safe because
it declares table ip nat before cleaning up any existing state. Update the nft
-f - batch to remove or destroy the existing nat table first, then recreate it
and load gen_nft_rules, keeping the whole flow idempotent. Use setup_nft as the
locator and prefer destroy table if supported by the target nftables version;
otherwise use a guarded delete before the new table declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee8ef3d2-ec93-4217-a948-2b996e6e1fea
📒 Files selected for processing (3)
egress/router/Dockerfileegress/router/egress-router.shegress/router/egress_router_test.go
✅ Files skipped from review due to trivial changes (1)
- egress/router/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- egress/router/egress_router_test.go
|
@Thealisyed: The following tests failed, say
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-nftshim with the nativenftablespackage in the egress router container imagegen_iptables_rules/setup_iptablestogen_nft_rules/setup_nftnft -f -, loading table creation, flush, and all generated rules in a single transactionWhy
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.Reference
nft rule syntax derived from the nftables wiki — Moving from iptables to nftables.
Summary by CodeRabbit
New Features
Bug Fixes