Skip to content

Commit

Permalink
Explicitly set Service Port Protocol for Jaeger Receivers (open-telem…
Browse files Browse the repository at this point in the history
…etry#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!
  • Loading branch information
KingJ authored Dec 1, 2020
1 parent 7d0f302 commit 5461b76
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 16 deletions.
28 changes: 18 additions & 10 deletions pkg/collector/parser/receiver_jaeger.go
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand 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)
}
Expand Down
17 changes: 11 additions & 6 deletions pkg/collector/parser/receiver_jaeger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
)

func TestJaegerSelfRegisters(t *testing.T) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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
Expand All @@ -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)
Expand Down

0 comments on commit 5461b76

Please sign in to comment.