From fad411133bc694be17d355d31b12a608fb710ac7 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 4 Nov 2021 15:23:38 -0700 Subject: [PATCH 01/15] put jump from forward to azure-npm chain above the one to kube-services --- npm/iptm/iptm.go | 43 +------------------------------------------ 1 file changed, 1 insertion(+), 42 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index c0e366e722..41497f9cd0 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -204,15 +204,6 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { }, } - // retrieve KUBE-SERVICES index - kubeServicesLine, err := iptMgr.getChainLineNumber(util.IptablesKubeServicesChain, util.IptablesForwardChain) - if err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to get index of KUBE-SERVICES in FORWARD chain with error: %s", err.Error()) - return err - } - - index := kubeServicesLine + 1 - exists, err := iptMgr.exists(entry) if err != nil { return err @@ -221,6 +212,7 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { if !exists { // position Azure-NPM chain after Kube-Forward and Kube-Service chains if it exists iptMgr.OperationFlag = util.IptablesInsertionFlag + index := 1 entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) if _, err = iptMgr.run(entry); err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain.") @@ -230,39 +222,6 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return nil } - npmChainLine, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) - if err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to get index of AZURE-NPM in FORWARD chain with error: %s", err.Error()) - return err - } - - // Kube-services line number is less than npm chain line number then all good - if kubeServicesLine < npmChainLine { - return nil - } else if kubeServicesLine <= 0 { - return nil - } - - errCode := 0 - // NPM Chain number is less than KUBE-SERVICES then - // delete existing NPM chain and add it in the right order - iptMgr.OperationFlag = util.IptablesDeletionFlag - metrics.SendErrorLogAndMetric(util.IptmID, "Info: Reconciler deleting and re-adding AZURE-NPM in FORWARD table.") - if errCode, err = iptMgr.run(entry); err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to delete AZURE-NPM chain from FORWARD chain with error code %d.", errCode) - return err - } - iptMgr.OperationFlag = util.IptablesInsertionFlag - // Reduce index for deleted AZURE-NPM chain - if index > 1 { - index-- - } - entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) - if errCode, err = iptMgr.run(entry); err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain with error code %d.", errCode) - return err - } - return nil } From 6ba442a7bba572e4059e570b1282e36662f6bcdb Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 4 Nov 2021 17:24:44 -0700 Subject: [PATCH 02/15] update unit test --- npm/iptm/iptm_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index ffe373e144..4b6df4e1ab 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -25,14 +25,8 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL - {Cmd: []string{"grep", "KUBE-SERVICES"}, Stdout: "4 "}, - - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL - {Cmd: []string{"grep", "AZURE-NPM"}, Stdout: "4 "}, - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, From a3fc86fdc79edd12cadce04f3f9f7fe88fa4e6da Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 5 Nov 2021 16:46:49 -0700 Subject: [PATCH 03/15] add toggle for chain position --- npm/config/config.go | 18 ++++++++++-------- npm/iptm/iptm.go | 16 +++++++++------- npm/npm.go | 2 +- .../controllers/v1/networkPolicyController.go | 4 ++-- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/npm/config/config.go b/npm/config/config.go index b7266f3235..d5658a7c06 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -14,10 +14,11 @@ var DefaultConfig = Config{ ListeningPort: defaultListeningPort, ListeningAddress: "0.0.0.0", Toggles: Toggles{ - EnablePrometheusMetrics: true, - EnablePprof: true, - EnableHTTPDebugAPI: true, - EnableV2Controllers: false, + EnablePrometheusMetrics: true, + EnablePprof: true, + EnableHTTPDebugAPI: true, + EnableV2Controllers: false, + PlaceAzureChainBeforeKubeForward: false, }, } @@ -29,8 +30,9 @@ type Config struct { } type Toggles struct { - EnablePrometheusMetrics bool - EnablePprof bool - EnableHTTPDebugAPI bool - EnableV2Controllers bool + EnablePrometheusMetrics bool + EnablePprof bool + EnableHTTPDebugAPI bool + EnableV2Controllers bool + PlaceAzureChainBeforeKubeForward bool } diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 41497f9cd0..85ae234077 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -48,9 +48,10 @@ type IptEntry struct { // IptablesManager stores iptables entries. type IptablesManager struct { - exec utilexec.Interface - io ioshim - OperationFlag string + exec utilexec.Interface + io ioshim + OperationFlag string + placeAzureChainBeforeKubeForward bool } func isDropsChain(chainName string) bool { @@ -63,11 +64,12 @@ func isDropsChain(chainName string) bool { } // NewIptablesManager creates a new instance for IptablesManager object. -func NewIptablesManager(exec utilexec.Interface, io ioshim) *IptablesManager { +func NewIptablesManager(exec utilexec.Interface, io ioshim, placeAzureChainBeforeKubeForward bool) *IptablesManager { iptMgr := &IptablesManager{ - exec: exec, - io: io, - OperationFlag: "", + exec: exec, + io: io, + OperationFlag: "", + placeAzureChainBeforeKubeForward: placeAzureChainBeforeKubeForward, } return iptMgr diff --git a/npm/npm.go b/npm/npm.go index 7404be5bd5..1d093920aa 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -116,7 +116,7 @@ func NewNetworkPolicyManager(config npmconfig.Config, // create NameSpace controller npMgr.namespaceControllerV1 = controllersv1.NewNameSpaceController(npMgr.nsInformer, npMgr.ipsMgr, npMgr.npmNamespaceCacheV1) // create network policy controller - npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr) + npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr, config.Toggles.PlaceAzureChainBeforeKubeForward) return npMgr } diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go index b42d1f9085..a3e259d9cc 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go @@ -43,7 +43,7 @@ type NetworkPolicyController struct { iptMgr *iptm.IptablesManager } -func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager) *NetworkPolicyController { +func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager, placeAzureChainBeforeKubeForward bool) *NetworkPolicyController { netPolController := &NetworkPolicyController{ netPolLister: npInformer.Lister(), workqueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "NetworkPolicy"), @@ -51,7 +51,7 @@ func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInfo // ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), isAzureNpmChainCreated: false, ipsMgr: ipsMgr, - iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim()), + iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim(), placeAzureChainBeforeKubeForward), } npInformer.Informer().AddEventHandler( From 4913cbf85d0aad59c9de8c17017f69e0b7a97cfc Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 8 Nov 2021 19:18:31 -0800 Subject: [PATCH 04/15] incorporate toggle in iptm and update UTs. v1 controller tests seem broken --- npm/iptm/iptm.go | 51 ++++++++++- npm/iptm/iptm_test.go | 86 ++++++++++++++++--- npm/npm_v1_test.go | 3 +- .../controllers/v1/controllers_v1_test.go | 4 +- .../v1/networkPolicyController_test.go | 3 +- 5 files changed, 131 insertions(+), 16 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 85ae234077..84286b16b6 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -206,6 +206,19 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { }, } + index := 1 // insert the jump to AZURE-NPM at the top + kubeServicesLine := 0 + if !iptMgr.placeAzureChainBeforeKubeForward { + // retrieve KUBE-SERVICES index + var err error + kubeServicesLine, err = iptMgr.getChainLineNumber(util.IptablesKubeServicesChain, util.IptablesForwardChain) + if err != nil { + metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to get index of KUBE-SERVICES in FORWARD chain with error: %s", err.Error()) + return err + } + index = kubeServicesLine + 1 // insert the jump to AZURE-NPM after the jump to KUBE-SERVICES + } + exists, err := iptMgr.exists(entry) if err != nil { return err @@ -214,7 +227,6 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { if !exists { // position Azure-NPM chain after Kube-Forward and Kube-Service chains if it exists iptMgr.OperationFlag = util.IptablesInsertionFlag - index := 1 entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) if _, err = iptMgr.run(entry); err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain.") @@ -224,6 +236,43 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return nil } + if iptMgr.placeAzureChainBeforeKubeForward { + return nil + } + + npmChainLine, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) + if err != nil { + metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to get index of AZURE-NPM in FORWARD chain with error: %s", err.Error()) + return err + } + + // Kube-services line number is less than npm chain line number then all good + if kubeServicesLine < npmChainLine { + return nil + } else if kubeServicesLine <= 0 { + return nil + } + + errCode := 0 + // NPM Chain number is less than KUBE-SERVICES then + // delete existing NPM chain and add it in the right order + iptMgr.OperationFlag = util.IptablesDeletionFlag + metrics.SendErrorLogAndMetric(util.IptmID, "Info: Reconciler deleting and re-adding AZURE-NPM in FORWARD table.") + if errCode, err = iptMgr.run(entry); err != nil { + metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to delete AZURE-NPM chain from FORWARD chain with error code %d.", errCode) + return err + } + iptMgr.OperationFlag = util.IptablesInsertionFlag + // Reduce index for deleted AZURE-NPM chain + if index > 1 { + index-- + } + entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) + if errCode, err = iptMgr.run(entry); err != nil { + metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain with error code %d.", errCode) + return err + } + return nil } diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 4b6df4e1ab..7d781996df 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -11,6 +11,11 @@ import ( "github.com/stretchr/testify/require" ) +const ( + placeAzureChainAfterKubeServices = false + placeAzureChainFirst = true +) + var ( initCalls = []testutils.TestCmd{ {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, @@ -25,8 +30,14 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL + {Cmd: []string{"grep", "KUBE-SERVICES"}, Stdout: "4 "}, + + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL + {Cmd: []string{"grep", "AZURE-NPM"}, Stdout: "4 "}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, @@ -81,6 +92,48 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-X", "AZURE-NPM-TARGET-SETS"}}, {Cmd: []string{"iptables", "-w", "60", "-X", "AZURE-NPM-INRGESS-DROPS"}}, // can we delete this rule now? } + + initWithJumpToAzureAtTopCalls = []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-ACCEPT"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-INGRESS"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-INGRESS-PORT"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-INGRESS-FROM"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-PORT"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-TO"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-INGRESS-DROPS"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM"}}, + + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-ACCEPT", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "ACCEPT-on-INGRESS-and-EGRESS-mark-0x3000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-ACCEPT", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "ACCEPT-on-INGRESS-mark-0x2000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-ACCEPT", "-m", "mark", "--mark", "0x1000", "-m", "comment", "--comment", "ACCEPT-on-EGRESS-mark-0x1000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-m", "state", "--state", "RELATED,ESTABLISHED", "-j", "ACCEPT", "-m", "comment", "--comment", "ACCEPT-on-connection-state"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-ACCEPT", "-j", "MARK", "--set-mark", "0x0", "-m", "comment", "--comment", "Clear-AZURE-NPM-MARKS"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-ACCEPT", "-j", "ACCEPT", "-m", "comment", "--comment", "ACCEPT-All-packets"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "AZURE-NPM-INGRESS-PORT"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "AZURE-NPM-INGRESS-DROPS"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, + /////////// + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "AZURE-NPM-INGRESS-FROM", "-m", "comment", "--comment", "ALL-JUMP-TO-AZURE-NPM-INGRESS-FROM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "AZURE-NPM-EGRESS-PORT"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "RETURN-on-EGRESS-and-INGRESS-mark-0x3000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x1000", "-m", "comment", "--comment", "RETURN-on-EGRESS-mark-0x1000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "AZURE-NPM-EGRESS-DROPS"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS-PORT", "-j", "RETURN", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "RETURN-on-EGRESS-and-INGRESS-mark-0x3000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS-PORT", "-j", "RETURN", "-m", "mark", "--mark", "0x1000", "-m", "comment", "--comment", "RETURN-on-EGRESS-mark-0x1000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS-PORT", "-j", "AZURE-NPM-EGRESS-TO", "-m", "comment", "--comment", "ALL-JUMP-TO-AZURE-NPM-EGRESS-TO"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-DROPS", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS-DROPS", "-j", "RETURN", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "RETURN-on-EGRESS-and-INGRESS-mark-0x3000"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS-DROPS", "-j", "RETURN", "-m", "mark", "--mark", "0x1000", "-m", "comment", "--comment", "RETURN-on-EGRESS-mark-0x1000"}}, + } ) func TestInitNpmChains(t *testing.T) { @@ -88,7 +141,18 @@ func TestInitNpmChains(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + + err := iptMgr.InitNpmChains() + require.NoError(t, err) +} + +func TestInitWithJumpToAzureAtTop(t *testing.T) { + calls := initWithJumpToAzureAtTopCalls + + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainFirst) err := iptMgr.InitNpmChains() require.NoError(t, err) @@ -99,7 +163,7 @@ func TestUninitNpmChains(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) if err := iptMgr.UninitNpmChains(); err != nil { t.Errorf("TestUninitNpmChains @ iptMgr.UninitNpmChains") @@ -113,7 +177,7 @@ func TestExists(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) iptMgr.OperationFlag = util.IptablesCheckFlag entry := &IptEntry{ @@ -135,7 +199,7 @@ func TestAddChain(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) if err := iptMgr.addChain("TEST-CHAIN"); err != nil { t.Errorf("TestAddChain failed @ iptMgr.addChain") @@ -150,7 +214,7 @@ func TestDeleteChain(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) if err := iptMgr.addChain("TEST-CHAIN"); err != nil { t.Errorf("TestDeleteChain failed @ iptMgr.addChain") @@ -168,7 +232,7 @@ func TestAdd(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 1, execCount+1) @@ -216,7 +280,7 @@ func TestDelete(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 0, execCount+1) @@ -244,7 +308,7 @@ func TestRun(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) iptMgr.OperationFlag = util.IptablesChainCreationFlag entry := &IptEntry{ @@ -263,7 +327,7 @@ func TestGetChainLineNumber(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) lineNum, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) require.NoError(t, err) diff --git a/npm/npm_v1_test.go b/npm/npm_v1_test.go index 230bfdf8fc..23792d8f0c 100644 --- a/npm/npm_v1_test.go +++ b/npm/npm_v1_test.go @@ -6,6 +6,7 @@ import ( "reflect" "testing" + npmconfig "github.com/Azure/azure-container-networking/npm/config" "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/iptm" "github.com/Azure/azure-container-networking/npm/metrics" @@ -80,7 +81,7 @@ func TestMarshalUnMarshalJSON(t *testing.T) { func TestMain(m *testing.M) { metrics.InitializeAll() exec := exec.New() - iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim()) + iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.PlaceAzureChainBeforeKubeForward) iptMgr.UninitNpmChains() ipsMgr := ipsm.NewIpsetManager(exec) diff --git a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go index dab79956cb..6f70ebc0d6 100644 --- a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go +++ b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go @@ -11,10 +11,12 @@ import ( "k8s.io/utils/exec" ) +const placeAzureChainAfterKubeServices = false + func TestMain(m *testing.M) { metrics.InitializeAll() realexec := exec.New() - iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim()) + iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) err := iptMgr.UninitNpmChains() if err != nil { fmt.Println("uninitnpmchains failed with %w", err) diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go index ccf7daf491..601f3f3047 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go @@ -45,7 +45,6 @@ func newNetPolFixture(t *testing.T, utilexec exec.Interface) *netPolFixture { netPolLister: []*networkingv1.NetworkPolicy{}, kubeobjects: []runtime.Object{}, ipsMgr: ipsm.NewIpsetManager(utilexec), - // iptMgr: iptm.NewIptablesManager(utilexec, iptm.NewFakeIptOperationShim()), } return f } @@ -54,7 +53,7 @@ func (f *netPolFixture) newNetPolController(stopCh chan struct{}) { kubeclient := k8sfake.NewSimpleClientset(f.kubeobjects...) f.kubeInformer = kubeinformers.NewSharedInformerFactory(kubeclient, noResyncPeriodFunc()) - f.netPolController = NewNetworkPolicyController(f.kubeInformer.Networking().V1().NetworkPolicies(), f.ipsMgr) + f.netPolController = NewNetworkPolicyController(f.kubeInformer.Networking().V1().NetworkPolicies(), f.ipsMgr, placeAzureChainAfterKubeServices) for _, netPol := range f.netPolLister { f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Add(netPol) From aee147fb297007cc7f3e39bb3feba670151d05da Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 8 Nov 2021 19:19:26 -0800 Subject: [PATCH 05/15] rename toggle name --- npm/config/config.go | 20 ++++++++--------- npm/iptm/iptm.go | 22 +++++++++---------- npm/npm.go | 2 +- npm/npm_v1_test.go | 2 +- .../controllers/v1/networkPolicyController.go | 4 ++-- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/npm/config/config.go b/npm/config/config.go index d5658a7c06..87a25e4910 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -14,11 +14,11 @@ var DefaultConfig = Config{ ListeningPort: defaultListeningPort, ListeningAddress: "0.0.0.0", Toggles: Toggles{ - EnablePrometheusMetrics: true, - EnablePprof: true, - EnableHTTPDebugAPI: true, - EnableV2Controllers: false, - PlaceAzureChainBeforeKubeForward: false, + EnablePrometheusMetrics: true, + EnablePprof: true, + EnableHTTPDebugAPI: true, + EnableV2Controllers: false, + ShouldPlaceAzureChainFirst: false, }, } @@ -30,9 +30,9 @@ type Config struct { } type Toggles struct { - EnablePrometheusMetrics bool - EnablePprof bool - EnableHTTPDebugAPI bool - EnableV2Controllers bool - PlaceAzureChainBeforeKubeForward bool + EnablePrometheusMetrics bool + EnablePprof bool + EnableHTTPDebugAPI bool + EnableV2Controllers bool + ShouldPlaceAzureChainFirst bool } diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 84286b16b6..97a8448872 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -48,10 +48,10 @@ type IptEntry struct { // IptablesManager stores iptables entries. type IptablesManager struct { - exec utilexec.Interface - io ioshim - OperationFlag string - placeAzureChainBeforeKubeForward bool + exec utilexec.Interface + io ioshim + OperationFlag string + shouldPlaceAzureChainFirst bool } func isDropsChain(chainName string) bool { @@ -64,12 +64,12 @@ func isDropsChain(chainName string) bool { } // NewIptablesManager creates a new instance for IptablesManager object. -func NewIptablesManager(exec utilexec.Interface, io ioshim, placeAzureChainBeforeKubeForward bool) *IptablesManager { +func NewIptablesManager(exec utilexec.Interface, io ioshim, shouldPlaceAzureChainFirst bool) *IptablesManager { iptMgr := &IptablesManager{ - exec: exec, - io: io, - OperationFlag: "", - placeAzureChainBeforeKubeForward: placeAzureChainBeforeKubeForward, + exec: exec, + io: io, + OperationFlag: "", + shouldPlaceAzureChainFirst: shouldPlaceAzureChainFirst, } return iptMgr @@ -208,7 +208,7 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { index := 1 // insert the jump to AZURE-NPM at the top kubeServicesLine := 0 - if !iptMgr.placeAzureChainBeforeKubeForward { + if !iptMgr.shouldPlaceAzureChainFirst { // retrieve KUBE-SERVICES index var err error kubeServicesLine, err = iptMgr.getChainLineNumber(util.IptablesKubeServicesChain, util.IptablesForwardChain) @@ -236,7 +236,7 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return nil } - if iptMgr.placeAzureChainBeforeKubeForward { + if iptMgr.shouldPlaceAzureChainFirst { return nil } diff --git a/npm/npm.go b/npm/npm.go index 1d093920aa..250852ed55 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -116,7 +116,7 @@ func NewNetworkPolicyManager(config npmconfig.Config, // create NameSpace controller npMgr.namespaceControllerV1 = controllersv1.NewNameSpaceController(npMgr.nsInformer, npMgr.ipsMgr, npMgr.npmNamespaceCacheV1) // create network policy controller - npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr, config.Toggles.PlaceAzureChainBeforeKubeForward) + npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr, config.Toggles.ShouldPlaceAzureChainFirst) return npMgr } diff --git a/npm/npm_v1_test.go b/npm/npm_v1_test.go index 23792d8f0c..68a58c12d0 100644 --- a/npm/npm_v1_test.go +++ b/npm/npm_v1_test.go @@ -81,7 +81,7 @@ func TestMarshalUnMarshalJSON(t *testing.T) { func TestMain(m *testing.M) { metrics.InitializeAll() exec := exec.New() - iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.PlaceAzureChainBeforeKubeForward) + iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.ShouldPlaceAzureChainFirst) iptMgr.UninitNpmChains() ipsMgr := ipsm.NewIpsetManager(exec) diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go index a3e259d9cc..ba82b135e3 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go @@ -43,7 +43,7 @@ type NetworkPolicyController struct { iptMgr *iptm.IptablesManager } -func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager, placeAzureChainBeforeKubeForward bool) *NetworkPolicyController { +func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager, shouldPlaceAzureChainFirst bool) *NetworkPolicyController { netPolController := &NetworkPolicyController{ netPolLister: npInformer.Lister(), workqueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "NetworkPolicy"), @@ -51,7 +51,7 @@ func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInfo // ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), isAzureNpmChainCreated: false, ipsMgr: ipsMgr, - iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim(), placeAzureChainBeforeKubeForward), + iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim(), shouldPlaceAzureChainFirst), } npInformer.Informer().AddEventHandler( From 89af13ecab8f660733e9c1b4dd235a633fe791f1 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 8 Nov 2021 19:31:04 -0800 Subject: [PATCH 06/15] jump to azure chain on new ct state and update default toggle (UTs will break) --- npm/config/config.go | 2 +- npm/iptm/iptm.go | 4 ++++ npm/iptm/iptm_test.go | 12 ++++++------ 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/npm/config/config.go b/npm/config/config.go index 87a25e4910..36778fdb13 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -18,7 +18,7 @@ var DefaultConfig = Config{ EnablePprof: true, EnableHTTPDebugAPI: true, EnableV2Controllers: false, - ShouldPlaceAzureChainFirst: false, + ShouldPlaceAzureChainFirst: true, // FIXME! default to false after validating }, } diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 97a8448872..b89baafd2e 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -100,6 +100,8 @@ func (iptMgr *IptablesManager) UninitNpmChains() error { Specs: []string{ util.IptablesJumpFlag, util.IptablesAzureChain, + util.IptablesCtstateFlag, + util.IptablesNewState, }, } iptMgr.OperationFlag = util.IptablesDeletionFlag @@ -203,6 +205,8 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { Specs: []string{ util.IptablesJumpFlag, util.IptablesAzureChain, + util.IptablesCtstateFlag, + util.IptablesNewState, }, } diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 7d781996df..d8bdeb6f95 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -33,11 +33,11 @@ var ( {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL {Cmd: []string{"grep", "KUBE-SERVICES"}, Stdout: "4 "}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL {Cmd: []string{"grep", "AZURE-NPM"}, Stdout: "4 "}, - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, @@ -66,7 +66,7 @@ var ( } unInitCalls = []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM-ACCEPT"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM-INGRESS"}}, @@ -106,8 +106,8 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM"}, ExitCode: 1}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}, ExitCode: 1}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, From ef6f3e650be8ce54153682924ad170d67b6acd1e Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 8 Nov 2021 19:43:14 -0800 Subject: [PATCH 07/15] make util constant for UTs and fix UT errors (besides ones I get for controllers) --- npm/iptm/iptm_test.go | 25 ++++++++----------- .../controllers/v1/controllers_v1_test.go | 5 ++-- .../v1/networkPolicyController_test.go | 3 ++- npm/util/const.go | 3 +++ 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index d8bdeb6f95..03b2d30668 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -11,11 +11,6 @@ import ( "github.com/stretchr/testify/require" ) -const ( - placeAzureChainAfterKubeServices = false - placeAzureChainFirst = true -) - var ( initCalls = []testutils.TestCmd{ {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, @@ -141,7 +136,7 @@ func TestInitNpmChains(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) err := iptMgr.InitNpmChains() require.NoError(t, err) @@ -152,7 +147,7 @@ func TestInitWithJumpToAzureAtTop(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainFirst) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) err := iptMgr.InitNpmChains() require.NoError(t, err) @@ -163,7 +158,7 @@ func TestUninitNpmChains(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) if err := iptMgr.UninitNpmChains(); err != nil { t.Errorf("TestUninitNpmChains @ iptMgr.UninitNpmChains") @@ -177,7 +172,7 @@ func TestExists(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) iptMgr.OperationFlag = util.IptablesCheckFlag entry := &IptEntry{ @@ -199,7 +194,7 @@ func TestAddChain(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) if err := iptMgr.addChain("TEST-CHAIN"); err != nil { t.Errorf("TestAddChain failed @ iptMgr.addChain") @@ -214,7 +209,7 @@ func TestDeleteChain(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) if err := iptMgr.addChain("TEST-CHAIN"); err != nil { t.Errorf("TestDeleteChain failed @ iptMgr.addChain") @@ -232,7 +227,7 @@ func TestAdd(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 1, execCount+1) @@ -280,7 +275,7 @@ func TestDelete(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 0, execCount+1) @@ -308,7 +303,7 @@ func TestRun(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) iptMgr.OperationFlag = util.IptablesChainCreationFlag entry := &IptEntry{ @@ -327,7 +322,7 @@ func TestGetChainLineNumber(t *testing.T) { fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) lineNum, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) require.NoError(t, err) diff --git a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go index 6f70ebc0d6..6ef6676bcb 100644 --- a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go +++ b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go @@ -8,15 +8,14 @@ import ( "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/iptm" "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/util" "k8s.io/utils/exec" ) -const placeAzureChainAfterKubeServices = false - func TestMain(m *testing.M) { metrics.InitializeAll() realexec := exec.New() - iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim(), placeAzureChainAfterKubeServices) + iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) err := iptMgr.UninitNpmChains() if err != nil { fmt.Println("uninitnpmchains failed with %w", err) diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go index 601f3f3047..683700841c 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController_test.go @@ -10,6 +10,7 @@ import ( "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/metrics/promutil" + "github.com/Azure/azure-container-networking/npm/util" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -53,7 +54,7 @@ func (f *netPolFixture) newNetPolController(stopCh chan struct{}) { kubeclient := k8sfake.NewSimpleClientset(f.kubeobjects...) f.kubeInformer = kubeinformers.NewSharedInformerFactory(kubeclient, noResyncPeriodFunc()) - f.netPolController = NewNetworkPolicyController(f.kubeInformer.Networking().V1().NetworkPolicies(), f.ipsMgr, placeAzureChainAfterKubeServices) + f.netPolController = NewNetworkPolicyController(f.kubeInformer.Networking().V1().NetworkPolicies(), f.ipsMgr, util.PlaceAzureChainAfterKubeServices) for _, netPol := range f.netPolLister { f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Add(netPol) diff --git a/npm/util/const.go b/npm/util/const.go index 2ac8af8e90..1f76af59d5 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -21,6 +21,9 @@ const ( // iptables related constants. const ( + PlaceAzureChainAfterKubeServices = false + PlaceAzureChainFirst = true + Iptables string = "iptables" Ip6tables string = "ip6tables" //nolint (avoid warning to capitalize this p) IptablesSave string = "iptables-save" From c2895eab32f758fb1d36be0e9cb0a48114133c64 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 8 Nov 2021 20:42:35 -0800 Subject: [PATCH 08/15] added missing module args for ctstate NEW --- npm/iptm/iptm.go | 4 ++++ npm/iptm/iptm_test.go | 12 ++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index b89baafd2e..27d5e8bb19 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -100,6 +100,8 @@ func (iptMgr *IptablesManager) UninitNpmChains() error { Specs: []string{ util.IptablesJumpFlag, util.IptablesAzureChain, + util.IptablesModuleFlag, + util.IptablesCtstateModuleFlag, util.IptablesCtstateFlag, util.IptablesNewState, }, @@ -205,6 +207,8 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { Specs: []string{ util.IptablesJumpFlag, util.IptablesAzureChain, + util.IptablesModuleFlag, + util.IptablesCtstateModuleFlag, util.IptablesCtstateFlag, util.IptablesNewState, }, diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 03b2d30668..4ba8dc6b6e 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -28,11 +28,11 @@ var ( {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL {Cmd: []string{"grep", "KUBE-SERVICES"}, Stdout: "4 "}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL {Cmd: []string{"grep", "AZURE-NPM"}, Stdout: "4 "}, - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, @@ -61,7 +61,7 @@ var ( } unInitCalls = []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM-ACCEPT"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM-INGRESS"}}, @@ -101,8 +101,8 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "--ctstate", "NEW"}, ExitCode: 1}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, ExitCode: 1}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, From 125d1519d8333bd1fb532b7c8d54f518b1e47de7 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 9 Nov 2021 10:15:46 -0800 Subject: [PATCH 09/15] reconcile jump to azure chain at top --- npm/iptm/iptm.go | 22 ++++----- npm/iptm/iptm_test.go | 108 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 115 insertions(+), 15 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 27d5e8bb19..bc4c71cdf8 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -244,21 +244,21 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return nil } - if iptMgr.shouldPlaceAzureChainFirst { - return nil - } - npmChainLine, err := iptMgr.getChainLineNumber(util.IptablesAzureChain, util.IptablesForwardChain) if err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to get index of AZURE-NPM in FORWARD chain with error: %s", err.Error()) return err } - // Kube-services line number is less than npm chain line number then all good - if kubeServicesLine < npmChainLine { - return nil - } else if kubeServicesLine <= 0 { - return nil + if iptMgr.shouldPlaceAzureChainFirst { + if npmChainLine == 1 { + return nil + } + } else { + // Kube-services line number is less than npm chain line number then all good + if kubeServicesLine < npmChainLine || kubeServicesLine <= 0 { + return nil + } } errCode := 0 @@ -271,8 +271,8 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return err } iptMgr.OperationFlag = util.IptablesInsertionFlag - // Reduce index for deleted AZURE-NPM chain - if index > 1 { + if !iptMgr.shouldPlaceAzureChainFirst { + // Reduce index for deleted AZURE-NPM chain index-- } entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 4ba8dc6b6e..82a089bb6a 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -25,6 +25,8 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM-EGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + // NOTE the following grep call stdouts are misleading. The first grep returns 3, and the second one returns "" (i.e. line 0) + // a fix is coming for fakeexec stdout and exit code problems from piping commands (e.g. what we do with grep) {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL {Cmd: []string{"grep", "KUBE-SERVICES"}, Stdout: "4 "}, @@ -142,6 +144,18 @@ func TestInitNpmChains(t *testing.T) { require.NoError(t, err) } +func TestUninitNpmChains(t *testing.T) { + calls := unInitCalls + + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + + if err := iptMgr.UninitNpmChains(); err != nil { + t.Errorf("TestUninitNpmChains @ iptMgr.UninitNpmChains") + } +} + func TestInitWithJumpToAzureAtTop(t *testing.T) { calls := initWithJumpToAzureAtTopCalls @@ -153,16 +167,102 @@ func TestInitWithJumpToAzureAtTop(t *testing.T) { require.NoError(t, err) } -func TestUninitNpmChains(t *testing.T) { - calls := unInitCalls +func TestReconcileMissingJumpToAzureAtTop(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, ExitCode: 1}, // "rule does not exist" + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + } + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) +} + +func TestReconcileMissingJumpToAzureAfterKubeServices(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}, ExitCode: 1}, // THIS IS THE EXIT CODE FOR CHECK command below ("rule doesn't exist") + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "4", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + } fexec := testutils.GetFakeExecWithScripts(calls) defer testutils.VerifyCalls(t, fexec, calls) iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) - if err := iptMgr.UninitNpmChains(); err != nil { - t.Errorf("TestUninitNpmChains @ iptMgr.UninitNpmChains") + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) +} + +func TestReconcileJumpToAzureAlreadyAtTop(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "1 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "AZURE-NPM"}}, + } + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) + + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) +} + +func TestReconcileJumpToAzureAlreadyAfterKubeServices(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "4 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, + {Cmd: []string{"grep", "AZURE-NPM"}}, + } + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) +} + +func TestReconcileMoveJumpToAzureToTop(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "5 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + } + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) + + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) +} + +func TestReconcileMoveJumpToAzureAfterKubeServices(t *testing.T) { + calls := []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "2 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, + {Cmd: []string{"grep", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, } + fexec := testutils.GetFakeExecWithScripts(calls) + defer testutils.VerifyCalls(t, fexec, calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) } func TestExists(t *testing.T) { From a33fda7e7f11abaa78521fe8675b0ebcff769623 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 9 Nov 2021 12:06:01 -0800 Subject: [PATCH 10/15] delete deprecated jump to azure chain on uninit, and fix go lint --- npm/iptm/iptm.go | 18 ++++++++++++++++-- npm/iptm/iptm_test.go | 5 +++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index bc4c71cdf8..ee04afa6e2 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -36,6 +36,14 @@ var IptablesAzureChainList = []string{ util.IptablesAzureEgressDropsChain, } +var deprecatedJumpToAzureEntry = &IptEntry{ + Chain: util.IptablesForwardChain, + Specs: []string{ + util.IptablesJumpFlag, + util.IptablesAzureChain, + }, +} + // IptEntry represents an iptables rule. type IptEntry struct { Command string @@ -95,6 +103,13 @@ func (iptMgr *IptablesManager) InitNpmChains() error { // UninitNpmChains uninitializes Azure NPM chains in iptables. func (iptMgr *IptablesManager) UninitNpmChains() error { // Remove AZURE-NPM chain from FORWARD chain. + iptMgr.OperationFlag = util.IptablesDeletionFlag + errCode, err := iptMgr.run(deprecatedJumpToAzureEntry) + if errCode != iptablesErrDoesNotExist && err != nil { + metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to delete deprecated jump from FORWARD chain to AZURE-NPM") + return err + } + entry := &IptEntry{ Chain: util.IptablesForwardChain, Specs: []string{ @@ -106,8 +121,7 @@ func (iptMgr *IptablesManager) UninitNpmChains() error { util.IptablesNewState, }, } - iptMgr.OperationFlag = util.IptablesDeletionFlag - errCode, err := iptMgr.run(entry) + errCode, err = iptMgr.run(entry) if errCode != iptablesErrDoesNotExist && err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to delete AZURE-NPM from Forward chain") return err diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 82a089bb6a..f6488059a2 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -48,7 +48,7 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "AZURE-NPM-INGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, - /////////// + // ///////// {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "AZURE-NPM-INGRESS-FROM", "-m", "comment", "--comment", "ALL-JUMP-TO-AZURE-NPM-INGRESS-FROM"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "AZURE-NPM-EGRESS-PORT"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "RETURN-on-EGRESS-and-INGRESS-mark-0x3000"}}, @@ -63,6 +63,7 @@ var ( } unInitCalls = []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: []string{"iptables", "-w", "60", "-F", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-F", "AZURE-NPM-ACCEPT"}}, @@ -118,7 +119,7 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS", "-j", "AZURE-NPM-INGRESS-DROPS"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "RETURN", "-m", "mark", "--mark", "0x2000", "-m", "comment", "--comment", "RETURN-on-INGRESS-mark-0x2000"}}, - /////////// + // ///////// {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-INGRESS-PORT", "-j", "AZURE-NPM-INGRESS-FROM", "-m", "comment", "--comment", "ALL-JUMP-TO-AZURE-NPM-INGRESS-FROM"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "AZURE-NPM-EGRESS-PORT"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM-EGRESS", "-j", "RETURN", "-m", "mark", "--mark", "0x3000", "-m", "comment", "--comment", "RETURN-on-EGRESS-and-INGRESS-mark-0x3000"}}, From 28eb4f442fb4d4db7045df9d16fe4dc187910704 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 9 Nov 2021 17:40:41 -0800 Subject: [PATCH 11/15] assign correct default toggle value --- npm/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/config/config.go b/npm/config/config.go index 36778fdb13..87a25e4910 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -18,7 +18,7 @@ var DefaultConfig = Config{ EnablePprof: true, EnableHTTPDebugAPI: true, EnableV2Controllers: false, - ShouldPlaceAzureChainFirst: true, // FIXME! default to false after validating + ShouldPlaceAzureChainFirst: false, }, } From d0ddd5cdd8a2b150c68c301a08faf9da14a5b550 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 11 Nov 2021 15:06:47 -0800 Subject: [PATCH 12/15] addressed comments --- npm/config/config.go | 20 +++++------ npm/iptm/iptm.go | 34 ++++++++++--------- npm/npm.go | 2 +- npm/npm_v1_test.go | 2 +- .../controllers/v1/controllers_v1_test.go | 4 +-- .../controllers/v1/networkPolicyController.go | 4 +-- 6 files changed, 34 insertions(+), 32 deletions(-) diff --git a/npm/config/config.go b/npm/config/config.go index 87a25e4910..5e79a74cca 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -14,11 +14,11 @@ var DefaultConfig = Config{ ListeningPort: defaultListeningPort, ListeningAddress: "0.0.0.0", Toggles: Toggles{ - EnablePrometheusMetrics: true, - EnablePprof: true, - EnableHTTPDebugAPI: true, - EnableV2Controllers: false, - ShouldPlaceAzureChainFirst: false, + EnablePrometheusMetrics: true, + EnablePprof: true, + EnableHTTPDebugAPI: true, + EnableV2Controllers: false, + PlaceAzureChainFirst: false, }, } @@ -30,9 +30,9 @@ type Config struct { } type Toggles struct { - EnablePrometheusMetrics bool - EnablePprof bool - EnableHTTPDebugAPI bool - EnableV2Controllers bool - ShouldPlaceAzureChainFirst bool + EnablePrometheusMetrics bool + EnablePprof bool + EnableHTTPDebugAPI bool + EnableV2Controllers bool + PlaceAzureChainFirst bool } diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index ee04afa6e2..91d072b65b 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -56,10 +56,10 @@ type IptEntry struct { // IptablesManager stores iptables entries. type IptablesManager struct { - exec utilexec.Interface - io ioshim - OperationFlag string - shouldPlaceAzureChainFirst bool + exec utilexec.Interface + io ioshim + OperationFlag string + placeAzureChainFirst bool } func isDropsChain(chainName string) bool { @@ -72,12 +72,12 @@ func isDropsChain(chainName string) bool { } // NewIptablesManager creates a new instance for IptablesManager object. -func NewIptablesManager(exec utilexec.Interface, io ioshim, shouldPlaceAzureChainFirst bool) *IptablesManager { +func NewIptablesManager(exec utilexec.Interface, io ioshim, placeAzureChainFirst bool) *IptablesManager { iptMgr := &IptablesManager{ - exec: exec, - io: io, - OperationFlag: "", - shouldPlaceAzureChainFirst: shouldPlaceAzureChainFirst, + exec: exec, + io: io, + OperationFlag: "", + placeAzureChainFirst: placeAzureChainFirst, } return iptMgr @@ -228,9 +228,9 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { }, } - index := 1 // insert the jump to AZURE-NPM at the top - kubeServicesLine := 0 - if !iptMgr.shouldPlaceAzureChainFirst { + var index int + var kubeServicesLine int + if !iptMgr.placeAzureChainFirst { // retrieve KUBE-SERVICES index var err error kubeServicesLine, err = iptMgr.getChainLineNumber(util.IptablesKubeServicesChain, util.IptablesForwardChain) @@ -249,7 +249,9 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { if !exists { // position Azure-NPM chain after Kube-Forward and Kube-Service chains if it exists iptMgr.OperationFlag = util.IptablesInsertionFlag - entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) + if !iptMgr.placeAzureChainFirst { + entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) + } if _, err = iptMgr.run(entry); err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain.") return err @@ -264,7 +266,7 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return err } - if iptMgr.shouldPlaceAzureChainFirst { + if iptMgr.placeAzureChainFirst { if npmChainLine == 1 { return nil } @@ -285,11 +287,11 @@ func (iptMgr *IptablesManager) checkAndAddForwardChain() error { return err } iptMgr.OperationFlag = util.IptablesInsertionFlag - if !iptMgr.shouldPlaceAzureChainFirst { + if !iptMgr.placeAzureChainFirst { // Reduce index for deleted AZURE-NPM chain index-- + entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) } - entry.Specs = append([]string{strconv.Itoa(index)}, entry.Specs...) if errCode, err = iptMgr.run(entry); err != nil { metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to add AZURE-NPM chain to FORWARD chain with error code %d.", errCode) return err diff --git a/npm/npm.go b/npm/npm.go index 250852ed55..9230c04657 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -116,7 +116,7 @@ func NewNetworkPolicyManager(config npmconfig.Config, // create NameSpace controller npMgr.namespaceControllerV1 = controllersv1.NewNameSpaceController(npMgr.nsInformer, npMgr.ipsMgr, npMgr.npmNamespaceCacheV1) // create network policy controller - npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr, config.Toggles.ShouldPlaceAzureChainFirst) + npMgr.netPolControllerV1 = controllersv1.NewNetworkPolicyController(npMgr.npInformer, npMgr.ipsMgr, config.Toggles.PlaceAzureChainFirst) return npMgr } diff --git a/npm/npm_v1_test.go b/npm/npm_v1_test.go index 68a58c12d0..d3b94e5acd 100644 --- a/npm/npm_v1_test.go +++ b/npm/npm_v1_test.go @@ -81,7 +81,7 @@ func TestMarshalUnMarshalJSON(t *testing.T) { func TestMain(m *testing.M) { metrics.InitializeAll() exec := exec.New() - iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.ShouldPlaceAzureChainFirst) + iptMgr := iptm.NewIptablesManager(exec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.PlaceAzureChainFirst) iptMgr.UninitNpmChains() ipsMgr := ipsm.NewIpsetManager(exec) diff --git a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go index 6ef6676bcb..1f0ed392e7 100644 --- a/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go +++ b/npm/pkg/controlplane/controllers/v1/controllers_v1_test.go @@ -5,17 +5,17 @@ import ( "os" "testing" + npmconfig "github.com/Azure/azure-container-networking/npm/config" "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/iptm" "github.com/Azure/azure-container-networking/npm/metrics" - "github.com/Azure/azure-container-networking/npm/util" "k8s.io/utils/exec" ) func TestMain(m *testing.M) { metrics.InitializeAll() realexec := exec.New() - iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) + iptMgr := iptm.NewIptablesManager(realexec, iptm.NewFakeIptOperationShim(), npmconfig.DefaultConfig.Toggles.PlaceAzureChainFirst) err := iptMgr.UninitNpmChains() if err != nil { fmt.Println("uninitnpmchains failed with %w", err) diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go index ba82b135e3..fb8856a7c4 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go @@ -43,7 +43,7 @@ type NetworkPolicyController struct { iptMgr *iptm.IptablesManager } -func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager, shouldPlaceAzureChainFirst bool) *NetworkPolicyController { +func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInformer, ipsMgr *ipsm.IpsetManager, placeAzureChainFirst bool) *NetworkPolicyController { netPolController := &NetworkPolicyController{ netPolLister: npInformer.Lister(), workqueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "NetworkPolicy"), @@ -51,7 +51,7 @@ func NewNetworkPolicyController(npInformer networkinginformers.NetworkPolicyInfo // ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), isAzureNpmChainCreated: false, ipsMgr: ipsMgr, - iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim(), shouldPlaceAzureChainFirst), + iptMgr: iptm.NewIptablesManager(exec.New(), iptm.NewIptOperationShim(), placeAzureChainFirst), } npInformer.Informer().AddEventHandler( From a9cb8b7720feb31f367b45b1042a713b19a510ec Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 11 Nov 2021 17:30:09 -0800 Subject: [PATCH 13/15] fix UTs after removing index 1 for placing chain first. Also make all tests subtests for check and add forward chain --- npm/iptm/iptm_test.go | 192 +++++++++++++++++++++--------------------- 1 file changed, 98 insertions(+), 94 deletions(-) diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index f6488059a2..fbeb95603a 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -105,7 +105,7 @@ var ( {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, ExitCode: 1}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-INGRESS"}}, // broken here {Cmd: []string{"iptables", "-w", "60", "-C", "AZURE-NPM", "-j", "AZURE-NPM-EGRESS"}}, @@ -168,102 +168,106 @@ func TestInitWithJumpToAzureAtTop(t *testing.T) { require.NoError(t, err) } -func TestReconcileMissingJumpToAzureAtTop(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, ExitCode: 1}, // "rule does not exist" - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) -} - -func TestReconcileMissingJumpToAzureAfterKubeServices(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"grep", "KUBE-SERVICES"}, ExitCode: 1}, // THIS IS THE EXIT CODE FOR CHECK command below ("rule doesn't exist") - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "4", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) -} - -func TestReconcileJumpToAzureAlreadyAtTop(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "1 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"grep", "AZURE-NPM"}}, - } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) -} - -func TestReconcileJumpToAzureAlreadyAfterKubeServices(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"grep", "KUBE-SERVICES"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "4 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, - {Cmd: []string{"grep", "AZURE-NPM"}}, +func TestCheckAndAddForwardChain(t *testing.T) { + type args struct { + calls []testutils.TestCmd + placeAzureChainFirst bool } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) -} - -func TestReconcileMoveJumpToAzureToTop(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "5 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"grep", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "1", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + tests := []struct { + name string + args args + }{ + { + name: "add missing jump to azure at top", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, ExitCode: 1}, // "rule does not exist" + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainFirst, + }, + }, + { + name: "add missing jump to azure after kube services", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}, ExitCode: 1}, // THIS IS THE EXIT CODE FOR CHECK command below ("rule doesn't exist") + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "4", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainAfterKubeServices, + }, + }, + { + name: "jump to azure already at top", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "1 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "AZURE-NPM"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainFirst, + }, + }, + { + name: "jump to azure already after kube services", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "4 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, + {Cmd: []string{"grep", "AZURE-NPM"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainAfterKubeServices, + }, + }, + { + name: "move jump to azure to top", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "5 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainFirst, + }, + }, + { + name: "move jump to azure after kube services", + args: args{ + calls: []testutils.TestCmd{ + {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"grep", "KUBE-SERVICES"}}, + {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "2 "}, // THIS IS THE GREP CALL STDOUT + {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, + {Cmd: []string{"grep", "AZURE-NPM"}}, + {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + }, + placeAzureChainFirst: util.PlaceAzureChainAfterKubeServices, + }, + }, } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainFirst) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) -} - -func TestReconcileMoveJumpToAzureAfterKubeServices(t *testing.T) { - calls := []testutils.TestCmd{ - {Cmd: []string{"iptables", "-w", "60", "-N", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}, Stdout: "3 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"grep", "KUBE-SERVICES"}}, - {Cmd: []string{"iptables", "-w", "60", "-C", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}, Stdout: "2 "}, // THIS IS THE GREP CALL STDOUT - {Cmd: []string{"iptables", "-t", "filter", "-n", "--list", "FORWARD", "--line-numbers"}}, - {Cmd: []string{"grep", "AZURE-NPM"}}, - {Cmd: []string{"iptables", "-w", "60", "-D", "FORWARD", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, - {Cmd: []string{"iptables", "-w", "60", "-I", "FORWARD", "3", "-j", "AZURE-NPM", "-m", "conntrack", "--ctstate", "NEW"}}, + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + fexec := testutils.GetFakeExecWithScripts(tt.args.calls) + defer testutils.VerifyCalls(t, fexec, tt.args.calls) + iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), tt.args.placeAzureChainFirst) + err := iptMgr.checkAndAddForwardChain() + require.NoError(t, err) + }) } - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim(), util.PlaceAzureChainAfterKubeServices) - - err := iptMgr.checkAndAddForwardChain() - require.NoError(t, err) } func TestExists(t *testing.T) { From bf8e7667f53d4ba60071fdbc3b420b54d38fbc7a Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 12 Nov 2021 10:35:19 -0800 Subject: [PATCH 14/15] set PlaceAzureChainFirst: true --- npm/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/config/config.go b/npm/config/config.go index 5e79a74cca..e731c884b0 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -18,7 +18,7 @@ var DefaultConfig = Config{ EnablePprof: true, EnableHTTPDebugAPI: true, EnableV2Controllers: false, - PlaceAzureChainFirst: false, + PlaceAzureChainFirst: true, }, } From af9453e5d2c71267feb6869412e98825c78d320e Mon Sep 17 00:00:00 2001 From: Hunter Gregory <42728408+huntergregory@users.noreply.github.com> Date: Mon, 15 Nov 2021 11:53:56 -0800 Subject: [PATCH 15/15] switch to correct default for PlaceAzureChainFirst --- npm/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/config/config.go b/npm/config/config.go index e731c884b0..5e79a74cca 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -18,7 +18,7 @@ var DefaultConfig = Config{ EnablePprof: true, EnableHTTPDebugAPI: true, EnableV2Controllers: false, - PlaceAzureChainFirst: true, + PlaceAzureChainFirst: false, }, }