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

@@ -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]struct{})
-	existingNATChains := make(map[utiliptables.Chain][]byte)
 	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.

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.
+// GetChainsFromTable parses iptables-save data to find the chains that are defined. It
-// It returns a map of iptables.Chain to []byte where the []byte is the chain line
+// assumes that save contains a single table's data, and returns a map with keys for every
-// from save (with counters etc.).
+// chain defined in that table.
-// Note that to avoid allocations memory is SHARED with save.
+func GetChainsFromTable(save []byte) map[Chain]struct{} {
-func GetChainLines(table Table, save []byte) map[Chain][]byte {
+	chainsMap := make(map[Chain]struct{})
-	chainsMap := make(map[Chain][]byte)
+
-	tablePrefix := []byte("*" + string(table))
+	for {
-	readIndex := 0
+		i := bytes.Index(save, []byte("\n:"))
-	// find beginning of table
+		if i == -1 {
-	for readIndex < len(save) {
-		line, n := readLine(readIndex, save)
-		readIndex = n
-		if bytes.HasPrefix(line, tablePrefix) {
 			break
 		}
-	}
+		start := i + 2
-	// parse table lines
+		save = save[start:]
-	for readIndex < len(save) {
+		end := bytes.Index(save, []byte(" "))
-		line, n := readLine(readIndex, save)
+		if i == -1 {
-		readIndex = n
+			// shouldn't happen, but...
-		if len(line) == 0 {
-			continue
-		}
-		if bytes.HasPrefix(line, commitBytes) || line[0] == '*' {
 			break
-		} else if line[0] == '#' {
-			continue
-		} else if line[0] == ':' && len(line) > 1 {
-			// We assume that the <line> 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
-}

pkg/util/iptables/save_restore_test.go

@@ -22,73 +22,23 @@ import (
 	"github.com/lithammer/dedent"
 )
 
-func TestReadLinesFromByteBuffer(t *testing.T) {
+func checkChains(t *testing.T, save []byte, expected map[Chain]struct{}) {
-	testFn := func(byteArray []byte, expected []string) {
+	chains := GetChainsFromTable(save)
-		index := 0
+	for chain := range expected {
-		readIndex := 0
+		if _, exists := chains[chain]; !exists {
-		for ; readIndex < len(byteArray); index++ {
+			t.Errorf("GetChainsFromTable expected chain not present: %s", chain)
-			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)
 		}
 	}
-
+	for chain := range chains {
-	byteArray1 := []byte("\n  Line 1  \n\n\n L ine4  \nLine 5 \n \n")
+		if _, exists := expected[chain]; !exists {
-	expected1 := []string{"", "Line 1", "", "", "L ine4", "Line 5", ""}
+			t.Errorf("GetChainsFromTable chain unexpectedly present: %s", chain)
-	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)
 		}
 	}
 }
 
-func TestGetChainLines(t *testing.T) {
+func TestGetChainsFromTable(t *testing.T) {
-	iptablesSave := dedent.Dedent(
+	iptablesSave := dedent.Dedent(`
-		`# Generated by iptables-save v1.4.7 on Wed Oct 29 14:56:01 2014
+		# Generated by iptables-save v1.4.21 on Fri Aug  7 14:47:37 2015
-		*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
 		*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{
+	expected := map[Chain]struct{}{
-		ChainPrerouting:                    ":PREROUTING ACCEPT [2:138]",
+		ChainPrerouting:                    {},
-		Chain("INPUT"):                     ":INPUT ACCEPT [0:0]",
+		Chain("INPUT"):                     {},
-		Chain("OUTPUT"):                    ":OUTPUT ACCEPT [0:0]",
+		Chain("OUTPUT"):                    {},
-		ChainPostrouting:                   ":POSTROUTING ACCEPT [0:0]",
+		ChainPostrouting:                   {},
-		Chain("DOCKER"):                    ":DOCKER - [0:0]",
+		Chain("DOCKER"):                    {},
-		Chain("KUBE-NODEPORT-CONTAINER"):   ":KUBE-NODEPORT-CONTAINER - [0:0]",
+		Chain("KUBE-NODEPORT-CONTAINER"):   {},
-		Chain("KUBE-NODEPORT-HOST"):        ":KUBE-NODEPORT-HOST - [0:0]",
+		Chain("KUBE-NODEPORT-HOST"):        {},
-		Chain("KUBE-PORTALS-CONTAINER"):    ":KUBE-PORTALS-CONTAINER - [0:0]",
+		Chain("KUBE-PORTALS-CONTAINER"):    {},
-		Chain("KUBE-PORTALS-HOST"):         ":KUBE-PORTALS-HOST - [0:0]",
+		Chain("KUBE-PORTALS-HOST"):         {},
-		Chain("KUBE-SVC-1111111111111111"): ":KUBE-SVC-1111111111111111 - [0:0]",
+		Chain("KUBE-SVC-1111111111111111"): {},
-		Chain("KUBE-SVC-2222222222222222"): ":KUBE-SVC-2222222222222222 - [0:0]",
+		Chain("KUBE-SVC-2222222222222222"): {},
-		Chain("KUBE-SVC-3333333333333333"): ":KUBE-SVC-3333333333333333 - [0:0]",
+		Chain("KUBE-SVC-3333333333333333"): {},
-		Chain("KUBE-SVC-4444444444444444"): ":KUBE-SVC-4444444444444444 - [0:0]",
+		Chain("KUBE-SVC-4444444444444444"): {},
-		Chain("KUBE-SVC-5555555555555555"): ":KUBE-SVC-5555555555555555 - [0:0]",
+		Chain("KUBE-SVC-5555555555555555"): {},
-		Chain("KUBE-SVC-6666666666666666"): ":KUBE-SVC-6666666666666666 - [0:0]",
+		Chain("KUBE-SVC-6666666666666666"): {},
 	}
-	checkAllLines(t, TableNAT, []byte(iptablesSave), expected)
+	checkChains(t, []byte(iptablesSave), expected)
 }