Skip to content

fix: [NPM] detect multi-digit numbers when getting iptables line number #1199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 24, 2022
Merged

fix: [NPM] detect multi-digit numbers when getting iptables line number #1199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82dc2c5
fix: detect multi-digit line number
huntergregory Jan 19, 2022
4848483
fix bug in v1
huntergregory Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 35 additions & 23 deletions npm/iptm/iptm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package iptm

import (
"bytes"
"fmt"
"strconv"
"strings"
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
60 changes: 51 additions & 9 deletions npm/iptm/iptm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
21 changes: 17 additions & 4 deletions npm/pkg/dataplane/policies/chain-management_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package policies
// This file contains code for booting up and reconciling iptables

import (
"bytes"
"errors"
"fmt"
"strconv"
Expand All @@ -23,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")
Expand Down Expand Up @@ -53,6 +55,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,
Expand Down Expand Up @@ -380,10 +386,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) {
Expand Down
43 changes: 30 additions & 13 deletions npm/pkg/dataplane/policies/chain-management_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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},
{
Expand All @@ -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",
Expand Down