From 5461b762bc6c5f482478d75d3997cdf892cc364a Mon Sep 17 00:00:00 2001 From: KingJ Date: Tue, 1 Dec 2020 09:27:45 +0000 Subject: [PATCH] Explicitly set Service Port Protocol for Jaeger Receivers (#117) The service port 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. For example, the following manifest will currently create service port entries with the wrong protocol for thrift_compact and thrift_binary protocols; ``` apiVersion: opentelemetry.io/v1alpha1 kind: OpenTelemetryCollector metadata: name: simplest spec: config: | receivers: jaeger: protocols: grpc: thrift_compact: thrift_binary: thrift_http: processors: queued_retry: exporters: logging: loglevel: debug service: pipelines: traces: receivers: [jaeger] processors: [queued_retry] exporters: [logging] ``` ``` Type: ClusterIP IP: 10.233.59.118 Port: jaeger-grpc 14250/TCP TargetPort: 14250/TCP Endpoints: 10.233.110.188:14250 Port: jaeger-thrift-http 14268/TCP TargetPort: 14268/TCP Endpoints: 10.233.110.188:14268 Port: jaeger-thrift-compact 6831/TCP TargetPort: 6831/TCP Endpoints: 10.233.110.188:6831 Port: jaeger-thrift-binary 6832/TCP TargetPort: 6832/TCP Endpoints: 10.233.110.188:6832 ``` This PR fixes this behavior by extending the protocol struct to also include each Jaeger receiver protocol's protocol and set the service port's protocol when a new Jaeger receiver is created. I have also updated the Jaeger receiver test cases to expect the correct service port protocol for each Jaeger receiver protocol, and they all pass. I have also tested this manually against my own cluster and confirmed that the service port protocol is set for UDP for thrift_binary and thrift_compact both when an endpoint is explicitly defined and when left unset. I'm still quite new to OpenTelemetry, so i'm open to feedback if i've misunderstood things! --- pkg/collector/parser/receiver_jaeger.go | 28 +++++++++++++------- pkg/collector/parser/receiver_jaeger_test.go | 17 +++++++----- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/pkg/collector/parser/receiver_jaeger.go b/pkg/collector/parser/receiver_jaeger.go index ec3ff16a4a..9ca3e3a037 100644 --- a/pkg/collector/parser/receiver_jaeger.go +++ b/pkg/collector/parser/receiver_jaeger.go @@ -46,24 +46,29 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { ports := []corev1.ServicePort{} for _, protocol := range []struct { - name string - defaultPort int32 + name string + defaultPort int32 + transportProtocol corev1.Protocol }{ { - name: "grpc", - defaultPort: defaultGRPCPort, + name: "grpc", + defaultPort: defaultGRPCPort, + transportProtocol: corev1.ProtocolTCP, }, { - name: "thrift_http", - defaultPort: defaultThriftHTTPPort, + name: "thrift_http", + defaultPort: defaultThriftHTTPPort, + transportProtocol: corev1.ProtocolTCP, }, { - name: "thrift_compact", - defaultPort: defaultThriftCompactPort, + name: "thrift_compact", + defaultPort: defaultThriftCompactPort, + transportProtocol: corev1.ProtocolUDP, }, { - name: "thrift_binary", - defaultPort: defaultThriftBinaryPort, + name: "thrift_binary", + defaultPort: defaultThriftBinaryPort, + transportProtocol: corev1.ProtocolUDP, }, } { // do we have the protocol specified at all? @@ -87,6 +92,9 @@ func (j *JaegerReceiverParser) Ports() ([]corev1.ServicePort, error) { } } + // 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 46bd0a16eb..3c1b12450f 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) { @@ -67,13 +70,14 @@ func TestJaegerExposeDefaultPorts(t *testing.T) { }) expectedResults := map[string]struct { - portNumber int32 - seen bool + portNumber int32 + seen bool + transportProtocol 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, 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 @@ -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.transportProtocol, port.Protocol) } for k, v := range expectedResults { assert.True(t, v.seen, "the port %s wasn't included in the service ports", k)