-
Notifications
You must be signed in to change notification settings - Fork 254
feat: remove jump to swift-postrouting in iptables legacy as rules already exist in iptables nftables #3958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the jump to swift-postrouting in iptables legacy since the same rules already exist in iptables nftables. It introduces an iptables legacy client interface to clean up legacy rules during SNAT rule programming.
- Adds a new iptablesLegacyClient interface for deleting legacy iptables rules
- Implements legacy iptables cleanup in SNAT rule programming
- Updates logging to be more descriptive about SWIFT-POSTROUTING chain operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
cns/restserver/restserver.go | Adds iptablesLegacyClient interface and getter method |
cns/restserver/internalapi_windows.go | Implements unsupported legacy client for Windows |
cns/restserver/internalapi_linux.go | Implements legacy iptables deletion and integrates cleanup into SNAT programming |
cns/restserver/internalapi_linux_test.go | Adds test coverage for legacy iptables deletion |
cns/fakes/iptablesfake.go | Adds mock implementation for legacy iptables client |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
e68f37b
e68f37b
to
99e339d
Compare
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
Reason for Change:
Contains same changes as #3930 but without the behavioral change of snat to node ip
Deletes iptables legacy jump to swift postrouting rule before adding nftables rules because:
If we delete before:
If iptables maps to iptables nftables, we should already have nftables rules programmed on the node because cns 1.7.1 has fully rolled out, so deleting first should not cause any connectivity blip.
If iptables somehow maps to iptables legacy, we have a blip and then write the rule immediately after
If we delete afterwards:
If iptables maps to iptables nftables, we write the rules and delete the legacy jump after as expected
If iptables somehow maps to iptables legacy, we would add the rules and then delete the jump we just added, breaking the node until the cns restarts
Issue Fixed:
Requirements:
Notes:
Tested upgrade from 1.7.0 --> 1.7.1 --> 1.7.2 (this PR) for cns