From f974fb37ff6a986e7a55aa36c334dee8b7eb81d2 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Thu, 31 Jul 2025 15:46:03 -0700 Subject: [PATCH 1/7] add iptables reconciliation to cns --- cns/fakes/iptablesfake.go | 96 +++++++- cns/restserver/internalapi_linux.go | 109 ++++++--- cns/restserver/internalapi_linux_test.go | 274 +++++++++++++++++++---- cns/restserver/restserver.go | 3 + cns/restserver/util.go | 11 + 5 files changed, 408 insertions(+), 85 deletions(-) diff --git a/cns/fakes/iptablesfake.go b/cns/fakes/iptablesfake.go index f80fd075c4..c36f24e389 100644 --- a/cns/fakes/iptablesfake.go +++ b/cns/fakes/iptablesfake.go @@ -2,6 +2,7 @@ package fakes import ( "errors" + "fmt" "strings" "github.com/Azure/azure-container-networking/iptables" @@ -11,6 +12,7 @@ var ( errChainExists = errors.New("chain already exists") errChainNotFound = errors.New("chain not found") errRuleExists = errors.New("rule already exists") + errRuleNotFound = errors.New("rule not found") ) type IPTablesMock struct { @@ -83,21 +85,101 @@ func (c *IPTablesMock) Exists(table, chain string, rulespec ...string) (bool, er func (c *IPTablesMock) Append(table, chain string, rulespec ...string) error { c.ensureTableExists(table) + chainRules := c.state[table][chain] + return c.Insert(table, chain, len(chainRules)+1, rulespec...) +} + +func (c *IPTablesMock) Insert(table, chain string, pos int, rulespec ...string) error { + c.ensureTableExists(table) + chainExists, _ := c.ChainExists(table, chain) if !chainExists { return errChainNotFound } - ruleExists, _ := c.Exists(table, chain, rulespec...) - if ruleExists { - return errRuleExists + targetRule := strings.Join(rulespec, " ") + chainRules := c.state[table][chain] + + // convert 1-based position to 0-based index + index := pos - 1 + if index < 0 { + index = 0 + } + + if index >= len(chainRules) { + c.state[table][chain] = append(chainRules, targetRule) + } else { + c.state[table][chain] = append(chainRules[:index], append([]string{targetRule}, chainRules[index:]...)...) } - targetRule := strings.Join(rulespec, " ") - c.state[table][chain] = append(c.state[table][chain], targetRule) return nil } -func (c *IPTablesMock) Insert(table, chain string, _ int, rulespec ...string) error { - return c.Append(table, chain, rulespec...) +func (c *IPTablesMock) List(table, chain string) ([]string, error) { + c.ensureTableExists(table) + + chainExists, _ := c.ChainExists(table, chain) + if !chainExists { + return nil, errChainNotFound + } + + var result []string + + // for built-in chains, start with policy -P, otherwise start with definition -N + builtins := []string{iptables.Input, iptables.Output, iptables.Prerouting, iptables.Postrouting, iptables.Forward} + isBuiltIn := false + for _, builtin := range builtins { + if chain == builtin { + isBuiltIn = true + break + } + } + + if isBuiltIn { + result = append(result, fmt.Sprintf("-P %s ACCEPT", chain)) + } else { + result = append(result, fmt.Sprintf("-N %s", chain)) + } + + // iptables with -S always outputs the rules in -A format + chainRules := c.state[table][chain] + for _, rule := range chainRules { + result = append(result, fmt.Sprintf("-A %s %s", chain, rule)) + } + + return result, nil +} + +func (c *IPTablesMock) ClearChain(table, chain string) error { + c.ensureTableExists(table) + + chainExists, _ := c.ChainExists(table, chain) + if !chainExists { + return errChainNotFound + } + + c.state[table][chain] = []string{} + return nil +} + +func (c *IPTablesMock) Delete(table, chain string, rulespec ...string) error { + c.ensureTableExists(table) + + chainExists, _ := c.ChainExists(table, chain) + if !chainExists { + return errChainNotFound + } + + targetRule := strings.Join(rulespec, " ") + chainRules := c.state[table][chain] + + // delete first match + for i, rule := range chainRules { + if rule == targetRule { + c.state[table][chain] = append(chainRules[:i], chainRules[i+1:]...) + return nil + } + } + + return errRuleNotFound } diff --git a/cns/restserver/internalapi_linux.go b/cns/restserver/internalapi_linux.go index ef30dabf03..277e6d16b3 100644 --- a/cns/restserver/internalapi_linux.go +++ b/cns/restserver/internalapi_linux.go @@ -39,30 +39,59 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer chainExist, err := ipt.ChainExists(iptables.Nat, SWIFT) if err != nil { - return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT chain: %v", err) + return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT-POSTROUTING chain: %v", err) } if !chainExist { // create and append chain if it doesn't exist logger.Printf("[Azure CNS] Creating SWIFT Chain ...") err = ipt.NewChain(iptables.Nat, SWIFT) if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to create SWIFT chain : " + err.Error() - } - logger.Printf("[Azure CNS] Append SWIFT Chain to POSTROUTING ...") - err = ipt.Append(iptables.Nat, iptables.Postrouting, "-j", SWIFT) - if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to append SWIFT chain : " + err.Error() + return types.FailedToRunIPTableCmd, "[Azure CNS] failed to create SWIFT-POSTROUTING chain : " + err.Error() } } - postroutingToSwiftJumpexist, err := ipt.Exists(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + // reconcile jump to SWIFT-POSTROUTING chain + rules, err := ipt.List(iptables.Nat, iptables.Postrouting) if err != nil { - return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of POSTROUTING to SWIFT chain jump: %v", err) + return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check rules in postrouting chain of nat table: %v", err) + } + swiftRuleIndex := len(rules) // append if neither jump rule from POSTROUTING is found + // one time migration from old SWIFT chain + // previously, CNI may have a jump to the SWIFT chain-- our jump to SWIFT-POSTROUTING needs to happen first + for index, rule := range rules { + if rule == "-A POSTROUTING -j SWIFT" { + // jump to SWIFT comes before jump to SWIFT-POSTROUTING, so potential reordering required + swiftRuleIndex = index + break + } + if rule == "-A POSTROUTING -j SWIFT-POSTROUTING" { + // jump to SWIFT-POSTROUTING comes before jump to SWIFT, which requires no further action + swiftRuleIndex = -1 + break + } } - if !postroutingToSwiftJumpexist { - logger.Printf("[Azure CNS] Append SWIFT Chain to POSTROUTING ...") - err = ipt.Append(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + if swiftRuleIndex != -1 { + // jump SWIFT rule exists, insert SWIFT-POSTROUTING rule at the same position so it ends up running first + // first, remove any existing SWIFT-POSTROUTING rules to avoid duplicates + swiftPostroutingExists, err := ipt.Exists(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + if err != nil { + return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT-POSTROUTING rule: %v", err) + } + if swiftPostroutingExists { + err = ipt.Delete(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + if err != nil { + return types.FailedToRunIPTableCmd, "[Azure CNS] failed to delete existing SWIFT-POSTROUTING rule : " + err.Error() + } + } + + // slice index is 0-based, iptables insert is 1-based, but list also gives us the -P POSTROUTING ACCEPT + // as the first rule so swiftRuleIndex gives us the correct 1-indexed iptables position. + // Example: + // -P POSTROUTING ACCEPT is at swiftRuleIndex 0 + // -A POSTROUTING -j SWIFT is at swiftRuleIndex 1, and iptables index 1 + logger.Printf("[Azure CNS] Inserting SWIFT-POSTROUTING Chain at iptables position %d", swiftRuleIndex) + err = ipt.Insert(iptables.Nat, iptables.Postrouting, swiftRuleIndex, "-j", SWIFT) if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to append SWIFT chain : " + err.Error() + return types.FailedToRunIPTableCmd, "[Azure CNS] failed to insert SWIFT-POSTROUTING chain : " + err.Error() } } @@ -71,39 +100,47 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer // put the ip address in standard cidr form (where we zero out the parts that are not relevant) _, podSubnet, _ := net.ParseCIDR(v.IPAddress + "/" + fmt.Sprintf("%d", req.IPConfiguration.IPSubnet.PrefixLength)) - snatUDPRuleExists, err := ipt.Exists(iptables.Nat, SWIFT, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.UDP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()) - if err != nil { - return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of pod SNAT UDP rule : %v", err) + // define all rules we want in the chain + rules := [][]string{ + {"-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.UDP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()}, + {"-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()}, + {"-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureIMDS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.HTTPPort), "-j", iptables.Snat, "--to", req.HostPrimaryIP}, } - if !snatUDPRuleExists { - logger.Printf("[Azure CNS] Inserting pod SNAT UDP rule ...") - err = ipt.Insert(iptables.Nat, SWIFT, 1, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.UDP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()) + + // check if all rules exist + allRulesExist := true + for _, rule := range rules { + exists, err := ipt.Exists(iptables.Nat, SWIFT, rule...) if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to insert pod SNAT UDP rule : " + err.Error() + return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of rule: %v", err) + } + if !exists { + allRulesExist = false + break } } - snatPodTCPRuleExists, err := ipt.Exists(iptables.Nat, SWIFT, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()) + // get current rule count in SWIFT-POSTROUTING chain + currentRules, err := ipt.List(iptables.Nat, SWIFT) if err != nil { - return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of pod SNAT TCP rule : %v", err) + return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to list rules in SWIFT-POSTROUTING chain: %v", err) } - if !snatPodTCPRuleExists { - logger.Printf("[Azure CNS] Inserting pod SNAT TCP rule ...") - err = ipt.Insert(iptables.Nat, SWIFT, 1, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureDNS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", ncPrimaryIP.String()) + + // if rule count doesn't match or not all rules exist, reconcile + // add one because there is always a singular starting rule in the chain, in addition to the ones we add + if len(currentRules) != len(rules)+1 || !allRulesExist { + logger.Printf("[Azure CNS] Reconciling SWIFT-POSTROUTING chain rules") + + err = ipt.ClearChain(iptables.Nat, SWIFT) if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to insert pod SNAT TCP rule : " + err.Error() + return types.FailedToRunIPTableCmd, "[Azure CNS] failed to flush SWIFT-POSTROUTING chain : " + err.Error() } - } - snatIMDSRuleexist, err := ipt.Exists(iptables.Nat, SWIFT, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureIMDS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.HTTPPort), "-j", iptables.Snat, "--to", req.HostPrimaryIP) - if err != nil { - return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of pod SNAT IMDS rule : %v", err) - } - if !snatIMDSRuleexist { - logger.Printf("[Azure CNS] Inserting pod SNAT IMDS rule ...") - err = ipt.Insert(iptables.Nat, SWIFT, 1, "-m", "addrtype", "!", "--dst-type", "local", "-s", podSubnet.String(), "-d", networkutils.AzureIMDS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.HTTPPort), "-j", iptables.Snat, "--to", req.HostPrimaryIP) - if err != nil { - return types.FailedToRunIPTableCmd, "[Azure CNS] failed to insert pod SNAT IMDS rule : " + err.Error() + for _, rule := range rules { + err = ipt.Append(iptables.Nat, SWIFT, rule...) + if err != nil { + return types.FailedToRunIPTableCmd, "[Azure CNS] failed to append rule to SWIFT-POSTROUTING chain : " + err.Error() + } } } diff --git a/cns/restserver/internalapi_linux_test.go b/cns/restserver/internalapi_linux_test.go index 731ca4d989..b51c723da8 100644 --- a/cns/restserver/internalapi_linux_test.go +++ b/cns/restserver/internalapi_linux_test.go @@ -27,16 +27,23 @@ func (c *FakeIPTablesProvider) GetIPTables() (iptablesClient, error) { } func TestAddSNATRules(t *testing.T) { - type expectedScenario struct { + type chainExpectation struct { + table string + chain string + expected []string + } + + type preExistingRule struct { table string chain string rule []string } tests := []struct { - name string - input *cns.CreateNetworkContainerRequest - expected []expectedScenario + name string + input *cns.CreateNetworkContainerRequest + preExistingRules []preExistingRule + expectedChains []chainExpectation }{ { // in pod subnet, the primary nic ip is in the same address space as the pod subnet @@ -56,29 +63,178 @@ func TestAddSNATRules(t *testing.T) { }, HostPrimaryIP: "10.0.0.4", }, - expected: []expectedScenario{ + expectedChains: []chainExpectation{ { table: iptables.Nat, chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", - networkutils.AzureDNS, "-p", iptables.UDP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", "240.1.2.1", + expected: []string{ + "-N SWIFT-POSTROUTING", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p tcp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureIMDS + " -p tcp --dport " + strconv.Itoa(iptables.HTTPPort) + " -j SNAT --to 10.0.0.4", + }, + }, + { + table: iptables.Nat, + chain: iptables.Postrouting, + expected: []string{ + "-P POSTROUTING ACCEPT", + "-A POSTROUTING -j SWIFT-POSTROUTING", + }, + }, + }, + }, + { + // test with pre-existing SWIFT rule that should be migrated + name: "migration from old SWIFT", + input: &cns.CreateNetworkContainerRequest{ + NetworkContainerid: ncID, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "240.1.2.1", + PrefixLength: 24, + }, + }, + SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ + "abc": { + IPAddress: "240.1.2.7", }, }, + HostPrimaryIP: "10.0.0.4", + }, + preExistingRules: []preExistingRule{ + { + table: iptables.Nat, + chain: iptables.Postrouting, + rule: []string{"-j", "SWIFT"}, + }, + { + // stale rule at lower priority should be cleaned up + table: iptables.Nat, + chain: iptables.Postrouting, + rule: []string{"-j", "SWIFT-POSTROUTING"}, + }, + { + // should be cleaned up + table: iptables.Nat, + chain: SWIFT, + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "99.1.2.1"}, + }, + { + table: iptables.Nat, + chain: "SWIFT", + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1"}, + }, + }, + expectedChains: []chainExpectation{ { table: iptables.Nat, chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", - networkutils.AzureDNS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", "240.1.2.1", + expected: []string{ + "-N SWIFT-POSTROUTING", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p tcp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureIMDS + " -p tcp --dport " + strconv.Itoa(iptables.HTTPPort) + " -j SNAT --to 10.0.0.4", + }, + }, + { + table: iptables.Nat, + chain: iptables.Postrouting, + expected: []string{ + "-P POSTROUTING ACCEPT", + "-A POSTROUTING -j SWIFT-POSTROUTING", + "-A POSTROUTING -j SWIFT", }, }, + { + // stale old rule can remain + table: iptables.Nat, + chain: "SWIFT", + expected: []string{ + "-N SWIFT", + "-A SWIFT -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 192.1.2.1", + }, + }, + }, + }, + { + // test after migration has already completed + name: "after migration from old SWIFT", + input: &cns.CreateNetworkContainerRequest{ + NetworkContainerid: ncID, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "240.1.2.1", + PrefixLength: 24, + }, + }, + SecondaryIPConfigs: map[string]cns.SecondaryIPConfig{ + "abc": { + IPAddress: "240.1.2.7", + }, + }, + HostPrimaryIP: "10.0.0.4", + }, + preExistingRules: []preExistingRule{ + { + // rule at higher priority means nothing happens + table: iptables.Nat, + chain: iptables.Postrouting, + rule: []string{"-j", "SWIFT-POSTROUTING"}, + }, + { + table: iptables.Nat, + chain: iptables.Postrouting, + rule: []string{"-j", "SWIFT"}, + }, { table: iptables.Nat, chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", - networkutils.AzureIMDS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.HTTPPort), "-j", iptables.Snat, "--to", "10.0.0.4", + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, + }, + { + table: iptables.Nat, + chain: SWIFT, + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "tcp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, + }, + { + table: iptables.Nat, + chain: SWIFT, + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureIMDS, "-p", "tcp", "--dport", strconv.Itoa(iptables.HTTPPort), "-j", "SNAT", "--to", "10.0.0.4"}, + }, + { + table: iptables.Nat, + chain: "SWIFT", + rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1"}, + }, + }, + expectedChains: []chainExpectation{ + { + table: iptables.Nat, + chain: SWIFT, + expected: []string{ + "-N SWIFT-POSTROUTING", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p tcp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureIMDS + " -p tcp --dport " + strconv.Itoa(iptables.HTTPPort) + " -j SNAT --to 10.0.0.4", + }, + }, + { + table: iptables.Nat, + chain: iptables.Postrouting, + expected: []string{ + "-P POSTROUTING ACCEPT", + "-A POSTROUTING -j SWIFT-POSTROUTING", + "-A POSTROUTING -j SWIFT", + }, + }, + { + // stale old rule can remain + table: iptables.Nat, + chain: "SWIFT", + expected: []string{ + "-N SWIFT", + "-A SWIFT -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 192.1.2.1", }, }, }, @@ -101,29 +257,23 @@ func TestAddSNATRules(t *testing.T) { }, HostPrimaryIP: "10.0.0.4", }, - expected: []expectedScenario{ + expectedChains: []chainExpectation{ { table: iptables.Nat, chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/28", "-d", - networkutils.AzureDNS, "-p", iptables.UDP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", "10.0.0.4", + expected: []string{ + "-N SWIFT-POSTROUTING", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/28 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 10.0.0.4", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/28 -d " + networkutils.AzureDNS + " -p tcp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 10.0.0.4", + "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/28 -d " + networkutils.AzureIMDS + " -p tcp --dport " + strconv.Itoa(iptables.HTTPPort) + " -j SNAT --to 10.0.0.4", }, }, { table: iptables.Nat, - chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/28", "-d", - networkutils.AzureDNS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.DNSPort), "-j", iptables.Snat, "--to", "10.0.0.4", - }, - }, - { - table: iptables.Nat, - chain: SWIFT, - rule: []string{ - "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/28", "-d", - networkutils.AzureIMDS, "-p", iptables.TCP, "--dport", strconv.Itoa(iptables.HTTPPort), "-j", iptables.Snat, "--to", "10.0.0.4", + chain: iptables.Postrouting, + expected: []string{ + "-P POSTROUTING ACCEPT", + "-A POSTROUTING -j SWIFT-POSTROUTING", }, }, }, @@ -131,18 +281,58 @@ func TestAddSNATRules(t *testing.T) { } for _, tt := range tests { - service := getTestService(cns.KubernetesCRD) - service.iptables = &FakeIPTablesProvider{} - resp, msg := service.programSNATRules(tt.input) - if resp != types.Success { - t.Fatal("failed to program snat rules", msg, " case: ", tt.name) - } - finalState, _ := service.iptables.GetIPTables() - for _, ex := range tt.expected { - exists, err := finalState.Exists(ex.table, ex.chain, ex.rule...) - if err != nil || !exists { - t.Fatal("rule not found", ex.rule, " case: ", tt.name) + t.Run(tt.name, func(t *testing.T) { + service := getTestService(cns.KubernetesCRD) + service.iptables = &FakeIPTablesProvider{} + + ipt, err := service.iptables.GetIPTables() + if err != nil { + t.Fatal("failed to get iptables client:", err) + } + + // setup pre-existing rules + if len(tt.preExistingRules) > 0 { + for _, preRule := range tt.preExistingRules { + chainExists, _ := ipt.ChainExists(preRule.table, preRule.chain) + + if !chainExists { + err = ipt.NewChain(preRule.table, preRule.chain) + if err != nil { + t.Fatal("failed to setup pre-existing rule chain:", err) + } + } + + err = ipt.Append(preRule.table, preRule.chain, preRule.rule...) + if err != nil { + t.Fatal("failed to setup pre-existing rule:", err) + } + } + } + + resp, msg := service.programSNATRules(tt.input) + if resp != types.Success { + t.Fatal("failed to program snat rules", msg) + } + + // verify chain contents using List + for _, chainExp := range tt.expectedChains { + actualRules, err := ipt.List(chainExp.table, chainExp.chain) + if err != nil { + t.Fatal("failed to list rules for chain", chainExp.chain, ":", err) + } + + if len(actualRules) != len(chainExp.expected) { + t.Fatalf("chain %s rule count mismatch: got %d, expected %d\nActual: %v\nExpected: %v", + chainExp.chain, len(actualRules), len(chainExp.expected), actualRules, chainExp.expected) + } + + for i, expectedRule := range chainExp.expected { + if actualRules[i] != expectedRule { + t.Fatalf("chain %s rule %d mismatch:\nActual: %s\nExpected: %s", + chainExp.chain, i, actualRules[i], expectedRule) + } + } } - } + }) } } diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index c467ab04e2..56b6e15a48 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -60,6 +60,9 @@ type iptablesClient interface { Append(table string, chain string, rulespec ...string) error Exists(table string, chain string, rulespec ...string) (bool, error) Insert(table string, chain string, pos int, rulespec ...string) error + List(table string, chain string) ([]string, error) + ClearChain(table string, chain string) error + Delete(table, chain string, rulespec ...string) error } type iptablesGetter interface { diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 43d1e1aef9..d05edaca04 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -17,6 +17,7 @@ import ( "github.com/Azure/azure-container-networking/cns/nodesubnet" "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/cns/wireserver" + "github.com/Azure/azure-container-networking/common" acn "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/platform" "github.com/Azure/azure-container-networking/store" @@ -577,6 +578,16 @@ func (service *HTTPRestService) restoreNetworkState() error { } } + // reconcile iptables rules on node when cns restarts + if service.Options[common.OptProgramSNATIPTables] == true && service.state != nil { + for _, container := range service.state.ContainerStatus { + returnCode, returnMessage := service.programSNATRules(&container.CreateNetworkContainerRequest) + if returnCode != 0 { + logger.Errorf(returnMessage) + } + } + } + if rebooted { for _, nwInfo := range service.state.Networks { enableSnat := true From bfb74f49861e0ef291792ff889fac150cded6d74 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Fri, 1 Aug 2025 16:27:18 -0700 Subject: [PATCH 2/7] add test to check if test case flushes chain --- cns/fakes/iptablesfake.go | 13 ++++++++-- cns/restserver/internalapi_linux.go | 1 + cns/restserver/internalapi_linux_test.go | 31 +++++++++++++++--------- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cns/fakes/iptablesfake.go b/cns/fakes/iptablesfake.go index c36f24e389..23063aee14 100644 --- a/cns/fakes/iptablesfake.go +++ b/cns/fakes/iptablesfake.go @@ -13,10 +13,12 @@ var ( errChainNotFound = errors.New("chain not found") errRuleExists = errors.New("rule already exists") errRuleNotFound = errors.New("rule not found") + errIndexBounds = errors.New("index out of bounds") ) type IPTablesMock struct { - state map[string]map[string][]string + state map[string]map[string][]string + clearChainCalled int } func NewIPTablesMock() *IPTablesMock { @@ -106,8 +108,10 @@ func (c *IPTablesMock) Insert(table, chain string, pos int, rulespec ...string) index = 0 } - if index >= len(chainRules) { + if index == len(chainRules) { c.state[table][chain] = append(chainRules, targetRule) + } else if index > len(chainRules) { + return errIndexBounds } else { c.state[table][chain] = append(chainRules[:index], append([]string{targetRule}, chainRules[index:]...)...) } @@ -151,6 +155,7 @@ func (c *IPTablesMock) List(table, chain string) ([]string, error) { } func (c *IPTablesMock) ClearChain(table, chain string) error { + c.clearChainCalled++ c.ensureTableExists(table) chainExists, _ := c.ChainExists(table, chain) @@ -183,3 +188,7 @@ func (c *IPTablesMock) Delete(table, chain string, rulespec ...string) error { return errRuleNotFound } + +func (c *IPTablesMock) ClearChainCallCount() int { + return c.clearChainCalled +} diff --git a/cns/restserver/internalapi_linux.go b/cns/restserver/internalapi_linux.go index 277e6d16b3..7381c0ca6e 100644 --- a/cns/restserver/internalapi_linux.go +++ b/cns/restserver/internalapi_linux.go @@ -72,6 +72,7 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer if swiftRuleIndex != -1 { // jump SWIFT rule exists, insert SWIFT-POSTROUTING rule at the same position so it ends up running first // first, remove any existing SWIFT-POSTROUTING rules to avoid duplicates + // note: inserting at len(rules) and deleting a jump to SWIFT-POSTROUTING is mutually exclusive swiftPostroutingExists, err := ipt.Exists(iptables.Nat, iptables.Postrouting, "-j", SWIFT) if err != nil { return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT-POSTROUTING rule: %v", err) diff --git a/cns/restserver/internalapi_linux_test.go b/cns/restserver/internalapi_linux_test.go index b51c723da8..6c7ce37551 100644 --- a/cns/restserver/internalapi_linux_test.go +++ b/cns/restserver/internalapi_linux_test.go @@ -40,10 +40,11 @@ func TestAddSNATRules(t *testing.T) { } tests := []struct { - name string - input *cns.CreateNetworkContainerRequest - preExistingRules []preExistingRule - expectedChains []chainExpectation + name string + input *cns.CreateNetworkContainerRequest + preExistingRules []preExistingRule + expectedChains []chainExpectation + expectedClearChainCalls int }{ { // in pod subnet, the primary nic ip is in the same address space as the pod subnet @@ -83,6 +84,7 @@ func TestAddSNATRules(t *testing.T) { }, }, }, + expectedClearChainCalls: 1, }, { // test with pre-existing SWIFT rule that should be migrated @@ -156,6 +158,7 @@ func TestAddSNATRules(t *testing.T) { }, }, }, + expectedClearChainCalls: 1, }, { // test after migration has already completed @@ -238,6 +241,7 @@ func TestAddSNATRules(t *testing.T) { }, }, }, + expectedClearChainCalls: 0, }, { // in vnet scale, the primary nic ip becomes the node ip (diff address space from pod subnet) @@ -277,17 +281,16 @@ func TestAddSNATRules(t *testing.T) { }, }, }, + expectedClearChainCalls: 1, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { service := getTestService(cns.KubernetesCRD) - service.iptables = &FakeIPTablesProvider{} - - ipt, err := service.iptables.GetIPTables() - if err != nil { - t.Fatal("failed to get iptables client:", err) + ipt := fakes.NewIPTablesMock() + service.iptables = &FakeIPTablesProvider{ + iptables: ipt, } // setup pre-existing rules @@ -296,13 +299,13 @@ func TestAddSNATRules(t *testing.T) { chainExists, _ := ipt.ChainExists(preRule.table, preRule.chain) if !chainExists { - err = ipt.NewChain(preRule.table, preRule.chain) + err := ipt.NewChain(preRule.table, preRule.chain) if err != nil { t.Fatal("failed to setup pre-existing rule chain:", err) } } - err = ipt.Append(preRule.table, preRule.chain, preRule.rule...) + err := ipt.Append(preRule.table, preRule.chain, preRule.rule...) if err != nil { t.Fatal("failed to setup pre-existing rule:", err) } @@ -333,6 +336,12 @@ func TestAddSNATRules(t *testing.T) { } } } + + // verify ClearChain was called the expected number of times + actualClearChainCalls := ipt.ClearChainCallCount() + if actualClearChainCalls != tt.expectedClearChainCalls { + t.Fatalf("ClearChain call count mismatch: got %d, expected %d", actualClearChainCalls, tt.expectedClearChainCalls) + } }) } } From f4af45bc3fe51992074702012a76014e020e35d3 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Fri, 1 Aug 2025 17:01:10 -0700 Subject: [PATCH 3/7] rename chain variable to reflect its value --- cns/fakes/iptablesfake.go | 8 ++++---- cns/restserver/internalapi_linux.go | 22 +++++++++++----------- cns/restserver/internalapi_linux_test.go | 16 ++++++++-------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/cns/fakes/iptablesfake.go b/cns/fakes/iptablesfake.go index 23063aee14..1329ade67b 100644 --- a/cns/fakes/iptablesfake.go +++ b/cns/fakes/iptablesfake.go @@ -17,8 +17,8 @@ var ( ) type IPTablesMock struct { - state map[string]map[string][]string - clearChainCalled int + state map[string]map[string][]string + clearChainCallCount int } func NewIPTablesMock() *IPTablesMock { @@ -155,7 +155,7 @@ func (c *IPTablesMock) List(table, chain string) ([]string, error) { } func (c *IPTablesMock) ClearChain(table, chain string) error { - c.clearChainCalled++ + c.clearChainCallCount++ c.ensureTableExists(table) chainExists, _ := c.ChainExists(table, chain) @@ -190,5 +190,5 @@ func (c *IPTablesMock) Delete(table, chain string, rulespec ...string) error { } func (c *IPTablesMock) ClearChainCallCount() int { - return c.clearChainCalled + return c.clearChainCallCount } diff --git a/cns/restserver/internalapi_linux.go b/cns/restserver/internalapi_linux.go index 7381c0ca6e..7b04c4f8e0 100644 --- a/cns/restserver/internalapi_linux.go +++ b/cns/restserver/internalapi_linux.go @@ -14,7 +14,7 @@ import ( "github.com/pkg/errors" ) -const SWIFT = "SWIFT-POSTROUTING" +const SWIFTPOSTROUTING = "SWIFT-POSTROUTING" type IPtablesProvider struct{} @@ -37,13 +37,13 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to create iptables interface : %v", err) } - chainExist, err := ipt.ChainExists(iptables.Nat, SWIFT) + chainExist, err := ipt.ChainExists(iptables.Nat, SWIFTPOSTROUTING) if err != nil { return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT-POSTROUTING chain: %v", err) } if !chainExist { // create and append chain if it doesn't exist - logger.Printf("[Azure CNS] Creating SWIFT Chain ...") - err = ipt.NewChain(iptables.Nat, SWIFT) + logger.Printf("[Azure CNS] Creating SWIFT-POSTROUTING Chain ...") + err = ipt.NewChain(iptables.Nat, SWIFTPOSTROUTING) if err != nil { return types.FailedToRunIPTableCmd, "[Azure CNS] failed to create SWIFT-POSTROUTING chain : " + err.Error() } @@ -73,12 +73,12 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer // jump SWIFT rule exists, insert SWIFT-POSTROUTING rule at the same position so it ends up running first // first, remove any existing SWIFT-POSTROUTING rules to avoid duplicates // note: inserting at len(rules) and deleting a jump to SWIFT-POSTROUTING is mutually exclusive - swiftPostroutingExists, err := ipt.Exists(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + swiftPostroutingExists, err := ipt.Exists(iptables.Nat, iptables.Postrouting, "-j", SWIFTPOSTROUTING) if err != nil { return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of SWIFT-POSTROUTING rule: %v", err) } if swiftPostroutingExists { - err = ipt.Delete(iptables.Nat, iptables.Postrouting, "-j", SWIFT) + err = ipt.Delete(iptables.Nat, iptables.Postrouting, "-j", SWIFTPOSTROUTING) if err != nil { return types.FailedToRunIPTableCmd, "[Azure CNS] failed to delete existing SWIFT-POSTROUTING rule : " + err.Error() } @@ -90,7 +90,7 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer // -P POSTROUTING ACCEPT is at swiftRuleIndex 0 // -A POSTROUTING -j SWIFT is at swiftRuleIndex 1, and iptables index 1 logger.Printf("[Azure CNS] Inserting SWIFT-POSTROUTING Chain at iptables position %d", swiftRuleIndex) - err = ipt.Insert(iptables.Nat, iptables.Postrouting, swiftRuleIndex, "-j", SWIFT) + err = ipt.Insert(iptables.Nat, iptables.Postrouting, swiftRuleIndex, "-j", SWIFTPOSTROUTING) if err != nil { return types.FailedToRunIPTableCmd, "[Azure CNS] failed to insert SWIFT-POSTROUTING chain : " + err.Error() } @@ -111,7 +111,7 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer // check if all rules exist allRulesExist := true for _, rule := range rules { - exists, err := ipt.Exists(iptables.Nat, SWIFT, rule...) + exists, err := ipt.Exists(iptables.Nat, SWIFTPOSTROUTING, rule...) if err != nil { return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to check for existence of rule: %v", err) } @@ -122,7 +122,7 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer } // get current rule count in SWIFT-POSTROUTING chain - currentRules, err := ipt.List(iptables.Nat, SWIFT) + currentRules, err := ipt.List(iptables.Nat, SWIFTPOSTROUTING) if err != nil { return types.UnexpectedError, fmt.Sprintf("[Azure CNS] Error. Failed to list rules in SWIFT-POSTROUTING chain: %v", err) } @@ -132,13 +132,13 @@ func (service *HTTPRestService) programSNATRules(req *cns.CreateNetworkContainer if len(currentRules) != len(rules)+1 || !allRulesExist { logger.Printf("[Azure CNS] Reconciling SWIFT-POSTROUTING chain rules") - err = ipt.ClearChain(iptables.Nat, SWIFT) + err = ipt.ClearChain(iptables.Nat, SWIFTPOSTROUTING) if err != nil { return types.FailedToRunIPTableCmd, "[Azure CNS] failed to flush SWIFT-POSTROUTING chain : " + err.Error() } for _, rule := range rules { - err = ipt.Append(iptables.Nat, SWIFT, rule...) + err = ipt.Append(iptables.Nat, SWIFTPOSTROUTING, rule...) if err != nil { return types.FailedToRunIPTableCmd, "[Azure CNS] failed to append rule to SWIFT-POSTROUTING chain : " + err.Error() } diff --git a/cns/restserver/internalapi_linux_test.go b/cns/restserver/internalapi_linux_test.go index 6c7ce37551..f0ef145f49 100644 --- a/cns/restserver/internalapi_linux_test.go +++ b/cns/restserver/internalapi_linux_test.go @@ -67,7 +67,7 @@ func TestAddSNATRules(t *testing.T) { expectedChains: []chainExpectation{ { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, expected: []string{ "-N SWIFT-POSTROUTING", "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", @@ -119,7 +119,7 @@ func TestAddSNATRules(t *testing.T) { { // should be cleaned up table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "99.1.2.1"}, }, { @@ -131,7 +131,7 @@ func TestAddSNATRules(t *testing.T) { expectedChains: []chainExpectation{ { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, expected: []string{ "-N SWIFT-POSTROUTING", "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", @@ -192,17 +192,17 @@ func TestAddSNATRules(t *testing.T) { }, { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, }, { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "tcp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, }, { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureIMDS, "-p", "tcp", "--dport", strconv.Itoa(iptables.HTTPPort), "-j", "SNAT", "--to", "10.0.0.4"}, }, { @@ -214,7 +214,7 @@ func TestAddSNATRules(t *testing.T) { expectedChains: []chainExpectation{ { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, expected: []string{ "-N SWIFT-POSTROUTING", "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/24 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 240.1.2.1", @@ -264,7 +264,7 @@ func TestAddSNATRules(t *testing.T) { expectedChains: []chainExpectation{ { table: iptables.Nat, - chain: SWIFT, + chain: SWIFTPOSTROUTING, expected: []string{ "-N SWIFT-POSTROUTING", "-A SWIFT-POSTROUTING -m addrtype ! --dst-type local -s 240.1.2.0/28 -d " + networkutils.AzureDNS + " -p udp --dport " + strconv.Itoa(iptables.DNSPort) + " -j SNAT --to 10.0.0.4", From bfc9d10980b7454300047af0eb22e3c00382b305 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Sat, 2 Aug 2025 11:55:43 -0700 Subject: [PATCH 4/7] address linter --- cns/fakes/iptablesfake.go | 14 ++++++++------ cns/restserver/util.go | 6 +++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cns/fakes/iptablesfake.go b/cns/fakes/iptablesfake.go index 1329ade67b..1845a6e61d 100644 --- a/cns/fakes/iptablesfake.go +++ b/cns/fakes/iptablesfake.go @@ -108,11 +108,12 @@ func (c *IPTablesMock) Insert(table, chain string, pos int, rulespec ...string) index = 0 } - if index == len(chainRules) { + switch { + case index == len(chainRules): c.state[table][chain] = append(chainRules, targetRule) - } else if index > len(chainRules) { + case index > len(chainRules): return errIndexBounds - } else { + default: c.state[table][chain] = append(chainRules[:index], append([]string{targetRule}, chainRules[index:]...)...) } @@ -127,7 +128,9 @@ func (c *IPTablesMock) List(table, chain string) ([]string, error) { return nil, errChainNotFound } - var result []string + chainRules := c.state[table][chain] + // preallocate: 1 for chain header + number of rules + result := make([]string, 0, 1+len(chainRules)) // for built-in chains, start with policy -P, otherwise start with definition -N builtins := []string{iptables.Input, iptables.Output, iptables.Prerouting, iptables.Postrouting, iptables.Forward} @@ -142,11 +145,10 @@ func (c *IPTablesMock) List(table, chain string) ([]string, error) { if isBuiltIn { result = append(result, fmt.Sprintf("-P %s ACCEPT", chain)) } else { - result = append(result, fmt.Sprintf("-N %s", chain)) + result = append(result, "-N "+chain) } // iptables with -S always outputs the rules in -A format - chainRules := c.state[table][chain] for _, rule := range chainRules { result = append(result, fmt.Sprintf("-A %s %s", chain, rule)) } diff --git a/cns/restserver/util.go b/cns/restserver/util.go index d05edaca04..ffec198153 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -17,7 +17,6 @@ import ( "github.com/Azure/azure-container-networking/cns/nodesubnet" "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/cns/wireserver" - "github.com/Azure/azure-container-networking/common" acn "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/platform" "github.com/Azure/azure-container-networking/store" @@ -579,8 +578,9 @@ func (service *HTTPRestService) restoreNetworkState() error { } // reconcile iptables rules on node when cns restarts - if service.Options[common.OptProgramSNATIPTables] == true && service.state != nil { - for _, container := range service.state.ContainerStatus { + if service.Options[acn.OptProgramSNATIPTables] == true && service.state != nil { + for containerID := range service.state.ContainerStatus { + container := service.state.ContainerStatus[containerID] returnCode, returnMessage := service.programSNATRules(&container.CreateNetworkContainerRequest) if returnCode != 0 { logger.Errorf(returnMessage) From 8ea80350bfb33568fa09262dd49f9e7f43ee3036 Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 4 Aug 2025 10:21:28 -0700 Subject: [PATCH 5/7] nolint logger usage for now --- cns/restserver/util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index ffec198153..7fa738ef92 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -583,7 +583,7 @@ func (service *HTTPRestService) restoreNetworkState() error { container := service.state.ContainerStatus[containerID] returnCode, returnMessage := service.programSNATRules(&container.CreateNetworkContainerRequest) if returnCode != 0 { - logger.Errorf(returnMessage) + logger.Errorf(returnMessage) // nolint } } } From c779251f7c68ba43daa05d408591f038f1d0ba2c Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 4 Aug 2025 10:48:19 -0700 Subject: [PATCH 6/7] address lll linter --- cns/restserver/internalapi_linux_test.go | 30 +++++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/cns/restserver/internalapi_linux_test.go b/cns/restserver/internalapi_linux_test.go index f0ef145f49..a292b30f98 100644 --- a/cns/restserver/internalapi_linux_test.go +++ b/cns/restserver/internalapi_linux_test.go @@ -120,12 +120,18 @@ func TestAddSNATRules(t *testing.T) { // should be cleaned up table: iptables.Nat, chain: SWIFTPOSTROUTING, - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "99.1.2.1"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, + "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "99.1.2.1", + }, }, { table: iptables.Nat, chain: "SWIFT", - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, + "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1", + }, }, }, expectedChains: []chainExpectation{ @@ -193,22 +199,34 @@ func TestAddSNATRules(t *testing.T) { { table: iptables.Nat, chain: SWIFTPOSTROUTING, - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, + "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1", + }, }, { table: iptables.Nat, chain: SWIFTPOSTROUTING, - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "tcp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, + "-p", "tcp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "240.1.2.1", + }, }, { table: iptables.Nat, chain: SWIFTPOSTROUTING, - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureIMDS, "-p", "tcp", "--dport", strconv.Itoa(iptables.HTTPPort), "-j", "SNAT", "--to", "10.0.0.4"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureIMDS, + "-p", "tcp", "--dport", strconv.Itoa(iptables.HTTPPort), "-j", "SNAT", "--to", "10.0.0.4", + }, }, { table: iptables.Nat, chain: "SWIFT", - rule: []string{"-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1"}, + rule: []string{ + "-m", "addrtype", "!", "--dst-type", "local", "-s", "240.1.2.0/24", "-d", networkutils.AzureDNS, + "-p", "udp", "--dport", strconv.Itoa(iptables.DNSPort), "-j", "SNAT", "--to", "192.1.2.1", + }, }, }, expectedChains: []chainExpectation{ From e7c71b5ead471d6deab8c1af7cfc16f74a4f6b6f Mon Sep 17 00:00:00 2001 From: QxBytes Date: Mon, 4 Aug 2025 14:58:33 -0700 Subject: [PATCH 7/7] remove code from startup as reconcile nnc always runs on cns startup --- cns/restserver/util.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 7fa738ef92..43d1e1aef9 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -577,17 +577,6 @@ func (service *HTTPRestService) restoreNetworkState() error { } } - // reconcile iptables rules on node when cns restarts - if service.Options[acn.OptProgramSNATIPTables] == true && service.state != nil { - for containerID := range service.state.ContainerStatus { - container := service.state.ContainerStatus[containerID] - returnCode, returnMessage := service.programSNATRules(&container.CreateNetworkContainerRequest) - if returnCode != 0 { - logger.Errorf(returnMessage) // nolint - } - } - } - if rebooted { for _, nwInfo := range service.state.Networks { enableSnat := true