From 521b9cc967bfea07f325000e58eb8ce03d355c22 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 5 Nov 2021 13:54:34 -0700 Subject: [PATCH 1/8] cleanup old policy chains and reboot iptables chains when there are no more policies --- .../policies/chain-management_linux.go | 73 +++++++++++++++---- .../policies/chain-management_linux_test.go | 61 +++++++++++++++- npm/pkg/dataplane/policies/policymanager.go | 31 +++++++- .../dataplane/policies/policymanager_linux.go | 20 +++-- .../policies/policymanager_linux_test.go | 56 +++++++++++++- .../policies/policymanager_windows.go | 15 ++++ npm/pkg/dataplane/policies/testutils_linux.go | 11 +++ 7 files changed, 236 insertions(+), 31 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index 2168706584..f146ee7bc1 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -5,7 +5,6 @@ import ( "fmt" "strconv" "strings" - "time" "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ioutil" @@ -54,6 +53,25 @@ var ( ingressOrEgressPolicyChainPattern = fmt.Sprintf("'Chain %s-\\|Chain %s-'", util.IptablesAzureIngressPolicyChainPrefix, util.IptablesAzureEgressPolicyChainPrefix) ) +type osTools struct { + chainsToCleanup map[string]struct{} +} + +func makeTools() osTools { + return osTools{make(map[string]struct{})} +} + +func (pMgr *PolicyManager) reboot() error { + // TODO for the sake of UTs, need to have a pMgr config specifying whether or not this reboot happens + // if err := pMgr.reset(); err != nil { + // return npmerrors.SimpleErrorWrapper("failed to remove NPM chains while rebooting", err) + // } + // if err := pMgr.initialize(); err != nil { + // return npmerrors.SimpleErrorWrapper("failed to initialize NPM chains while rebooting", err) + // } + return nil +} + func (pMgr *PolicyManager) initialize() error { if err := pMgr.initializeNPMChains(); err != nil { return npmerrors.SimpleErrorWrapper("failed to initialize NPM chains", err) @@ -65,6 +83,7 @@ func (pMgr *PolicyManager) reset() error { if err := pMgr.removeNPMChains(); err != nil { return npmerrors.SimpleErrorWrapper("failed to remove NPM chains", err) } + pMgr.chainsToCleanup = make(map[string]struct{}) return nil } @@ -123,26 +142,48 @@ func (pMgr *PolicyManager) removeNPMChains() error { return nil } -// ReconcileChains periodically creates the jump rule from FORWARD chain to AZURE-NPM chain (if it d.n.e) -// and makes sure it's after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). -func (pMgr *PolicyManager) ReconcileChains(stopChannel <-chan struct{}) { - go pMgr.reconcileChains(stopChannel) +// reconcile does the following: +// - cleans up old policy chains +// - creates the jump rule from FORWARD chain to AZURE-NPM chain (if it d.n.e) and makes sure it's after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). +func (pMgr *PolicyManager) reconcile(stopChannel <-chan struct{}) { + if err := pMgr.positionAzureChainJumpRule(); err != nil { + klog.Errorf("failed to reconcile jump rule to Azure-NPM due to %s", err.Error()) + } + if err := pMgr.cleanupChains(pMgr.oldPolicyChains()); err != nil { + klog.Errorf("failed to clean up old policy chains with the following error %s", err.Error()) + } +} + +func (pMgr *PolicyManager) oldPolicyChains() []string { + result := make([]string, len(pMgr.chainsToCleanup)) + k := 0 + for chain := range pMgr.chainsToCleanup { + result[k] = chain + k++ + } + return result } -func (pMgr *PolicyManager) reconcileChains(stopChannel <-chan struct{}) { - ticker := time.NewTicker(time.Minute * time.Duration(reconcileChainTimeInMinutes)) - defer ticker.Stop() - - for { - select { - case <-stopChannel: - return - case <-ticker.C: - if err := pMgr.positionAzureChainJumpRule(); err != nil { - metrics.SendErrorLogAndMetric(util.NpmID, "Error: failed to reconcile jump rule to Azure-NPM due to %s", err.Error()) +// have to use slice argument for deterministic behavior for UTs +func (pMgr *PolicyManager) cleanupChains(chains []string) error { + var aggregateError error + for _, chain := range chains { + errCode, err := pMgr.runIPTablesCommand(util.IptablesDestroyFlag, chain) // TODO run the one that ignores doesNotExistErrorCode + if err == nil || errCode == doesNotExistErrorCode { + delete(pMgr.chainsToCleanup, chain) + } else { + currentErrString := fmt.Sprintf("failed to clean up policy chain %s with err [%v]", chain, err) + if aggregateError == nil { + aggregateError = npmerrors.SimpleError(currentErrString) + } else { + aggregateError = npmerrors.SimpleErrorWrapper(fmt.Sprintf("%s and had previous error", currentErrString), aggregateError) } } } + if aggregateError != nil { + return npmerrors.SimpleErrorWrapper("failed to clean up some policy chains with errors", aggregateError) + } + return nil } // this function has a direct comparison in NPM v1 iptables manager (iptm.go) diff --git a/npm/pkg/dataplane/policies/chain-management_linux_test.go b/npm/pkg/dataplane/policies/chain-management_linux_test.go index 73482e4abe..5a9f6c0d4f 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux_test.go +++ b/npm/pkg/dataplane/policies/chain-management_linux_test.go @@ -2,6 +2,7 @@ package policies import ( "fmt" + "sort" "strings" "testing" @@ -11,6 +12,58 @@ import ( "github.com/stretchr/testify/require" ) +const ( + testChain1 = "chain1" + testChain2 = "chain2" + testChain3 = "chain3" +) + +func TestOldPolicyChains(t *testing.T) { + pMgr := NewPolicyManager(common.NewMockIOShim(nil)) + pMgr.chainsToCleanup[testChain1] = struct{}{} + pMgr.chainsToCleanup[testChain2] = struct{}{} + chainsToCleanup := pMgr.oldPolicyChains() + require.Equal(t, 2, len(chainsToCleanup)) + require.True(t, chainsToCleanup[0] == testChain1 || chainsToCleanup[1] == testChain1) + require.True(t, chainsToCleanup[0] == testChain2 || chainsToCleanup[1] == testChain2) +} + +func TestCleanupChainsSuccess(t *testing.T) { + calls := []testutils.TestCmd{ + getFakeDestroyCommand(testChain1), + getFakeDestroyCommandWithExitCode(testChain2, 1), // exit code 1 means the chain d.n.e. + } + ioshim := common.NewMockIOShim(calls) + // TODO defer ioshim.VerifyCalls(t, ioshim, calls) + pMgr := NewPolicyManager(ioshim) + + pMgr.chainsToCleanup[testChain1] = struct{}{} + pMgr.chainsToCleanup[testChain2] = struct{}{} + chainsToCleanup := pMgr.oldPolicyChains() + sort.Strings(chainsToCleanup) + require.NoError(t, pMgr.cleanupChains(chainsToCleanup)) + assertEqualCleanupContents(t, pMgr) +} + +func TestCleanupChainsFailure(t *testing.T) { + calls := []testutils.TestCmd{ + getFakeDestroyCommandWithExitCode(testChain1, 2), + getFakeDestroyCommand(testChain2), + getFakeDestroyCommandWithExitCode(testChain3, 2), + } + ioshim := common.NewMockIOShim(calls) + // TODO defer ioshim.VerifyCalls(t, ioshim, calls) + pMgr := NewPolicyManager(ioshim) + + pMgr.chainsToCleanup[testChain1] = struct{}{} + pMgr.chainsToCleanup[testChain2] = struct{}{} + pMgr.chainsToCleanup[testChain3] = struct{}{} + chainsToCleanup := pMgr.oldPolicyChains() + sort.Strings(chainsToCleanup) + require.Error(t, pMgr.cleanupChains(chainsToCleanup)) + assertEqualCleanupContents(t, pMgr, testChain1, testChain3) +} + func TestInitChainsCreator(t *testing.T) { pMgr := NewPolicyManager(common.NewMockIOShim(nil)) creator := pMgr.getCreatorForInitChains() // doesn't make any exec calls @@ -109,7 +162,7 @@ func TestRemoveChainsCreator(t *testing.T) { func TestRemoveChainsSuccess(t *testing.T) { calls := GetResetTestCalls() - for _, chain := range iptablesOldAndNewChains { + for _, chain := range iptablesOldAndNewChains { // TODO write these out, don't use variable calls = append(calls, getFakeDestroyCommand(chain)) } calls = append( @@ -162,7 +215,7 @@ func TestRemoveChainsFailureOnDestroy(t *testing.T) { {Cmd: []string{"grep", ingressOrEgressPolicyChainPattern}}, // ExitCode 0 for the iptables restore command fakeIPTablesRestoreCommand, } - calls = append(calls, getFakeDestroyFailureCommand(iptablesOldAndNewChains[0])) // this ExitCode here will actually impact the next below + calls = append(calls, getFakeDestroyCommandWithExitCode(iptablesOldAndNewChains[0], 2)) // this ExitCode here will actually impact the next below for _, chain := range iptablesOldAndNewChains[1:] { calls = append(calls, getFakeDestroyCommand(chain)) } @@ -411,8 +464,8 @@ func getFakeDestroyCommand(chain string) testutils.TestCmd { return testutils.TestCmd{Cmd: []string{"iptables", "-w", "60", "-X", chain}} } -func getFakeDestroyFailureCommand(chain string) testutils.TestCmd { +func getFakeDestroyCommandWithExitCode(chain string, exitCode int) testutils.TestCmd { command := getFakeDestroyCommand(chain) - command.ExitCode = 1 + command.ExitCode = exitCode return command } diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index 87ad8c38db..257bc61184 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -2,6 +2,8 @@ package policies import ( "fmt" + "sync" + "time" "github.com/Azure/azure-container-networking/common" npmerrors "github.com/Azure/azure-container-networking/npm/util/errors" @@ -15,6 +17,8 @@ type PolicyMap struct { type PolicyManager struct { policyMap *PolicyMap ioShim *common.IOShim + osTools + sync.Mutex } func NewPolicyManager(ioShim *common.IOShim) *PolicyManager { @@ -22,7 +26,8 @@ func NewPolicyManager(ioShim *common.IOShim) *PolicyManager { policyMap: &PolicyMap{ cache: make(map[string]*NPMNetworkPolicy), }, - ioShim: ioShim, + ioShim: ioShim, + osTools: makeTools(), } } @@ -40,6 +45,24 @@ func (pMgr *PolicyManager) Reset() error { return nil } +func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { + go func() { + ticker := time.NewTicker(time.Minute * time.Duration(reconcileChainTimeInMinutes)) + defer ticker.Stop() + + for { + select { + case <-stopChannel: + return + case <-ticker.C: + pMgr.Lock() + defer pMgr.Unlock() + pMgr.reconcile(stopChannel) + } + } + }() +} + func (pMgr *PolicyManager) PolicyExists(name string) bool { _, ok := pMgr.policyMap.cache[name] return ok @@ -87,6 +110,12 @@ func (pMgr *PolicyManager) RemovePolicy(name string, endpointList map[string]str } delete(pMgr.policyMap.cache, name) + if len(pMgr.policyMap.cache) == 0 { + klog.Infof("rebooting policy manager since there are no policies remaining in the cache") + if err := pMgr.reboot(); err != nil { + klog.Errorf("failed to reboot when there were no policies remaining") + } + } return nil } diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index 0313b4bac8..214f3465ea 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -21,11 +21,15 @@ const ( // shouldn't call this if the np has no ACLs (check in generic) func (pMgr *PolicyManager) addPolicy(networkPolicy *NPMNetworkPolicy, _ map[string]string) error { // TODO check for newPolicy errors - creator := pMgr.getCreatorForNewNetworkPolicies(networkPolicy) + allChainNames := getAllChainNames([]*NPMNetworkPolicy{networkPolicy}) + creator := pMgr.getCreatorForNewNetworkPolicies(allChainNames, networkPolicy) err := restore(creator) if err != nil { return npmerrors.SimpleErrorWrapper("failed to restore iptables with updated policies", err) } + for _, chain := range allChainNames { + delete(pMgr.chainsToCleanup, chain) + } return nil } @@ -34,11 +38,15 @@ func (pMgr *PolicyManager) removePolicy(networkPolicy *NPMNetworkPolicy, _ map[s if deleteErr != nil { return npmerrors.SimpleErrorWrapper("failed to delete jumps to policy chains", deleteErr) } - creator := pMgr.getCreatorForRemovingPolicies(networkPolicy) + allChainNames := getAllChainNames([]*NPMNetworkPolicy{networkPolicy}) + creator := pMgr.getCreatorForRemovingPolicies(allChainNames) restoreErr := restore(creator) if restoreErr != nil { return npmerrors.SimpleErrorWrapper("failed to flush policies", restoreErr) } + for _, chain := range allChainNames { + pMgr.chainsToCleanup[chain] = struct{}{} + } return nil } @@ -50,8 +58,8 @@ func restore(creator *ioutil.FileCreator) error { return nil } -func (pMgr *PolicyManager) getCreatorForRemovingPolicies(networkPolicies ...*NPMNetworkPolicy) *ioutil.FileCreator { - allChainNames := getAllChainNames(networkPolicies) +// TODO use array instead of ... +func (pMgr *PolicyManager) getCreatorForRemovingPolicies(allChainNames []string) *ioutil.FileCreator { creator := pMgr.getNewCreatorWithChains(allChainNames) creator.AddLine("", nil, util.IptablesRestoreCommit) return creator @@ -165,8 +173,8 @@ func getEgressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string { } // noflush add to chains impacted -func (pMgr *PolicyManager) getCreatorForNewNetworkPolicies(networkPolicies ...*NPMNetworkPolicy) *ioutil.FileCreator { - allChainNames := getAllChainNames(networkPolicies) +// TODO use array instead of ... +func (pMgr *PolicyManager) getCreatorForNewNetworkPolicies(allChainNames []string, networkPolicies ...*NPMNetworkPolicy) *ioutil.FileCreator { creator := pMgr.getNewCreatorWithChains(allChainNames) ingressJumpLineNumber := 1 diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index da5ed289ef..687289090d 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -8,6 +8,7 @@ import ( "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" dptestutils "github.com/Azure/azure-container-networking/npm/pkg/dataplane/testutils" + "github.com/Azure/azure-container-networking/npm/util" testutils "github.com/Azure/azure-container-networking/test/utils" "github.com/stretchr/testify/require" ) @@ -33,10 +34,17 @@ var ( testACLRule4 = fmt.Sprintf("-j AZURE-NPM-ACCEPT -p all -m set --match-set %s src -m comment --comment comment4", ipsets.TestCIDRSet.HashedName) ) +func TestChainNames(t *testing.T) { + expectedName := fmt.Sprintf("AZURE-NPM-INGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) + require.Equal(t, expectedName, TestNetworkPolicies[0].getIngressChainName()) + expectedName = fmt.Sprintf("AZURE-NPM-EGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) + require.Equal(t, expectedName, TestNetworkPolicies[0].getEgressChainName()) +} + func TestAddPolicies(t *testing.T) { calls := []testutils.TestCmd{fakeIPTablesRestoreCommand} pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - creator := pMgr.getCreatorForNewNetworkPolicies(TestNetworkPolicies...) + creator := pMgr.getCreatorForNewNetworkPolicies(getAllChainNames(TestNetworkPolicies), TestNetworkPolicies...) actualLines := strings.Split(creator.ToString(), "\n") expectedLines := []string{ "*filter", @@ -81,7 +89,7 @@ func TestRemovePolicies(t *testing.T) { fakeIPTablesRestoreCommand, } pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - creator := pMgr.getCreatorForRemovingPolicies(TestNetworkPolicies...) + creator := pMgr.getCreatorForRemovingPolicies(getAllChainNames(TestNetworkPolicies)) actualLines := strings.Split(creator.ToString(), "\n") expectedLines := []string{ "*filter", @@ -113,7 +121,7 @@ func TestRemovePoliciesErrorOnRestore(t *testing.T) { require.Error(t, err) } -func TestRemovePoliciesErrorOnIngressRule(t *testing.T) { +func TestRemovePoliciesErrorOnDeleteForIngress(t *testing.T) { calls := []testutils.TestCmd{ fakeIPTablesRestoreCommand, getFakeDeleteJumpCommandWithCode("AZURE-NPM-INGRESS", testPolicy1IngressJump, 1), // anything but 0 or 2 @@ -125,7 +133,7 @@ func TestRemovePoliciesErrorOnIngressRule(t *testing.T) { require.Error(t, err) } -func TestRemovePoliciesErrorOnEgressRule(t *testing.T) { +func TestRemovePoliciesErrorOnDeleteForEgress(t *testing.T) { calls := []testutils.TestCmd{ fakeIPTablesRestoreCommand, getFakeDeleteJumpCommand("AZURE-NPM-INGRESS", testPolicy1IngressJump), @@ -137,3 +145,43 @@ func TestRemovePoliciesErrorOnEgressRule(t *testing.T) { err = pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil) require.Error(t, err) } + +func TestUpdatingChainsToCleanup(t *testing.T) { + calls := GetAddPolicyTestCalls(TestNetworkPolicies[0]) + calls = append(calls, GetRemovePolicyTestCalls(TestNetworkPolicies[0])...) + calls = append(calls, GetAddPolicyTestCalls(TestNetworkPolicies[1])...) + calls = append(calls, GetRemovePolicyFailureTestCalls(TestNetworkPolicies[1])...) + calls = append(calls, GetAddPolicyTestCalls(TestNetworkPolicies[2])...) + calls = append(calls, GetRemovePolicyTestCalls(TestNetworkPolicies[2])...) + calls = append(calls, GetAddPolicyFailureTestCalls(TestNetworkPolicies[2])...) + calls = append(calls, GetAddPolicyTestCalls(TestNetworkPolicies[0])...) + ioshim := common.NewMockIOShim(calls) + // TODO defer ioshim.VerifyCalls(t, ioshim, calls) + pMgr := NewPolicyManager(ioshim) + + // FIXME off because of grep stuff + require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[0], nil)) + assertEqualCleanupContents(t, pMgr) + require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil)) + assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + + // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[1], nil)) + // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[1].Name, nil)) + // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + + // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[2], nil)) + // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[2].Name, nil)) + // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain, testPolicy3EgressChain) + // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[0], nil)) + // assertEqualCleanupContents(t, pMgr, testPolicy3EgressChain) +} + +func assertEqualCleanupContents(t *testing.T, pMgr *PolicyManager, expectedChains ...string) { + require.Equal(t, len(expectedChains), len(pMgr.chainsToCleanup), "incorrectly tracking chains for cleanup") + for _, chain := range expectedChains { + _, exists := pMgr.chainsToCleanup[chain] + require.True(t, exists, "incorrectly tracking chains for cleanup") + } +} diff --git a/npm/pkg/dataplane/policies/policymanager_windows.go b/npm/pkg/dataplane/policies/policymanager_windows.go index 6a56e2d6e3..b1157b0800 100644 --- a/npm/pkg/dataplane/policies/policymanager_windows.go +++ b/npm/pkg/dataplane/policies/policymanager_windows.go @@ -15,11 +15,22 @@ var ( ErrFailedUnMarshalACLSettings = errors.New("Failed to unmarshal ACL settings") ) +type osTools struct{} + type endpointPolicyBuilder struct { aclPolicies []*NPMACLPolSettings otherPolicies []hcn.EndpointPolicy } +func makeTools() osTools { + return osTools{} +} + +func (pMgr *PolicyManager) reboot() error { + // TODO should we something here? + return nil +} + func (pMgr *PolicyManager) initialize() error { // TODO return nil @@ -30,6 +41,10 @@ func (pMgr *PolicyManager) reset() error { return nil } +func (pMgr *PolicyManager) reconcile(stopChannel <-chan struct{}) { + // TODO +} + func (pMgr *PolicyManager) addPolicy(policy *NPMNetworkPolicy, endpointList map[string]string) error { klog.Infof("[DataPlane Windows] adding policy %s on %+v", policy.Name, endpointList) if endpointList == nil { diff --git a/npm/pkg/dataplane/policies/testutils_linux.go b/npm/pkg/dataplane/policies/testutils_linux.go index 0710a74d39..e82619e603 100644 --- a/npm/pkg/dataplane/policies/testutils_linux.go +++ b/npm/pkg/dataplane/policies/testutils_linux.go @@ -19,6 +19,10 @@ func GetAddPolicyTestCalls(_ *NPMNetworkPolicy) []testutils.TestCmd { return []testutils.TestCmd{fakeIPTablesRestoreCommand} } +func GetAddPolicyFailureTestCalls(_ *NPMNetworkPolicy) []testutils.TestCmd { + return []testutils.TestCmd{fakeIPTablesRestoreFailureCommand} +} + func GetRemovePolicyTestCalls(policy *NPMNetworkPolicy) []testutils.TestCmd { calls := []testutils.TestCmd{} hasIngress, hasEgress := policy.hasIngressAndEgress() @@ -37,6 +41,13 @@ func GetRemovePolicyTestCalls(policy *NPMNetworkPolicy) []testutils.TestCmd { return calls } +// GetRemovePolicyFailureTestCalls fails on the restore +func GetRemovePolicyFailureTestCalls(policy *NPMNetworkPolicy) []testutils.TestCmd { + calls := GetRemovePolicyTestCalls(policy) + calls[len(calls)-1] = fakeIPTablesRestoreFailureCommand // replace the restore success with a failure + return calls +} + func GetInitializeTestCalls() []testutils.TestCmd { return []testutils.TestCmd{ fakeIPTablesRestoreCommand, // gives correct exit code From beb9b04442ed6767bd335c6d0d6b4def39a85e70 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 5 Nov 2021 17:52:07 -0700 Subject: [PATCH 2/8] remove get prefix for all functions per junguks feedback --- .../policies/chain-management_linux.go | 46 +++++----- .../policies/chain-management_linux_test.go | 12 +-- .../dataplane/policies/policymanager_linux.go | 90 +++++++++---------- .../policies/policymanager_linux_test.go | 16 ++-- npm/pkg/dataplane/policies/testutils_linux.go | 4 +- 5 files changed, 84 insertions(+), 84 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index f146ee7bc1..ecb2c1cb2b 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -91,7 +91,7 @@ func (pMgr *PolicyManager) reset() error { // AZURE-NPM chain is after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). func (pMgr *PolicyManager) initializeNPMChains() error { klog.Infof("Initializing AZURE-NPM chains.") - creator := pMgr.getCreatorForInitChains() + creator := pMgr.creatorForInitChains() err := restore(creator) if err != nil { return npmerrors.SimpleErrorWrapper("failed to create chains and rules", err) @@ -120,7 +120,7 @@ func (pMgr *PolicyManager) removeNPMChains() error { } // flush all chains (will create any chain, including deprecated ones, if they don't exist) - creatorToFlush, chainsToDelete := pMgr.getCreatorAndChainsForReset() + creatorToFlush, chainsToDelete := pMgr.creatorAndChainsForReset() restoreError := restore(creatorToFlush) if restoreError != nil { return npmerrors.SimpleErrorWrapper("failed to flush chains", restoreError) @@ -211,8 +211,8 @@ func (pMgr *PolicyManager) runIPTablesCommand(operationFlag string, args ...stri return 0, nil } -func (pMgr *PolicyManager) getCreatorForInitChains() *ioutil.FileCreator { - creator := pMgr.getNewCreatorWithChains(iptablesAzureChains) +func (pMgr *PolicyManager) creatorForInitChains() *ioutil.FileCreator { + creator := pMgr.newCreatorWithChains(iptablesAzureChains) // add AZURE-NPM chain rules creator.AddLine("", nil, util.IptablesAppendFlag, util.IptablesAzureChain, util.IptablesJumpFlag, util.IptablesAzureIngressChain) @@ -221,32 +221,32 @@ func (pMgr *PolicyManager) getCreatorForInitChains() *ioutil.FileCreator { // add AZURE-NPM-INGRESS chain rules ingressDropSpecs := []string{util.IptablesAppendFlag, util.IptablesAzureIngressChain, util.IptablesJumpFlag, util.IptablesDrop} - ingressDropSpecs = append(ingressDropSpecs, getOnMarkSpecs(util.IptablesAzureIngressDropMarkHex)...) - ingressDropSpecs = append(ingressDropSpecs, getCommentSpecs(fmt.Sprintf("DROP-ON-INGRESS-DROP-MARK-%s", util.IptablesAzureIngressDropMarkHex))...) + ingressDropSpecs = append(ingressDropSpecs, onMarkSpecs(util.IptablesAzureIngressDropMarkHex)...) + ingressDropSpecs = append(ingressDropSpecs, commentSpecs(fmt.Sprintf("DROP-ON-INGRESS-DROP-MARK-%s", util.IptablesAzureIngressDropMarkHex))...) creator.AddLine("", nil, ingressDropSpecs...) // add AZURE-NPM-INGRESS-ALLOW-MARK chain markIngressAllowSpecs := []string{util.IptablesAppendFlag, util.IptablesAzureIngressAllowMarkChain} - markIngressAllowSpecs = append(markIngressAllowSpecs, getSetMarkSpecs(util.IptablesAzureIngressAllowMarkHex)...) - markIngressAllowSpecs = append(markIngressAllowSpecs, getCommentSpecs(fmt.Sprintf("SET-INGRESS-ALLOW-MARK-%s", util.IptablesAzureIngressAllowMarkHex))...) + markIngressAllowSpecs = append(markIngressAllowSpecs, setMarkSpecs(util.IptablesAzureIngressAllowMarkHex)...) + markIngressAllowSpecs = append(markIngressAllowSpecs, commentSpecs(fmt.Sprintf("SET-INGRESS-ALLOW-MARK-%s", util.IptablesAzureIngressAllowMarkHex))...) creator.AddLine("", nil, markIngressAllowSpecs...) creator.AddLine("", nil, util.IptablesAppendFlag, util.IptablesAzureIngressAllowMarkChain, util.IptablesJumpFlag, util.IptablesAzureEgressChain) // add AZURE-NPM-EGRESS chain rules egressDropSpecs := []string{util.IptablesAppendFlag, util.IptablesAzureEgressChain, util.IptablesJumpFlag, util.IptablesDrop} - egressDropSpecs = append(egressDropSpecs, getOnMarkSpecs(util.IptablesAzureEgressDropMarkHex)...) - egressDropSpecs = append(egressDropSpecs, getCommentSpecs(fmt.Sprintf("DROP-ON-EGRESS-DROP-MARK-%s", util.IptablesAzureEgressDropMarkHex))...) + egressDropSpecs = append(egressDropSpecs, onMarkSpecs(util.IptablesAzureEgressDropMarkHex)...) + egressDropSpecs = append(egressDropSpecs, commentSpecs(fmt.Sprintf("DROP-ON-EGRESS-DROP-MARK-%s", util.IptablesAzureEgressDropMarkHex))...) creator.AddLine("", nil, egressDropSpecs...) jumpOnIngressMatchSpecs := []string{util.IptablesAppendFlag, util.IptablesAzureEgressChain, util.IptablesJumpFlag, util.IptablesAzureAcceptChain} - jumpOnIngressMatchSpecs = append(jumpOnIngressMatchSpecs, getOnMarkSpecs(util.IptablesAzureIngressAllowMarkHex)...) - jumpOnIngressMatchSpecs = append(jumpOnIngressMatchSpecs, getCommentSpecs(fmt.Sprintf("ACCEPT-ON-INGRESS-ALLOW-MARK-%s", util.IptablesAzureIngressAllowMarkHex))...) + jumpOnIngressMatchSpecs = append(jumpOnIngressMatchSpecs, onMarkSpecs(util.IptablesAzureIngressAllowMarkHex)...) + jumpOnIngressMatchSpecs = append(jumpOnIngressMatchSpecs, commentSpecs(fmt.Sprintf("ACCEPT-ON-INGRESS-ALLOW-MARK-%s", util.IptablesAzureIngressAllowMarkHex))...) creator.AddLine("", nil, jumpOnIngressMatchSpecs...) // add AZURE-NPM-ACCEPT chain rules clearSpecs := []string{util.IptablesAppendFlag, util.IptablesAzureAcceptChain} - clearSpecs = append(clearSpecs, getSetMarkSpecs(util.IptablesAzureClearMarkHex)...) - clearSpecs = append(clearSpecs, getCommentSpecs("Clear-AZURE-NPM-MARKS")...) + clearSpecs = append(clearSpecs, setMarkSpecs(util.IptablesAzureClearMarkHex)...) + clearSpecs = append(clearSpecs, commentSpecs("Clear-AZURE-NPM-MARKS")...) creator.AddLine("", nil, clearSpecs...) creator.AddLine("", nil, util.IptablesAppendFlag, util.IptablesAzureAcceptChain, util.IptablesJumpFlag, util.IptablesAccept) creator.AddLine("", nil, util.IptablesRestoreCommit) @@ -256,7 +256,7 @@ func (pMgr *PolicyManager) getCreatorForInitChains() *ioutil.FileCreator { // add/reposition AZURE-NPM chain after KUBE-FORWARD and KUBE-SERVICE chains if they exist // this function has a direct comparison in NPM v1 iptables manager (iptm.go) func (pMgr *PolicyManager) positionAzureChainJumpRule() error { - kubeServicesLine, kubeServicesLineNumErr := pMgr.getChainLineNumber(util.IptablesKubeServicesChain) + kubeServicesLine, kubeServicesLineNumErr := pMgr.chainLineNumber(util.IptablesKubeServicesChain) if kubeServicesLineNumErr != nil { // not possible to cover this branch currently because of testing limitations for PipeCommandToGrep() baseErrString := "failed to get index of jump from KUBE-SERVICES chain to FORWARD chain with error" @@ -266,7 +266,7 @@ func (pMgr *PolicyManager) positionAzureChainJumpRule() error { index := kubeServicesLine + 1 - // TODO could call getChainLineNumber instead, and say it doesn't exist for lineNum == 0 + // TODO could call chainLineNumber instead, and say it doesn't exist for lineNum == 0 jumpRuleErrCode, checkErr := pMgr.runIPTablesCommand(util.IptablesCheckFlag, jumpFromForwardToAzureChainArgs...) hadCheckError := checkErr != nil && jumpRuleErrCode != doesNotExistErrorCode if hadCheckError { @@ -293,7 +293,7 @@ func (pMgr *PolicyManager) positionAzureChainJumpRule() error { return nil } - npmChainLine, npmLineNumErr := pMgr.getChainLineNumber(util.IptablesAzureChain) + npmChainLine, npmLineNumErr := pMgr.chainLineNumber(util.IptablesAzureChain) if npmLineNumErr != nil { // not possible to cover this branch currently because of testing limitations for PipeCommandToGrep() baseErrString := "failed to get index of jump from FORWARD chain to AZURE-NPM chain" @@ -334,7 +334,7 @@ func (pMgr *PolicyManager) positionAzureChainJumpRule() error { // returns 0 if the chain d.n.e. // this function has a direct comparison in NPM v1 iptables manager (iptm.go) -func (pMgr *PolicyManager) getChainLineNumber(chain string) (int, error) { +func (pMgr *PolicyManager) chainLineNumber(chain string) (int, error) { // TODO could call this once and use regex instead of grep to cut down on OS calls listForwardEntriesCommand := pMgr.ioShim.Exec.Command(util.Iptables, util.IptablesWaitFlag, defaultlockWaitTimeInSeconds, util.IptablesTableFlag, util.IptablesFilterTable, @@ -357,20 +357,20 @@ func (pMgr *PolicyManager) getChainLineNumber(chain string) (int, error) { } // make this a function for easier testing -func (pMgr *PolicyManager) getCreatorAndChainsForReset() (creator *ioutil.FileCreator, chainsToFlush []string) { - oldPolicyChains, err := pMgr.getPolicyChainNames() +func (pMgr *PolicyManager) creatorAndChainsForReset() (creator *ioutil.FileCreator, chainsToFlush []string) { + oldPolicyChains, err := pMgr.policyChainNames() if err != nil { // not possible to cover this branch currently because of testing limitations for PipeCommandToGrep() metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to determine NPM ingress/egress policy chains to delete") } chainsToFlush = iptablesOldAndNewChains chainsToFlush = append(chainsToFlush, oldPolicyChains...) // will work even if oldPolicyChains is nil - creator = pMgr.getNewCreatorWithChains(chainsToFlush) + creator = pMgr.newCreatorWithChains(chainsToFlush) creator.AddLine("", nil, util.IptablesRestoreCommit) return } -func (pMgr *PolicyManager) getPolicyChainNames() ([]string, error) { +func (pMgr *PolicyManager) policyChainNames() ([]string, error) { iptablesListCommand := pMgr.ioShim.Exec.Command(util.Iptables, util.IptablesWaitFlag, defaultlockWaitTimeInSeconds, util.IptablesTableFlag, util.IptablesFilterTable, util.IptablesNumericFlag, util.IptablesListFlag, @@ -396,7 +396,7 @@ func (pMgr *PolicyManager) getPolicyChainNames() ([]string, error) { return chainNames, nil } -func getOnMarkSpecs(mark string) []string { +func onMarkSpecs(mark string) []string { return []string{ util.IptablesModuleFlag, util.IptablesMarkVerb, diff --git a/npm/pkg/dataplane/policies/chain-management_linux_test.go b/npm/pkg/dataplane/policies/chain-management_linux_test.go index 5a9f6c0d4f..644a7e39f1 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux_test.go +++ b/npm/pkg/dataplane/policies/chain-management_linux_test.go @@ -66,7 +66,7 @@ func TestCleanupChainsFailure(t *testing.T) { func TestInitChainsCreator(t *testing.T) { pMgr := NewPolicyManager(common.NewMockIOShim(nil)) - creator := pMgr.getCreatorForInitChains() // doesn't make any exec calls + creator := pMgr.creatorForInitChains() // doesn't make any exec calls actualLines := strings.Split(creator.ToString(), "\n") expectedLines := []string{"*filter"} for _, chain := range iptablesAzureChains { @@ -130,7 +130,7 @@ func TestRemoveChainsCreator(t *testing.T) { } pMgr := NewPolicyManager(common.NewMockIOShim(creatorCalls)) - creator, chainsToFlush := pMgr.getCreatorAndChainsForReset() + creator, chainsToFlush := pMgr.creatorAndChainsForReset() expectedChainsToFlush := []string{ "AZURE-NPM", "AZURE-NPM-INGRESS", @@ -408,7 +408,7 @@ func TestGetChainLineNumber(t *testing.T) { grepCommand, } pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - lineNum, err := pMgr.getChainLineNumber(testChainName) + lineNum, err := pMgr.chainLineNumber(testChainName) require.Equal(t, 3, lineNum) require.NoError(t, err) @@ -421,7 +421,7 @@ func TestGetChainLineNumber(t *testing.T) { grepCommand, } pMgr = NewPolicyManager(common.NewMockIOShim(calls)) - lineNum, err = pMgr.getChainLineNumber(testChainName) + lineNum, err = pMgr.chainLineNumber(testChainName) require.Equal(t, 0, lineNum) require.NoError(t, err) } @@ -437,7 +437,7 @@ func TestGetPolicyChainNames(t *testing.T) { grepCommand, } pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - chainNames, err := pMgr.getPolicyChainNames() + chainNames, err := pMgr.policyChainNames() expectedChainNames := []string{ "AZURE-NPM-INGRESS-123456", "AZURE-NPM-EGRESS-123456", @@ -454,7 +454,7 @@ func TestGetPolicyChainNames(t *testing.T) { grepCommand, } pMgr = NewPolicyManager(common.NewMockIOShim(calls)) - chainNames, err = pMgr.getPolicyChainNames() + chainNames, err = pMgr.policyChainNames() expectedChainNames = nil require.Equal(t, expectedChainNames, chainNames) require.NoError(t, err) diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index 214f3465ea..bf8b8a2488 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -21,8 +21,8 @@ const ( // shouldn't call this if the np has no ACLs (check in generic) func (pMgr *PolicyManager) addPolicy(networkPolicy *NPMNetworkPolicy, _ map[string]string) error { // TODO check for newPolicy errors - allChainNames := getAllChainNames([]*NPMNetworkPolicy{networkPolicy}) - creator := pMgr.getCreatorForNewNetworkPolicies(allChainNames, networkPolicy) + allChainNames := allChainNames([]*NPMNetworkPolicy{networkPolicy}) + creator := pMgr.creatorForNewNetworkPolicies(allChainNames, networkPolicy) err := restore(creator) if err != nil { return npmerrors.SimpleErrorWrapper("failed to restore iptables with updated policies", err) @@ -38,8 +38,8 @@ func (pMgr *PolicyManager) removePolicy(networkPolicy *NPMNetworkPolicy, _ map[s if deleteErr != nil { return npmerrors.SimpleErrorWrapper("failed to delete jumps to policy chains", deleteErr) } - allChainNames := getAllChainNames([]*NPMNetworkPolicy{networkPolicy}) - creator := pMgr.getCreatorForRemovingPolicies(allChainNames) + allChainNames := allChainNames([]*NPMNetworkPolicy{networkPolicy}) + creator := pMgr.creatorForRemovingPolicies(allChainNames) restoreErr := restore(creator) if restoreErr != nil { return npmerrors.SimpleErrorWrapper("failed to flush policies", restoreErr) @@ -59,23 +59,23 @@ func restore(creator *ioutil.FileCreator) error { } // TODO use array instead of ... -func (pMgr *PolicyManager) getCreatorForRemovingPolicies(allChainNames []string) *ioutil.FileCreator { - creator := pMgr.getNewCreatorWithChains(allChainNames) +func (pMgr *PolicyManager) creatorForRemovingPolicies(allChainNames []string) *ioutil.FileCreator { + creator := pMgr.newCreatorWithChains(allChainNames) creator.AddLine("", nil, util.IptablesRestoreCommit) return creator } // returns all chain names (ingress and egress policy chain names) -func getAllChainNames(networkPolicies []*NPMNetworkPolicy) []string { +func allChainNames(networkPolicies []*NPMNetworkPolicy) []string { chainNames := make([]string, 0) for _, networkPolicy := range networkPolicies { hasIngress, hasEgress := networkPolicy.hasIngressAndEgress() if hasIngress { - chainNames = append(chainNames, networkPolicy.getIngressChainName()) + chainNames = append(chainNames, networkPolicy.ingressChainName()) } if hasEgress { - chainNames = append(chainNames, networkPolicy.getEgressChainName()) + chainNames = append(chainNames, networkPolicy.egressChainName()) } } return chainNames @@ -92,20 +92,20 @@ func (networkPolicy *NPMNetworkPolicy) hasIngressAndEgress() (hasIngress, hasEgr return } -func (networkPolicy *NPMNetworkPolicy) getEgressChainName() string { - return networkPolicy.getChainName(util.IptablesAzureEgressPolicyChainPrefix) +func (networkPolicy *NPMNetworkPolicy) egressChainName() string { + return networkPolicy.chainName(util.IptablesAzureEgressPolicyChainPrefix) } -func (networkPolicy *NPMNetworkPolicy) getIngressChainName() string { - return networkPolicy.getChainName(util.IptablesAzureIngressPolicyChainPrefix) +func (networkPolicy *NPMNetworkPolicy) ingressChainName() string { + return networkPolicy.chainName(util.IptablesAzureIngressPolicyChainPrefix) } -func (networkPolicy *NPMNetworkPolicy) getChainName(prefix string) string { +func (networkPolicy *NPMNetworkPolicy) chainName(prefix string) string { policyHash := util.Hash(networkPolicy.Name) // assuming the name is unique return joinWithDash(prefix, policyHash) } -func (pMgr *PolicyManager) getNewCreatorWithChains(chainNames []string) *ioutil.FileCreator { +func (pMgr *PolicyManager) newCreatorWithChains(chainNames []string) *ioutil.FileCreator { creator := ioutil.NewFileCreator(pMgr.ioShim, maxRetryCount, knownLineErrorPattern, unknownLineErrorPattern) // TODO pass an array instead of this ... thing creator.AddLine("", nil, "*"+util.IptablesFilterTable) // specify the table @@ -140,13 +140,13 @@ func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, isIngress bo var baseChainName string var chainName string if isIngress { - specs = getIngressJumpSpecs(policy) + specs = ingressJumpSpecs(policy) baseChainName = util.IptablesAzureIngressChain - chainName = policy.getIngressChainName() + chainName = policy.ingressChainName() } else { - specs = getEgressJumpSpecs(policy) + specs = egressJumpSpecs(policy) baseChainName = util.IptablesAzureEgressChain - chainName = policy.getEgressChainName() + chainName = policy.egressChainName() } specs = append([]string{baseChainName}, specs...) @@ -160,22 +160,22 @@ func (pMgr *PolicyManager) deleteJumpRule(policy *NPMNetworkPolicy, isIngress bo return nil } -func getIngressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string { - chainName := networkPolicy.getIngressChainName() +func ingressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string { + chainName := networkPolicy.ingressChainName() specs := []string{util.IptablesJumpFlag, chainName} - return append(specs, getMatchSetSpecsForNetworkPolicy(networkPolicy, DstMatch)...) + return append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, DstMatch)...) } -func getEgressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string { - chainName := networkPolicy.getEgressChainName() +func egressJumpSpecs(networkPolicy *NPMNetworkPolicy) []string { + chainName := networkPolicy.egressChainName() specs := []string{util.IptablesJumpFlag, chainName} - return append(specs, getMatchSetSpecsForNetworkPolicy(networkPolicy, SrcMatch)...) + return append(specs, matchSetSpecsForNetworkPolicy(networkPolicy, SrcMatch)...) } // noflush add to chains impacted // TODO use array instead of ... -func (pMgr *PolicyManager) getCreatorForNewNetworkPolicies(allChainNames []string, networkPolicies ...*NPMNetworkPolicy) *ioutil.FileCreator { - creator := pMgr.getNewCreatorWithChains(allChainNames) +func (pMgr *PolicyManager) creatorForNewNetworkPolicies(allChainNames []string, networkPolicies ...*NPMNetworkPolicy) *ioutil.FileCreator { + creator := pMgr.newCreatorWithChains(allChainNames) ingressJumpLineNumber := 1 egressJumpLineNumber := 1 @@ -185,12 +185,12 @@ func (pMgr *PolicyManager) getCreatorForNewNetworkPolicies(allChainNames []strin // add jump rule(s) to policy chain(s) hasIngress, hasEgress := networkPolicy.hasIngressAndEgress() if hasIngress { - ingressJumpSpecs := getInsertSpecs(util.IptablesAzureIngressChain, ingressJumpLineNumber, getIngressJumpSpecs(networkPolicy)) + ingressJumpSpecs := insertSpecs(util.IptablesAzureIngressChain, ingressJumpLineNumber, ingressJumpSpecs(networkPolicy)) creator.AddLine("", nil, ingressJumpSpecs...) // TODO error handler ingressJumpLineNumber++ } if hasEgress { - egressJumpSpecs := getInsertSpecs(util.IptablesAzureEgressChain, egressJumpLineNumber, getEgressJumpSpecs(networkPolicy)) + egressJumpSpecs := insertSpecs(util.IptablesAzureEgressChain, egressJumpLineNumber, egressJumpSpecs(networkPolicy)) creator.AddLine("", nil, egressJumpSpecs...) // TODO error handler egressJumpLineNumber++ } @@ -205,40 +205,40 @@ func writeNetworkPolicyRules(creator *ioutil.FileCreator, networkPolicy *NPMNetw var chainName string var actionSpecs []string if aclPolicy.hasIngress() { - chainName = networkPolicy.getIngressChainName() + chainName = networkPolicy.ingressChainName() if aclPolicy.Target == Allowed { actionSpecs = []string{util.IptablesJumpFlag, util.IptablesAzureEgressChain} } else { - actionSpecs = getSetMarkSpecs(util.IptablesAzureIngressDropMarkHex) + actionSpecs = setMarkSpecs(util.IptablesAzureIngressDropMarkHex) } } else { - chainName = networkPolicy.getEgressChainName() + chainName = networkPolicy.egressChainName() if aclPolicy.Target == Allowed { actionSpecs = []string{util.IptablesJumpFlag, util.IptablesAzureAcceptChain} } else { - actionSpecs = getSetMarkSpecs(util.IptablesAzureEgressDropMarkHex) + actionSpecs = setMarkSpecs(util.IptablesAzureEgressDropMarkHex) } } line := []string{"-A", chainName} line = append(line, actionSpecs...) - line = append(line, getIPTablesRuleSpecs(aclPolicy)...) + line = append(line, iptablesRuleSpecs(aclPolicy)...) creator.AddLine("", nil, line...) // TODO add error handler } } -func getIPTablesRuleSpecs(aclPolicy *ACLPolicy) []string { +func iptablesRuleSpecs(aclPolicy *ACLPolicy) []string { specs := make([]string, 0) specs = append(specs, util.IptablesProtFlag, string(aclPolicy.Protocol)) // NOTE: protocol must be ALL instead of nil - specs = append(specs, getPortSpecs([]Ports{aclPolicy.DstPorts})...) - specs = append(specs, getMatchSetSpecsFromSetInfo(aclPolicy.SrcList)...) - specs = append(specs, getMatchSetSpecsFromSetInfo(aclPolicy.DstList)...) + specs = append(specs, portSpecs([]Ports{aclPolicy.DstPorts})...) + specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.SrcList)...) + specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.DstList)...) if aclPolicy.Comment != "" { - specs = append(specs, getCommentSpecs(aclPolicy.Comment)...) + specs = append(specs, commentSpecs(aclPolicy.Comment)...) } return specs } -func getPortSpecs(portRanges []Ports) []string { +func portSpecs(portRanges []Ports) []string { // TODO(jungukcho): do not need to take slices since it can only have one dst port if len(portRanges) != 1 { return []string{} @@ -252,7 +252,7 @@ func getPortSpecs(portRanges []Ports) []string { return []string{util.IptablesDstPortFlag, portRanges[0].toIPTablesString()} } -func getMatchSetSpecsForNetworkPolicy(networkPolicy *NPMNetworkPolicy, matchType MatchType) []string { +func matchSetSpecsForNetworkPolicy(networkPolicy *NPMNetworkPolicy, matchType MatchType) []string { // TODO update to use included boolean/new data structure from Junguk's PR specs := make([]string, 0, maxLengthForMatchSetSpecs*len(networkPolicy.PodSelectorIPSets)) for _, translatedIPSet := range networkPolicy.PodSelectorIPSets { @@ -263,7 +263,7 @@ func getMatchSetSpecsForNetworkPolicy(networkPolicy *NPMNetworkPolicy, matchType return specs } -func getMatchSetSpecsFromSetInfo(setInfoList []SetInfo) []string { +func matchSetSpecsFromSetInfo(setInfoList []SetInfo) []string { specs := make([]string, 0, maxLengthForMatchSetSpecs*len(setInfoList)) for _, setInfo := range setInfoList { matchString := setInfo.MatchType.toIPTablesString() @@ -277,7 +277,7 @@ func getMatchSetSpecsFromSetInfo(setInfoList []SetInfo) []string { return specs } -func getSetMarkSpecs(mark string) []string { +func setMarkSpecs(mark string) []string { return []string{ util.IptablesJumpFlag, util.IptablesMark, @@ -286,7 +286,7 @@ func getSetMarkSpecs(mark string) []string { } } -func getCommentSpecs(comment string) []string { +func commentSpecs(comment string) []string { return []string{ util.IptablesModuleFlag, util.IptablesCommentModuleFlag, @@ -295,7 +295,7 @@ func getCommentSpecs(comment string) []string { } } -func getInsertSpecs(chainName string, index int, specs []string) []string { +func insertSpecs(chainName string, index int, specs []string) []string { indexString := fmt.Sprint(index) insertSpecs := []string{util.IptablesInsertionFlag, chainName, indexString} return append(insertSpecs, specs...) diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 687289090d..4db7df8afc 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -14,10 +14,10 @@ import ( ) var ( - testPolicy1IngressChain = TestNetworkPolicies[0].getIngressChainName() - testPolicy1EgressChain = TestNetworkPolicies[0].getEgressChainName() - testPolicy2IngressChain = TestNetworkPolicies[1].getIngressChainName() - testPolicy3EgressChain = TestNetworkPolicies[2].getEgressChainName() + testPolicy1IngressChain = TestNetworkPolicies[0].ingressChainName() + testPolicy1EgressChain = TestNetworkPolicies[0].egressChainName() + testPolicy2IngressChain = TestNetworkPolicies[1].ingressChainName() + testPolicy3EgressChain = TestNetworkPolicies[2].egressChainName() testPolicy1IngressJump = fmt.Sprintf("-j %s -m set --match-set %s dst", testPolicy1IngressChain, ipsets.TestKVNSList.HashedName) testPolicy1EgressJump = fmt.Sprintf("-j %s -m set --match-set %s src", testPolicy1EgressChain, ipsets.TestKVNSList.HashedName) @@ -36,15 +36,15 @@ var ( func TestChainNames(t *testing.T) { expectedName := fmt.Sprintf("AZURE-NPM-INGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) - require.Equal(t, expectedName, TestNetworkPolicies[0].getIngressChainName()) + require.Equal(t, expectedName, TestNetworkPolicies[0].ingressChainName()) expectedName = fmt.Sprintf("AZURE-NPM-EGRESS-%s", util.Hash(TestNetworkPolicies[0].Name)) - require.Equal(t, expectedName, TestNetworkPolicies[0].getEgressChainName()) + require.Equal(t, expectedName, TestNetworkPolicies[0].egressChainName()) } func TestAddPolicies(t *testing.T) { calls := []testutils.TestCmd{fakeIPTablesRestoreCommand} pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - creator := pMgr.getCreatorForNewNetworkPolicies(getAllChainNames(TestNetworkPolicies), TestNetworkPolicies...) + creator := pMgr.creatorForNewNetworkPolicies(allChainNames(TestNetworkPolicies), TestNetworkPolicies...) actualLines := strings.Split(creator.ToString(), "\n") expectedLines := []string{ "*filter", @@ -89,7 +89,7 @@ func TestRemovePolicies(t *testing.T) { fakeIPTablesRestoreCommand, } pMgr := NewPolicyManager(common.NewMockIOShim(calls)) - creator := pMgr.getCreatorForRemovingPolicies(getAllChainNames(TestNetworkPolicies)) + creator := pMgr.creatorForRemovingPolicies(allChainNames(TestNetworkPolicies)) actualLines := strings.Split(creator.ToString(), "\n") expectedLines := []string{ "*filter", diff --git a/npm/pkg/dataplane/policies/testutils_linux.go b/npm/pkg/dataplane/policies/testutils_linux.go index e82619e603..2220aba949 100644 --- a/npm/pkg/dataplane/policies/testutils_linux.go +++ b/npm/pkg/dataplane/policies/testutils_linux.go @@ -28,12 +28,12 @@ func GetRemovePolicyTestCalls(policy *NPMNetworkPolicy) []testutils.TestCmd { hasIngress, hasEgress := policy.hasIngressAndEgress() if hasIngress { deleteIngressJumpSpecs := []string{"iptables", "-w", "60", "-D", util.IptablesAzureIngressChain} - deleteIngressJumpSpecs = append(deleteIngressJumpSpecs, getIngressJumpSpecs(policy)...) + deleteIngressJumpSpecs = append(deleteIngressJumpSpecs, ingressJumpSpecs(policy)...) calls = append(calls, testutils.TestCmd{Cmd: deleteIngressJumpSpecs}) } if hasEgress { deleteEgressJumpSpecs := []string{"iptables", "-w", "60", "-D", util.IptablesAzureEgressChain} - deleteEgressJumpSpecs = append(deleteEgressJumpSpecs, getEgressJumpSpecs(policy)...) + deleteEgressJumpSpecs = append(deleteEgressJumpSpecs, egressJumpSpecs(policy)...) calls = append(calls, testutils.TestCmd{Cmd: deleteEgressJumpSpecs}) } From 95b6e407cdd4e6cf5f1139c3b83de55fca19cfa0 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 5 Nov 2021 17:58:23 -0700 Subject: [PATCH 3/8] clean up code for port specs and fix a lint --- npm/pkg/dataplane/policies/policymanager_linux.go | 15 ++++----------- .../policies/policymanager_linux_test.go | 3 +-- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index bf8b8a2488..bab521cc0e 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -229,7 +229,7 @@ func writeNetworkPolicyRules(creator *ioutil.FileCreator, networkPolicy *NPMNetw func iptablesRuleSpecs(aclPolicy *ACLPolicy) []string { specs := make([]string, 0) specs = append(specs, util.IptablesProtFlag, string(aclPolicy.Protocol)) // NOTE: protocol must be ALL instead of nil - specs = append(specs, portSpecs([]Ports{aclPolicy.DstPorts})...) + specs = append(specs, dstPortSpecs(aclPolicy.DstPorts)...) specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.SrcList)...) specs = append(specs, matchSetSpecsFromSetInfo(aclPolicy.DstList)...) if aclPolicy.Comment != "" { @@ -238,18 +238,11 @@ func iptablesRuleSpecs(aclPolicy *ACLPolicy) []string { return specs } -func portSpecs(portRanges []Ports) []string { - // TODO(jungukcho): do not need to take slices since it can only have one dst port - if len(portRanges) != 1 { +func dstPortSpecs(portRange Ports) []string { + if portRange.Port == 0 && portRange.EndPort == 0 { return []string{} } - - // TODO(jungukcho): temporary solution and need to fix it. - if portRanges[0].Port == 0 && portRanges[0].EndPort == 0 { - return []string{} - } - - return []string{util.IptablesDstPortFlag, portRanges[0].toIPTablesString()} + return []string{util.IptablesDstPortFlag, portRange.toIPTablesString()} } func matchSetSpecsForNetworkPolicy(networkPolicy *NPMNetworkPolicy, matchType MatchType) []string { diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 4db7df8afc..23c344a967 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -159,17 +159,16 @@ func TestUpdatingChainsToCleanup(t *testing.T) { // TODO defer ioshim.VerifyCalls(t, ioshim, calls) pMgr := NewPolicyManager(ioshim) - // FIXME off because of grep stuff require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[0], nil)) assertEqualCleanupContents(t, pMgr) require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil)) assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // TODO uncomment when grep stuff is fixed // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[1], nil)) // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[1].Name, nil)) // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) - // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[2], nil)) // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[2].Name, nil)) From 1504b70aa5f711e18669ea9a930fe64a8299f0cb Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 12 Nov 2021 12:44:39 -0800 Subject: [PATCH 4/8] address comments --- .../policies/chain-management_linux.go | 48 ++++++++++++------- .../policies/chain-management_linux_test.go | 35 +++++++++----- npm/pkg/dataplane/policies/policymanager.go | 11 +++-- .../dataplane/policies/policymanager_linux.go | 4 +- .../policies/policymanager_linux_test.go | 22 +++------ .../policies/policymanager_windows.go | 6 +-- 6 files changed, 70 insertions(+), 56 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index ecb2c1cb2b..7b4ea8b241 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -53,12 +53,35 @@ var ( ingressOrEgressPolicyChainPattern = fmt.Sprintf("'Chain %s-\\|Chain %s-'", util.IptablesAzureIngressPolicyChainPrefix, util.IptablesAzureEgressPolicyChainPrefix) ) -type osTools struct { +type staleChains struct { chainsToCleanup map[string]struct{} } -func makeTools() osTools { - return osTools{make(map[string]struct{})} +func newStaleChains() *staleChains { + return &staleChains{make(map[string]struct{})} +} + +func (s *staleChains) add(chain string) { + s.chainsToCleanup[chain] = struct{}{} +} + +func (s *staleChains) remove(chain string) { + delete(s.chainsToCleanup, chain) +} + +func (s *staleChains) emptyAndGetAll() []string { + result := make([]string, len(s.chainsToCleanup)) + k := 0 + for chain := range s.chainsToCleanup { + result[k] = chain + s.remove(chain) + k++ + } + return result +} + +func (s *staleChains) empty() { + s.chainsToCleanup = make(map[string]struct{}) } func (pMgr *PolicyManager) reboot() error { @@ -83,7 +106,7 @@ func (pMgr *PolicyManager) reset() error { if err := pMgr.removeNPMChains(); err != nil { return npmerrors.SimpleErrorWrapper("failed to remove NPM chains", err) } - pMgr.chainsToCleanup = make(map[string]struct{}) + pMgr.staleChains.empty() return nil } @@ -149,29 +172,18 @@ func (pMgr *PolicyManager) reconcile(stopChannel <-chan struct{}) { if err := pMgr.positionAzureChainJumpRule(); err != nil { klog.Errorf("failed to reconcile jump rule to Azure-NPM due to %s", err.Error()) } - if err := pMgr.cleanupChains(pMgr.oldPolicyChains()); err != nil { + if err := pMgr.cleanupChains(pMgr.staleChains.emptyAndGetAll()); err != nil { klog.Errorf("failed to clean up old policy chains with the following error %s", err.Error()) } } -func (pMgr *PolicyManager) oldPolicyChains() []string { - result := make([]string, len(pMgr.chainsToCleanup)) - k := 0 - for chain := range pMgr.chainsToCleanup { - result[k] = chain - k++ - } - return result -} - // have to use slice argument for deterministic behavior for UTs func (pMgr *PolicyManager) cleanupChains(chains []string) error { var aggregateError error for _, chain := range chains { errCode, err := pMgr.runIPTablesCommand(util.IptablesDestroyFlag, chain) // TODO run the one that ignores doesNotExistErrorCode - if err == nil || errCode == doesNotExistErrorCode { - delete(pMgr.chainsToCleanup, chain) - } else { + if err != nil && errCode != doesNotExistErrorCode { + pMgr.staleChains.add(chain) currentErrString := fmt.Sprintf("failed to clean up policy chain %s with err [%v]", chain, err) if aggregateError == nil { aggregateError = npmerrors.SimpleError(currentErrString) diff --git a/npm/pkg/dataplane/policies/chain-management_linux_test.go b/npm/pkg/dataplane/policies/chain-management_linux_test.go index 644a7e39f1..cbd582e1ae 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux_test.go +++ b/npm/pkg/dataplane/policies/chain-management_linux_test.go @@ -18,14 +18,23 @@ const ( testChain3 = "chain3" ) -func TestOldPolicyChains(t *testing.T) { +func TestEmptyAndGetAll(t *testing.T) { pMgr := NewPolicyManager(common.NewMockIOShim(nil)) - pMgr.chainsToCleanup[testChain1] = struct{}{} - pMgr.chainsToCleanup[testChain2] = struct{}{} - chainsToCleanup := pMgr.oldPolicyChains() + pMgr.staleChains.add(testChain1) + pMgr.staleChains.add(testChain2) + chainsToCleanup := pMgr.staleChains.emptyAndGetAll() require.Equal(t, 2, len(chainsToCleanup)) require.True(t, chainsToCleanup[0] == testChain1 || chainsToCleanup[1] == testChain1) require.True(t, chainsToCleanup[0] == testChain2 || chainsToCleanup[1] == testChain2) + assertStaleChainsContain(t, pMgr.staleChains) +} + +func assertStaleChainsContain(t *testing.T, s *staleChains, expectedChains ...string) { + require.Equal(t, len(expectedChains), len(s.chainsToCleanup), "incorrectly tracking chains for cleanup") + for _, chain := range expectedChains { + _, exists := s.chainsToCleanup[chain] + require.True(t, exists, "incorrectly tracking chains for cleanup") + } } func TestCleanupChainsSuccess(t *testing.T) { @@ -37,12 +46,12 @@ func TestCleanupChainsSuccess(t *testing.T) { // TODO defer ioshim.VerifyCalls(t, ioshim, calls) pMgr := NewPolicyManager(ioshim) - pMgr.chainsToCleanup[testChain1] = struct{}{} - pMgr.chainsToCleanup[testChain2] = struct{}{} - chainsToCleanup := pMgr.oldPolicyChains() + pMgr.staleChains.add(testChain1) + pMgr.staleChains.add(testChain2) + chainsToCleanup := pMgr.staleChains.emptyAndGetAll() sort.Strings(chainsToCleanup) require.NoError(t, pMgr.cleanupChains(chainsToCleanup)) - assertEqualCleanupContents(t, pMgr) + assertStaleChainsContain(t, pMgr.staleChains) } func TestCleanupChainsFailure(t *testing.T) { @@ -55,13 +64,13 @@ func TestCleanupChainsFailure(t *testing.T) { // TODO defer ioshim.VerifyCalls(t, ioshim, calls) pMgr := NewPolicyManager(ioshim) - pMgr.chainsToCleanup[testChain1] = struct{}{} - pMgr.chainsToCleanup[testChain2] = struct{}{} - pMgr.chainsToCleanup[testChain3] = struct{}{} - chainsToCleanup := pMgr.oldPolicyChains() + pMgr.staleChains.add(testChain1) + pMgr.staleChains.add(testChain2) + pMgr.staleChains.add(testChain3) + chainsToCleanup := pMgr.staleChains.emptyAndGetAll() sort.Strings(chainsToCleanup) require.Error(t, pMgr.cleanupChains(chainsToCleanup)) - assertEqualCleanupContents(t, pMgr, testChain1, testChain3) + assertStaleChainsContain(t, pMgr.staleChains, testChain1, testChain3) } func TestInitChainsCreator(t *testing.T) { diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index 257bc61184..b394cbab08 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -15,9 +15,9 @@ type PolicyMap struct { } type PolicyManager struct { - policyMap *PolicyMap - ioShim *common.IOShim - osTools + policyMap *PolicyMap + ioShim *common.IOShim + staleChains *staleChains sync.Mutex } @@ -26,8 +26,8 @@ func NewPolicyManager(ioShim *common.IOShim) *PolicyManager { policyMap: &PolicyMap{ cache: make(map[string]*NPMNetworkPolicy), }, - ioShim: ioShim, - osTools: makeTools(), + ioShim: ioShim, + staleChains: newStaleChains(), } } @@ -45,6 +45,7 @@ func (pMgr *PolicyManager) Reset() error { return nil } +// TODO Windows doesn't need this go routine func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { go func() { ticker := time.NewTicker(time.Minute * time.Duration(reconcileChainTimeInMinutes)) diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index bab521cc0e..48d55a5fe3 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -28,7 +28,7 @@ func (pMgr *PolicyManager) addPolicy(networkPolicy *NPMNetworkPolicy, _ map[stri return npmerrors.SimpleErrorWrapper("failed to restore iptables with updated policies", err) } for _, chain := range allChainNames { - delete(pMgr.chainsToCleanup, chain) + pMgr.staleChains.remove(chain) } return nil } @@ -45,7 +45,7 @@ func (pMgr *PolicyManager) removePolicy(networkPolicy *NPMNetworkPolicy, _ map[s return npmerrors.SimpleErrorWrapper("failed to flush policies", restoreErr) } for _, chain := range allChainNames { - pMgr.chainsToCleanup[chain] = struct{}{} + pMgr.staleChains.add(chain) } return nil } diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 23c344a967..c6d2722cff 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -160,27 +160,19 @@ func TestUpdatingChainsToCleanup(t *testing.T) { pMgr := NewPolicyManager(ioshim) require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[0], nil)) - assertEqualCleanupContents(t, pMgr) + assertStaleChainsContain(t, pMgr.staleChains) require.NoError(t, pMgr.RemovePolicy(TestNetworkPolicies[0].Name, nil)) - assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // TODO uncomment when grep stuff is fixed // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[1], nil)) - // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[1].Name, nil)) - // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[2], nil)) - // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain) + // assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain) // require.Error(t, pMgr.RemovePolicy(TestNetworkPolicies[2].Name, nil)) - // assertEqualCleanupContents(t, pMgr, testPolicy1IngressChain, testPolicy1EgressChain, testPolicy3EgressChain) + // assertStaleChainsContain(t, pMgr.staleChains, testPolicy1IngressChain, testPolicy1EgressChain, testPolicy3EgressChain) // require.NoError(t, pMgr.AddPolicy(TestNetworkPolicies[0], nil)) - // assertEqualCleanupContents(t, pMgr, testPolicy3EgressChain) -} - -func assertEqualCleanupContents(t *testing.T, pMgr *PolicyManager, expectedChains ...string) { - require.Equal(t, len(expectedChains), len(pMgr.chainsToCleanup), "incorrectly tracking chains for cleanup") - for _, chain := range expectedChains { - _, exists := pMgr.chainsToCleanup[chain] - require.True(t, exists, "incorrectly tracking chains for cleanup") - } + // assertStaleChainsContain(t, pMgr.staleChains, testPolicy3EgressChain) } diff --git a/npm/pkg/dataplane/policies/policymanager_windows.go b/npm/pkg/dataplane/policies/policymanager_windows.go index b1157b0800..cb288c1802 100644 --- a/npm/pkg/dataplane/policies/policymanager_windows.go +++ b/npm/pkg/dataplane/policies/policymanager_windows.go @@ -15,15 +15,15 @@ var ( ErrFailedUnMarshalACLSettings = errors.New("Failed to unmarshal ACL settings") ) -type osTools struct{} +type staleChains struct{} // unused in Windows type endpointPolicyBuilder struct { aclPolicies []*NPMACLPolSettings otherPolicies []hcn.EndpointPolicy } -func makeTools() osTools { - return osTools{} +func newStaleChains() *staleChains { + return &staleChains{} } func (pMgr *PolicyManager) reboot() error { From 77bc75f531acd58a56d31c09f49883708bd53e42 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 12 Nov 2021 12:56:36 -0800 Subject: [PATCH 5/8] remove stop channel in OS-specific reconcile --- npm/pkg/dataplane/policies/chain-management_linux.go | 2 +- npm/pkg/dataplane/policies/policymanager.go | 3 +-- npm/pkg/dataplane/policies/policymanager_windows.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index 7b4ea8b241..5f52ba281d 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -168,7 +168,7 @@ func (pMgr *PolicyManager) removeNPMChains() error { // reconcile does the following: // - cleans up old policy chains // - creates the jump rule from FORWARD chain to AZURE-NPM chain (if it d.n.e) and makes sure it's after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). -func (pMgr *PolicyManager) reconcile(stopChannel <-chan struct{}) { +func (pMgr *PolicyManager) reconcile() { if err := pMgr.positionAzureChainJumpRule(); err != nil { klog.Errorf("failed to reconcile jump rule to Azure-NPM due to %s", err.Error()) } diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index b394cbab08..0f38574f7f 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -45,7 +45,6 @@ func (pMgr *PolicyManager) Reset() error { return nil } -// TODO Windows doesn't need this go routine func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { go func() { ticker := time.NewTicker(time.Minute * time.Duration(reconcileChainTimeInMinutes)) @@ -58,7 +57,7 @@ func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { case <-ticker.C: pMgr.Lock() defer pMgr.Unlock() - pMgr.reconcile(stopChannel) + pMgr.reconcile() } } }() diff --git a/npm/pkg/dataplane/policies/policymanager_windows.go b/npm/pkg/dataplane/policies/policymanager_windows.go index cb288c1802..acbcb55267 100644 --- a/npm/pkg/dataplane/policies/policymanager_windows.go +++ b/npm/pkg/dataplane/policies/policymanager_windows.go @@ -41,7 +41,7 @@ func (pMgr *PolicyManager) reset() error { return nil } -func (pMgr *PolicyManager) reconcile(stopChannel <-chan struct{}) { +func (pMgr *PolicyManager) reconcile() { // TODO } From 1112cca1f1b617dfc63725cde52a1353967d6a84 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 15 Nov 2021 18:02:39 -0800 Subject: [PATCH 6/8] move policy methods to policy_linux.go --- npm/pkg/dataplane/policies/policy_linux.go | 27 +++++++++++++++++++ .../dataplane/policies/policymanager_linux.go | 24 ----------------- 2 files changed, 27 insertions(+), 24 deletions(-) create mode 100644 npm/pkg/dataplane/policies/policy_linux.go diff --git a/npm/pkg/dataplane/policies/policy_linux.go b/npm/pkg/dataplane/policies/policy_linux.go new file mode 100644 index 0000000000..9683a65642 --- /dev/null +++ b/npm/pkg/dataplane/policies/policy_linux.go @@ -0,0 +1,27 @@ +package policies + +import "github.com/Azure/azure-container-networking/npm/util" + +// returns two booleans indicating whether the network policy has ingress and egress respectively +func (networkPolicy *NPMNetworkPolicy) hasIngressAndEgress() (hasIngress, hasEgress bool) { + hasIngress = false + hasEgress = false + for _, aclPolicy := range networkPolicy.ACLs { + hasIngress = hasIngress || aclPolicy.hasIngress() + hasEgress = hasEgress || aclPolicy.hasEgress() + } + return +} + +func (networkPolicy *NPMNetworkPolicy) egressChainName() string { + return networkPolicy.chainName(util.IptablesAzureEgressPolicyChainPrefix) +} + +func (networkPolicy *NPMNetworkPolicy) ingressChainName() string { + return networkPolicy.chainName(util.IptablesAzureIngressPolicyChainPrefix) +} + +func (networkPolicy *NPMNetworkPolicy) chainName(prefix string) string { + policyHash := util.Hash(networkPolicy.Name) // assuming the name is unique + return joinWithDash(prefix, policyHash) +} diff --git a/npm/pkg/dataplane/policies/policymanager_linux.go b/npm/pkg/dataplane/policies/policymanager_linux.go index 48d55a5fe3..a40a3ba59e 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux.go +++ b/npm/pkg/dataplane/policies/policymanager_linux.go @@ -81,30 +81,6 @@ func allChainNames(networkPolicies []*NPMNetworkPolicy) []string { return chainNames } -// returns two booleans indicating whether the network policy has ingress and egress respectively -func (networkPolicy *NPMNetworkPolicy) hasIngressAndEgress() (hasIngress, hasEgress bool) { - hasIngress = false - hasEgress = false - for _, aclPolicy := range networkPolicy.ACLs { - hasIngress = hasIngress || aclPolicy.hasIngress() - hasEgress = hasEgress || aclPolicy.hasEgress() - } - return -} - -func (networkPolicy *NPMNetworkPolicy) egressChainName() string { - return networkPolicy.chainName(util.IptablesAzureEgressPolicyChainPrefix) -} - -func (networkPolicy *NPMNetworkPolicy) ingressChainName() string { - return networkPolicy.chainName(util.IptablesAzureIngressPolicyChainPrefix) -} - -func (networkPolicy *NPMNetworkPolicy) chainName(prefix string) string { - policyHash := util.Hash(networkPolicy.Name) // assuming the name is unique - return joinWithDash(prefix, policyHash) -} - func (pMgr *PolicyManager) newCreatorWithChains(chainNames []string) *ioutil.FileCreator { creator := ioutil.NewFileCreator(pMgr.ioShim, maxRetryCount, knownLineErrorPattern, unknownLineErrorPattern) // TODO pass an array instead of this ... thing From ae909ff1ac2c2a31b0f50ff7352fadf4ab2d7101 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 16 Nov 2021 15:46:48 -0800 Subject: [PATCH 7/8] add comments based on suggestions --- npm/pkg/dataplane/policies/chain-management_linux.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index 5f52ba281d..a43c31f2e7 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -84,6 +84,9 @@ func (s *staleChains) empty() { s.chainsToCleanup = make(map[string]struct{}) } +// A proactive approach to avoid time to install default chains when the first networkpolicy comes again. +// Different from v1, which uninits when there are no policies and initializes when there are policies. +// The dataplane also initializes when it's created, so this keeps the policymanager in-line with that philosophy of having chains initialized at all times. func (pMgr *PolicyManager) reboot() error { // TODO for the sake of UTs, need to have a pMgr config specifying whether or not this reboot happens // if err := pMgr.reset(); err != nil { @@ -166,8 +169,8 @@ func (pMgr *PolicyManager) removeNPMChains() error { } // reconcile does the following: -// - cleans up old policy chains -// - creates the jump rule from FORWARD chain to AZURE-NPM chain (if it d.n.e) and makes sure it's after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). +// - cleans up stale policy chains +// - creates the jump rule from FORWARD chain to AZURE-NPM chain (if it does not exist) and makes sure it's after the jumps to KUBE-FORWARD & KUBE-SERVICES chains (if they exist). func (pMgr *PolicyManager) reconcile() { if err := pMgr.positionAzureChainJumpRule(); err != nil { klog.Errorf("failed to reconcile jump rule to Azure-NPM due to %s", err.Error()) From fae1c4cdbe2b05b384c520e29df8b8071eaf6538 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Wed, 17 Nov 2021 09:33:21 -0800 Subject: [PATCH 8/8] fix build issue: move a constant from linux file to generic file --- npm/pkg/dataplane/policies/chain-management_linux.go | 1 - npm/pkg/dataplane/policies/policymanager.go | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/npm/pkg/dataplane/policies/chain-management_linux.go b/npm/pkg/dataplane/policies/chain-management_linux.go index a43c31f2e7..4ce6053caa 100644 --- a/npm/pkg/dataplane/policies/chain-management_linux.go +++ b/npm/pkg/dataplane/policies/chain-management_linux.go @@ -16,7 +16,6 @@ import ( const ( defaultlockWaitTimeInSeconds string = "60" - reconcileChainTimeInMinutes int = 5 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 diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index 0f38574f7f..e6cc1fc462 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -10,6 +10,8 @@ import ( "k8s.io/klog" ) +const reconcileChainTimeInMinutes = 5 + type PolicyMap struct { cache map[string]*NPMNetworkPolicy }