From 45b847dc7eefb29e3a55c66f597bb8537a74b428 Mon Sep 17 00:00:00 2001 From: Kingsley Jarrett Date: Sat, 14 Nov 2020 15:15:19 +0000 Subject: [PATCH 1/4] Explicitly set Service Port Protocol for Jaeger Receivers The servicePort created for each Jaeger receiver does not currently set a Protocol, and therefore defaults to TCP. Some Jaeger receiver protocols (i.e. thrift_binary and thrift_compact) are UDP based rather than TCP, and therefore the operator currently creates service port entries that cannot be used due to the protocol mismatch. --- pkg/collector/parser/receiver_jaeger.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/collector/parser/receiver_jaeger.go b/pkg/collector/parser/receiver_jaeger.go index ec3ff16a4a..5b852e4618 100644 --- a/pkg/collector/parser/receiver_jaeger.go +++ b/pkg/collector/parser/receiver_jaeger.go @@ -48,22 +48,27 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { for _, protocol := range []struct { name string defaultPort int32 + protocol corev1.Protocol }{ { name: "grpc", defaultPort: defaultGRPCPort, + protocol: corev1.ProtocolTCP, }, { name: "thrift_http", defaultPort: defaultThriftHTTPPort, + protocol: corev1.ProtocolTCP, }, { name: "thrift_compact", defaultPort: defaultThriftCompactPort, + protocol: corev1.ProtocolUDP, }, { name: "thrift_binary", defaultPort: defaultThriftBinaryPort, + protocol: corev1.ProtocolUDP, }, } { // do we have the protocol specified at all? @@ -82,9 +87,13 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { // if not, we use the default port if protocolPort == nil { protocolPort = &corev1.ServicePort{ - Name: portName(nameWithProtocol, protocol.defaultPort), - Port: protocol.defaultPort, + Name: portName(nameWithProtocol, protocol.defaultPort), + Port: protocol.defaultPort, + Protocol: protocol.protocol, } + } else { + // port from configuration block has been used, but protocol needs to be set + protocolPort.Protocol = protocol.protocol } // at this point, we *have* a port specified, add it to the list of ports From f044a5cd3e43040761e1eecdc5f4a209f206cd30 Mon Sep 17 00:00:00 2001 From: Kingsley Jarrett Date: Sat, 28 Nov 2020 15:21:06 +0000 Subject: [PATCH 2/4] Update tests after changes in 0e20605e48cd5bb775baeab4a7c304f83665d8d8 --- pkg/collector/parser/receiver_jaeger_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/collector/parser/receiver_jaeger_test.go b/pkg/collector/parser/receiver_jaeger_test.go index 46bd0a16eb..f370721574 100644 --- a/pkg/collector/parser/receiver_jaeger_test.go +++ b/pkg/collector/parser/receiver_jaeger_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" ) func TestJaegerSelfRegisters(t *testing.T) { @@ -34,6 +35,7 @@ func TestJaegerMinimalConfiguration(t *testing.T) { assert.NoError(t, err) assert.Len(t, ports, 1) assert.EqualValues(t, 14250, ports[0].Port) + assert.EqualValues(t, corev1.ProtocolTCP, ports[0].Protocol) } func TestJaegerPortsOverridden(t *testing.T) { @@ -53,6 +55,7 @@ func TestJaegerPortsOverridden(t *testing.T) { assert.NoError(t, err) assert.Len(t, ports, 1) assert.EqualValues(t, 1234, ports[0].Port) + assert.EqualValues(t, corev1.ProtocolTCP, ports[0].Protocol) } func TestJaegerExposeDefaultPorts(t *testing.T) { @@ -69,11 +72,12 @@ func TestJaegerExposeDefaultPorts(t *testing.T) { expectedResults := map[string]struct { portNumber int32 seen bool + protocol corev1.Protocol }{ - "jaeger-grpc": {portNumber: 14250}, - "jaeger-thrift-http": {portNumber: 14268}, - "jaeger-thrift-compact": {portNumber: 6831}, - "jaeger-thrift-binary": {portNumber: 6832}, + "jaeger-grpc": {portNumber: 14250, protocol: corev1.ProtocolTCP}, + "jaeger-thrift-http": {portNumber: 14268, protocol: corev1.ProtocolTCP}, + "jaeger-thrift-compact": {portNumber: 6831, protocol: corev1.ProtocolUDP}, + "jaeger-thrift-binary": {portNumber: 6832, protocol: corev1.ProtocolUDP}, } // test @@ -88,6 +92,7 @@ func TestJaegerExposeDefaultPorts(t *testing.T) { r.seen = true expectedResults[port.Name] = r assert.EqualValues(t, r.portNumber, port.Port) + assert.EqualValues(t, r.protocol, port.Protocol) } for k, v := range expectedResults { assert.True(t, v.seen, "the port %s wasn't included in the service ports", k) From 7feb1d80083a86f0672d0102f321d80ff121f4c7 Mon Sep 17 00:00:00 2001 From: Kingsley Jarrett Date: Sat, 28 Nov 2020 15:29:15 +0000 Subject: [PATCH 3/4] Set receiver port's protocol prior to appending to the list of ports Reduces code duplication by setting the port's protocol consistently, rather than conditionally based the supplied configuration. --- pkg/collector/parser/receiver_jaeger.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/collector/parser/receiver_jaeger.go b/pkg/collector/parser/receiver_jaeger.go index 5b852e4618..4540875b10 100644 --- a/pkg/collector/parser/receiver_jaeger.go +++ b/pkg/collector/parser/receiver_jaeger.go @@ -87,15 +87,14 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { // if not, we use the default port if protocolPort == nil { protocolPort = &corev1.ServicePort{ - Name: portName(nameWithProtocol, protocol.defaultPort), - Port: protocol.defaultPort, - Protocol: protocol.protocol, + Name: portName(nameWithProtocol, protocol.defaultPort), + Port: protocol.defaultPort, } - } else { - // port from configuration block has been used, but protocol needs to be set - protocolPort.Protocol = protocol.protocol } + // set the appropriate TCP/UDP protocol for this kind of receiver protocol + protocolPort.Protocol = protocol.protocol + // at this point, we *have* a port specified, add it to the list of ports ports = append(ports, *protocolPort) } From 1f1d8511a3ba2589595edbeefdd4097d3ba3ff0a Mon Sep 17 00:00:00 2001 From: Kingsley Jarrett Date: Mon, 30 Nov 2020 20:03:45 +0000 Subject: [PATCH 4/4] Disambiguate transport protocol references Rename protocol to transportProtocol, where protocol refers to a transport protocol (i.e. TCP/UDP) to reduce ambiguity between transport protocols and application protocols. --- pkg/collector/parser/receiver_jaeger.go | 34 ++++++++++---------- pkg/collector/parser/receiver_jaeger_test.go | 16 ++++----- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/pkg/collector/parser/receiver_jaeger.go b/pkg/collector/parser/receiver_jaeger.go index 4540875b10..9ca3e3a037 100644 --- a/pkg/collector/parser/receiver_jaeger.go +++ b/pkg/collector/parser/receiver_jaeger.go @@ -46,29 +46,29 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { ports := []corev1.ServicePort{} for _, protocol := range []struct { - name string - defaultPort int32 - protocol corev1.Protocol + name string + defaultPort int32 + transportProtocol corev1.Protocol }{ { - name: "grpc", - defaultPort: defaultGRPCPort, - protocol: corev1.ProtocolTCP, + name: "grpc", + defaultPort: defaultGRPCPort, + transportProtocol: corev1.ProtocolTCP, }, { - name: "thrift_http", - defaultPort: defaultThriftHTTPPort, - protocol: corev1.ProtocolTCP, + name: "thrift_http", + defaultPort: defaultThriftHTTPPort, + transportProtocol: corev1.ProtocolTCP, }, { - name: "thrift_compact", - defaultPort: defaultThriftCompactPort, - protocol: corev1.ProtocolUDP, + name: "thrift_compact", + defaultPort: defaultThriftCompactPort, + transportProtocol: corev1.ProtocolUDP, }, { - name: "thrift_binary", - defaultPort: defaultThriftBinaryPort, - protocol: corev1.ProtocolUDP, + name: "thrift_binary", + defaultPort: defaultThriftBinaryPort, + transportProtocol: corev1.ProtocolUDP, }, } { // do we have the protocol specified at all? @@ -92,8 +92,8 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { } } - // set the appropriate TCP/UDP protocol for this kind of receiver protocol - protocolPort.Protocol = protocol.protocol + // set the appropriate transport protocol (i.e. TCP/UDP) for this kind of receiver protocol + protocolPort.Protocol = protocol.transportProtocol // at this point, we *have* a port specified, add it to the list of ports ports = append(ports, *protocolPort) diff --git a/pkg/collector/parser/receiver_jaeger_test.go b/pkg/collector/parser/receiver_jaeger_test.go index f370721574..3c1b12450f 100644 --- a/pkg/collector/parser/receiver_jaeger_test.go +++ b/pkg/collector/parser/receiver_jaeger_test.go @@ -70,14 +70,14 @@ func TestJaegerExposeDefaultPorts(t *testing.T) { }) expectedResults := map[string]struct { - portNumber int32 - seen bool - protocol corev1.Protocol + portNumber int32 + seen bool + transportProtocol corev1.Protocol }{ - "jaeger-grpc": {portNumber: 14250, protocol: corev1.ProtocolTCP}, - "jaeger-thrift-http": {portNumber: 14268, protocol: corev1.ProtocolTCP}, - "jaeger-thrift-compact": {portNumber: 6831, protocol: corev1.ProtocolUDP}, - "jaeger-thrift-binary": {portNumber: 6832, protocol: corev1.ProtocolUDP}, + "jaeger-grpc": {portNumber: 14250, transportProtocol: corev1.ProtocolTCP}, + "jaeger-thrift-http": {portNumber: 14268, transportProtocol: corev1.ProtocolTCP}, + "jaeger-thrift-compact": {portNumber: 6831, transportProtocol: corev1.ProtocolUDP}, + "jaeger-thrift-binary": {portNumber: 6832, transportProtocol: corev1.ProtocolUDP}, } // test @@ -92,7 +92,7 @@ func TestJaegerExposeDefaultPorts(t *testing.T) { r.seen = true expectedResults[port.Name] = r assert.EqualValues(t, r.portNumber, port.Port) - assert.EqualValues(t, r.protocol, port.Protocol) + assert.EqualValues(t, r.transportProtocol, port.Protocol) } for k, v := range expectedResults { assert.True(t, v.seen, "the port %s wasn't included in the service ports", k)