Skip to content

[CNI][Fix] Make iptable calls idempotent for swift podsubnet scenario #1795

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 9, 2023
Merged

[CNI][Fix] Make iptable calls idempotent for swift podsubnet scenario #1795

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8eea64
cns cilium
tamilmani1989 Sep 23, 2022
56c440c
make swift iptable calls idempotent
tamilmani1989 Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions cni/network/invoker_cns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,16 +183,30 @@ func setHostOptions(ncSubnetPrefix *net.IPNet, options map[string]interface{}, i
snatPrimaryIPJump := fmt.Sprintf("%s --to %s", iptables.Snat, info.ncPrimaryIP)
// we need to snat IMDS traffic to node IP, this sets up snat '--to'
snatHostIPJump := fmt.Sprintf("%s --to %s", iptables.Snat, info.hostPrimaryIP)
options[network.IPTablesKey] = []iptables.IPTableEntry{
iptables.GetCreateChainCmd(iptables.V4, iptables.Nat, iptables.Swift),
iptables.GetAppendIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Postrouting, "", iptables.Swift),
// add a snat rules to primary NC IP for DNS
iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureDNSUDPMatch, snatPrimaryIPJump),
iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureDNSTCPMatch, snatPrimaryIPJump),
// add a snat rule to node IP for IMDS http traffic
iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureIMDSMatch, snatHostIPJump),

var iptableCmds []iptables.IPTableEntry
if !iptables.ChainExists(iptables.V4, iptables.Nat, iptables.Swift) {
Copy link
Member

@paulgmiller paulgmiller Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assumign *exits functions don't take a lock.

Copy link
Member Author

@tamilmani1989 tamilmani1989 Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't but what would be the problem even if it does.. that call may fail but retries ensure eventually it will succeed right?

Copy link
Member

@paulgmiller paulgmiller Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exist check is only at start up time.

so if somone put a retry around handle common options and the first one failed at third step then the retry wont re-run exist. and you will fail till your retry expires.

Exist check closter to create/append/insert would be more reliable but its hypothetical if we're not really retrying.

Copy link
Member Author

@tamilmani1989 tamilmani1989 Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not retrying so should be good.. you re right but i have to make considerable code change to achieve that. anyway this iptables part gonna move to cns in long run. even if it fails after exist check, it succeeds in next attempt. it wouldn't endup stuck in that error indefinitely as like now.

iptableCmds = append(iptableCmds, iptables.GetCreateChainCmd(iptables.V4, iptables.Nat, iptables.Swift))
}

if !iptables.RuleExists(iptables.V4, iptables.Nat, iptables.Postrouting, "", iptables.Swift) {
iptableCmds = append(iptableCmds, iptables.GetAppendIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Postrouting, "", iptables.Swift))
}

if !iptables.RuleExists(iptables.V4, iptables.Nat, iptables.Swift, azureDNSUDPMatch, snatPrimaryIPJump) {
iptableCmds = append(iptableCmds, iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureDNSUDPMatch, snatPrimaryIPJump))
}

if !iptables.RuleExists(iptables.V4, iptables.Nat, iptables.Swift, azureDNSTCPMatch, snatPrimaryIPJump) {
iptableCmds = append(iptableCmds, iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureDNSTCPMatch, snatPrimaryIPJump))
}

if !iptables.RuleExists(iptables.V4, iptables.Nat, iptables.Swift, azureIMDSMatch, snatHostIPJump) {
iptableCmds = append(iptableCmds, iptables.GetInsertIptableRuleCmd(iptables.V4, iptables.Nat, iptables.Swift, azureIMDSMatch, snatHostIPJump))
}

options[network.IPTablesKey] = iptableCmds
Copy link
Member

@paulgmiller paulgmiller Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done at invokeation of azure cni process
So if we ever tried to retry witin the cni we might still hit the same thing but that would only block one pod i guess.

another way to do this is to haave all cmds first run the exit. I assume there is no way to do them all atomically with one lock.

Copy link
Member Author

@tamilmani1989 tamilmani1989 Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today cni executes sequentially since we acquire lock at beginning and there can be only one CNI process executes these iptable cmds at a time but we have a plan to parallelize cni .. In parallel execution of cni, there are possiblities that there will be race in adding iptable rules but eventually one will succeed...if cni returns error, containerd runtime will retry the operation


return nil
}

Expand Down