From c14aed9490c00221109793dfb7b85638db028c05 Mon Sep 17 00:00:00 2001 From: Shaun Crampton Date: Tue, 11 Jul 2017 15:14:14 +0100 Subject: [PATCH] Add UTs for iptables lock. --- config/config_params.go | 5 +- felix.go | 2 +- iptables/lock.go | 60 +++++++++----- iptables/lock_test.go | 175 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 217 insertions(+), 25 deletions(-) create mode 100644 iptables/lock_test.go diff --git a/config/config_params.go b/config/config_params.go index 69c2aa07a1..f850c8c0b5 100644 --- a/config/config_params.go +++ b/config/config_params.go @@ -109,13 +109,14 @@ type Config struct { Ipv6Support bool `config:"bool;true"` IgnoreLooseRPF bool `config:"bool;false"` - IPSetsRefreshInterval time.Duration `config:"seconds;10"` RouteRefreshInterval time.Duration `config:"seconds;90"` IptablesRefreshInterval time.Duration `config:"seconds;90"` IptablesPostWriteCheckIntervalSecs time.Duration `config:"seconds;30"` IptablesLockFilePath string `config:"file;/run/xtables.lock"` IptablesLockTimeoutSecs time.Duration `config:"seconds;10"` IptablesLockProbeIntervalMillis time.Duration `config:"millis;50"` + IpsetsRefreshInterval time.Duration `config:"seconds;10"` + MaxIpsetSize int `config:"int;1048576;non-zero"` MetadataAddr string `config:"hostname;127.0.0.1;die-on-fail"` MetadataPort int `config:"int(0,65535);8775;die-on-fail"` @@ -143,8 +144,6 @@ type Config struct { EndpointReportingEnabled bool `config:"bool;false"` EndpointReportingDelaySecs time.Duration `config:"seconds;1"` - MaxIpsetSize int `config:"int;1048576;non-zero"` - IptablesMarkMask uint32 `config:"mark-bitmask;0xff000000;non-zero,die-on-fail"` DisableConntrackInvalidCheck bool `config:"bool;false"` diff --git a/felix.go b/felix.go index aef77cf8ec..7d01506fe0 100644 --- a/felix.go +++ b/felix.go @@ -284,7 +284,7 @@ configRetry: IPIPMTU: configParams.IpInIpMtu, IptablesRefreshInterval: configParams.IptablesRefreshInterval, RouteRefreshInterval: configParams.RouteRefreshInterval, - IPSetsRefreshInterval: configParams.IPSetsRefreshInterval, + IPSetsRefreshInterval: configParams.IpsetsRefreshInterval, IptablesPostWriteCheckInterval: configParams.IptablesPostWriteCheckIntervalSecs, IptablesInsertMode: configParams.ChainInsertMode, IptablesLockFilePath: configParams.IptablesLockFilePath, diff --git a/iptables/lock.go b/iptables/lock.go index a652e5c52e..d4f318b412 100644 --- a/iptables/lock.go +++ b/iptables/lock.go @@ -77,7 +77,7 @@ type SharedLock struct { lockTimeout time.Duration lockProbeInterval time.Duration - GrabIptablesLocks func(lockFilePath string, timeout, probeInterval time.Duration) (io.Closer, error) + GrabIptablesLocks func(lockFilePath, socketName string, timeout, probeInterval time.Duration) (io.Closer, error) } func (l *SharedLock) Lock() { @@ -86,8 +86,15 @@ func (l *SharedLock) Lock() { if l.referenceCount == 0 { // The lock isn't currently held. Acquire it. - lockHandle, err := l.GrabIptablesLocks(l.lockFilePath, l.lockTimeout, l.lockProbeInterval) + lockHandle, err := l.GrabIptablesLocks( + l.lockFilePath, + "@xtables", + l.lockTimeout, + l.lockProbeInterval, + ) if err != nil { + // We give the lock plenty of time so err on the side of assuming a + // programming bug. log.WithError(err).Panic("Failed to acquire iptables lock") } l.iptablesLockHandle = lockHandle @@ -105,30 +112,40 @@ func (l *SharedLock) Unlock() { } if l.referenceCount == 0 { log.Debug("Releasing iptables lock.") - l.iptablesLockHandle.Close() + err := l.iptablesLockHandle.Close() + if err != nil { + // We haven't done anything with the file or socket so we shouldn't be + // able to hit any "deferred flush" type errors from the close. Panic + // since we're not sure what's going on. + log.WithError(err).Panic("Error while closing iptables lock.") + } l.iptablesLockHandle = nil } } -type locker struct { - lock16 *os.File - lock14 *net.UnixListener +type Locker struct { + Lock16 io.Closer + Lock14 io.Closer } -func (l *locker) Close() error { - if l.lock16 != nil { - err := l.lock16.Close() +func (l *Locker) Close() error { + var err error + if l.Lock16 != nil { + err = l.Lock16.Close() if err != nil { - log.WithError(err).Error("Error while closing iptables lock file") + log.WithError(err).Error("Error while closing lock file.") } } - if l.lock14 != nil { - err := l.lock14.Close() - if err != nil { - log.WithError(err).Error("Error while closing iptables lock socket") + if l.Lock14 != nil { + err14 := l.Lock14.Close() + if err14 != nil { + log.WithError(err14).Error("Error while closing lock socket.") + } + if err14 != nil && err == nil { + err = err14 } } - return nil + return err } var ( @@ -136,12 +153,12 @@ var ( Err16LockTimeout = errors.New("Timed out waiting for iptables 1.6 lock") ) -func GrabIptablesLocks(lockFilePath string, timeout, probeInterval time.Duration) (io.Closer, error) { +func GrabIptablesLocks(lockFilePath, socketName string, timeout, probeInterval time.Duration) (io.Closer, error) { var err error var success bool - l := &locker{} - defer func(l *locker) { + l := &Locker{} + defer func(l *Locker) { // Clean up immediately on failure if !success { l.Close() @@ -153,14 +170,15 @@ func GrabIptablesLocks(lockFilePath string, timeout, probeInterval time.Duration // can't assume which lock method it'll use. // Roughly duplicate iptables 1.6.x xtables_lock() function. - l.lock16, err = os.OpenFile(lockFilePath, os.O_CREATE, 0600) + f, err := os.OpenFile(lockFilePath, os.O_CREATE, 0600) + l.Lock16 = f if err != nil { return nil, fmt.Errorf("failed to open iptables lock %s: %v", lockFilePath, err) } startTime := time.Now() for { - if err := grabIptablesFileLock(l.lock16); err == nil { + if err := grabIptablesFileLock(f); err == nil { break } if time.Since(startTime) > timeout { @@ -173,7 +191,7 @@ func GrabIptablesLocks(lockFilePath string, timeout, probeInterval time.Duration startTime14 := time.Now() for { - l.lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: "@xtables", Net: "unix"}) + l.Lock14, err = net.ListenUnix("unix", &net.UnixAddr{Name: socketName, Net: "unix"}) if err == nil { break } diff --git a/iptables/lock_test.go b/iptables/lock_test.go new file mode 100644 index 0000000000..9f382c6a42 --- /dev/null +++ b/iptables/lock_test.go @@ -0,0 +1,175 @@ +// Copyright (c) 2017 Tigera, Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iptables_test + +import ( + "io" + "io/ioutil" + "os" + "time" + + . "github.com/projectcalico/felix/iptables" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("SharedLock", func() { + var lock *SharedLock + var mockIptablesLock *mockLock + var mockErr error + + mockGrabLock := func(lockFilePath, socketName string, timeout, probeInterval time.Duration) (io.Closer, error) { + return mockIptablesLock, mockErr + } + + BeforeEach(func() { + mockErr = nil + mockIptablesLock = &mockLock{} + lock = NewSharedLock("/foo/bar.lock", time.Second, time.Millisecond) + lock.GrabIptablesLocks = mockGrabLock + }) + + It("should close the lock after final unlock call", func() { + lock.Lock() + Expect(mockIptablesLock.Closed).To(BeFalse()) + lock.Unlock() + Expect(mockIptablesLock.Closed).To(BeTrue()) + }) + It("should allow multiple holders", func() { + lock.Lock() + lock.Lock() + Expect(mockIptablesLock.Closed).To(BeFalse()) + lock.Unlock() + Expect(mockIptablesLock.Closed).To(BeFalse()) + lock.Unlock() + Expect(mockIptablesLock.Closed).To(BeTrue()) + }) + It("should panic on misuse", func() { + Expect(lock.Unlock).To(Panic()) + }) + It("should panic on failure to acquire", func() { + mockErr = Err14LockTimeout + Expect(lock.Lock).To(Panic()) + }) + It("should panic on failure to close", func() { + lock.Lock() + mockIptablesLock.Err = Err14LockTimeout + Expect(lock.Unlock).To(Panic()) + }) +}) + +type mockLock struct { + Closed bool + Err error +} + +func (l *mockLock) Close() error { + Expect(l.Closed).To(BeFalse()) + l.Closed = true + return l.Err +} + +var _ = Describe("GrabIptablesLocks FV", func() { + var fileName string + + BeforeEach(func() { + f, err := ioutil.TempFile("", "iptlocktest") + Expect(err).NotTo(HaveOccurred()) + fileName = f.Name() + f.Close() + }) + AfterEach(func() { + os.Remove(fileName) + }) + + It("should block concurrent invocations", func() { + l, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + Expect(err).NotTo(HaveOccurred()) + defer l.Close() + + l2, err := GrabIptablesLocks(fileName, "@dummytables", 100*time.Millisecond, 10*time.Millisecond) + Expect(err).To(Equal(Err16LockTimeout)) + Expect(l2).To(BeNil()) + }) + It("should allow access after being released", func() { + l, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + Expect(err).NotTo(HaveOccurred()) + l.Close() + + l2, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + Expect(err).NotTo(HaveOccurred()) + l2.Close() + }) + It("should block concurrent invocations using only iptables 1.4 version of lock", func() { + l, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + // Sneakily remove the lockfile after it's been locked so that we fall through to + // the v1.4 lock. + os.Remove(fileName) + Expect(err).NotTo(HaveOccurred()) + defer l.Close() + + l2, err := GrabIptablesLocks(fileName, "@dummytables", 100*time.Millisecond, 10*time.Millisecond) + Expect(err).To(Equal(Err14LockTimeout)) + Expect(l2).To(BeNil()) + }) + It("should allow access after being released using only iptables 1.4 version of lock", func() { + l, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + // Sneakily remove the lockfile after it's been locked so that we fall through to + // the v1.4 lock. + os.Remove(fileName) + Expect(err).NotTo(HaveOccurred()) + l.Close() + + l2, err := GrabIptablesLocks(fileName, "@dummytables", 1*time.Second, 50*time.Millisecond) + Expect(err).NotTo(HaveOccurred()) + l2.Close() + }) +}) + +var _ = Describe("locker", func() { + var l *Locker + var lock14 mockCloser + var lock16 mockCloser + + BeforeEach(func() { + lock14 = mockCloser{} + lock16 = mockCloser{} + l = &Locker{ + Lock14: &lock14, + Lock16: &lock16, + } + }) + + It("should return nil with no err", func() { + Expect(l.Close()).NotTo(HaveOccurred()) + }) + It("should return lock16 err", func() { + lock16.Err = Err16LockTimeout + Expect(l.Close()).To(Equal(Err16LockTimeout)) + }) + It("should return lock14 err", func() { + lock14.Err = Err14LockTimeout + Expect(l.Close()).To(Equal(Err14LockTimeout)) + }) +}) + +type mockCloser struct { + Err error +} + +func (c *mockCloser) Close() error { + return c.Err +}