Skip to content

fix: [NPM] v2 backwards compatibility #1154

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 3 commits into from
Dec 14, 2021
Merged

fix: [NPM] v2 backwards compatibility #1154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc0d1f
delete deprecated jump to azure chain
huntergregory Dec 3, 2021
ecd01b0
fix UT
huntergregory Dec 8, 2021
2c20c66
fix logs
huntergregory Dec 10, 2021
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
41 changes: 34 additions & 7 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 @@ -46,6 +46,11 @@ var (
util.IptablesCtstateFlag,
util.IptablesNewState,
}
deprecatedJumpFromForwardToAzureChainArgs = []string{
util.IptablesForwardChain,
util.IptablesJumpFlag,
util.IptablesAzureChain,
}
)

type staleChains struct {
Expand Down Expand Up @@ -130,16 +135,37 @@ func (pMgr *PolicyManager) initializeNPMChains() error {
// removeNPMChains removes the jump rule from FORWARD chain to AZURE-NPM chain
// and flushes and deletes all NPM Chains.
func (pMgr *PolicyManager) removeNPMChains() error {
// 1.1 delete the deprecated jump rule from FORWARD chain to AZURE-NPM chain, if it exists
deprecatedErrCode, deprecatedErr := pMgr.runIPTablesCommand(util.IptablesDeletionFlag, deprecatedJumpFromForwardToAzureChainArgs...)
if deprecatedErr == nil {
klog.Infof("deleted deprecated jump rule from FORWARD chain to AZURE-NPM chain")
} else {
switch deprecatedErrCode {
case couldntLoadTargetErrorCode:
// couldntLoadTargetErrorCode happens when AZURE-NPM chain doesn't exist (and hence the jump rule doesn't exist too)
klog.Infof("didn't delete deprecated jump rule from FORWARD chain to AZURE-NPM chain likely because AZURE-NPM chain doesn't exist. Exit code %d and error: %s", deprecatedErrCode, deprecatedErr)
case doesNotExistErrorCode:
// doesNotExistErrorCode happens when AZURE-NPM chain exists, but this jump rule doesn't exist
klog.Infof("didn't delete deprecated jump rule from FORWARD chain to AZURE-NPM chain likely because NPM v1 was not used prior. Exit code %d and error: %s", deprecatedErrCode, deprecatedErr)
default:
klog.Errorf("failed to delete deprecated jump rule from FORWARD chain to AZURE-NPM chain for unexpected reason with exit code %d and error: %s", deprecatedErrCode, deprecatedErr.Error())
}
}

// 1.2 delete the jump rule that has ctstate NEW
deleteErrCode, deleteErr := pMgr.runIPTablesCommand(util.IptablesDeletionFlag, jumpFromForwardToAzureChainArgs...)
// couldntLoadTargetErrorCode happens when AZURE-NPM chain doesn't exist (and hence the jump rule doesn't exist too)
// we can ignore this error code, since there's no problem if the rule doesn't exist
hadDeleteError := deleteErr != nil && deleteErrCode != couldntLoadTargetErrorCode
if hadDeleteError {
// log as an error because this is unexpected, but don't return an error because for example, we could have AZURE-NPM chain exists but the jump to it doesn't exist
metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to delete jump from FORWARD chain to AZURE-NPM chain with exit code %d and error: %s", deleteErrCode, deleteErr.Error())
// FIXME update ID
if deleteErr != nil {
switch deleteErrCode {
case couldntLoadTargetErrorCode:
klog.Infof("didn't delete jump from FORWARD chain to AZURE-NPM chain likely because AZURE-NPM chain doesn't exist. Exit code %d and error: %s", deleteErrCode, deleteErr.Error())
case doesNotExistErrorCode:
klog.Infof("didn't delete jump from FORWARD chain to AZURE-NPM chain likely because we're transitioning from v1.4.9 or older. Exit code %d and error: %s", deleteErrCode, deleteErr.Error())
default:
klog.Errorf("failed to delete jump from FORWARD chain to AZURE-NPM chain for unexpected reason with exit code %d and error: %s", deleteErrCode, deleteErr.Error())
}
}

// 2. flush current NPM chains
creatorToFlush, chainsToDelete := pMgr.creatorAndChainsForReset()
if len(chainsToDelete) == 0 {
return nil
Expand All @@ -149,6 +175,7 @@ func (pMgr *PolicyManager) removeNPMChains() error {
return npmerrors.SimpleErrorWrapper("failed to flush chains", restoreError)
}

// 3. delete current NPM chains
// TODO aggregate an error for each chain that failed to delete
var anyDeleteErr error
for _, chainName := range chainsToDelete {
Expand Down
75 changes: 63 additions & 12 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 @@ -193,6 +193,7 @@ func TestRemoveNPMChains(t *testing.T) {
{
name: "success when there are chains currently",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{
Expand All @@ -211,27 +212,17 @@ func TestRemoveNPMChains(t *testing.T) {
{
name: "success when there are no chains currently",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "no error on failure to delete",
calls: []testutils.TestCmd{
{
Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"},
ExitCode: 1, // delete failure
},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "failure on restore",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{
Expand All @@ -245,6 +236,7 @@ func TestRemoveNPMChains(t *testing.T) {
{
name: "failure on destroy",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{
Expand All @@ -260,6 +252,65 @@ func TestRemoveNPMChains(t *testing.T) {
},
wantErr: true,
},
{
name: "does not exist error while deleting jump",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{
Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"},
ExitCode: 1, // does not exist error code
},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "no AZURE-NPM chain while deleting both jumps",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 2}, // same code as the next command
{
Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"},
ExitCode: 2, // couldn't load target error code
},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "unknown error while deleting jump",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, // deprecated rule did not exist
{
Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"},
ExitCode: 3, // unknown delete failure
},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "successfully delete deprecated jump",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}}, // deprecated rule existed
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
{
name: "unknown error while deleting deprecated jump",
calls: []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 3},
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{Cmd: []string{"grep", "Chain AZURE-NPM"}, ExitCode: 1},
},
wantErr: false,
},
}
for _, tt := range tests {
tt := tt
Expand Down
1 change: 1 addition & 0 deletions npm/pkg/dataplane/policies/testutils_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ func GetInitializeTestCalls() []testutils.TestCmd {

func GetResetTestCalls() []testutils.TestCmd {
return []testutils.TestCmd{
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}},
{Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}},
{Cmd: listPolicyChainNamesCommandStrings, PipedToCommand: true},
{
Expand Down