v1beta1: apply telemetry config defaults in webhook (#3361)

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Update .chloggen/default_telemetry_settings.yaml

add another webhook test

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

avoid using mapstructure

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

test: assert on addr

.chloggen/default_telemetry_settings.yaml

@@ -0,0 +1,19 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# 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: Expose the Collector telemetry endpoint by default.
+
+# One or more tracking issues related to the change
+issues: [3361]
+
+# (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: |
+  The collector v0.111.0 changes the default binding of the telemetry metrics endpoint from `0.0.0.0` to `localhost`.
+  To avoid any disruption we fallback to "0.0.0.0:{PORT}" as default address.
+  Details can be found here: [opentelemetry-collector#11251](https://github.com/open-telemetry/opentelemetry-collector/pull/11251)

apis/v1beta1/collector_webhook_test.go

@@ -169,7 +169,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 					Mode:            v1beta1.ModeDeployment,
 					UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic,
 					Config: func() v1beta1.Config {
-						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"endpoint":"0.0.0.0:4317"},"http":{"endpoint":"0.0.0.0:4318"}}}},"exporters":{"debug":null},"service":{"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
+						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"endpoint":"0.0.0.0:4317"},"http":{"endpoint":"0.0.0.0:4318"}}}},"exporters":{"debug":null},"service":{"telemetry":{"metrics":{"address":"0.0.0.0:8888"}},"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
 						var cfg v1beta1.Config
 						require.NoError(t, yaml.Unmarshal([]byte(input), &cfg))
 						return cfg
@@ -182,7 +182,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 			otelcol: v1beta1.OpenTelemetryCollector{
 				Spec: v1beta1.OpenTelemetryCollectorSpec{
 					Config: func() v1beta1.Config {
-						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
+						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"telemetry":{"metrics":{"address":"1.2.3.4:7654"}},"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
 						var cfg v1beta1.Config
 						require.NoError(t, yaml.Unmarshal([]byte(input), &cfg))
 						return cfg
@@ -201,7 +201,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 					Mode:            v1beta1.ModeDeployment,
 					UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic,
 					Config: func() v1beta1.Config {
-						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"endpoint":"0.0.0.0:4317","headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
+						const input = `{"receivers":{"otlp":{"protocols":{"grpc":{"endpoint":"0.0.0.0:4317","headers":{"example":"another"}},"http":{"endpoint":"0.0.0.0:4000"}}}},"exporters":{"debug":null},"service":{"telemetry":{"metrics":{"address":"1.2.3.4:7654"}},"pipelines":{"traces":{"receivers":["otlp"],"exporters":["debug"]}}}}`
 						var cfg v1beta1.Config
 						require.NoError(t, yaml.Unmarshal([]byte(input), &cfg))
 						return cfg
@@ -554,6 +554,9 @@ func TestCollectorDefaultingWebhook(t *testing.T) {
 			)
 			ctx := context.Background()
 			err := cvw.Default(ctx, &test.otelcol)
+			if test.expected.Spec.Config.Service.Telemetry == nil {
+				assert.NoError(t, test.expected.Spec.Config.Service.ApplyDefaults(), "could not apply defaults")
+			}
 			assert.NoError(t, err)
 			assert.Equal(t, test.expected, test.otelcol)
 		})

apis/v1beta1/config.go

@@ -228,6 +228,9 @@ func (c *Config) getPortsForComponentKinds(logger logr.Logger, componentKinds ..
 
 // applyDefaultForComponentKinds applies defaults to the endpoints for the given ComponentKind(s).
 func (c *Config) applyDefaultForComponentKinds(logger logr.Logger, componentKinds ...ComponentKind) error {
+	if err := c.Service.ApplyDefaults(); err != nil {
+		return err
+	}
 	enabledComponents := c.GetEnabledComponents()
 	for _, componentKind := range componentKinds {
 		var retriever components.ParserRetriever
@@ -371,24 +374,55 @@ type Service struct {
 	Pipelines map[string]*Pipeline `json:"pipelines" yaml:"pipelines"`
 }
 
-// MetricsPort gets the port number for the metrics endpoint from the collector config if it has been set.
-func (s *Service) MetricsPort() (int32, error) {
+// MetricsEndpoint gets the port number and host address for the metrics endpoint from the collector config if it has been set.
+func (s *Service) MetricsEndpoint() (string, int32, error) {
+	defaultAddr := "0.0.0.0"
 	if s.GetTelemetry() == nil {
 		// telemetry isn't set, use the default
-		return 8888, nil
+		return defaultAddr, 8888, nil
 	}
-	_, port, netErr := net.SplitHostPort(s.GetTelemetry().Metrics.Address)
+	host, port, netErr := net.SplitHostPort(s.GetTelemetry().Metrics.Address)
 	if netErr != nil && strings.Contains(netErr.Error(), "missing port in address") {
-		return 8888, nil
+		return defaultAddr, 8888, nil
 	} else if netErr != nil {
-		return 0, netErr
+		return "", 0, netErr
 	}
 	i64, err := strconv.ParseInt(port, 10, 32)
 	if err != nil {
-		return 0, err
+		return "", 0, err
+	}
+
+	if host == "" {
+		host = defaultAddr
+	}
+
+	return host, int32(i64), nil
+}
+
+// ApplyDefaults inserts configuration defaults if it has not been set.
+func (s *Service) ApplyDefaults() error {
+	telemetryAddr, telemetryPort, err := s.MetricsEndpoint()
+	if err != nil {
+		return err
+	}
+	tm := &AnyConfig{
+		Object: map[string]interface{}{
+			"metrics": map[string]interface{}{
+				"address": fmt.Sprintf("%s:%d", telemetryAddr, telemetryPort),
+			},
+		},
 	}
 
-	return int32(i64), nil
+	if s.Telemetry == nil {
+		s.Telemetry = tm
+		return nil
+	}
+	// NOTE: Merge without overwrite. If a telemetry endpoint is specified, the defaulting
+	// respects the configuration and returns an equal value.
+	if err := mergo.Merge(s.Telemetry, tm); err != nil {
+		return fmt.Errorf("telemetry config merge failed: %w", err)
+	}
+	return nil
 }
 
 // MetricsConfig comes from the collector.

apis/v1beta1/config_test.go

@@ -220,11 +220,13 @@ func TestConfigToMetricsPort(t *testing.T) {
 
 	for _, tt := range []struct {
 		desc         string
+		expectedAddr string
 		expectedPort int32
 		config       Service
 	}{
 		{
 			"custom port",
+			"0.0.0.0",
 			9090,
 			Service{
 				Telemetry: &AnyConfig{
@@ -238,6 +240,7 @@ func TestConfigToMetricsPort(t *testing.T) {
 		},
 		{
 			"bad address",
+			"0.0.0.0",
 			8888,
 			Service{
 				Telemetry: &AnyConfig{
@@ -251,6 +254,7 @@ func TestConfigToMetricsPort(t *testing.T) {
 		},
 		{
 			"missing address",
+			"0.0.0.0",
 			8888,
 			Service{
 				Telemetry: &AnyConfig{
@@ -264,21 +268,38 @@ func TestConfigToMetricsPort(t *testing.T) {
 		},
 		{
 			"missing metrics",
+			"0.0.0.0",
 			8888,
 			Service{
 				Telemetry: &AnyConfig{},
 			},
 		},
 		{
 			"missing telemetry",
+			"0.0.0.0",
 			8888,
 			Service{},
 		},
+		{
+			"configured telemetry",
+			"1.2.3.4",
+			4567,
+			Service{
+				Telemetry: &AnyConfig{
+					Object: map[string]interface{}{
+						"metrics": map[string]interface{}{
+							"address": "1.2.3.4:4567",
+						},
+					},
+				},
+			},
+		},
 	} {
 		t.Run(tt.desc, func(t *testing.T) {
 			// these are acceptable failures, we return to the collector's default metric port
-			port, err := tt.config.MetricsPort()
+			addr, port, err := tt.config.MetricsEndpoint()
 			assert.NoError(t, err)
+			assert.Equal(t, tt.expectedAddr, addr)
 			assert.Equal(t, tt.expectedPort, port)
 		})
 	}

internal/manifests/collector/container.go

@@ -223,7 +223,7 @@ func getConfigContainerPorts(logger logr.Logger, conf v1beta1.Config) (map[strin
 		}
 	}
 
-	metricsPort, err := conf.Service.MetricsPort()
+	_, metricsPort, err := conf.Service.MetricsEndpoint()
 	if err != nil {
 		logger.Info("couldn't determine metrics port from configuration, using 8888 default value", "error", err)
 		metricsPort = 8888

internal/manifests/collector/service.go

@@ -83,7 +83,7 @@ func MonitoringService(params manifests.Params) (*corev1.Service, error) {
 		return nil, err
 	}
 
-	metricsPort, err := params.OtelCol.Spec.Config.Service.MetricsPort()
+	_, metricsPort, err := params.OtelCol.Spec.Config.Service.MetricsEndpoint()
 	if err != nil {
 		return nil, err
 	}

pkg/collector/upgrade/upgrade_test.go

@@ -141,7 +141,7 @@ func TestEnvVarUpdates(t *testing.T) {
 	require.Equal(t, collectorInstance.Status.Version, persisted.Status.Version)
 
 	currentV := version.Get()
-	currentV.OpenTelemetryCollector = "0.110.0"
+	currentV.OpenTelemetryCollector = "0.111.0"
 	up := &upgrade.VersionUpgrade{
 		Log:      logger,
 		Version:  currentV,

pkg/collector/upgrade/v0_111_0.go

@@ -0,0 +1,23 @@
+// 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 upgrade
+
+import (
+	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
+)
+
+func upgrade0_111_0(_ VersionUpgrade, otelcol *v1beta1.OpenTelemetryCollector) (*v1beta1.OpenTelemetryCollector, error) { //nolint:unparam
+	return otelcol, otelcol.Spec.Config.Service.ApplyDefaults()
+}

pkg/collector/upgrade/v0_111_0_test.go

@@ -0,0 +1,98 @@
+// 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 upgrade_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/record"
+
+	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
+	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade"
+)
+
+func Test0_111_0Upgrade(t *testing.T) {
+
+	defaultCollector := v1beta1.OpenTelemetryCollector{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "otel-my-instance",
+			Namespace: "somewhere",
+		},
+		Status: v1beta1.OpenTelemetryCollectorStatus{
+			Version: "0.110.0",
+		},
+		Spec: v1beta1.OpenTelemetryCollectorSpec{
+			OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{},
+			Config:                    v1beta1.Config{},
+		},
+	}
+
+	defaultCollectorWithConfig := defaultCollector.DeepCopy()
+
+	defaultCollectorWithConfig.Spec.Config.Service.Telemetry = &v1beta1.AnyConfig{
+		Object: map[string]interface{}{
+			"metrics": map[string]interface{}{
+				"address": "1.2.3.4:8888",
+			},
+		},
+	}
+
+	tt := []struct {
+		name     string
+		input    v1beta1.OpenTelemetryCollector
+		expected v1beta1.OpenTelemetryCollector
+	}{
+		{
+			name:     "telemetry settings exist",
+			input:    *defaultCollectorWithConfig,
+			expected: *defaultCollectorWithConfig,
+		},
+		{
+			name:  "telemetry settings do not exist",
+			input: *defaultCollector.DeepCopy(),
+			expected: func() v1beta1.OpenTelemetryCollector {
+				col := defaultCollector.DeepCopy()
+				col.Spec.Config.Service.Telemetry = &v1beta1.AnyConfig{
+					Object: map[string]interface{}{
+						"metrics": map[string]interface{}{
+							"address": "0.0.0.0:8888",
+						},
+					},
+				}
+				return *col
+			}(),
+		},
+	}
+
+	versionUpgrade := &upgrade.VersionUpgrade{
+		Log:      logger,
+		Version:  makeVersion("0.111.0"),
+		Client:   k8sClient,
+		Recorder: record.NewFakeRecorder(upgrade.RecordBufferSize),
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			col, err := versionUpgrade.ManagedInstance(context.Background(), tc.input)
+			if err != nil {
+				t.Errorf("expect err: nil but got: %v", err)
+			}
+			assert.Equal(t, tc.expected.Spec.Config.Service.Telemetry, col.Spec.Config.Service.Telemetry)
+		})
+	}
+}

pkg/collector/upgrade/versions.go

@@ -106,6 +106,10 @@ var (
 			Version:        *semver.MustParse("0.110.0"),
 			upgradeV1beta1: upgrade0_110_0,
 		},
+		{
+			Version:        *semver.MustParse("0.111.0"),
+			upgradeV1beta1: upgrade0_111_0,
+		},
 	}
 
 	// Latest represents the latest version that we need to upgrade. This is not necessarily the latest known version.

tests/e2e-multi-instrumentation/instrumentation-multi-multicontainer-go/02-assert.yaml

@@ -92,6 +92,9 @@ spec:
         exporters:
           debug: null
         service:
+          telemetry:
+            metrics:
+              address: 0.0.0.0:8888
           pipelines:
             traces:
               exporters:

tests/e2e-multi-instrumentation/instrumentation-multi-multicontainer/01-assert.yaml

@@ -331,6 +331,9 @@ spec:
         exporters:
           debug: null
         service:
+          telemetry:
+            metrics:
+              address: 0.0.0.0:8888
           pipelines:
             traces:
               exporters: