From 53695fb8dd93bc351c2a360e99eccffdb9cf1c6f Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Sat, 13 Jul 2024 16:17:20 -0400 Subject: [PATCH] Add nop parser for exporters (#3125) * add nop parser for exporter use * chlog * Fix exporter tests * another one --- .chloggen/fix-parsing-bug.yaml | 16 +++++++ apis/v1beta1/config.go | 1 + apis/v1beta1/config_test.go | 15 +----- controllers/reconcile_test.go | 36 ++++++++++++++ controllers/suite_test.go | 1 + controllers/testdata/otlp_test.yaml | 23 +++++++++ internal/components/exporters/helpers.go | 2 +- internal/components/exporters/helpers_test.go | 3 +- internal/components/multi_endpoint.go | 2 +- internal/components/nop_parser.go | 48 +++++++++++++++++++ internal/manifests/collector/service_test.go | 41 ++++++++++++++++ 11 files changed, 170 insertions(+), 18 deletions(-) create mode 100755 .chloggen/fix-parsing-bug.yaml create mode 100644 controllers/testdata/otlp_test.yaml create mode 100644 internal/components/nop_parser.go diff --git a/.chloggen/fix-parsing-bug.yaml b/.chloggen/fix-parsing-bug.yaml new file mode 100755 index 0000000000..3c1f00ca8c --- /dev/null +++ b/.chloggen/fix-parsing-bug.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action) +component: collector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Fixes a bug where an exporter would cause a port collision + +# One or more tracking issues related to the change +issues: [3124] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/apis/v1beta1/config.go b/apis/v1beta1/config.go index 3dd2350ae8..61fecb3ad1 100644 --- a/apis/v1beta1/config.go +++ b/apis/v1beta1/config.go @@ -141,6 +141,7 @@ type Config struct { // getPortsForComponentKinds gets the ports for the given ComponentKind(s). func (c *Config) getPortsForComponentKinds(logger logr.Logger, componentKinds ...ComponentKind) ([]corev1.ServicePort, error) { + var ports []corev1.ServicePort enabledComponents := c.GetEnabledComponents() for _, componentKind := range componentKinds { diff --git a/apis/v1beta1/config_test.go b/apis/v1beta1/config_test.go index 117ad5a414..a64944da94 100644 --- a/apis/v1beta1/config_test.go +++ b/apis/v1beta1/config_test.go @@ -497,25 +497,12 @@ func TestConfig_GetExporterPorts(t *testing.T) { Name: "prometheus", Port: 8889, }, - { - Name: "otlp", - Port: 4317, - }, - { - Name: "zipkin", - Port: 9411, - }, }, }, { name: "extensions", file: "testdata/otelcol-extensions.yaml", - want: []v1.ServicePort{ - { - Name: "otlp-auth", - Port: 4317, - }, - }, + want: nil, }, { name: "filelog", diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go index 53385015c1..38205f0be9 100644 --- a/controllers/reconcile_test.go +++ b/controllers/reconcile_test.go @@ -99,6 +99,7 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) { deploymentExtraPorts.Annotations = map[string]string{ "new-annotation": "new-value", } + baseOTLPParams := testCollectorAssertNoErr(t, "test-otlp", "", otlpTestFile) ingressParams := testCollectorAssertNoErr(t, "test-ingress", "", testFileIngress) ingressParams.Spec.Ingress.Type = "ingress" updatedIngressParams := testCollectorAssertNoErr(t, "test-ingress", "", testFileIngress) @@ -220,6 +221,41 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) { }, }, }, + + { + name: "otlp receiver collector", + args: args{ + params: baseOTLPParams, + updates: []v1alpha1.OpenTelemetryCollector{}, + }, + want: []want{ + { + result: controllerruntime.Result{}, + checks: []check[v1alpha1.OpenTelemetryCollector]{ + func(t *testing.T, params v1alpha1.OpenTelemetryCollector) { + d := appsv1.StatefulSet{} + exists, err := populateObjectIfExists(t, &d, namespacedObjectName(naming.Collector(params.Name), params.Namespace)) + assert.NoError(t, err) + assert.True(t, exists) + assert.Equal(t, int32(1), *d.Spec.Replicas) + svc := &v1.Service{} + exists, err = populateObjectIfExists(t, svc, namespacedObjectName(naming.Service(params.Name), params.Namespace)) + assert.NoError(t, err) + assert.True(t, exists) + assert.Equal(t, svc.Spec.Selector, map[string]string{ + "app.kubernetes.io/component": "opentelemetry-collector", + "app.kubernetes.io/instance": "default.test-otlp", + "app.kubernetes.io/managed-by": "opentelemetry-operator", + "app.kubernetes.io/part-of": "opentelemetry", + }) + assert.Len(t, svc.Spec.Ports, 4) + }, + }, + wantErr: assert.NoError, + validateErr: assert.NoError, + }, + }, + }, { name: "invalid mode", args: args{ diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 0b8ee89adf..219043c4a8 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -91,6 +91,7 @@ const ( promFile = "testdata/test.yaml" updatedPromFile = "testdata/test_ta_update.yaml" testFileIngress = "testdata/ingress_testdata.yaml" + otlpTestFile = "testdata/otlp_test.yaml" ) var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) diff --git a/controllers/testdata/otlp_test.yaml b/controllers/testdata/otlp_test.yaml new file mode 100644 index 0000000000..0d35e11188 --- /dev/null +++ b/controllers/testdata/otlp_test.yaml @@ -0,0 +1,23 @@ +receivers: + otlp: + protocols: + grpc: + http: +processors: +exporters: + otlp: + endpoint: jaeger-allinone-collector-headless.chainsaw-otlp-metrics.svc:4317 + tls: + insecure: true + prometheus: + endpoint: 0.0.0.0:8889 + resource_to_telemetry_conversion: + enabled: true # by default resource attributes are dropped +service: + pipelines: + traces: + receivers: [otlp] + exporters: [otlp] + metrics: + receivers: [otlp] + exporters: [prometheus] diff --git a/internal/components/exporters/helpers.go b/internal/components/exporters/helpers.go index 644477cb5d..e8626f73b5 100644 --- a/internal/components/exporters/helpers.go +++ b/internal/components/exporters/helpers.go @@ -38,7 +38,7 @@ func ParserFor(name string) components.ComponentPortParser { return parser } // We want the default for exporters to fail silently. - return components.NewSilentSinglePortParser(components.ComponentType(name), components.UnsetPort) + return components.NewNopParser(components.ComponentType(name), components.UnsetPort) } var ( diff --git a/internal/components/exporters/helpers_test.go b/internal/components/exporters/helpers_test.go index 5e449cf74f..80dfed3a20 100644 --- a/internal/components/exporters/helpers_test.go +++ b/internal/components/exporters/helpers_test.go @@ -34,8 +34,7 @@ func TestParserForReturns(t *testing.T) { "endpoint": "localhost:9000", }) assert.NoError(t, err) - assert.Len(t, ports, 1) - assert.Equal(t, ports[0].Port, int32(9000)) + assert.Len(t, ports, 0) // Should use the nop parser } func TestCanRegister(t *testing.T) { diff --git a/internal/components/multi_endpoint.go b/internal/components/multi_endpoint.go index e089465f5d..0f3294d1b9 100644 --- a/internal/components/multi_endpoint.go +++ b/internal/components/multi_endpoint.go @@ -53,8 +53,8 @@ func (m *MultiPortReceiver) Ports(logger logr.Logger, name string, config interf port := defaultSvc.Port if ec != nil { port = ec.GetPortNumOrDefault(logger, port) - defaultSvc.Name = naming.PortName(fmt.Sprintf("%s-%s", name, protocol), port) } + defaultSvc.Name = naming.PortName(fmt.Sprintf("%s-%s", name, protocol), port) ports = append(ports, ConstructServicePort(defaultSvc, port)) } else { return nil, fmt.Errorf("unknown protocol set: %s", protocol) diff --git a/internal/components/nop_parser.go b/internal/components/nop_parser.go new file mode 100644 index 0000000000..4f66c7ec8c --- /dev/null +++ b/internal/components/nop_parser.go @@ -0,0 +1,48 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package components + +import ( + "fmt" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" +) + +var ( + _ ComponentPortParser = &NopParser{} +) + +// SingleEndpointParser is a special parser for a generic receiver that has an endpoint or listen_address in its +// configuration. It doesn't self-register and should be created/used directly. +type NopParser struct { + name string +} + +func (n *NopParser) Ports(logger logr.Logger, name string, config interface{}) ([]corev1.ServicePort, error) { + return nil, nil +} + +func (n *NopParser) ParserType() string { + return ComponentType(n.name) +} + +func (n *NopParser) ParserName() string { + return fmt.Sprintf("__%s", n.name) +} + +func NewNopParser(name string, port int32, opts ...PortBuilderOption) *NopParser { + return &NopParser{name: name} +} diff --git a/internal/manifests/collector/service_test.go b/internal/manifests/collector/service_test.go index 2a5cd8d08f..bf35dc7a61 100644 --- a/internal/manifests/collector/service_test.go +++ b/internal/manifests/collector/service_test.go @@ -226,6 +226,47 @@ func TestDesiredService(t *testing.T) { assert.Equal(t, expected, *actual) }) + t.Run("should return service with OTLP ports", func(t *testing.T) { + params := manifests.Params{ + Config: config.Config{}, + Log: logger, + OtelCol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{Config: v1beta1.Config{ + Receivers: v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "otlp": map[string]interface{}{ + "protocols": map[string]interface{}{ + "grpc": nil, + "http": nil, + }, + }, + }, + }, + Exporters: v1beta1.AnyConfig{ + Object: map[string]interface{}{ + "otlp": map[string]interface{}{ + "endpoint": "jaeger-allinone-collector-headless.chainsaw-otlp-metrics.svc:4317", + }, + }, + }, + Service: v1beta1.Service{ + Pipelines: map[string]*v1beta1.Pipeline{ + "traces": { + Receivers: []string{"otlp"}, + Exporters: []string{"otlp"}, + }, + }, + }, + }}, + }, + } + + actual, err := Service(params) + assert.NotNil(t, actual) + assert.Len(t, actual.Spec.Ports, 2) + assert.NoError(t, err) + }) + } func TestHeadlessService(t *testing.T) {