From 82dc2c5894e95b29422834eb2d525a276037436b Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Wed, 19 Jan 2022 00:18:45 -0800 Subject: [PATCH 1/2] fix: detect multi-digit line number --- .../policies/chain-management_linux.go | 18 ++++++-- .../policies/chain-management_linux_test.go | 43 +++++++++++++------ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index d617ebca21..38e36cbb0b 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -3,6 +3,7 @@ package policies // This file contains code for booting up and reconciling iptables import ( + "bytes" "errors" "fmt" "strconv" @@ -53,6 +54,10 @@ var ( util.IptablesNewState, } + spaceByte = []byte(" ") + errNoLineNumber = errors.New("no line number found") + errUnexpectedLineNumberString = errors.New("unexpected line number string") + errInvalidGrepResult = errors.New("unexpectedly got no lines while grepping for current Azure chains") deprecatedJumpFromForwardToAzureChainArgs = []string{ util.IptablesForwardChain, @@ -380,10 +385,17 @@ func (pMgr *PolicyManager) chainLineNumber(chain string) (int, error) { return 0, nil } if len(searchResults) >= minLineNumberStringLength { - lineNum, _ := strconv.Atoi(string(searchResults[0])) // FIXME this returns the first digit of the line number. What if the chain was at line 11? Then we would think it's at line 1 - return lineNum, nil + firstSpaceIndex := bytes.Index(searchResults, spaceByte) + if firstSpaceIndex > 0 && firstSpaceIndex < len(searchResults) { + lineNumberString := string(searchResults[0:firstSpaceIndex]) + lineNum, err := strconv.Atoi(lineNumberString) + if err != nil { + return 0, errNoLineNumber + } + return lineNum, nil + } } - return 0, nil + return 0, errUnexpectedLineNumberString } func (pMgr *PolicyManager) allCurrentAzureChains() (map[string]struct{}, error) { diff --git a/npm/pkg/dataplane/policies/chain-management_linux_test.go b/npm/pkg/dataplane/policies/chain-management_linux_test.go index 4b3f6c03b5..843794ff3c 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux_test.go +++ b/npm/pkg/dataplane/policies/chain-management_linux_test.go @@ -385,10 +385,7 @@ func TestBootupLinux(t *testing.T) { }, fakeIPTablesRestoreCommand, {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, - { - Cmd: []string{"grep", "AZURE-NPM"}, - Stdout: grepOutputAzureV1Chains, - }, + {Cmd: []string{"grep", "AZURE-NPM"}, ExitCode: 1}, {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, }, wantErr: false, @@ -404,10 +401,7 @@ func TestBootupLinux(t *testing.T) { }, fakeIPTablesRestoreCommand, {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, - { - Cmd: []string{"grep", "AZURE-NPM"}, - Stdout: grepOutputAzureV1Chains, - }, + {Cmd: []string{"grep", "AZURE-NPM"}, ExitCode: 1}, {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, }, wantErr: false, @@ -579,15 +573,14 @@ func TestChainLineNumber(t *testing.T) { {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, { Cmd: []string{"grep", testChainName}, - Stdout: fmt.Sprintf("3 %s all -- 0.0.0.0/0 0.0.0.0/0 ", testChainName), + Stdout: fmt.Sprintf("12 %s all -- 0.0.0.0/0 0.0.0.0/0 ", testChainName), }, }, - expectedLineNum: 3, + expectedLineNum: 12, wantErr: false, }, - // TODO test for chain line number with 2+ digits { - name: "ignore unexpected grep output", + name: "unexpected grep output (too short)", calls: []testutils.TestCmd{ {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, { @@ -596,7 +589,31 @@ func TestChainLineNumber(t *testing.T) { }, }, expectedLineNum: 0, - wantErr: false, + wantErr: true, + }, + { + name: "unexpected grep output (no space)", + calls: []testutils.TestCmd{ + {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, + { + Cmd: []string{"grep", testChainName}, + Stdout: "345678", + }, + }, + expectedLineNum: 0, + wantErr: true, + }, + { + name: "unexpected grep output (no line number)", + calls: []testutils.TestCmd{ + {Cmd: listLineNumbersCommandStrings, PipedToCommand: true}, + { + Cmd: []string{"grep", testChainName}, + Stdout: "unexpected stuff", + }, + }, + expectedLineNum: 0, + wantErr: true, }, { name: "chain doesn't exist", From 484848325e6fa86c36b43caa93360a68cdf7eef1 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Wed, 19 Jan 2022 15:15:24 -0800 Subject: [PATCH 2/2] fix bug in v1 --- npm/iptm/iptm.go | 58 +++++++++++------- npm/iptm/iptm_test.go | 60 ++++++++++++++++--- .../policies/chain-management_linux.go | 3 +- 3 files changed, 88 insertions(+), 33 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 3b8a81f87f..fb7821bd8c 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -4,6 +4,7 @@ package iptm import ( + "bytes" "fmt" "strconv" "strings" @@ -20,29 +21,34 @@ const ( defaultlockWaitTimeInSeconds string = "60" iptablesErrDoesNotExist int = 1 reconcileChainTimeInMinutes = 5 + minLineNumberStringLength int = 3 ) -// IptablesAzureChainList contains list of all NPM chains -var IptablesAzureChainList = []string{ - util.IptablesAzureChain, - util.IptablesAzureAcceptChain, - util.IptablesAzureIngressChain, - util.IptablesAzureEgressChain, - util.IptablesAzureIngressPortChain, - util.IptablesAzureIngressFromChain, - util.IptablesAzureEgressPortChain, - util.IptablesAzureEgressToChain, - util.IptablesAzureIngressDropsChain, - util.IptablesAzureEgressDropsChain, -} - -var deprecatedJumpToAzureEntry = &IptEntry{ - Chain: util.IptablesForwardChain, - Specs: []string{ - util.IptablesJumpFlag, +var ( + // IptablesAzureChainList contains list of all NPM chains + IptablesAzureChainList = []string{ util.IptablesAzureChain, - }, -} + util.IptablesAzureAcceptChain, + util.IptablesAzureIngressChain, + util.IptablesAzureEgressChain, + util.IptablesAzureIngressPortChain, + util.IptablesAzureIngressFromChain, + util.IptablesAzureEgressPortChain, + util.IptablesAzureEgressToChain, + util.IptablesAzureIngressDropsChain, + util.IptablesAzureEgressDropsChain, + } + + deprecatedJumpToAzureEntry = &IptEntry{ + Chain: util.IptablesForwardChain, + Specs: []string{ + util.IptablesJumpFlag, + util.IptablesAzureChain, + }, + } + + spaceByte = []byte(" ") +) // IptEntry represents an iptables rule. type IptEntry struct { @@ -431,9 +437,15 @@ func (iptMgr *IptablesManager) getChainLineNumber(chain string, parentChain stri return 0, nil } - if len(output) > 2 { - lineNum, _ := strconv.Atoi(string(output[0])) - return lineNum, nil + // NOTE: v2 has different behavior for unexpected grep outputs (v2 will throw an error) + // want to fix the bug here (not detecting numbers with 2+ digits), but don't want to modify the error-throwing behavior + if len(output) >= minLineNumberStringLength { + firstSpaceIndex := bytes.Index(output, spaceByte) + if firstSpaceIndex > 0 && firstSpaceIndex < len(output) { + lineNumberString := string(output[0:firstSpaceIndex]) + lineNum, _ := strconv.Atoi(lineNumberString) + return lineNum, nil + } } return 0, nil } diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index fbeb95603a..b7126bd73d 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -420,18 +420,60 @@ func TestRun(t *testing.T) { } func TestGetChainLineNumber(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 AZURE-NPM all -- 0.0.0.0/0 0.0.0.0/0 "}, // expected output from iptables - {Cmd: []string{"grep", "AZURE-NPM"}}, + tests := []struct { + name string + exitCode int + stdout string + want int + }{ + { + name: "no match", + exitCode: 1, + want: 0, + }, + { + name: "match", + exitCode: 0, + stdout: "24 AZURE-NPM all --", + want: 24, + }, + { + name: "unexpected output (no line number)", + exitCode: 0, + stdout: "no line number", + want: 0, + }, + { + name: "unexpected output (no space)", + exitCode: 0, + stdout: "123456", + want: 0, + }, + { + name: "unexpected output (too short)", + exitCode: 0, + stdout: "12", + want: 0, + }, } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, PipedToCommand: true}, + {Cmd: []string{"grep", "AZURE-NPM"}, Stdout: tt.stdout, ExitCode: tt.exitCode}, + } + fexec := testutils.GetFakeExecWithScripts(calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + + lineNum, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) + require.NoError(t, err) + require.Equal(t, tt.want, lineNum) - lineNum, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) - require.NoError(t, err) - require.Equal(t, lineNum, 3) + testutils.VerifyCalls(t, fexec, calls) + }) + } } func TestMain(m *testing.M) { diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index 38e36cbb0b..68b30f4724 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -24,7 +24,8 @@ const ( doesNotExistErrorCode int = 1 // Bad rule (does a matching rule exist in that chain?) couldntLoadTargetErrorCode int = 2 // Couldn't load target `AZURE-NPM-EGRESS':No such file or directory - minLineNumberStringLength int = 3 // TODO transferred from iptm.go and not sure why this length is important, but will update the function its used in later anyways + // transferred from iptm.go and not sure why this length is important + minLineNumberStringLength int = 3 azureChainGrepPattern string = "Chain AZURE-NPM" minAzureChainNameLength int = len("AZURE-NPM")