Do not add Compliance Region to the packet if it's a 0 value

Author: mitch292

protocol/conn_test.go

@@ -0,0 +1,67 @@
+package protocol
+
+import (
+	"bytes"
+	"crypto/rand"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestRespond(t *testing.T) {
+	tests := []struct {
+		name                  string
+		payloadSize           int
+		expectedTotalByteSize int64
+		expectedLengthHeader  uint16
+	}{
+		{
+			name:                  "minimum packet size is 1024",
+			payloadSize:           1000,
+			expectedTotalByteSize: 1024,
+			expectedLengthHeader:  1016, // minimum size - header or payload tlv + opcode tlv + padding
+		},
+		{
+			name:                  "packet size over 1024 bytes",
+			payloadSize:           2000,
+			expectedTotalByteSize: 2015, // payload TLV + 4 bytes for the Opcode + 8 bytes for header
+			expectedLengthHeader:  2007, // expectedTotalByteSize - header
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require := require.New(t)
+
+			var buf bytes.Buffer
+			payload := make([]byte, tt.payloadSize)
+			rand.Read(payload)
+
+			Respond(&buf, 1, payload)
+
+			var pkt Packet
+			size, err := pkt.ReadFrom(&buf)
+			require.NoError(err)
+			require.Equal(tt.expectedTotalByteSize, size)
+			require.Equal(tt.expectedLengthHeader, pkt.Length)
+			// the only two non-header fields on the response should be Opcode and Payload
+			// if other fields are added, it can be a breaking change for the client depending
+			// the implementation of the protocol.
+			require.NotEmpty(pkt.Opcode)
+			require.NotEmpty(pkt.Payload)
+
+			require.Empty(pkt.Extra)
+			require.Empty(pkt.SKI)
+			require.Empty(pkt.Digest)
+			require.Empty(pkt.ClientIP)
+			require.Empty(pkt.ServerIP)
+			require.Empty(pkt.SNI)
+			require.Empty(pkt.CertID)
+			require.Empty(pkt.ForwardingSvc)
+			require.Empty(pkt.CustomFuncName)
+			require.Empty(pkt.JaegerSpan)
+			require.Empty(pkt.ReqContext)
+			require.Empty(pkt.ComplianceRegion)
+		})
+	}
+}

protocol/protocol.go

@@ -527,7 +527,9 @@ func (o *Operation) Bytes() uint16 {
 		add(tlvLen(len(o.ReqContext)))
 	}
 	// compliance region
-	add(tlvLen(1))
+	if o.ComplianceRegion != 0 {
+		add(tlvLen(1))
+	}
 	if int(length)+headerSize < paddedLength {
 		// TODO: Are we sure that's the right behavior?
 
@@ -602,7 +604,9 @@ func (o *Operation) MarshalBinary() ([]byte, error) {
 		b = append(b, tlvBytes(TagReqContext, o.ReqContext)...)
 	}
 
-	b = append(b, tlvBytes(TagComplianceRegion, []byte{byte(o.ComplianceRegion)})...)
+	if o.ComplianceRegion != 0 {
+		b = append(b, tlvBytes(TagComplianceRegion, []byte{byte(o.ComplianceRegion)})...)
+	}
 
 	if len(b)+headerSize < paddedLength {
 		// TODO: Are we sure that's the right behavior?

protocol/protocol_test.go

@@ -42,8 +42,36 @@ func TestMarshalBinary(t *testing.T) {
 	require.NoError(err)
 
 	var pkt2 Packet
-	_, err = pkt2.ReadFrom(bytes.NewReader(b))
+	size, err := pkt2.ReadFrom(bytes.NewReader(b))
 	require.NoError(err)
 	require.Equal(pkt.ID, pkt2.ID)
 	require.Equal(op, pkt2.Operation)
+
+	// now do the same test with a 0 value for compliance region
+	globalOp := Operation{
+		Opcode:         OpECDSASignSHA256,
+		Payload:        payload,
+		Extra:          extra,
+		Digest:         sha256.Sum256([]byte("Digest")),
+		SKI:            sha1.Sum([]byte("SKI")),
+		ClientIP:       net.ParseIP("1.1.1.1").To4(),
+		ServerIP:       net.ParseIP("2.2.2.2").To4(),
+		SNI:            "SNI",
+		CertID:         "SNI",
+		CustomFuncName: "CustomFuncName",
+		JaegerSpan:     []byte("615f730ad5fe896f:615f730ad5fe896f:1"),
+		ReqContext:     reqCtx,
+	}
+	globalPkt := NewPacket(42, globalOp)
+	gb, err := globalPkt.MarshalBinary()
+	require.NoError(err)
+
+	var globalPkt2 Packet
+	globalSize, err := globalPkt2.ReadFrom(bytes.NewReader(gb))
+	require.NoError(err)
+	require.Equal(globalPkt.ID, globalPkt2.ID)
+	require.Equal(globalOp, globalPkt2.Operation)
+
+	// the global op should be 4 bytes smaller because it does not include an ComplianceRegion TLV
+	require.Equal(size, globalSize+4)
 }

tracing/tracing.go

@@ -39,15 +39,16 @@ func SpanContextToBinary(sc opentracing.SpanContext) ([]byte, error) {
 // SetOperationSpanTags sets span tags with all of the operation's fields
 func SetOperationSpanTags(span opentracing.Span, op *protocol.Operation) {
 	tags := map[string]interface{}{
-		"operation.opcode":         op.Opcode.String(),
-		"operation.ski":            op.SKI,
-		"operation.digest":         fmt.Sprintf("%02x", op.Digest),
-		"operation.clientip":       op.ClientIP,
-		"operation.serverip":       op.ServerIP,
-		"operation.sni":            op.SNI,
-		"operation.certid":         op.CertID,
-		"operation.customfuncname": op.CustomFuncName,
-		"operation.forwardingsvc":  fmt.Sprintf("%d", op.ForwardingSvc),
+		"operation.opcode":            op.Opcode.String(),
+		"operation.ski":               op.SKI,
+		"operation.digest":            fmt.Sprintf("%02x", op.Digest),
+		"operation.clientip":          op.ClientIP,
+		"operation.serverip":          op.ServerIP,
+		"operation.sni":               op.SNI,
+		"operation.certid":            op.CertID,
+		"operation.customfuncname":    op.CustomFuncName,
+		"operation.forwardingsvc":     fmt.Sprintf("%d", op.ForwardingSvc),
+		"operation.compliance_region": op.ComplianceRegion.String(),
 	}
 	for k, v := range tags {
 		span.SetTag(k, v)