Skip to content

Commit 334d8c0

Browse files
huntergregoryhuntergregory
committed
fix: address comments
Signed-off-by: Hunter Gregory <[email protected]>
1 parent 5f3fa4b commit 334d8c0

File tree

4 files changed

+69
-43
lines changed

4 files changed

+69
-43
lines changed

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

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ const (
2525
// transferred from iptm.go and not sure why this length is important
2626
minLineNumberStringLength int = 3
2727

28-
detectingErrMsg = "failed to detect iptables version. failed to find KUBE chains in iptables-legacy-save and iptables-nft-save and failed to get kernel version. NPM will crash to retry"
28+
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"
2929
)
3030

3131
var (
@@ -249,53 +249,54 @@ func (pMgr *PolicyManager) bootupAfterDetectAndCleanup() error {
249249
func (pMgr *PolicyManager) detectIptablesVersion() error {
250250
klog.Info("first attempt detecting iptables version. running: iptables-nft-save -t mangle")
251251
cmd := pMgr.ioShim.Exec.Command(util.IptablesSaveNft, "-t", "mangle")
252-
output, err := cmd.CombinedOutput()
253-
if err == nil && strings.Contains(string(output), "KUBE-IPTABLES-HINT") || strings.Contains(string(output), "KUBE-KUBELET-CANARY") {
252+
output, nftErr := cmd.CombinedOutput()
253+
if nftErr != nil {
254+
msg := "failed to detect iptables version on first attempt. error running iptables-nft-save. will try detecting using iptables-legacy-save. err: %s"
255+
metrics.SendErrorLogAndMetric(util.IptmID, msg, nftErr.Error())
256+
} else if strings.Contains(string(output), "KUBE-IPTABLES-HINT") || strings.Contains(string(output), "KUBE-KUBELET-CANARY") {
254257
msg := "detected iptables version on first attempt. found KUBE chains in nft iptables. NPM will use iptables-nft"
255258
klog.Info(msg)
256259
metrics.SendLog(util.IptmID, msg, metrics.DonotPrint)
257260
util.SetIptablesToNft()
258261
return nil
259262
}
260263

261-
if err != nil {
262-
msg := "failed to detect iptables version on first attempt. error running iptables-nft-save. will try detecting using iptables-legacy-save. err: %s"
263-
metrics.SendErrorLogAndMetric(util.IptmID, msg, err.Error())
264-
}
265-
266264
klog.Info("second attempt detecting iptables version. running: iptables-legacy-save -t mangle")
267265
lCmd := pMgr.ioShim.Exec.Command(util.IptablesSaveLegacy, "-t", "mangle")
268-
loutput, err := lCmd.CombinedOutput()
269-
if err == nil && strings.Contains(string(loutput), "KUBE-IPTABLES-HINT") || strings.Contains(string(loutput), "KUBE-KUBELET-CANARY") {
270-
msg := "detected iptables version on second attempt. found KUBE chains in legacy tables. NPM will use iptables-legacy"
271-
klog.Info(msg)
272-
metrics.SendLog(util.IptmID, msg, metrics.DonotPrint)
273-
util.SetIptablesToLegacy()
274-
return nil
275-
}
276-
277-
if err != nil {
266+
loutput, legacyErr := lCmd.CombinedOutput()
267+
if legacyErr != nil {
278268
msg := "failed to detect iptables version on second attempt. error running iptables-legacy-save. will try detecting using kernel version. err: %s"
279-
metrics.SendErrorLogAndMetric(util.IptmID, msg, err.Error())
269+
metrics.SendErrorLogAndMetric(util.IptmID, msg, legacyErr.Error())
270+
} else {
271+
if strings.Contains(string(loutput), "KUBE-IPTABLES-HINT") || strings.Contains(string(loutput), "KUBE-KUBELET-CANARY") {
272+
msg := "detected iptables version on second attempt. found KUBE chains in legacy tables. NPM will use iptables-legacy"
273+
klog.Info(msg)
274+
metrics.SendLog(util.IptmID, msg, metrics.DonotPrint)
275+
util.SetIptablesToLegacy()
276+
return nil
277+
} else if nftErr != nil {
278+
msg := "NPM will use iptables-nft. iptables-nft-save failed earlier, but iptables-legacy-save didn't have KUBE chains"
279+
klog.Info(msg)
280+
metrics.SendLog(util.IptmID, msg, metrics.DonotPrint)
281+
util.SetIptablesToNft()
282+
return nil
283+
}
280284
}
281285

286+
// we are here if either:
287+
// 1. both nft and legacy save commands failed
288+
// 2. both nft and legacy save commands didn't have KUBE chains
282289
klog.Info("third attempt detecting iptables version. getting kernel version")
283-
kernelRelease := ""
290+
var majorVersion int
291+
var versionError error
284292
if pMgr.debug {
285293
// for testing purposes
286-
kernelRelease = pMgr.debugKernelVersion
294+
majorVersion = pMgr.debugKernelVersion
295+
versionError = pMgr.debugKernelVersionErr
287296
} else {
288-
kernelRelease = util.KernelRelease()
289-
}
290-
kernelVersion := strings.Split(kernelRelease, ".")[0]
291-
if kernelVersion == "" {
292-
metrics.SendErrorLogAndMetric(util.IptmID, "failed to detect iptables version on third attempt. error getting kernel version")
293-
return errDetectingIptablesVersion
297+
majorVersion, versionError = util.KernelReleaseMajorVersion()
294298
}
295-
296-
majorVersion, err := strconv.Atoi(kernelVersion)
297-
if err != nil {
298-
metrics.SendErrorLogAndMetric(util.IptmID, "failed to detect iptables version on third attempt. error converting kernel version to int. err: %s", err.Error())
299+
if versionError != nil {
299300
return errDetectingIptablesVersion
300301
}
301302

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

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -900,7 +900,8 @@ func stringsToMap(items []string) map[string]struct{} {
900900
func TestDetectIptablesVersion(t *testing.T) {
901901
type args struct {
902902
name string
903-
kernelVersion string
903+
kernelVersion int
904+
kernelVersionErr error
904905
calls []testutils.TestCmd
905906
expectedErr bool
906907
expectedIptablesVersion string
@@ -935,7 +936,7 @@ func TestDetectIptablesVersion(t *testing.T) {
935936
},
936937
{
937938
name: "iptables-nft-save and iptables-save both fail: kernel version >= 5",
938-
kernelVersion: "5.0.0",
939+
kernelVersion: 5,
939940
calls: []testutils.TestCmd{
940941
{
941942
Cmd: []string{"iptables-nft-save", "-t", "mangle"},
@@ -951,7 +952,7 @@ func TestDetectIptablesVersion(t *testing.T) {
951952
},
952953
{
953954
name: "no kube chains: kernel version >= 5",
954-
kernelVersion: "5.0.0",
955+
kernelVersion: 5,
955956
calls: []testutils.TestCmd{
956957
{
957958
Cmd: []string{"iptables-nft-save", "-t", "mangle"},
@@ -967,7 +968,7 @@ func TestDetectIptablesVersion(t *testing.T) {
967968
},
968969
{
969970
name: "no kube chains: kernel version < 5",
970-
kernelVersion: "4.5.5",
971+
kernelVersion: 4,
971972
calls: []testutils.TestCmd{
972973
{
973974
Cmd: []string{"iptables-nft-save", "-t", "mangle"},
@@ -982,8 +983,8 @@ func TestDetectIptablesVersion(t *testing.T) {
982983
expectedIptablesVersion: util.IptablesLegacy,
983984
},
984985
{
985-
name: "no kube chains: kernel version is empty",
986-
kernelVersion: "",
986+
name: "no kube chains: kernel version error",
987+
kernelVersionErr: fmt.Errorf("kernel version error"),
987988
calls: []testutils.TestCmd{
988989
{
989990
Cmd: []string{"iptables-nft-save", "-t", "mangle"},

‎npm/pkg/dataplane/policies/policymanager.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@ type PolicyManagerCfg struct {
3232
// debug is only used for testing. Currently just indicating whether to use debug kernel version
3333
debug bool
3434
// debugKernelVersion is only used for testing
35-
debugKernelVersion string
35+
debugKernelVersion int
36+
// debugKernelVersionErr is only used for testing
37+
debugKernelVersionErr error
38+
3639
// NodeIP is only used in Windows
3740
NodeIP string
3841
// PolicyMode only affects Windows

‎npm/util/sysinfo_linux.go‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,41 @@ package util
33
// this file has bits copied from github.com/zcalusic/sysinfo (v1.1.2)
44

55
import (
6+
"fmt"
67
"os"
8+
"strconv"
79
"strings"
810
)
911

10-
func KernelRelease() string {
11-
return slurpFile("/proc/sys/kernel/osrelease")
12+
const kernelReleaseFilepath = "/proc/sys/kernel/osrelease"
13+
14+
var errNoKernelRelease = fmt.Errorf("error finding kernel release")
15+
16+
func KernelReleaseMajorVersion() (int, error) {
17+
rel, err := slurpFile(kernelReleaseFilepath)
18+
if err != nil {
19+
return 0, err
20+
}
21+
22+
majorString := strings.Split(rel, ".")[0]
23+
if majorString == "" {
24+
return 0, errNoKernelRelease
25+
}
26+
27+
majorInt, err := strconv.Atoi(majorString)
28+
if err != nil {
29+
return 0, fmt.Errorf("failed to convert kernel major version to int. version: %s. err: %w", majorString, err)
30+
}
31+
32+
return majorInt, nil
1233
}
1334

1435
// Read one-liner text files, strip newline.
15-
func slurpFile(path string) string {
36+
func slurpFile(path string) (string, error) {
1637
data, err := os.ReadFile(path)
1738
if err != nil {
18-
return ""
39+
return "", fmt.Errorf("failed to read file. file: %s. err: %w", path, err)
1940
}
2041

21-
return strings.TrimSpace(string(data))
42+
return strings.TrimSpace(string(data)), nil
2243
}

0 commit comments

Comments
 (0)