From 9b503a6482cdac1467a56f0d7fde70b3d33385c0 Mon Sep 17 00:00:00 2001 From: Shaun Crampton Date: Tue, 11 Jul 2017 09:28:37 +0100 Subject: [PATCH 1/2] Only delete conntrack entries for orig-src and reply-src. This reduces the number of scans of the conntrack table we need to do. --- conntrack/conntrack.go | 17 +++++++++++++---- conntrack/conntrack_test.go | 18 ------------------ 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/conntrack/conntrack.go b/conntrack/conntrack.go index 11b6ef08e0..6b24e360cb 100644 --- a/conntrack/conntrack.go +++ b/conntrack/conntrack.go @@ -22,11 +22,20 @@ import ( log "github.com/Sirupsen/logrus" ) -var conntrackDirections = []string{ +// For TCP/UDP, each conntrack entry holds two copies of the tuple +// (src addr, dst addr, src port, dst port). One copy for the original direction and one copy for +// the reply direction. This is how the kernel handles NAT: by looking up the tuple for a packet +// by its original tuple and mapping onto the corresponding reply direction tuple (or vice versa). +// +// When we delete conntrack entries by IP address, we need to specify which element of the tuple +// to look in. This slice holds the flags corresponding to the fields we care about. Since we're +// deleting entries for local workload endpoints, either the endpoint originated the traffic, or it +// received the traffic and replied to it. In the originating case, the "original source" will be +// set to the endpoint's IP; in the other case, the "reply source". Hence, it's sufficient to only +// look in those two fields. +var deleteDirections = []string{ "--orig-src", - "--orig-dst", "--reply-src", - "--reply-dst", } const numRetries = 3 @@ -65,7 +74,7 @@ func (c Conntrack) RemoveConntrackFlows(ipVersion uint8, ipAddr net.IP) { log.WithField("version", ipVersion).Panic("Unknown IP version") } log.WithField("ip", ipAddr).Info("Removing conntrack flows") - for _, direction := range conntrackDirections { + for _, direction := range deleteDirections { logCxt := log.WithFields(log.Fields{"ip": ipAddr, "direction": direction}) // Retry a few times because the conntrack command seems to fail at random. for retry := 0; retry <= numRetries; retry += 1 { diff --git a/conntrack/conntrack_test.go b/conntrack/conntrack_test.go index ed73ce6d89..161edf6f7d 100644 --- a/conntrack/conntrack_test.go +++ b/conntrack/conntrack_test.go @@ -35,18 +35,14 @@ var _ = Describe("Conntrack", func() { conntrack.RemoveConntrackFlows(4, net.ParseIP("10.0.0.1")) Expect(cmdRec.cmdArgs).To(Equal([][]string{ []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, })) }) It("IPv6: Should remove all directions", func() { conntrack.RemoveConntrackFlows(6, net.ParseIP("fe80::beef")) Expect(cmdRec.cmdArgs).To(Equal([][]string{ []string{"--family", "ipv6", "--delete", "--orig-src", "fe80::beef"}, - []string{"--family", "ipv6", "--delete", "--orig-dst", "fe80::beef"}, []string{"--family", "ipv6", "--delete", "--reply-src", "fe80::beef"}, - []string{"--family", "ipv6", "--delete", "--reply-dst", "fe80::beef"}, })) }) It("should panic on unknown IP version", func() { @@ -62,9 +58,7 @@ var _ = Describe("Conntrack", func() { conntrack.RemoveConntrackFlows(4, net.ParseIP("10.0.0.1")) Expect(cmdRec.cmdArgs).To(Equal([][]string{ []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, })) }) }) @@ -78,9 +72,7 @@ var _ = Describe("Conntrack", func() { Expect(cmdRec.cmdArgs).To(Equal([][]string{ []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, })) }) }) @@ -97,20 +89,10 @@ var _ = Describe("Conntrack", func() { []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--orig-src", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--orig-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, []string{"--family", "ipv4", "--delete", "--reply-src", "10.0.0.1"}, - - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, - []string{"--family", "ipv4", "--delete", "--reply-dst", "10.0.0.1"}, })) }) }) From 80fe398137bf24cb642e1aa6d699fdbd05fab2d5 Mon Sep 17 00:00:00 2001 From: Shaun Crampton Date: Tue, 11 Jul 2017 11:54:58 +0100 Subject: [PATCH 2/2] Code review markups. --- conntrack/conntrack.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/conntrack/conntrack.go b/conntrack/conntrack.go index 6b24e360cb..081fd4654b 100644 --- a/conntrack/conntrack.go +++ b/conntrack/conntrack.go @@ -26,6 +26,8 @@ import ( // (src addr, dst addr, src port, dst port). One copy for the original direction and one copy for // the reply direction. This is how the kernel handles NAT: by looking up the tuple for a packet // by its original tuple and mapping onto the corresponding reply direction tuple (or vice versa). +// The reply tuple is calculated when the original outgoing packet is processed (and possibly +// NATted). // // When we delete conntrack entries by IP address, we need to specify which element of the tuple // to look in. This slice holds the flags corresponding to the fields we care about. Since we're