Skip to content

Commit 7740a36

Browse files
MikeSpreitzerMikeSpreitzer
committed
Refactored version testing for iptables
Reduced the number of calls to ParseGeneric.
1 parent 5895634 commit 7740a36

File tree

5 files changed

+67
-84
lines changed

5 files changed

+67
-84
lines changed

‎pkg/proxy/iptables/proxier.go‎

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import (
3636

3737
v1 "k8s.io/api/core/v1"
3838
"k8s.io/apimachinery/pkg/types"
39-
utilversion "k8s.io/apimachinery/pkg/util/version"
4039
"k8s.io/apimachinery/pkg/util/wait"
4140
"k8s.io/client-go/tools/record"
4241
"k8s.io/kubernetes/pkg/proxy"
@@ -61,10 +60,6 @@ const (
6160
// This is the "new" Proxier, so we require "new" versions of tools.
6261
iptablesMinVersion = utiliptables.MinCheckVersion
6362

64-
// iptablesRandomFullyVersion is the minimum iptables version that supports
65-
// the --random-fully flag on `-j MASQUERADE`.
66-
iptablesRandomFullyVersion = "1.6.2"
67-
6863
// the services chain
6964
kubeServicesChain utiliptables.Chain = "KUBE-SERVICES"
7065

@@ -105,20 +100,9 @@ type KernelCompatTester interface {
105100
// an error if it fails to get the iptables version without error, in which
106101
// case it will also return false.
107102
func CanUseIPTablesProxier(iptver Versioner, kcompat KernelCompatTester) (bool, error) {
108-
minVersion, err := utilversion.ParseGeneric(iptablesMinVersion)
109-
if err != nil {
110-
return false, err
111-
}
112-
versionString, err := iptver.GetVersion()
113-
if err != nil {
114-
return false, err
115-
}
116-
version, err := utilversion.ParseGeneric(versionString)
117-
if err != nil {
118-
return false, err
119-
}
120-
if version.LessThan(minVersion) {
121-
return false, nil
103+
outcome := utiliptables.VersionIsAtLeast(iptver, iptablesMinVersion)
104+
if outcome.Err != nil || !outcome.Answer {
105+
return false, outcome.Err
122106
}
123107

124108
// Check that the kernel supports what we need.
@@ -313,19 +297,7 @@ func NewProxier(ipt utiliptables.Interface,
313297
klog.Warning("missing br-netfilter module or unset sysctl br-nf-call-iptables; proxy may not work as intended")
314298
}
315299

316-
masqueradeRandomFully := false
317-
iptVersionString, err := ipt.GetVersion()
318-
if err != nil {
319-
klog.Warningf("Unable to get iptables version string: %v", err)
320-
} else {
321-
iptVersion, err := utilversion.ParseGeneric(iptVersionString)
322-
if err != nil {
323-
klog.Warningf("Unable to parse iptables version string %q: %v", iptVersionString, err)
324-
} else {
325-
needVersion, err := utilversion.ParseGeneric(iptablesRandomFullyVersion)
326-
masqueradeRandomFully = err == nil && iptVersion.AtLeast(needVersion)
327-
}
328-
}
300+
canRandomFully := utiliptables.VersionIsAtLeast(ipt, utiliptables.MinMasqueradeRandomFullyVersion)
329301

330302
// Generate the masquerade mark to use for SNAT rules.
331303
masqueradeValue := 1 << uint(masqueradeBit)
@@ -352,7 +324,7 @@ func NewProxier(ipt utiliptables.Interface,
352324
endpointsMap: make(proxy.EndpointsMap),
353325
endpointsChanges: proxy.NewEndpointChangeTracker(hostname, newEndpointInfo, &isIPv6, recorder),
354326
iptables: ipt,
355-
masqueradeRandomFully: masqueradeRandomFully,
327+
masqueradeRandomFully: canRandomFully.Answer,
356328
masqueradeAll: masqueradeAll,
357329
masqueradeMark: masqueradeMark,
358330
exec: exec,

‎pkg/proxy/ipvs/proxier.go‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ import (
3333
"k8s.io/api/core/v1"
3434
"k8s.io/apimachinery/pkg/types"
3535
"k8s.io/apimachinery/pkg/util/sets"
36-
utilversion "k8s.io/apimachinery/pkg/util/version"
3736
"k8s.io/apimachinery/pkg/util/wait"
3837
"k8s.io/client-go/tools/record"
3938
"k8s.io/kubernetes/pkg/proxy"
@@ -51,10 +50,6 @@ import (
5150
)
5251

5352
const (
54-
// iptablesRandomFullyVersion is the minimum iptables version that supports
55-
// the --random-fully flag on `-j MASQUERADE`.
56-
iptablesRandomFullyVersion = "1.6.2"
57-
5853
// kubeServicesChain is the services portal chain
5954
kubeServicesChain utiliptables.Chain = "KUBE-SERVICES"
6055

@@ -384,19 +379,7 @@ func NewProxier(ipt utiliptables.Interface,
384379
}
385380
}
386381

387-
masqueradeRandomFully := false
388-
iptVersionString, err := ipt.GetVersion()
389-
if err != nil {
390-
klog.Warningf("Unable to get iptables version string: %v", err)
391-
} else {
392-
iptVersion, err := utilversion.ParseGeneric(iptVersionString)
393-
if err != nil {
394-
klog.Warningf("Unable to parse iptables version string %q: %v", iptVersionString, err)
395-
} else {
396-
needVersion, err := utilversion.ParseGeneric(iptablesRandomFullyVersion)
397-
masqueradeRandomFully = err == nil && iptVersion.AtLeast(needVersion)
398-
}
399-
}
382+
canRandomFully := utiliptables.VersionIsAtLeast(ipt, utiliptables.MinMasqueradeRandomFullyVersion)
400383

401384
// Generate the masquerade mark to use for SNAT rules.
402385
masqueradeValue := 1 << uint(masqueradeBit)
@@ -434,7 +417,7 @@ func NewProxier(ipt utiliptables.Interface,
434417
minSyncPeriod: minSyncPeriod,
435418
excludeCIDRs: parseExcludedCIDRs(excludeCIDRs),
436419
iptables: ipt,
437-
masqueradeRandomFully: masqueradeRandomFully,
420+
masqueradeRandomFully: canRandomFully.Answer,
438421
masqueradeAll: masqueradeAll,
439422
masqueradeMark: masqueradeMark,
440423
exec: exec,

‎pkg/util/iptables/iptables.go‎

Lines changed: 37 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,39 @@ const WaitSecondsMinVersion = "1.4.22"
129129
const WaitString = "-w"
130130
const WaitSecondsValue = "5"
131131

132+
// MinMasqueradeFullyVersion is the minimum iptables version that supports the `--random-fully` modifier on `-j MASQUERADE`
133+
const MinMasqueradeRandomFullyVersion = "1.6.2"
134+
135+
type TestOutcome struct {
136+
Answer bool
137+
Err error
138+
}
139+
140+
// VersionStringIsAtLeast compares two alleged version strings
141+
func VersionStringIsAtLeast(versionStr, minVersionStr string) TestOutcome {
142+
version, err := utilversion.ParseGeneric(versionStr)
143+
if err != nil {
144+
klog.Warningf("%q is not a valid version string: %v", versionStr, err)
145+
return TestOutcome{false, err}
146+
}
147+
minVersion, err := utilversion.ParseGeneric(minVersionStr)
148+
if err != nil {
149+
klog.Warningf("%q is not a valid version string: %v", minVersionStr, err)
150+
return TestOutcome{false, err}
151+
}
152+
return TestOutcome{version.AtLeast(minVersion), nil}
153+
}
154+
155+
// VersionIsAtLeast tests whether the version of the given item is at least the given bound
156+
func VersionIsAtLeast(ifc interface{ GetVersion() (string, error) }, minVersionStr string) TestOutcome {
157+
versionStr, err := ifc.GetVersion()
158+
if err != nil {
159+
klog.Warningf("Unable to determine version string: %v", err)
160+
return TestOutcome{false, err}
161+
}
162+
return VersionStringIsAtLeast(versionStr, minVersionStr)
163+
}
164+
132165
const LockfilePath16x = "/run/xtables.lock"
133166

134167
// runner implements Interface in terms of exec("iptables").
@@ -541,42 +574,16 @@ func makeFullArgs(table Table, chain Chain, args ...string) []string {
541574

542575
// Checks if iptables has the "-C" flag
543576
func getIPTablesHasCheckCommand(vstring string) bool {
544-
minVersion, err := utilversion.ParseGeneric(MinCheckVersion)
545-
if err != nil {
546-
klog.Errorf("MinCheckVersion (%s) is not a valid version string: %v", MinCheckVersion, err)
547-
return true
548-
}
549-
version, err := utilversion.ParseGeneric(vstring)
550-
if err != nil {
551-
klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err)
552-
return true
553-
}
554-
return version.AtLeast(minVersion)
577+
outcome := VersionStringIsAtLeast(vstring, MinCheckVersion)
578+
return outcome.Answer && outcome.Err == nil
555579
}
556580

557581
// Checks if iptables version has a "wait" flag
558582
func getIPTablesWaitFlag(vstring string) []string {
559-
version, err := utilversion.ParseGeneric(vstring)
560-
if err != nil {
561-
klog.Errorf("vstring (%s) is not a valid version string: %v", vstring, err)
562-
return nil
563-
}
564-
565-
minVersion, err := utilversion.ParseGeneric(WaitMinVersion)
566-
if err != nil {
567-
klog.Errorf("WaitMinVersion (%s) is not a valid version string: %v", WaitMinVersion, err)
568-
return nil
569-
}
570-
if version.LessThan(minVersion) {
571-
return nil
572-
}
573-
574-
minVersion, err = utilversion.ParseGeneric(WaitSecondsMinVersion)
575-
if err != nil {
576-
klog.Errorf("WaitSecondsMinVersion (%s) is not a valid version string: %v", WaitSecondsMinVersion, err)
583+
if !VersionStringIsAtLeast(vstring, WaitMinVersion).Answer {
577584
return nil
578585
}
579-
if version.LessThan(minVersion) {
586+
if !VersionStringIsAtLeast(vstring, WaitSecondsMinVersion).Answer {
580587
return []string{WaitString}
581588
}
582589
return []string{WaitString, WaitSecondsValue}

‎pkg/util/iptables/iptables_test.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,16 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) {
5757
func() ([]byte, error) { return []byte(iptablesRestoreCmd + version), nil },
5858
// iptables version response (for call to runner.GetVersion())
5959
func() ([]byte, error) { return []byte(iptablesCmd + version), nil },
60+
// iptables version response (for call to VersionIsAtLeast())
61+
func() ([]byte, error) { return []byte(iptablesCmd + version), nil },
6062
},
6163
}
6264
fexec := fakeexec.FakeExec{
6365
CommandScript: []fakeexec.FakeCommandAction{
6466
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
6567
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
6668
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
69+
func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) },
6770
},
6871
}
6972
runner := New(&fexec, dbus.NewFake(nil, nil), protocol)
@@ -88,6 +91,16 @@ func testIPTablesVersionCmds(t *testing.T, protocol Protocol) {
8891
if !sets.NewString(fcmd.CombinedOutputLog[2]...).HasAll(iptablesCmd, "--version") {
8992
t.Errorf("%s GetVersion: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[2])
9093
}
94+
95+
versionOutcome := VersionIsAtLeast(runner, MinMasqueradeRandomFullyVersion)
96+
if versionOutcome.Err != nil || !versionOutcome.Answer {
97+
t.Errorf("%s VersionIsAtLeast(%q) returned %v", protoStr, MinMasqueradeRandomFullyVersion, versionOutcome)
98+
}
99+
100+
// Check that proper iptables version command was used for runner.GetVersion
101+
if !sets.NewString(fcmd.CombinedOutputLog[3]...).HasAll(iptablesCmd, "--version") {
102+
t.Errorf("%s GetVersion: Expected cmd '%s --version', Got '%s'", protoStr, iptablesCmd, fcmd.CombinedOutputLog[3])
103+
}
91104
}
92105

93106
func TestIPTablesVersionCmdsIPv4(t *testing.T) {

‎pkg/util/iptables/testing/fake.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,22 @@ type Rule map[string]string
4141

4242
// no-op implementation of iptables Interface
4343
type FakeIPTables struct {
44-
Lines []byte
44+
Lines []byte
45+
Version string
4546
}
4647

4748
func NewFake() *FakeIPTables {
4849
return &FakeIPTables{}
4950
}
5051

51-
func (*FakeIPTables) GetVersion() (string, error) {
52+
func NewVersionedFake(version string) *FakeIPTables {
53+
return &FakeIPTables{Version: version}
54+
}
55+
56+
func (f *FakeIPTables) GetVersion() (string, error) {
57+
if f.Version != "" {
58+
return f.Version, nil
59+
}
5260
return "0.0.0", nil
5361
}
5462

0 commit comments

Comments
 (0)