From a3556edba13df7310e498abb783137da4ea8aa57 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 1 Jun 2022 10:58:01 -0400 Subject: [PATCH 1/3] Stop trying to "preserve" iptables counters that are always 0 The iptables and ipvs proxies have code to try to preserve certain iptables counters when modifying chains via iptables-restore, but the counters in question only actually exist for the built-in chains (eg INPUT, FORWARD, PREROUTING, etc), which we never modify via iptables-restore (and in fact, *can't* safely modify via iptables-restore), so we are really just doing a lot of unnecessary work to copy the constant string "[0:0]" over from iptables-save output to iptables-restore input. So stop doing that. Also fix a confused error message when iptables-save fails. --- pkg/proxy/iptables/proxier.go | 75 +++++++++-------------------------- pkg/proxy/ipvs/proxier.go | 30 +------------- 2 files changed, 21 insertions(+), 84 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index c8d133c180ac3..ae3f86c6756d7 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -431,16 +431,16 @@ func CleanupLeftovers(ipt utiliptables.Interface) (encounteredError bool) { for _, chain := range []utiliptables.Chain{kubeServicesChain, kubeNodePortsChain, kubePostroutingChain} { if _, found := existingNATChains[chain]; found { chainString := string(chain) - natChains.WriteBytes(existingNATChains[chain]) // flush - natRules.Write("-X", chainString) // delete + natChains.Write(utiliptables.MakeChainLine(chain)) // flush + natRules.Write("-X", chainString) // delete } } // Hunt for service and endpoint chains. for chain := range existingNATChains { chainString := string(chain) if isServiceChainName(chainString) { - natChains.WriteBytes(existingNATChains[chain]) // flush - natRules.Write("-X", chainString) // delete + natChains.Write(utiliptables.MakeChainLine(chain)) // flush + natRules.Write("-X", chainString) // delete } } natRules.Write("COMMIT") @@ -467,7 +467,7 @@ func CleanupLeftovers(ipt utiliptables.Interface) (encounteredError bool) { for _, chain := range []utiliptables.Chain{kubeServicesChain, kubeExternalServicesChain, kubeForwardChain, kubeNodePortsChain} { if _, found := existingFilterChains[chain]; found { chainString := string(chain) - filterChains.WriteBytes(existingFilterChains[chain]) + filterChains.Write(utiliptables.MakeChainLine(chain)) filterRules.Write("-X", chainString) } } @@ -890,22 +890,13 @@ func (proxier *Proxier) syncProxyRules() { // Get iptables-save output so we can check for existing chains and rules. // This will be a map of chain name to chain with rules as stored in iptables-save/iptables-restore - existingFilterChains := make(map[utiliptables.Chain][]byte) - proxier.existingFilterChainsData.Reset() - err := proxier.iptables.SaveInto(utiliptables.TableFilter, proxier.existingFilterChainsData) - if err != nil { // if we failed to get any rules - klog.ErrorS(err, "Failed to execute iptables-save, syncing all rules") - } else { // otherwise parse the output - existingFilterChains = utiliptables.GetChainLines(utiliptables.TableFilter, proxier.existingFilterChainsData.Bytes()) - } - // IMPORTANT: existingNATChains may share memory with proxier.iptablesData. existingNATChains := make(map[utiliptables.Chain][]byte) proxier.iptablesData.Reset() - err = proxier.iptables.SaveInto(utiliptables.TableNAT, proxier.iptablesData) - if err != nil { // if we failed to get any rules - klog.ErrorS(err, "Failed to execute iptables-save, syncing all rules") - } else { // otherwise parse the output + err := proxier.iptables.SaveInto(utiliptables.TableNAT, proxier.iptablesData) + if err != nil { + klog.ErrorS(err, "Failed to execute iptables-save: stale chains will not be deleted") + } else { existingNATChains = utiliptables.GetChainLines(utiliptables.TableNAT, proxier.iptablesData.Bytes()) } @@ -919,18 +910,10 @@ func (proxier *Proxier) syncProxyRules() { // Make sure we keep stats for the top-level chains, if they existed // (which most should have because we created them above). for _, chainName := range []utiliptables.Chain{kubeServicesChain, kubeExternalServicesChain, kubeForwardChain, kubeNodePortsChain} { - if chain, ok := existingFilterChains[chainName]; ok { - proxier.filterChains.WriteBytes(chain) - } else { - proxier.filterChains.Write(utiliptables.MakeChainLine(chainName)) - } + proxier.filterChains.Write(utiliptables.MakeChainLine(chainName)) } for _, chainName := range []utiliptables.Chain{kubeServicesChain, kubeNodePortsChain, kubePostroutingChain, kubeMarkMasqChain} { - if chain, ok := existingNATChains[chainName]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(chainName)) - } + proxier.natChains.Write(utiliptables.MakeChainLine(chainName)) } // Install the kubernetes-specific postrouting rules. We use a whole chain for @@ -1028,12 +1011,8 @@ func (proxier *Proxier) syncProxyRules() { endpointChain := epInfo.ChainName - // Create the endpoint chain, retaining counters if possible. - if chain, ok := existingNATChains[endpointChain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(endpointChain)) - } + // Create the endpoint chain + proxier.natChains.Write(utiliptables.MakeChainLine(endpointChain)) activeNATChains[endpointChain] = true args = append(args[:0], "-A", string(endpointChain)) @@ -1077,22 +1056,14 @@ func (proxier *Proxier) syncProxyRules() { // Declare the clusterPolicyChain if needed. if hasEndpoints && svcInfo.UsesClusterEndpoints() { - // Create the Cluster traffic policy chain, retaining counters if possible. - if chain, ok := existingNATChains[clusterPolicyChain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(clusterPolicyChain)) - } + // Create the Cluster traffic policy chain + proxier.natChains.Write(utiliptables.MakeChainLine(clusterPolicyChain)) activeNATChains[clusterPolicyChain] = true } // Declare the localPolicyChain if needed. if hasEndpoints && svcInfo.UsesLocalEndpoints() { - if chain, ok := existingNATChains[localPolicyChain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(localPolicyChain)) - } + proxier.natChains.Write(utiliptables.MakeChainLine(localPolicyChain)) activeNATChains[localPolicyChain] = true } @@ -1101,11 +1072,7 @@ func (proxier *Proxier) syncProxyRules() { // jump to externalTrafficChain, which will handle some special-cases // and then jump to externalPolicyChain. if hasEndpoints && svcInfo.ExternallyAccessible() { - if chain, ok := existingNATChains[externalTrafficChain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(externalTrafficChain)) - } + proxier.natChains.Write(utiliptables.MakeChainLine(externalTrafficChain)) activeNATChains[externalTrafficChain] = true if !svcInfo.ExternalPolicyLocal() { @@ -1233,11 +1200,7 @@ func (proxier *Proxier) syncProxyRules() { fwChain := svcInfo.firewallChainName // Declare the service firewall chain. - if chain, ok := existingNATChains[fwChain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(fwChain)) - } + proxier.natChains.Write(utiliptables.MakeChainLine(fwChain)) activeNATChains[fwChain] = true // The firewall chain will jump to the "external destination" @@ -1384,7 +1347,7 @@ func (proxier *Proxier) syncProxyRules() { // We must (as per iptables) write a chain-line for it, which has // the nice effect of flushing the chain. Then we can remove the // chain. - proxier.natChains.WriteBytes(existingNATChains[chain]) + proxier.natChains.Write(utiliptables.MakeChainLine(chain)) proxier.natRules.Write("-X", chainString) } } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 74e2f1c471ce0..823ca8e532406 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1839,27 +1839,15 @@ func (proxier *Proxier) acceptIPVSTraffic() { // createAndLinkKubeChain create all kube chains that ipvs proxier need and write basic link. func (proxier *Proxier) createAndLinkKubeChain() { - existingFilterChains := proxier.getExistingChains(proxier.filterChainsData, utiliptables.TableFilter) - existingNATChains := proxier.getExistingChains(proxier.iptablesData, utiliptables.TableNAT) - - // Make sure we keep stats for the top-level chains for _, ch := range iptablesChains { if _, err := proxier.iptables.EnsureChain(ch.table, ch.chain); err != nil { klog.ErrorS(err, "Failed to ensure chain exists", "table", ch.table, "chain", ch.chain) return } if ch.table == utiliptables.TableNAT { - if chain, ok := existingNATChains[ch.chain]; ok { - proxier.natChains.WriteBytes(chain) - } else { - proxier.natChains.Write(utiliptables.MakeChainLine(ch.chain)) - } + proxier.natChains.Write(utiliptables.MakeChainLine(ch.chain)) } else { - if chain, ok := existingFilterChains[ch.chain]; ok { - proxier.filterChains.WriteBytes(chain) - } else { - proxier.filterChains.Write(utiliptables.MakeChainLine(ch.chain)) - } + proxier.filterChains.Write(utiliptables.MakeChainLine(ch.chain)) } } @@ -1872,20 +1860,6 @@ func (proxier *Proxier) createAndLinkKubeChain() { } -// getExistingChains get iptables-save output so we can check for existing chains and rules. -// This will be a map of chain name to chain with rules as stored in iptables-save/iptables-restore -// Result may SHARE memory with contents of buffer. -func (proxier *Proxier) getExistingChains(buffer *bytes.Buffer, table utiliptables.Table) map[utiliptables.Chain][]byte { - buffer.Reset() - err := proxier.iptables.SaveInto(table, buffer) - if err != nil { // if we failed to get any rules - klog.ErrorS(err, "Failed to execute iptables-save, syncing all rules") - } else { // otherwise parse the output - return utiliptables.GetChainLines(table, buffer.Bytes()) - } - return nil -} - // After a UDP or SCTP endpoint has been removed, we must flush any pending conntrack entries to it, or else we // risk sending more traffic to it, all of which will be lost (because UDP). // This assumes the proxier mutex is held From 4988699c2f26965ca99928f853656c37df6a1c48 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 1 Jun 2022 10:55:15 -0400 Subject: [PATCH 2/3] Use dedent to fix GetChainLines() tests The test was calling GetChainLines() on invalid pseudo-iptables-save output where most of the lines were indented. GetChainLines() happened to still parse this "correctly", but it would be better to be testing it on actually-correct data. --- pkg/util/iptables/save_restore_test.go | 122 +++++++++++++------------ 1 file changed, 63 insertions(+), 59 deletions(-) diff --git a/pkg/util/iptables/save_restore_test.go b/pkg/util/iptables/save_restore_test.go index 5a7695f3f682f..52b4f42051121 100644 --- a/pkg/util/iptables/save_restore_test.go +++ b/pkg/util/iptables/save_restore_test.go @@ -18,6 +18,8 @@ package iptables import ( "testing" + + "github.com/lithammer/dedent" ) func TestReadLinesFromByteBuffer(t *testing.T) { @@ -67,14 +69,15 @@ func checkAllLines(t *testing.T, table Table, save []byte, expectedLines map[Cha } func TestGetChainLines(t *testing.T) { - iptablesSave := `# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014 - *nat - :PREROUTING ACCEPT [2136997:197881818] - :POSTROUTING ACCEPT [4284525:258542680] - :OUTPUT ACCEPT [5901660:357267963] - -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER - COMMIT - # Completed on Wed Oct 29 14:56:01 2014` + iptablesSave := dedent.Dedent( + `# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014 + *nat + :PREROUTING ACCEPT [2136997:197881818] + :POSTROUTING ACCEPT [4284525:258542680] + :OUTPUT ACCEPT [5901660:357267963] + -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER + COMMIT + # Completed on Wed Oct 29 14:56:01 2014`) expected := map[Chain]string{ ChainPrerouting: ":PREROUTING ACCEPT [2136997:197881818]", ChainPostrouting: ":POSTROUTING ACCEPT [4284525:258542680]", @@ -84,57 +87,58 @@ func TestGetChainLines(t *testing.T) { } func TestGetChainLinesMultipleTables(t *testing.T) { - iptablesSave := `# Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 - *nat - :PREROUTING ACCEPT [2:138] - :INPUT ACCEPT [0:0] - :OUTPUT ACCEPT [0:0] - :POSTROUTING ACCEPT [0:0] - :DOCKER - [0:0] - :KUBE-NODEPORT-CONTAINER - [0:0] - :KUBE-NODEPORT-HOST - [0:0] - :KUBE-PORTALS-CONTAINER - [0:0] - :KUBE-PORTALS-HOST - [0:0] - :KUBE-SVC-1111111111111111 - [0:0] - :KUBE-SVC-2222222222222222 - [0:0] - :KUBE-SVC-3333333333333333 - [0:0] - :KUBE-SVC-4444444444444444 - [0:0] - :KUBE-SVC-5555555555555555 - [0:0] - :KUBE-SVC-6666666666666666 - [0:0] - -A PREROUTING -m comment --comment "handle ClusterIPs; NOTE: this must be before the NodePort rules" -j KUBE-PORTALS-CONTAINER - -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER - -A PREROUTING -m addrtype --dst-type LOCAL -m comment --comment "handle service NodePorts; NOTE: this must be the last rule in the chain" -j KUBE-NODEPORT-CONTAINER - -A OUTPUT -m comment --comment "handle ClusterIPs; NOTE: this must be before the NodePort rules" -j KUBE-PORTALS-HOST - -A OUTPUT ! -d 127.0.0.0/8 -m addrtype --dst-type LOCAL -j DOCKER - -A OUTPUT -m addrtype --dst-type LOCAL -m comment --comment "handle service NodePorts; NOTE: this must be the last rule in the chain" -j KUBE-NODEPORT-HOST - -A POSTROUTING -s 10.246.1.0/24 ! -o cbr0 -j MASQUERADE - -A POSTROUTING -s 10.0.2.15 -d 10.0.2.15 -m comment --comment "handle pod connecting to self" -j MASQUERADE - -A KUBE-PORTALS-CONTAINER -d 10.247.0.1 -p tcp -m comment --comment "portal for default/kubernetes:" -m state --state NEW -m tcp --dport 443 -j KUBE-SVC-5555555555555555 - -A KUBE-PORTALS-CONTAINER -d 10.247.0.10 -p udp -m comment --comment "portal for kube-system/kube-dns:dns" -m state --state NEW -m udp --dport 53 -j KUBE-SVC-6666666666666666 - -A KUBE-PORTALS-CONTAINER -d 10.247.0.10 -p tcp -m comment --comment "portal for kube-system/kube-dns:dns-tcp" -m state --state NEW -m tcp --dport 53 -j KUBE-SVC-2222222222222222 - -A KUBE-PORTALS-HOST -d 10.247.0.1 -p tcp -m comment --comment "portal for default/kubernetes:" -m state --state NEW -m tcp --dport 443 -j KUBE-SVC-5555555555555555 - -A KUBE-PORTALS-HOST -d 10.247.0.10 -p udp -m comment --comment "portal for kube-system/kube-dns:dns" -m state --state NEW -m udp --dport 53 -j KUBE-SVC-6666666666666666 - -A KUBE-PORTALS-HOST -d 10.247.0.10 -p tcp -m comment --comment "portal for kube-system/kube-dns:dns-tcp" -m state --state NEW -m tcp --dport 53 -j KUBE-SVC-2222222222222222 - -A KUBE-SVC-1111111111111111 -p udp -m comment --comment "kube-system/kube-dns:dns" -m recent --set --name KUBE-SVC-1111111111111111 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.246.1.2:53 - -A KUBE-SVC-2222222222222222 -m comment --comment "kube-system/kube-dns:dns-tcp" -j KUBE-SVC-3333333333333333 - -A KUBE-SVC-3333333333333333 -p tcp -m comment --comment "kube-system/kube-dns:dns-tcp" -m recent --set --name KUBE-SVC-3333333333333333 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.246.1.2:53 - -A KUBE-SVC-4444444444444444 -p tcp -m comment --comment "default/kubernetes:" -m recent --set --name KUBE-SVC-4444444444444444 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.245.1.2:443 - -A KUBE-SVC-5555555555555555 -m comment --comment "default/kubernetes:" -j KUBE-SVC-4444444444444444 - -A KUBE-SVC-6666666666666666 -m comment --comment "kube-system/kube-dns:dns" -j KUBE-SVC-1111111111111111 - COMMIT - # Completed on Fri Aug 7 14:47:37 2015 - # Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 - *filter - :INPUT ACCEPT [17514:83115836] - :FORWARD ACCEPT [0:0] - :OUTPUT ACCEPT [8909:688225] - :DOCKER - [0:0] - -A FORWARD -o cbr0 -j DOCKER - -A FORWARD -o cbr0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT - -A FORWARD -i cbr0 ! -o cbr0 -j ACCEPT - -A FORWARD -i cbr0 -o cbr0 -j ACCEPT - COMMIT - ` + iptablesSave := dedent.Dedent( + `# Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 + *nat + :PREROUTING ACCEPT [2:138] + :INPUT ACCEPT [0:0] + :OUTPUT ACCEPT [0:0] + :POSTROUTING ACCEPT [0:0] + :DOCKER - [0:0] + :KUBE-NODEPORT-CONTAINER - [0:0] + :KUBE-NODEPORT-HOST - [0:0] + :KUBE-PORTALS-CONTAINER - [0:0] + :KUBE-PORTALS-HOST - [0:0] + :KUBE-SVC-1111111111111111 - [0:0] + :KUBE-SVC-2222222222222222 - [0:0] + :KUBE-SVC-3333333333333333 - [0:0] + :KUBE-SVC-4444444444444444 - [0:0] + :KUBE-SVC-5555555555555555 - [0:0] + :KUBE-SVC-6666666666666666 - [0:0] + -A PREROUTING -m comment --comment "handle ClusterIPs; NOTE: this must be before the NodePort rules" -j KUBE-PORTALS-CONTAINER + -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER + -A PREROUTING -m addrtype --dst-type LOCAL -m comment --comment "handle service NodePorts; NOTE: this must be the last rule in the chain" -j KUBE-NODEPORT-CONTAINER + -A OUTPUT -m comment --comment "handle ClusterIPs; NOTE: this must be before the NodePort rules" -j KUBE-PORTALS-HOST + -A OUTPUT ! -d 127.0.0.0/8 -m addrtype --dst-type LOCAL -j DOCKER + -A OUTPUT -m addrtype --dst-type LOCAL -m comment --comment "handle service NodePorts; NOTE: this must be the last rule in the chain" -j KUBE-NODEPORT-HOST + -A POSTROUTING -s 10.246.1.0/24 ! -o cbr0 -j MASQUERADE + -A POSTROUTING -s 10.0.2.15 -d 10.0.2.15 -m comment --comment "handle pod connecting to self" -j MASQUERADE + -A KUBE-PORTALS-CONTAINER -d 10.247.0.1 -p tcp -m comment --comment "portal for default/kubernetes:" -m state --state NEW -m tcp --dport 443 -j KUBE-SVC-5555555555555555 + -A KUBE-PORTALS-CONTAINER -d 10.247.0.10 -p udp -m comment --comment "portal for kube-system/kube-dns:dns" -m state --state NEW -m udp --dport 53 -j KUBE-SVC-6666666666666666 + -A KUBE-PORTALS-CONTAINER -d 10.247.0.10 -p tcp -m comment --comment "portal for kube-system/kube-dns:dns-tcp" -m state --state NEW -m tcp --dport 53 -j KUBE-SVC-2222222222222222 + -A KUBE-PORTALS-HOST -d 10.247.0.1 -p tcp -m comment --comment "portal for default/kubernetes:" -m state --state NEW -m tcp --dport 443 -j KUBE-SVC-5555555555555555 + -A KUBE-PORTALS-HOST -d 10.247.0.10 -p udp -m comment --comment "portal for kube-system/kube-dns:dns" -m state --state NEW -m udp --dport 53 -j KUBE-SVC-6666666666666666 + -A KUBE-PORTALS-HOST -d 10.247.0.10 -p tcp -m comment --comment "portal for kube-system/kube-dns:dns-tcp" -m state --state NEW -m tcp --dport 53 -j KUBE-SVC-2222222222222222 + -A KUBE-SVC-1111111111111111 -p udp -m comment --comment "kube-system/kube-dns:dns" -m recent --set --name KUBE-SVC-1111111111111111 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.246.1.2:53 + -A KUBE-SVC-2222222222222222 -m comment --comment "kube-system/kube-dns:dns-tcp" -j KUBE-SVC-3333333333333333 + -A KUBE-SVC-3333333333333333 -p tcp -m comment --comment "kube-system/kube-dns:dns-tcp" -m recent --set --name KUBE-SVC-3333333333333333 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.246.1.2:53 + -A KUBE-SVC-4444444444444444 -p tcp -m comment --comment "default/kubernetes:" -m recent --set --name KUBE-SVC-4444444444444444 --mask 255.255.255.255 --rsource -j DNAT --to-destination 10.245.1.2:443 + -A KUBE-SVC-5555555555555555 -m comment --comment "default/kubernetes:" -j KUBE-SVC-4444444444444444 + -A KUBE-SVC-6666666666666666 -m comment --comment "kube-system/kube-dns:dns" -j KUBE-SVC-1111111111111111 + COMMIT + # Completed on Fri Aug 7 14:47:37 2015 + # Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 + *filter + :INPUT ACCEPT [17514:83115836] + :FORWARD ACCEPT [0:0] + :OUTPUT ACCEPT [8909:688225] + :DOCKER - [0:0] + -A FORWARD -o cbr0 -j DOCKER + -A FORWARD -o cbr0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT + -A FORWARD -i cbr0 ! -o cbr0 -j ACCEPT + -A FORWARD -i cbr0 -o cbr0 -j ACCEPT + COMMIT + `) expected := map[Chain]string{ ChainPrerouting: ":PREROUTING ACCEPT [2:138]", Chain("INPUT"): ":INPUT ACCEPT [0:0]", From 7c27cf0b9bf2daf9e13ce997da6949cf9f0bb593 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 1 Jun 2022 10:58:01 -0400 Subject: [PATCH 3/3] Simplify iptables-save parsing We don't need to parse out the counter values from the iptables-save output (since they are always 0 for the chains we care about). Just parse the chain names themselves. Also, all of the callers of GetChainLines() pass it input that contains only a single table, so just assume that, rather than carefully parsing only a single table's worth of the input. --- pkg/proxy/iptables/proxier.go | 9 +- pkg/util/iptables/save_restore.go | 104 ++++------------------ pkg/util/iptables/save_restore_test.go | 118 ++++++------------------- 3 files changed, 49 insertions(+), 182 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index ae3f86c6756d7..54593353a9256 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -423,7 +423,7 @@ func CleanupLeftovers(ipt utiliptables.Interface) (encounteredError bool) { klog.ErrorS(err, "Failed to execute iptables-save", "table", utiliptables.TableNAT) encounteredError = true } else { - existingNATChains := utiliptables.GetChainLines(utiliptables.TableNAT, iptablesData.Bytes()) + existingNATChains := utiliptables.GetChainsFromTable(iptablesData.Bytes()) natChains := &utilproxy.LineBuffer{} natRules := &utilproxy.LineBuffer{} natChains.Write("*nat") @@ -460,7 +460,7 @@ func CleanupLeftovers(ipt utiliptables.Interface) (encounteredError bool) { klog.ErrorS(err, "Failed to execute iptables-save", "table", utiliptables.TableFilter) encounteredError = true } else { - existingFilterChains := utiliptables.GetChainLines(utiliptables.TableFilter, iptablesData.Bytes()) + existingFilterChains := utiliptables.GetChainsFromTable(iptablesData.Bytes()) filterChains := &utilproxy.LineBuffer{} filterRules := &utilproxy.LineBuffer{} filterChains.Write("*filter") @@ -890,14 +890,13 @@ func (proxier *Proxier) syncProxyRules() { // Get iptables-save output so we can check for existing chains and rules. // This will be a map of chain name to chain with rules as stored in iptables-save/iptables-restore - // IMPORTANT: existingNATChains may share memory with proxier.iptablesData. - existingNATChains := make(map[utiliptables.Chain][]byte) + existingNATChains := make(map[utiliptables.Chain]struct{}) proxier.iptablesData.Reset() err := proxier.iptables.SaveInto(utiliptables.TableNAT, proxier.iptablesData) if err != nil { klog.ErrorS(err, "Failed to execute iptables-save: stale chains will not be deleted") } else { - existingNATChains = utiliptables.GetChainLines(utiliptables.TableNAT, proxier.iptablesData.Bytes()) + existingNATChains = utiliptables.GetChainsFromTable(proxier.iptablesData.Bytes()) } // Reset all buffers used later. diff --git a/pkg/util/iptables/save_restore.go b/pkg/util/iptables/save_restore.go index 7ec11e4f00f14..b788beb91135f 100644 --- a/pkg/util/iptables/save_restore.go +++ b/pkg/util/iptables/save_restore.go @@ -21,102 +21,32 @@ import ( "fmt" ) -var ( - commitBytes = []byte("COMMIT") - spaceBytes = []byte(" ") -) - // MakeChainLine return an iptables-save/restore formatted chain line given a Chain func MakeChainLine(chain Chain) string { return fmt.Sprintf(":%s - [0:0]", chain) } -// GetChainLines parses a table's iptables-save data to find chains in the table. -// It returns a map of iptables.Chain to []byte where the []byte is the chain line -// from save (with counters etc.). -// Note that to avoid allocations memory is SHARED with save. -func GetChainLines(table Table, save []byte) map[Chain][]byte { - chainsMap := make(map[Chain][]byte) - tablePrefix := []byte("*" + string(table)) - readIndex := 0 - // find beginning of table - for readIndex < len(save) { - line, n := readLine(readIndex, save) - readIndex = n - if bytes.HasPrefix(line, tablePrefix) { +// GetChainsFromTable parses iptables-save data to find the chains that are defined. It +// assumes that save contains a single table's data, and returns a map with keys for every +// chain defined in that table. +func GetChainsFromTable(save []byte) map[Chain]struct{} { + chainsMap := make(map[Chain]struct{}) + + for { + i := bytes.Index(save, []byte("\n:")) + if i == -1 { break } - } - // parse table lines - for readIndex < len(save) { - line, n := readLine(readIndex, save) - readIndex = n - if len(line) == 0 { - continue - } - if bytes.HasPrefix(line, commitBytes) || line[0] == '*' { + start := i + 2 + save = save[start:] + end := bytes.Index(save, []byte(" ")) + if i == -1 { + // shouldn't happen, but... break - } else if line[0] == '#' { - continue - } else if line[0] == ':' && len(line) > 1 { - // We assume that the contains space - chain lines have 3 fields, - // space delimited. If there is no space, this line will panic. - spaceIndex := bytes.Index(line, spaceBytes) - if spaceIndex == -1 { - panic(fmt.Sprintf("Unexpected chain line in iptables-save output: %v", string(line))) - } - chain := Chain(line[1:spaceIndex]) - chainsMap[chain] = line } + chain := Chain(save[:end]) + chainsMap[chain] = struct{}{} + save = save[end:] } return chainsMap } - -func readLine(readIndex int, byteArray []byte) ([]byte, int) { - currentReadIndex := readIndex - - // consume left spaces - for currentReadIndex < len(byteArray) { - if byteArray[currentReadIndex] == ' ' { - currentReadIndex++ - } else { - break - } - } - - // leftTrimIndex stores the left index of the line after the line is left-trimmed - leftTrimIndex := currentReadIndex - - // rightTrimIndex stores the right index of the line after the line is right-trimmed - // it is set to -1 since the correct value has not yet been determined. - rightTrimIndex := -1 - - for ; currentReadIndex < len(byteArray); currentReadIndex++ { - if byteArray[currentReadIndex] == ' ' { - // set rightTrimIndex - if rightTrimIndex == -1 { - rightTrimIndex = currentReadIndex - } - } else if (byteArray[currentReadIndex] == '\n') || (currentReadIndex == (len(byteArray) - 1)) { - // end of line or byte buffer is reached - if currentReadIndex <= leftTrimIndex { - return nil, currentReadIndex + 1 - } - // set the rightTrimIndex - if rightTrimIndex == -1 { - rightTrimIndex = currentReadIndex - if currentReadIndex == (len(byteArray)-1) && (byteArray[currentReadIndex] != '\n') { - // ensure that the last character is part of the returned string, - // unless the last character is '\n' - rightTrimIndex = currentReadIndex + 1 - } - } - // Avoid unnecessary allocation. - return byteArray[leftTrimIndex:rightTrimIndex], currentReadIndex + 1 - } else { - // unset rightTrimIndex - rightTrimIndex = -1 - } - } - return nil, currentReadIndex -} diff --git a/pkg/util/iptables/save_restore_test.go b/pkg/util/iptables/save_restore_test.go index 52b4f42051121..2a4c383e12f0b 100644 --- a/pkg/util/iptables/save_restore_test.go +++ b/pkg/util/iptables/save_restore_test.go @@ -22,73 +22,23 @@ import ( "github.com/lithammer/dedent" ) -func TestReadLinesFromByteBuffer(t *testing.T) { - testFn := func(byteArray []byte, expected []string) { - index := 0 - readIndex := 0 - for ; readIndex < len(byteArray); index++ { - line, n := readLine(readIndex, byteArray) - readIndex = n - if expected[index] != string(line) { - t.Errorf("expected:%q, actual:%q", expected[index], line) - } - } // for - if readIndex < len(byteArray) { - t.Errorf("Byte buffer was only partially read. Buffer length is:%d, readIndex is:%d", len(byteArray), readIndex) - } - if index < len(expected) { - t.Errorf("All expected strings were not compared. expected arr length:%d, matched count:%d", len(expected), index-1) +func checkChains(t *testing.T, save []byte, expected map[Chain]struct{}) { + chains := GetChainsFromTable(save) + for chain := range expected { + if _, exists := chains[chain]; !exists { + t.Errorf("GetChainsFromTable expected chain not present: %s", chain) } } - - byteArray1 := []byte("\n Line 1 \n\n\n L ine4 \nLine 5 \n \n") - expected1 := []string{"", "Line 1", "", "", "L ine4", "Line 5", ""} - testFn(byteArray1, expected1) - - byteArray1 = []byte("") - expected1 = []string{} - testFn(byteArray1, expected1) - - byteArray1 = []byte("\n\n") - expected1 = []string{"", ""} - testFn(byteArray1, expected1) -} - -func checkAllLines(t *testing.T, table Table, save []byte, expectedLines map[Chain]string) { - chainLines := GetChainLines(table, save) - for chain, lineBytes := range chainLines { - line := string(lineBytes) - if expected, exists := expectedLines[chain]; exists { - if expected != line { - t.Errorf("getChainLines expected chain line not present. For chain: %s Expected: %s Got: %s", chain, expected, line) - } - } else { - t.Errorf("getChainLines expected chain not present: %s", chain) + for chain := range chains { + if _, exists := expected[chain]; !exists { + t.Errorf("GetChainsFromTable chain unexpectedly present: %s", chain) } } } -func TestGetChainLines(t *testing.T) { - iptablesSave := dedent.Dedent( - `# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014 - *nat - :PREROUTING ACCEPT [2136997:197881818] - :POSTROUTING ACCEPT [4284525:258542680] - :OUTPUT ACCEPT [5901660:357267963] - -A PREROUTING -m addrtype --dst-type LOCAL -j DOCKER - COMMIT - # Completed on Wed Oct 29 14:56:01 2014`) - expected := map[Chain]string{ - ChainPrerouting: ":PREROUTING ACCEPT [2136997:197881818]", - ChainPostrouting: ":POSTROUTING ACCEPT [4284525:258542680]", - ChainOutput: ":OUTPUT ACCEPT [5901660:357267963]", - } - checkAllLines(t, TableNAT, []byte(iptablesSave), expected) -} - -func TestGetChainLinesMultipleTables(t *testing.T) { - iptablesSave := dedent.Dedent( - `# Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 +func TestGetChainsFromTable(t *testing.T) { + iptablesSave := dedent.Dedent(` + # Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 *nat :PREROUTING ACCEPT [2:138] :INPUT ACCEPT [0:0] @@ -126,35 +76,23 @@ func TestGetChainLinesMultipleTables(t *testing.T) { -A KUBE-SVC-5555555555555555 -m comment --comment "default/kubernetes:" -j KUBE-SVC-4444444444444444 -A KUBE-SVC-6666666666666666 -m comment --comment "kube-system/kube-dns:dns" -j KUBE-SVC-1111111111111111 COMMIT - # Completed on Fri Aug 7 14:47:37 2015 - # Generated by iptables-save v1.4.21 on Fri Aug 7 14:47:37 2015 - *filter - :INPUT ACCEPT [17514:83115836] - :FORWARD ACCEPT [0:0] - :OUTPUT ACCEPT [8909:688225] - :DOCKER - [0:0] - -A FORWARD -o cbr0 -j DOCKER - -A FORWARD -o cbr0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT - -A FORWARD -i cbr0 ! -o cbr0 -j ACCEPT - -A FORWARD -i cbr0 -o cbr0 -j ACCEPT - COMMIT `) - expected := map[Chain]string{ - ChainPrerouting: ":PREROUTING ACCEPT [2:138]", - Chain("INPUT"): ":INPUT ACCEPT [0:0]", - Chain("OUTPUT"): ":OUTPUT ACCEPT [0:0]", - ChainPostrouting: ":POSTROUTING ACCEPT [0:0]", - Chain("DOCKER"): ":DOCKER - [0:0]", - Chain("KUBE-NODEPORT-CONTAINER"): ":KUBE-NODEPORT-CONTAINER - [0:0]", - Chain("KUBE-NODEPORT-HOST"): ":KUBE-NODEPORT-HOST - [0:0]", - Chain("KUBE-PORTALS-CONTAINER"): ":KUBE-PORTALS-CONTAINER - [0:0]", - Chain("KUBE-PORTALS-HOST"): ":KUBE-PORTALS-HOST - [0:0]", - Chain("KUBE-SVC-1111111111111111"): ":KUBE-SVC-1111111111111111 - [0:0]", - Chain("KUBE-SVC-2222222222222222"): ":KUBE-SVC-2222222222222222 - [0:0]", - Chain("KUBE-SVC-3333333333333333"): ":KUBE-SVC-3333333333333333 - [0:0]", - Chain("KUBE-SVC-4444444444444444"): ":KUBE-SVC-4444444444444444 - [0:0]", - Chain("KUBE-SVC-5555555555555555"): ":KUBE-SVC-5555555555555555 - [0:0]", - Chain("KUBE-SVC-6666666666666666"): ":KUBE-SVC-6666666666666666 - [0:0]", + expected := map[Chain]struct{}{ + ChainPrerouting: {}, + Chain("INPUT"): {}, + Chain("OUTPUT"): {}, + ChainPostrouting: {}, + Chain("DOCKER"): {}, + Chain("KUBE-NODEPORT-CONTAINER"): {}, + Chain("KUBE-NODEPORT-HOST"): {}, + Chain("KUBE-PORTALS-CONTAINER"): {}, + Chain("KUBE-PORTALS-HOST"): {}, + Chain("KUBE-SVC-1111111111111111"): {}, + Chain("KUBE-SVC-2222222222222222"): {}, + Chain("KUBE-SVC-3333333333333333"): {}, + Chain("KUBE-SVC-4444444444444444"): {}, + Chain("KUBE-SVC-5555555555555555"): {}, + Chain("KUBE-SVC-6666666666666666"): {}, } - checkAllLines(t, TableNAT, []byte(iptablesSave), expected) + checkChains(t, []byte(iptablesSave), expected) }