Skip to content

Commit 7588807

Browse files
danwinshipdanwinship
committed
Fix iptables version detection code to handle RHEL 7 correctly
1 parent 2b7ceb2 commit 7588807

File tree

2 files changed

+135
-22
lines changed

2 files changed

+135
-22
lines changed

‎pkg/util/iptables/iptables.go‎

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func newInternal(exec utilexec.Interface, dbus utildbus.Interface, protocol Prot
177177
hasListener: false,
178178
hasRandomFully: version.AtLeast(RandomFullyMinVersion),
179179
waitFlag: getIPTablesWaitFlag(version),
180-
restoreWaitFlag: getIPTablesRestoreWaitFlag(version),
180+
restoreWaitFlag: getIPTablesRestoreWaitFlag(version, exec, protocol),
181181
lockfilePath: lockfilePath,
182182
}
183183
return runner
@@ -578,12 +578,46 @@ func getIPTablesWaitFlag(version *utilversion.Version) []string {
578578
}
579579

580580
// Checks if iptables-restore has a "wait" flag
581-
func getIPTablesRestoreWaitFlag(version *utilversion.Version) []string {
581+
func getIPTablesRestoreWaitFlag(version *utilversion.Version, exec utilexec.Interface, protocol Protocol) []string {
582582
if version.AtLeast(WaitRestoreMinVersion) {
583583
return []string{WaitString, WaitSecondsValue}
584-
} else {
584+
}
585+
586+
// Older versions may have backported features; if iptables-restore supports
587+
// --version, assume it also supports --wait
588+
vstring, err := getIPTablesRestoreVersionString(exec, protocol)
589+
if err != nil || vstring == "" {
590+
klog.V(3).Infof("couldn't get iptables-restore version; assuming it doesn't support --wait")
591+
return nil
592+
}
593+
if _, err := utilversion.ParseGeneric(vstring); err != nil {
594+
klog.V(3).Infof("couldn't parse iptables-restore version; assuming it doesn't support --wait")
585595
return nil
586596
}
597+
return []string{WaitString}
598+
}
599+
600+
// getIPTablesRestoreVersionString runs "iptables-restore --version" to get the version string
601+
// in the form "X.X.X"
602+
func getIPTablesRestoreVersionString(exec utilexec.Interface, protocol Protocol) (string, error) {
603+
// this doesn't access mutable state so we don't need to use the interface / runner
604+
605+
// iptables-restore hasn't always had --version, and worse complains
606+
// about unrecognized commands but doesn't exit when it gets them.
607+
// Work around that by setting stdin to nothing so it exits immediately.
608+
iptablesRestoreCmd := iptablesRestoreCommand(protocol)
609+
cmd := exec.Command(iptablesRestoreCmd, "--version")
610+
cmd.SetStdin(bytes.NewReader([]byte{}))
611+
bytes, err := cmd.CombinedOutput()
612+
if err != nil {
613+
return "", err
614+
}
615+
versionMatcher := regexp.MustCompile("v([0-9]+(\\.[0-9]+)+)")
616+
match := versionMatcher.FindStringSubmatch(string(bytes))
617+
if match == nil {
618+
return "", fmt.Errorf("no iptables version found in string: %s", bytes)
619+
}
620+
return match[1], nil
587621
}
588622

589623
// goroutine to listen for D-Bus signals

‎pkg/util/iptables/iptables_test.go‎

Lines changed: 98 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,23 @@ func protocolStr(protocol Protocol) string {
4545
}
4646

4747
func testIPTablesVersionCmds(t *testing.T, protocol Protocol) {
48-
version := " v1.9.22"
48+
version := " v1.4.22"
4949
iptablesCmd := iptablesCommand(protocol)
50+
iptablesRestoreCmd := iptablesRestoreCommand(protocol)
5051
protoStr := protocolStr(protocol)
5152

5253
fcmd := fakeexec.FakeCmd{
5354
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
5455
// iptables version response (for runner instantiation)
5556
func() ([]byte, error) { return []byte(iptablesCmd + version), nil },
57+
// iptables-restore version response (for runner instantiation)
58+
func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil },
5659
},
5760
}
5861
fexec := fakeexec.FakeExec{
5962
CommandScript: []fakeexec.FakeCommandAction{
6063
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
64+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
6165
},
6266
}
6367
runner := New(&fexec, dbus.NewFake(nil, nil), protocol)
@@ -67,6 +71,11 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) {
6771
if !sets.NewString(fcmd.CombinedOutputLog[0]...).HasAll(iptablesCmd, "--version") {
6872
t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[0])
6973
}
74+
75+
// Check that proper iptables restore version command was used during runner instantiation
76+
if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll(iptablesRestoreCmd, "--version") {
77+
t.Errorf("%s runner instantiate: Expected cmd '%s --version', Got '%s'", protoStr, iptablesRestoreCmd, fcmd.CombinedOutputLog[1])
78+
}
7079
}
7180

7281
func TestIPTablesVersionCmdsIPv4(t *testing.T) {
@@ -486,11 +495,13 @@ func TestGetIPTablesHasCheckCommand(t *testing.T) {
486495
fcmd := fakeexec.FakeCmd{
487496
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
488497
func() ([]byte, error) { return []byte(testCase.Version), nil },
498+
func() ([]byte, error) { return []byte(testCase.Version), nil },
489499
},
490500
}
491501
fexec := fakeexec.FakeExec{
492502
CommandScript: []fakeexec.FakeCommandAction{
493503
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
504+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
494505
},
495506
}
496507
ipt := New(&fexec, nil, ProtocolIpv4)
@@ -639,6 +650,8 @@ func TestWaitFlagUnavailable(t *testing.T) {
639650
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
640651
// iptables version check
641652
func() ([]byte, error) { return []byte("iptables v1.4.19"), nil },
653+
// iptables-restore version check
654+
func() ([]byte, error) { return []byte{}, nil },
642655
// Success.
643656
func() ([]byte, error) { return []byte{}, nil },
644657
},
@@ -647,6 +660,8 @@ func TestWaitFlagUnavailable(t *testing.T) {
647660
CommandScript: []fakeexec.FakeCommandAction{
648661
// iptables version check
649662
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
663+
// iptables-restore version check
664+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
650665
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
651666
},
652667
}
@@ -656,10 +671,10 @@ func TestWaitFlagUnavailable(t *testing.T) {
656671
if err != nil {
657672
t.Errorf("expected success, got %v", err)
658673
}
659-
if fcmd.CombinedOutputCalls != 2 {
660-
t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
674+
if fcmd.CombinedOutputCalls != 3 {
675+
t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
661676
}
662-
if sets.NewString(fcmd.CombinedOutputLog[1]...).Has(WaitString) {
677+
if sets.NewString(fcmd.CombinedOutputLog[2]...).Has(WaitString) {
663678
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
664679
}
665680
}
@@ -669,6 +684,8 @@ func TestWaitFlagOld(t *testing.T) {
669684
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
670685
// iptables version check
671686
func() ([]byte, error) { return []byte("iptables v1.4.20"), nil },
687+
// iptables-restore version check
688+
func() ([]byte, error) { return []byte{}, nil },
672689
// Success.
673690
func() ([]byte, error) { return []byte{}, nil },
674691
},
@@ -677,6 +694,7 @@ func TestWaitFlagOld(t *testing.T) {
677694
CommandScript: []fakeexec.FakeCommandAction{
678695
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
679696
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
697+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
680698
},
681699
}
682700
runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4)
@@ -685,14 +703,14 @@ func TestWaitFlagOld(t *testing.T) {
685703
if err != nil {
686704
t.Errorf("expected success, got %v", err)
687705
}
688-
if fcmd.CombinedOutputCalls != 2 {
689-
t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
706+
if fcmd.CombinedOutputCalls != 3 {
707+
t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
690708
}
691-
if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", WaitString) {
692-
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1])
709+
if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString) {
710+
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
693711
}
694-
if sets.NewString(fcmd.CombinedOutputLog[1]...).Has(WaitSecondsValue) {
695-
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1])
712+
if sets.NewString(fcmd.CombinedOutputLog[2]...).Has(WaitSecondsValue) {
713+
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
696714
}
697715
}
698716

@@ -701,6 +719,8 @@ func TestWaitFlagNew(t *testing.T) {
701719
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
702720
// iptables version check
703721
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
722+
// iptables-restore version check
723+
func() ([]byte, error) { return []byte{}, nil },
704724
// Success.
705725
func() ([]byte, error) { return []byte{}, nil },
706726
},
@@ -709,6 +729,7 @@ func TestWaitFlagNew(t *testing.T) {
709729
CommandScript: []fakeexec.FakeCommandAction{
710730
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
711731
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
732+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
712733
},
713734
}
714735
runner := New(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4)
@@ -717,11 +738,11 @@ func TestWaitFlagNew(t *testing.T) {
717738
if err != nil {
718739
t.Errorf("expected success, got %v", err)
719740
}
720-
if fcmd.CombinedOutputCalls != 2 {
721-
t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
741+
if fcmd.CombinedOutputCalls != 3 {
742+
t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
722743
}
723-
if !sets.NewString(fcmd.CombinedOutputLog[1]...).HasAll("iptables", WaitString, WaitSecondsValue) {
724-
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[1])
744+
if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll("iptables", WaitString, WaitSecondsValue) {
745+
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
725746
}
726747
}
727748

@@ -736,7 +757,7 @@ func TestReload(t *testing.T) {
736757
fcmd := fakeexec.FakeCmd{
737758
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
738759
// iptables version check
739-
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
760+
func() ([]byte, error) { return []byte("iptables v1.6.4"), nil },
740761

741762
// first reload
742763
// EnsureChain
@@ -1096,6 +1117,8 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) {
10961117
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
10971118
// iptables version check
10981119
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
1120+
// iptables-restore version check
1121+
func() ([]byte, error) { return []byte{}, nil },
10991122
func() ([]byte, error) { return []byte{}, nil },
11001123
func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} },
11011124
},
@@ -1105,6 +1128,7 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) {
11051128
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
11061129
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
11071130
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1131+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
11081132
},
11091133
}
11101134
runner := newInternal(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4, TestLockfilePath)
@@ -1116,16 +1140,16 @@ func TestRestoreAllWaitOldIptablesRestore(t *testing.T) {
11161140
t.Fatalf("expected success, got %v", err)
11171141
}
11181142

1119-
commandSet := sets.NewString(fcmd.CombinedOutputLog[1]...)
1143+
commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...)
11201144
if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") {
11211145
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
11221146
}
1123-
if commandSet.HasAll(WaitString, WaitSecondsValue) {
1147+
if commandSet.HasAll(WaitString) {
11241148
t.Errorf("wrong CombinedOutput() log (unexpected %s option), got %s", WaitString, fcmd.CombinedOutputLog[1])
11251149
}
11261150

1127-
if fcmd.CombinedOutputCalls != 2 {
1128-
t.Errorf("expected 2 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
1151+
if fcmd.CombinedOutputCalls != 3 {
1152+
t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
11291153
}
11301154

11311155
// Failure.
@@ -1143,11 +1167,14 @@ func TestRestoreAllGrabNewLock(t *testing.T) {
11431167
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
11441168
// iptables version check
11451169
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
1170+
// iptables-restore version check
1171+
func() ([]byte, error) { return []byte{}, nil },
11461172
},
11471173
}
11481174
fexec := fakeexec.FakeExec{
11491175
CommandScript: []fakeexec.FakeCommandAction{
11501176
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1177+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
11511178
},
11521179
}
11531180

@@ -1183,11 +1210,14 @@ func TestRestoreAllGrabOldLock(t *testing.T) {
11831210
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
11841211
// iptables version check
11851212
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
1213+
// iptables-restore version check
1214+
func() ([]byte, error) { return []byte{}, nil },
11861215
},
11871216
}
11881217
fexec := fakeexec.FakeExec{
11891218
CommandScript: []fakeexec.FakeCommandAction{
11901219
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1220+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
11911221
},
11921222
}
11931223

@@ -1210,3 +1240,52 @@ func TestRestoreAllGrabOldLock(t *testing.T) {
12101240
t.Errorf("expected timeout error, got %v", err)
12111241
}
12121242
}
1243+
1244+
// TestRestoreAllWaitBackportedIptablesRestore tests that the "wait" flag is passed
1245+
// to a seemingly-old-but-actually-new iptables-restore
1246+
func TestRestoreAllWaitBackportedIptablesRestore(t *testing.T) {
1247+
fcmd := fakeexec.FakeCmd{
1248+
CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{
1249+
// iptables version check
1250+
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
1251+
// iptables-restore version check
1252+
func() ([]byte, error) { return []byte("iptables v1.4.22"), nil },
1253+
func() ([]byte, error) { return []byte{}, nil },
1254+
func() ([]byte, error) { return nil, &fakeexec.FakeExitError{Status: 1} },
1255+
},
1256+
}
1257+
fexec := fakeexec.FakeExec{
1258+
CommandScript: []fakeexec.FakeCommandAction{
1259+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1260+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1261+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1262+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
1263+
},
1264+
}
1265+
runner := newInternal(&fexec, dbus.NewFake(nil, nil), ProtocolIpv4, TestLockfilePath)
1266+
defer os.Remove(TestLockfilePath)
1267+
defer runner.Destroy()
1268+
1269+
err := runner.RestoreAll([]byte{}, NoFlushTables, RestoreCounters)
1270+
if err != nil {
1271+
t.Fatalf("expected success, got %v", err)
1272+
}
1273+
1274+
commandSet := sets.NewString(fcmd.CombinedOutputLog[2]...)
1275+
if !commandSet.HasAll("iptables-restore", "--counters", "--noflush") {
1276+
t.Errorf("wrong CombinedOutput() log, got %s", fcmd.CombinedOutputLog[2])
1277+
}
1278+
if !commandSet.HasAll(WaitString) {
1279+
t.Errorf("wrong CombinedOutput() log (expected %s option), got %s", WaitString, fcmd.CombinedOutputLog[1])
1280+
}
1281+
1282+
if fcmd.CombinedOutputCalls != 3 {
1283+
t.Errorf("expected 3 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls)
1284+
}
1285+
1286+
// Failure.
1287+
err = runner.Restore(TableNAT, []byte{}, FlushTables, RestoreCounters)
1288+
if err == nil {
1289+
t.Errorf("expected failure")
1290+
}
1291+
}

0 commit comments

Comments
 (0)