CORENET-7125: iptables to nftables#3038
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bpickard22: This pull request references CORENET-7125 which is a valid jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR migrates ovnkube-node Geneve and Azure ICMP handling from iptables to nftables, updates the related DaemonSet mounts, and extends kube-proxy configuration generation and tests for nftables and node-port address parsing. Changesovn-kubernetes nftables migration
Estimated code review effort: 4 (Complex) | ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bpickard22 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 529-538: The nftables setup in the ovn_notrack bootstrap block is
not idempotent, so a restart can fail when the table already exists. Update the
script section that creates the ovn_notrack table and its prerouting/output
chains to safely replace or remove the existing table first, then recreate it
before adding the GenevePort and OVNHybridOverlayVXLANPort notrack rules.
In `@bindata/network/ovn-kubernetes/managed/ovnkube-node.yaml`:
- Around line 600-618: Make the Azure nftables setup restart-safe in the
ovnkube-node init script by handling existing host-level state gracefully. In
the nft programming block and the generated add_nft_icmp.sh helper, update the
logic around nft add table inet azure_icmp and nft add element inet azure_icmp
icmp_sources so repeated starts or duplicate IP observations do not return
non-zero. Reuse the existing add_nft_icmp.sh and azure_icmp setup symbols, and
prefer idempotent checks or ignoring “already exists” cases so the oc observe
flow keeps running after container restarts.
- Around line 618-622: The steady-state rule path in the ovnkube-node setup
should not emit packet or host-network diagnostics; update the nft rule in the
azure_icmp section to use a non-logging drop action instead of counter log drop,
and remove the unconditional ip addr show, ip route show, and nft list table
commands. If diagnostics are still needed, gate them behind an explicit debug
flag in the same startup path so the default behavior stays quiet.
In `@bindata/network/ovn-kubernetes/self-hosted/ovnkube-node.yaml`:
- Around line 606-624: Make the OVN node ICMP nftables setup restart-safe in the
add_nft_icmp.sh bootstrap block: the existing `nft add table inet azure_icmp`
and related setup should tolerate a pre-existing `azure_icmp` table/objects on
container restart, and the helper should not exit non-zero when the same host IP
is observed more than once. Update the script to use idempotent nft commands or
explicit existence checks around the `azure_icmp` table/set/chain creation and
make the `nft add element inet azure_icmp icmp_sources` path ignore duplicate
elements so `oc observe` continues running.
- Around line 624-628: The steady-state rule path in ovnkube-node is emitting
packet logs and host-network diagnostics; update the nft rule to use counter
drop instead of counter log drop, and remove the unconditional ip addr show, ip
route show, and nft list table commands. Keep any detailed network inspection
behind a debug-only path so the normal execution path remains quiet and does not
expose node network details.
🪄 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: 11c5345f-60a6-4374-a906-2b29caaff474
📒 Files selected for processing (4)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/managed/ovnkube-node.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-node.yamlpkg/util/k8s/kubeproxy.go
| touch /var/run/ovn/add_nft_icmp.sh | ||
| chmod 0755 /var/run/ovn/add_nft_icmp.sh | ||
| cat <<'EOF' > /var/run/ovn/add_nft_icmp.sh | ||
| #!/bin/sh | ||
| if [ -z "$3" ] | ||
| then | ||
| echo "Called with host address missing, ignore" | ||
| exit 0 | ||
| fi | ||
| echo "Adding ICMP drop rule for '$3' " | ||
| if iptables -C CHECK_ICMP_SOURCE -p icmp -s $3 -j ICMP_ACTION | ||
| then | ||
| echo "iptables already set for $3" | ||
| else | ||
| iptables -A CHECK_ICMP_SOURCE -p icmp -s $3 -j ICMP_ACTION | ||
| fi | ||
| echo "Adding ICMP drop rule for '$3'" | ||
| nft add element inet azure_icmp icmp_sources "{ $3 }" | ||
| EOF | ||
|
|
||
| echo "I$(date "+%m%d %H:%M:%S.%N") - drop-icmp - start drop-icmp ${K8S_NODE}" | ||
| iptables -X CHECK_ICMP_SOURCE || true | ||
| iptables -N CHECK_ICMP_SOURCE || true | ||
| iptables -F CHECK_ICMP_SOURCE | ||
| iptables -D INPUT -p icmp --icmp-type fragmentation-needed -j CHECK_ICMP_SOURCE || true | ||
| iptables -I INPUT -p icmp --icmp-type fragmentation-needed -j CHECK_ICMP_SOURCE | ||
| iptables -N ICMP_ACTION || true | ||
| iptables -F ICMP_ACTION | ||
| iptables -A ICMP_ACTION -j LOG | ||
| iptables -A ICMP_ACTION -j DROP | ||
| nft add table inet azure_icmp | ||
| nft flush table inet azure_icmp | ||
| nft 'add set inet azure_icmp icmp_sources { type ipv4_addr; }' | ||
| nft 'add chain inet azure_icmp input { type filter hook input priority 0; policy accept; }' | ||
| nft add rule inet azure_icmp input icmp type destination-unreachable icmp code frag-needed ip saddr @icmp_sources counter log drop |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the Azure nftables programming restart-safe.
azure_icmp is host-level state. On container restart, nft add table inet azure_icmp can fail because the previous table still exists, and duplicate host IP observations can make add_nft_icmp.sh return non-zero. That can terminate the long-running oc observe path.
Proposed fix
echo "Adding ICMP drop rule for '$3'"
- nft add element inet azure_icmp icmp_sources "{ $3 }"
+ nft add element inet azure_icmp icmp_sources "{ $3 }" 2>/dev/null || true
EOF
echo "I$(date "+%m%d %H:%M:%S.%N") - drop-icmp - start drop-icmp ${K8S_NODE}"
+ nft delete table inet azure_icmp 2>/dev/null || true
nft add table inet azure_icmpAlso applies to: 623-623
🤖 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 `@bindata/network/ovn-kubernetes/managed/ovnkube-node.yaml` around lines 600 -
618, Make the Azure nftables setup restart-safe in the ovnkube-node init script
by handling existing host-level state gracefully. In the nft programming block
and the generated add_nft_icmp.sh helper, update the logic around nft add table
inet azure_icmp and nft add element inet azure_icmp icmp_sources so repeated
starts or duplicate IP observations do not return non-zero. Reuse the existing
add_nft_icmp.sh and azure_icmp setup symbols, and prefer idempotent checks or
ignoring “already exists” cases so the oc observe flow keeps running after
container restarts.
There was a problem hiding this comment.
nft add element is also idempotent for duplicate elements
There was a problem hiding this comment.
@bpickard22 Good catch — you're right, the duplicate-element part of my comment doesn't apply here.
The remaining issue is just the restart path for:
nft add table inet azure_icmpSince azure_icmp is host-level state, that can still fail if the table already exists from a previous container instance, and with set -xe the script would exit before reaching the flush.
bpickard22
force-pushed
the
iptables-to-nftables
branch
from
cce8cd3 to
36ca925
Compare
|
/retest |
danwinship
left a comment
danwinship
left a comment
There was a problem hiding this comment.
This should be split up into separate commits per feature; notrack, azure thing, kube-proxy, etc.
| iptables -t raw -D OUTPUT -p udp --dport {{.GenevePort}} -j NOTRACK 2>/dev/null || true | ||
| ip6tables -t raw -D PREROUTING -p udp --dport {{.GenevePort}} -j NOTRACK 2>/dev/null || true | ||
| ip6tables -t raw -D OUTPUT -p udp --dport {{.GenevePort}} -j NOTRACK 2>/dev/null || true | ||
| nft add table inet ovn_notrack |
There was a problem hiding this comment.
add a comment to the table, for debuggability
| name: systemd-units | ||
| readOnly: true | ||
| # for the iptables wrapper | ||
| # for the nftables wrapper |
There was a problem hiding this comment.
This was talking about the old iptables wrapper scripts to run the iptables binaries off the host filesystem, but we haven't used those since we stopped supporting RHEL 7... you should be able to just remove this mountpoint. (It looks like nothing else uses it.)
| iptables -X CHECK_ICMP_SOURCE 2>/dev/null || true | ||
| iptables -F ICMP_ACTION 2>/dev/null || true | ||
| iptables -X ICMP_ACTION 2>/dev/null || true | ||
| nft add table inet azure_icmp |
There was a problem hiding this comment.
could be ip4 rather than inet since this is just (IPv4) ICMP
bpickard22
force-pushed
the
iptables-to-nftables
branch
from
36ca925 to
7aa5bda
Compare
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 `@pkg/util/k8s/kubeproxy.go`:
- Around line 96-104: Reinstate validation in kubeproxy.go for the
nodePortAddresses parsing path so only valid kube-proxy values are accepted. In
the logic around getString("nodeport-addresses") and
getString("node-port-addresses"), validate each comma-separated entry before
appending to kpc.NodePortAddresses, allowing only CIDRs or the special primary
value and rejecting anything else with a clear error. Make sure the fix is
applied in the nodePortAddresses handling code so invalid overrides fail fast
instead of being passed through.
🪄 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: 080a5a9f-a921-43ce-945c-fa4242f7bd62
📒 Files selected for processing (5)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/managed/ovnkube-node.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-node.yamlpkg/util/k8s/kubeproxy.gopkg/util/k8s/kubeproxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- bindata/network/ovn-kubernetes/common/008-script-lib.yaml
- pkg/util/k8s/kubeproxy_test.go
- bindata/network/ovn-kubernetes/self-hosted/ovnkube-node.yaml
| if nodePortAddrs != "" { | ||
| for _, addr := range strings.Split(nodePortAddrs, ",") { | ||
| if addr != "" { | ||
| kpc.NodePortAddresses = append(kpc.NodePortAddresses, addr) |
There was a problem hiding this comment.
oh... I didn't realize it had to be split up to be assigned to kpc.NodePortAddresses... in that case, it might be more idiomatic to have a ka.getStringList() method to do the splitting, so you can just assign it here. (Also, I don't think there's any reason to try to handle "" specially.)
Replace iptables/ip6tables raw table NOTRACK commands with nft rules using the inet family, which handles both IPv4 and IPv6 in a single rule. Add iptables cleanup for the upgrade path with a comment noting it can be removed once all nodes have been upgraded. Assisted By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Pickard <bpickard@redhat.com>
bpickard22
force-pushed
the
iptables-to-nftables
branch
2 times, most recently
from
5a38383 to
7139876
Compare
| hostPath: | ||
| path: / | ||
| path: /etc/systemd/system | ||
|
|
There was a problem hiding this comment.
bad edit when splitting the commits?
| hostPath: | ||
| path: / | ||
| path: /etc/systemd/system | ||
|
|
| hostPath: | ||
| path: / | ||
|
|
||
| path: /etc/systemd/system |
There was a problem hiding this comment.
ah, looks like it's broken after the 2nd commit and fixed in the third. can you make the 2nd commit correct by itself?
Replace iptables chain-based Azure ICMP filtering with nftables using an ipv4_addr set for pod source IPs. Uses the ip family since this is IPv4-only ICMP. Add iptables cleanup for the upgrade path. Assisted By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Pickard <bpickard@redhat.com>
Remove the /host volume mount and host-slash volume definition from ovnkube-node daemonsets. These were used for old iptables wrapper scripts that ran iptables binaries from the host filesystem, unused since RHEL 7 support was dropped. With the nftables migration, no container in the pod references /host. Assisted By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Pickard <bpickard@redhat.com>
- Add NFTables arg mapping (masquerade-bit, masquerade-all, sync-period, min-sync-period) to allow proxy-mode: nftables - Add conntrack-tcp-be-liberal arg mapping - Add getStringList helper and use it for nodeport-addresses since it will support non-CIDR values in the future - Accept both nodeport-addresses (correct) and node-port-addresses (legacy misspelling) for NodePortAddresses config Assisted By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Pickard <bpickard@redhat.com>
bpickard22
force-pushed
the
iptables-to-nftables
branch
from
7139876 to
569c2da
Compare
|
@bpickard22: 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. |
|
@bpickard22 now the yaml indentation got screwed up... |
3 participants
Migrate from iptables to an nftables implementation
Migrates the NOTRACK rules to nftables, replaces Azure icmp drop rules, and added an nftables config for kubeproxy to allow for use of proxy-mode: nftables
assisted by: Cluade Opus 4.6
Summary by CodeRabbit