Refactor sending and receiving of packets #25

henridevieux · 2017-06-21T22:26:23Z

This marks a very large set of changes on how Arachne sends and receives Echo requests. The agents will now open a single file descriptor for each the sending and receiving of packets - thus eliminating the need to create a socket for each outbound echo.

As a side effect of that change, the need arose for agents to manually craft IP headers, as the DSCP value can no longer be set on a per-socket basis. This change uses gopacket for the creation of these headers as well as for the decoding of incoming packets.

Lastly, pcap proved to be too inaccurate for testing in a sub-millisecond environment. This was due gopacket's pcap timeout minimum value of 1ms. And, although pcap_set_immediate_mode could be used, it would a very tight loop of requesting packets and error handling which is ultimately more overhead. The decision here is that creating a raw socket and applying a BPF filter (much to the same overall effect as pcap would achieve) is a more efficient and controllable solution.

billf

first pass

billf · 2017-06-28T03:19:17Z

glide.yaml

@@ -7,7 +7,6 @@ import:
 - package: github.com/google/gopacket
  version: ^1
  subpackages:
-  - pcap


This was due gopacket's pcap timeout minimum value of 1ms.

That is unfortunate.

billf · 2017-06-28T03:20:25Z

internal/ip/ip.go

+	}
+	if filter, err := getBPFFilter(ipHeaderOffset, uint32(listenPort)); err != nil {
+		logger.Warn("Failed to compile BPF Filter", zap.Error(err))
+	} else {


go idiom: early return, remove the else

https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

billf · 2017-06-28T03:20:49Z

internal/ip/ip.go

+	} else {
+		// Attempt to attach the BPF filter.
+		// This is currently only supported on Linux systems.
+		if err = rs.attachBPF(filter); err != nil {


err := rs.attachBPF

billf · 2017-06-28T03:22:39Z

internal/ip/ip_linux.go

@@ -22,13 +22,20 @@ package ip

 import (
 	"net"
+	"syscall"
+	"unsafe"


in general, minimize "unsafe" code to the absolute minimum. in this case, that means splitting this file into iplayer_linux.go that contains the non-unsafe, linux-specific code.

billf · 2017-06-28T03:24:51Z

internal/tcp/tcp.go

-		if err := unByteTime.UnmarshalBinary(ts); err == nil {
-			pkt.Ts = unByteTime
+// parsePktIP parses the IP header of an incoming packet and extracts the src IP addr and DSCP value.
+func parsePktIP(pkt gopacket.Packet) (srcIP net.IP, dscpv DSCPValue, err error) {


https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

billf · 2017-06-28T03:35:53Z

internal/tcp/tcp.go

-				if err != nil {
+				ackNum := tcpHeader.Seq + 1
+				flags := tcpFlags{syn: true, ack: true}
+				if err = send(conn, &srcIP, listenPort, srcPortRange, DSCPv,


err := send(

billf · 2017-06-28T03:37:00Z

internal/tcp/tcp.go

-						Payload: pkt.tcpPayload.Ts},
-					Seq: pkt.seqNum,
-					Ack: pkt.ackNum,
+						Payload: payloadTime},


consider writing as

Ts: Timestamp{ Run: receiveTime, Payload: payloadTime, },

as you come across these

billf · 2017-06-28T03:39:56Z

internal/tcp/tcp_test.go

+func TestParsePktTCP(t *testing.T) {
+	assert := assert.New(t)
+
+	pktData := []byte{69, 0, 0, 55, 145, 204, 64, 0, 56, 6, 81, 145, 10, 1, 1, 10, 10, 0, 0, 10, 121, 24, 172, 79, 118, 141, 174, 112, 251, 246, 199, 17, 80, 18, 170, 170, 154, 237, 0, 0, 1, 0, 0, 0, 14, 208, 222, 40, 104, 36, 169, 82, 116, 0, 0}


humans can't read this so it should be documented in some way. what is the nature of this packet that makes it suitable for this test.

billf · 2017-06-28T03:40:25Z

internal/tcp/tcp_test.go

+	assert.Equal(tcpHeader.DstPort, layers.TCPPort(44111), "unexpectedly formatted TCP Dst Port")
+
+	expectedTime, err := time.Parse(time.RFC3339, "2017-06-22T21:06:48.615076468+00:00")
+	if err != nil {


require.NoError(t, err, "Error parsing timestamp")

billf · 2017-06-28T03:40:46Z

internal/tcp/tcp_test.go

-		t.Fatal("Non IPv4 or IPv6 family should not be handled")
+}
+
+func TestParsePktIP(t *testing.T) {


same comments as above

Vasilis · 2017-06-21T22:57:31Z

internal/ip/ip.go

 	"net"
 	"syscall"

+	"golang.org/x/net/bpf"


This should go in the bottom of the imports with the rest of the external ones.

Vasilis · 2017-07-06T00:44:29Z

README.md

+### Note on BPF filtering
+
+When receiving packets, Arachne attempts to apply a BPF filter to the raw socket
+so that processing of packets occurs on a much smaller set of (ones destined


"of ..."? Parenthesis needed or not?

Vasilis · 2017-07-06T00:45:15Z

README.md

+When receiving packets, Arachne attempts to apply a BPF filter to the raw socket
+so that processing of packets occurs on a much smaller set of (ones destined
+specifically for Arachne agent testing). This is currently supported only on
+Linux and thus performance will be significantly worse on BSD-based systems


significantly?

Vasilis · 2017-07-06T00:48:53Z

bootstrap.go

@@ -150,8 +164,7 @@ func Run(ec *config.Extended, opts ...Option) {
 		if !*gl.CLI.SenderOnlyMode {
 			// Listen for responses or probes from other IPv4 arachne agents.
 			killC.Receiver = make(chan struct{})
-			err = tcp.Receiver("ip4", &gl.RemoteConfig.SrcAddress, gl.RemoteConfig.TargetTCPPort,
-				gl.RemoteConfig.InterfaceName, sentC, rcvdC, killC.Receiver, logger)
+			err = tcp.Receiver(connIPv4, gl.RemoteConfig.TargetTCPPort, sentC, rcvdC, killC.Receiver, logger)


Is gl.RemoteConfig.TargetTCPPort redundant here? Can it not be extracted from connIPv4?

Vasilis · 2017-07-06T00:50:13Z

collector/collector_test.go

@@ -117,7 +117,7 @@ func TestRun(t *testing.T) {
 		Type:    tcp.EchoRequest,
 		SrcAddr: source,
 		DstAddr: target,
-		Af:      network.Family(&target),
+		Af:      defines.AfInet,


Mind if we keep the following import consistent?
import defines as d

Importing of defines was previous code. We can address changing defines to d throughout the entire codebase in a separate diff.

Vasilis · 2017-07-06T01:06:29Z

internal/tcp/tcp.go

@@ -81,47 +75,14 @@ func (q echoType) text(logger *log.Logger) string {
 }

 const tcpHdrSize int = 20 // 20 bytes without any TCP Options


Move these constants to defines.go?

Vasilis · 2017-07-06T01:12:58Z

internal/tcp/tcp.go

-		}...)
-	default:
-		return 0, errors.New("unhandled AF family")
+	ipLayer, err := ip.GetIPHeaderLayer(af, uint8(dscpv), uint16(tcpLen), srcAddr, dstAddr)


Isn't dscpv already uint8? If not, we can fix the argument of the calling function.

Vasilis · 2017-07-06T01:15:29Z

internal/tcp/tcp.go

-			if !destinedToArachne {
+			srcIP, DSCPv, err := parsePktIP(pkt)
+			if err != nil {
+				logger.Error("Error parsing packet IP layer", zap.Error(err), zap.Any("packet", pkt))


Capitalized error message... See previous comment please.

Vasilis · 2017-07-06T01:17:12Z

internal/tcp/tcp.go


 				// Replying with SYN+ACK to Arachne agent
-				srcPortRange := PortRange{pkt.srcPort, pkt.srcPort}
+				srcPortRange := PortRange{uint16(tcpHeader.SrcPort), uint16(tcpHeader.SrcPort)}


Why cast here? Can we take care of this above (in PortRange for instance if needed)?

Vasilis · 2017-07-06T01:19:43Z

internal/tcp/tcp_test.go


+	"github.com/google/gopacket"


Imports on line 30 and 31 should come with set of line 34.

henridevieux · 2017-07-10T22:41:16Z

internal/ip/iplayer_linux.go

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package ip


See previous conversation - I agreed with @billf that non-unsafe code should be separated out from unsafe code.

Vasilis

Once these are addressed, I believe we're good for prime time!

Vasilis · 2017-07-13T06:14:24Z

bootstrap.go

@@ -179,7 +192,7 @@ func Run(ec *config.Extended, opts ...Option) {
 			logger.Debug("Received SIG")


Line 187 exceeds the 120 char width limit.

Vasilis · 2017-07-13T06:17:02Z

collector/collector.go

@@ -27,8 +27,10 @@ import (
 	"sync"
 	"time"

+	"github.com/google/gopacket/layers"


This import should follow after the uber arachne imports with one line of separation from latter.

Vasilis · 2017-07-13T06:22:02Z

config/config.go

@@ -37,6 +37,7 @@ import (
 	"github.com/uber/arachne/internal/tcp"
 	"github.com/uber/arachne/metrics"

+	"github.com/google/gopacket/layers"
 	"github.com/jawher/mow.cli"
 	"github.com/pkg/errors"
 	"github.com/uber/arachne/defines"


Can we please move this under line 38 above?

Vasilis · 2017-07-13T06:31:06Z

internal/ip/iplayer_linux.go

@@ -0,0 +1,55 @@
+// Copyright (c) 2016 Uber Technologies, Inc.


Since this is really the equivalent of ip_darwin.go in terms of functions included, I would call this file ip_linux.go and the other linux file I would call it ip_linux_bpf.go or ip_linux_WHATEVER.go so that it follows alphabetically the original file.

Vasilis · 2017-07-13T06:32:08Z

internal/tcp/tcp.go

 	"time"

+	"github.com/google/gopacket"


See comment above.

Vasilis · 2017-07-13T06:46:09Z

internal/tcp/tcp.go

-
-	if pkt.dataOffset > 5 {
-		binary.Read(r, binary.BigEndian, &pkt.options)
+	tcp := layer.(*layers.TCP)


Warning: name of variable starts with package name.

Vasilis · 2017-07-13T06:51:52Z

internal/tcp/tcp.go

-				if err != nil {
+				ackNum := tcpHeader.Seq + 1
+				flags := tcpFlags{syn: true, ack: true}
+				// Replies are sent to the same port as this agent is listening on (conn.ListenPort)


"as the one this agent..."

Vasilis · 2017-07-13T06:52:42Z

internal/tcp/tcp.go

 				}
 				// Send 'echo' reply message received to collector
 				in <- inMsg

-				if inMsg.FromExternalTarget(listenPort) {
+				if inMsg.FromExternalTarget(conn.ListenPort) {
 					//TODO verify


Vasilis · 2017-07-13T06:54:18Z

internal/tcp/tcp.go

@@ -612,14 +360,14 @@ func EchoTargets(
 ) {
 	go func() {
 		for {
-			for i := range GetDSCP {
+			for i := range ip.GetDSCP {
 				t0 := time.Now()


Change to monoNow()?

We can address in a separate change.

Vasilis · 2017-07-13T06:57:04Z

internal/tcp/tcp.go

 		flag = "RST"
 	default:
 		flag = ""
 	}

 	go func() {
-		defer syscall.Close(sendSocket)
-
 		rand.Seed(time.Now().UnixNano())


Shouldn't this be in Receiver()?

Same as above.

* Add support for location info to targets * Print out target location info

Refactor sending and receiving of packets

2ca9420

henridevieux requested review from Vasilis and billf June 21, 2017 22:26

henridevieux mentioned this pull request Jun 21, 2017

New Socket created for each outbound packet #8

Closed

Henri Devieux added 5 commits June 22, 2017 18:12

Fix tcp_test to match refactoring changes

a5ba851

Remove formatting in call to Fatal

afc3ccb

Update glide.lock for new dependencies

b89bcea

Fix AF in collector testing functions

c9daffd

Fix ordering of imports

719e225

billf reviewed Jun 28, 2017

View reviewed changes

Henri Devieux added 2 commits July 5, 2017 14:36

Address comments

6f152a4

Add iplayer_linux.go

ca64ff2

Vasilis reviewed Jul 6, 2017

View reviewed changes

Make all tcp ports type gopacket/layers.TCPPort

13ae426

henridevieux commented Jul 10, 2017

View reviewed changes

Vasilis reviewed Jul 13, 2017

View reviewed changes

Vasilis approved these changes Jul 13, 2017

View reviewed changes

Vasilis and others added 6 commits July 14, 2017 15:40

Add support for target location info (#28)

567a1d8

* Add support for location info to targets * Print out target location info

Bump version

18f0934

Refactor sending and receiving of packets

76f9aab

Address comments, rename ip files

38ac3a9

add BPF specific ip files

edbb785

Merge branch 'master' into shared_sockets

aeb4f02

henridevieux merged commit f930c36 into master Jul 22, 2017

		@@ -81,47 +75,14 @@ func (q echoType) text(logger *log.Logger) string {
		}

		const tcpHdrSize int = 20 // 20 bytes without any TCP Options

		@@ -179,7 +192,7 @@ func Run(ec *config.Extended, opts ...Option) {
		logger.Debug("Received SIG")

		@@ -0,0 +1,55 @@
		// Copyright (c) 2016 Uber Technologies, Inc.

Refactor sending and receiving of packets #25

Refactor sending and receiving of packets #25

Conversation

henridevieux commented Jun 21, 2017

billf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridevieux Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vasilis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridevieux Jul 10, 2017 •

edited

Loading