fix: stop checking kernel version. default nft, never crash

Signed-off-by: Hunter Gregory <[email protected]>

Author: huntergregory

npm/pkg/dataplane/policies/chain-management_linux.go

@@ -24,15 +24,9 @@ const (
 
 	// transferred from iptm.go and not sure why this length is important
 	minLineNumberStringLength int = 3
-
-	detectingErrMsg = "failed to detect iptables version. failed to run iptables-legacy-save, run iptables-nft-save, and get kernel version. NPM will crash to retry"
-
-	minNftKernelVersion = 5
 )
 
 var (
-	errDetectingIptablesVersion = errors.New(detectingErrMsg)
-
 	// Must loop through a slice because we need a deterministic order for fexec commands for UTs.
 	iptablesAzureChains = []string{
 		util.IptablesAzureChain,
@@ -194,9 +188,7 @@ func (pMgr *PolicyManager) bootup(_ []string) error {
 	klog.Infof("booting up iptables Azure chains")
 
 	// 0.1. Detect iptables version
-	if err := pMgr.detectIptablesVersion(); err != nil {
-		return npmerrors.SimpleErrorWrapper("failed to detect iptables version", err)
-	}
+	pMgr.detectIptablesVersion()
 
 	// Stop reconciling so we don't contend for iptables, and so we don't update the staleChains at the same time as reconcile()
 	// Reconciling would only be happening if this function were called to reset iptables well into the azure-npm pod lifecycle.
@@ -254,47 +246,22 @@ func (pMgr *PolicyManager) bootupAfterDetectAndCleanup() error {
 // NPM should use the same iptables version as kube-proxy.
 // kube-proxy creates an iptables chain as a hint for which version it uses.
 // For more details, see: https://kubernetes.io/blog/2022/09/07/iptables-chains-not-api/#use-case-iptables-mode
-func (pMgr *PolicyManager) detectIptablesVersion() error {
+func (pMgr *PolicyManager) detectIptablesVersion() {
 	klog.Info("first attempt detecting iptables version. looking for hint/canary chain in iptables-nft")
 	if pMgr.hintOrCanaryChainExist(util.IptablesNft) {
 		util.SetIptablesToNft()
-		return nil
+		return
 	}
 
 	klog.Info("second attempt detecting iptables version. looking for hint/canary chain in iptables-legacy")
 	if pMgr.hintOrCanaryChainExist(util.IptablesLegacy) {
 		util.SetIptablesToLegacy()
-		return nil
-	}
-
-	// at this point, chains do not exist in iptables legacy/nft and/or iptables commands have failed for other reasons
-	klog.Info("third attempt detecting iptables version. getting kernel version")
-	var majorVersion int
-	var versionError error
-	if pMgr.debug {
-		// for testing purposes
-		majorVersion = pMgr.debugKernelVersion
-		versionError = pMgr.debugKernelVersionErr
-	} else {
-		majorVersion, versionError = util.KernelReleaseMajorVersion()
-	}
-	if versionError != nil {
-		return errDetectingIptablesVersion
-	}
-
-	if majorVersion >= minNftKernelVersion {
-		msg := "detected iptables version on third attempt. found kernel version >= 5. NPM will use iptables-nft. kernel version: %d"
-		klog.Infof(msg, majorVersion)
-		metrics.SendLog(util.IptmID, fmt.Sprintf(msg, majorVersion), metrics.DonotPrint)
-		util.SetIptablesToNft()
-		return nil
+		return
 	}
 
-	msg := "detected iptables version on third attempt. found kernel version < 5. NPM will use iptables-legacy. kernel version: %d"
-	klog.Infof(msg, majorVersion)
-	metrics.SendLog(util.IptmID, fmt.Sprintf(msg, majorVersion), metrics.DonotPrint)
-	util.SetIptablesToLegacy()
-	return nil
+	// default to nft if nothing is found
+	util.SetIptablesToNft()
+	return
 }
 
 func (pMgr *PolicyManager) hintOrCanaryChainExist(iptablesCmd string) bool {

npm/pkg/dataplane/policies/chain-management_linux_test.go

@@ -1,7 +1,6 @@
 package policies
 
 import (
-	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -45,8 +44,6 @@ Chain AZURE-NPM-INGRESS (1 references)
 `
 )
 
-var errKernelVersion = errors.New("kernel error")
-
 func TestStaleChainsForceLock(t *testing.T) {
 	testChains := []string{}
 	for i := 0; i < 100000; i++ {
@@ -59,7 +56,6 @@ func TestStaleChainsForceLock(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	// don't verify calls because there shouldn't be as many commands as we create if forceLock works properly
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	start := make(chan struct{}, 1)
 	done := make(chan struct{}, 1)
@@ -146,7 +142,6 @@ func TestCleanupChainsSuccess(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	pMgr.staleChains.add(testChain1)
 	pMgr.staleChains.add(testChain2)
@@ -165,7 +160,6 @@ func TestCleanupChainsFailure(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	pMgr.staleChains.add(testChain1)
 	pMgr.staleChains.add(testChain2)
@@ -484,7 +478,6 @@ func TestBootupLinux(t *testing.T) {
 			ioshim := common.NewMockIOShim(tt.calls)
 			defer ioshim.VerifyCalls(t, tt.calls)
 			pMgr := NewPolicyManager(ioshim, ipsetConfig)
-			util.SetIptablesToNft()
 			err := pMgr.bootupAfterDetectAndCleanup()
 			if tt.wantErr {
 				require.Error(t, err)
@@ -773,7 +766,6 @@ func TestPositionAzureChainJumpRule(t *testing.T) {
 				PlaceAzureChainFirst: tt.placeAzureChainFirst,
 			}
 			pMgr := NewPolicyManager(ioshim, cfg)
-			util.SetIptablesToNft()
 
 			err := pMgr.positionAzureChainJumpRule()
 			if tt.wantErr {
@@ -866,7 +858,6 @@ func TestChainLineNumber(t *testing.T) {
 			ioshim := common.NewMockIOShim(tt.calls)
 			defer ioshim.VerifyCalls(t, tt.calls)
 			pMgr := NewPolicyManager(ioshim, ipsetConfig)
-			util.SetIptablesToNft()
 
 			lineNum, err := pMgr.chainLineNumber(testChainName)
 			if tt.wantErr {
@@ -903,10 +894,7 @@ func stringsToMap(items []string) map[string]struct{} {
 func TestDetectIptablesVersion(t *testing.T) {
 	type args struct {
 		name                    string
-		kernelVersion           int
-		kernelVersionErr        error
 		calls                   []testutils.TestCmd
-		expectedErr             bool
 		expectedIptablesVersion string
 	}
 
@@ -919,7 +907,6 @@ func TestDetectIptablesVersion(t *testing.T) {
 					ExitCode: 0,
 				},
 			},
-			expectedErr:             false,
 			expectedIptablesVersion: util.IptablesNft,
 		},
 		{
@@ -934,7 +921,6 @@ func TestDetectIptablesVersion(t *testing.T) {
 					ExitCode: 0,
 				},
 			},
-			expectedErr:             false,
 			expectedIptablesVersion: util.IptablesNft,
 		},
 		{
@@ -953,36 +939,10 @@ func TestDetectIptablesVersion(t *testing.T) {
 					ExitCode: 0,
 				},
 			},
-			expectedErr:             false,
 			expectedIptablesVersion: util.IptablesLegacy,
 		},
 		{
-			name:          "nft and legacy both fail: kernel version >= 5",
-			kernelVersion: 5,
-			calls: []testutils.TestCmd{
-				{
-					Cmd:      []string{"iptables-nft", "-w", "60", "-L", "KUBE-IPTABLES-HINT", "-t", "mangle", "-n"},
-					ExitCode: 2,
-				},
-				{
-					Cmd:      []string{"iptables-nft", "-w", "60", "-L", "KUBE-KUBELET-CANARY", "-t", "mangle", "-n"},
-					ExitCode: 2,
-				},
-				{
-					Cmd:      []string{"iptables", "-w", "60", "-L", "KUBE-IPTABLES-HINT", "-t", "mangle", "-n"},
-					ExitCode: 2,
-				},
-				{
-					Cmd:      []string{"iptables", "-w", "60", "-L", "KUBE-KUBELET-CANARY", "-t", "mangle", "-n"},
-					ExitCode: 2,
-				},
-			},
-			expectedErr:             false,
-			expectedIptablesVersion: util.IptablesNft,
-		},
-		{
-			name:          "no kube chains: kernel version < 5",
-			kernelVersion: 4,
+			name: "no kube chains: default nft",
 			calls: []testutils.TestCmd{
 				{
 					Cmd:      []string{"iptables-nft", "-w", "60", "-L", "KUBE-IPTABLES-HINT", "-t", "mangle", "-n"},
@@ -1001,64 +961,50 @@ func TestDetectIptablesVersion(t *testing.T) {
 					ExitCode: 1,
 				},
 			},
-			expectedErr:             false,
-			expectedIptablesVersion: util.IptablesLegacy,
+			expectedIptablesVersion: util.IptablesNft,
 		},
 		{
-			name:             "no kube chains: kernel version error",
-			kernelVersionErr: errKernelVersion,
+			name: "nft and legacy both fail: default nft",
 			calls: []testutils.TestCmd{
 				{
 					Cmd:      []string{"iptables-nft", "-w", "60", "-L", "KUBE-IPTABLES-HINT", "-t", "mangle", "-n"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables-nft", "-w", "60", "-L", "KUBE-KUBELET-CANARY", "-t", "mangle", "-n"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-L", "KUBE-IPTABLES-HINT", "-t", "mangle", "-n"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-L", "KUBE-KUBELET-CANARY", "-t", "mangle", "-n"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 			},
-			expectedErr: true,
+			expectedIptablesVersion: util.IptablesNft,
 		},
 	}
 
 	for _, tt := range tests {
 		tt := tt
 
-		if tt.name != "no kube chains: kernel version error" {
-			continue
-		}
-
 		t.Run(tt.name, func(t *testing.T) {
 
 			metrics.InitializeAll()
 
 			ioshim := common.NewMockIOShim(tt.calls)
 			defer ioshim.VerifyCalls(t, tt.calls)
 			cfg := &PolicyManagerCfg{
-				debug:                 true,
-				debugKernelVersion:    tt.kernelVersion,
-				debugKernelVersionErr: tt.kernelVersionErr,
-				NodeIP:                "6.7.8.9",
-				PolicyMode:            IPSetPolicyMode,
-				PlaceAzureChainFirst:  util.PlaceAzureChainFirst,
+				NodeIP:               "6.7.8.9",
+				PolicyMode:           IPSetPolicyMode,
+				PlaceAzureChainFirst: util.PlaceAzureChainFirst,
 			}
 			pMgr := NewPolicyManager(ioshim, cfg)
-			err := pMgr.detectIptablesVersion()
+			pMgr.detectIptablesVersion()
 
-			if tt.expectedErr {
-				require.Error(t, err)
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, tt.expectedIptablesVersion, util.Iptables)
-			}
+			require.Equal(t, tt.expectedIptablesVersion, util.Iptables)
 		})
 	}
 }
@@ -1268,19 +1214,19 @@ func TestCleanupOtherChains(t *testing.T) {
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-F", "AZURE-NPM"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-F", "AZURE-NPM-INGRESS"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-X", "AZURE-NPM"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 				{
 					Cmd:      []string{"iptables", "-w", "60", "-X", "AZURE-NPM-INGRESS"},
-					ExitCode: 1,
+					ExitCode: 2,
 				},
 			},
 			expectedErr: false,
@@ -1402,6 +1348,10 @@ func TestCleanupOtherChains(t *testing.T) {
 				util.SetIptablesToNft()
 			} else {
 				util.SetIptablesToLegacy()
+				// set back to default
+				defer func() {
+					util.SetIptablesToNft()
+				}()
 			}
 
 			err := pMgr.cleanupOtherIptables()

npm/pkg/dataplane/policies/policymanager.go

@@ -29,13 +29,6 @@ const (
 )
 
 type PolicyManagerCfg struct {
-	// debug is only used for testing. Currently just indicating whether to use debug kernel version
-	debug bool //nolint:unused // only used in linux
-	// debugKernelVersion is only used for testing
-	debugKernelVersion int //nolint:unused // only used in linux
-	// debugKernelVersionErr is only used for testing
-	debugKernelVersionErr error //nolint:unused // only used in linux
-
 	// NodeIP is only used in Windows
 	NodeIP string
 	// PolicyMode only affects Windows

npm/pkg/dataplane/policies/policymanager_linux_test.go

@@ -212,7 +212,6 @@ func TestAddPolicyFailure(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	require.Error(t, pMgr.AddPolicies([]*NPMNetworkPolicy{testNetPol}, nil))
 	_, ok := pMgr.GetPolicy(testNetPol.PolicyKey)
@@ -225,7 +224,6 @@ func TestCreatorForAddPolicies(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	// 1. test with activation
 	policies := []*NPMNetworkPolicy{allTestNetworkPolicies[0]}
@@ -289,7 +287,6 @@ func TestCreatorForRemovePolicies(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	// 1. test without deactivation (i.e. flushing azure chain when removing the last policy)
 	// hack: the cache is empty (and len(cache) != len(allTestNetworkPolicies)), so shouldDeactivate will be false
@@ -337,7 +334,6 @@ func TestRemovePoliciesAcceptableError(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 	require.NoError(t, pMgr.AddPolicies([]*NPMNetworkPolicy{bothDirectionsNetPol}, epList))
 	require.NoError(t, pMgr.RemovePolicy(bothDirectionsNetPol.PolicyKey))
 	_, ok := pMgr.GetPolicy(bothDirectionsNetPol.PolicyKey)
@@ -384,7 +380,6 @@ func TestRemovePoliciesError(t *testing.T) {
 			ioshim := common.NewMockIOShim(tt.calls)
 			defer ioshim.VerifyCalls(t, tt.calls)
 			pMgr := NewPolicyManager(ioshim, ipsetConfig)
-			util.SetIptablesToNft()
 			err := pMgr.AddPolicies([]*NPMNetworkPolicy{bothDirectionsNetPol}, nil)
 			require.NoError(t, err)
 			err = pMgr.RemovePolicy(bothDirectionsNetPol.PolicyKey)
@@ -407,7 +402,6 @@ func TestUpdatingStaleChains(t *testing.T) {
 	ioshim := common.NewMockIOShim(calls)
 	defer ioshim.VerifyCalls(t, calls)
 	pMgr := NewPolicyManager(ioshim, ipsetConfig)
-	util.SetIptablesToNft()
 
 	// add so we can remove. no stale chains to start
 	require.NoError(t, pMgr.AddPolicies([]*NPMNetworkPolicy{bothDirectionsNetPol}, nil))