Fix webhook error when metrics service address uses env var expansion

.chloggen/fix-metrics-service-address-env-var.yaml

@@ -0,0 +1,18 @@
+# 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: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix the admission webhook to when metrics service address host uses env var expansion
+
+# One or more tracking issues related to the change
+issues: [3513]
+
+# (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: |
+  This should allow the metrics service address to have the host portion expanded from an environment variable,
+  like `$(env:POD_IP)` instead of using `0.0.0.0`, which is the [recommended by the Collector](https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/security-best-practices.md#safeguards-against-denial-of-service-attacks).

apis/v1beta1/collector_webhook_test.go

@@ -555,7 +555,7 @@ 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, test.expected.Spec.Config.Service.ApplyDefaults(logr.Discard()), "could not apply defaults")
 			}
 			assert.NoError(t, err)
 			assert.Equal(t, test.expected, test.otelcol)
@@ -588,7 +588,17 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 	five := int32(5)
 	maxInt := int32(math.MaxInt32)
 
-	cfg := v1beta1.Config{}
+	cfg := v1beta1.Config{
+		Service: v1beta1.Service{
+			Telemetry: &v1beta1.AnyConfig{
+				Object: map[string]interface{}{
+					"metrics": map[string]interface{}{
+						"address": "${env:POD_ID}:8888",
+					},
+				},
+			},
+		},
+	}
 	err := yaml.Unmarshal([]byte(cfgYaml), &cfg)
 	require.NoError(t, err)
 

apis/v1beta1/config.go

@@ -18,7 +18,6 @@
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"net"
 	"reflect"
 	"sort"
 	"strconv"
@@ -269,7 +268,7 @@
 
 // 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 {
+	if err := c.Service.ApplyDefaults(logger); err != nil {
 		return err
 	}
 	enabledComponents := c.GetEnabledComponents()
@@ -427,37 +426,46 @@
 	Pipelines map[string]*Pipeline `json:"pipelines" yaml:"pipelines"`
 }
 
-// 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 defaultAddr, 8888, nil
-	}
-	host, port, netErr := net.SplitHostPort(s.GetTelemetry().Metrics.Address)
-	if netErr != nil && strings.Contains(netErr.Error(), "missing port in address") {
-		return defaultAddr, 8888, nil
-	} else if netErr != nil {
-		return "", 0, netErr
-	}
-	i64, err := strconv.ParseInt(port, 10, 32)
-	if err != nil {
-		return "", 0, err
-	}
+type serviceParseError string
 
-	if host == "" {
-		host = defaultAddr
-	}
-
-	return host, int32(i64), nil
+func (s serviceParseError) Error() string {
+	return string(s)
 }
 
-// ApplyDefaults inserts configuration defaults if it has not been set.
-func (s *Service) ApplyDefaults() error {
-	telemetryAddr, telemetryPort, err := s.MetricsEndpoint()
+const (
+	defaultServicePort int32 = 8888
+	defaultServiceHost       = "0.0.0.0"
+)
+
+// MetricsEndpoint attempts gets the host and port number from the host address without doing any validation regarding the
+// address itself.
+// It works even before env var expansion happens, when a simple `net.SplitHostPort` would fail because of  the extra colon
+// from the env var, i.e. the address looks like "${env:POD_IP}:4317" or "${env:POD_IP}".
+// It does not work in cases which the port itself is a variable, i.e. "${env:POD_IP}:${env:PORT}".
+// It does not work for IPv6 hostnames/addresses.
+func (s *Service) MetricsEndpoint(logger logr.Logger) (string, int32) {
+	telemetry := s.GetTelemetry()
+	if telemetry == nil {
+		return defaultServiceHost, defaultServicePort
+	}
+	splitAddress := strings.Split(telemetry.Metrics.Address, ":")
+	if len(splitAddress) == 1 {
+		return defaultServiceHost, defaultServicePort
+	}
+	port, err := strconv.ParseInt(splitAddress[len(splitAddress)-1], 10, 32)
 	if err != nil {
-		return err
+		errMsg := fmt.Sprintf("couldn't determine metrics port from configuration, using default: %s:%d",
+			defaultServiceHost, defaultServicePort)
+		logger.Info(errMsg, "error", err)
+		return defaultServiceHost, defaultServicePort
 	}
+	host := strings.Join(splitAddress[0:len(splitAddress)-1], ":")
+	return host, int32(port)
+}
+
+// ApplyDefaults inserts configuration defaults if it has not been set.
+func (s *Service) ApplyDefaults(logger logr.Logger) error {
+	telemetryAddr, telemetryPort := s.MetricsEndpoint(logger)
 	tm := &AnyConfig{
 		Object: map[string]interface{}{
 			"metrics": map[string]interface{}{

apis/v1beta1/config_test.go

@@ -216,8 +216,7 @@ func TestGetTelemetryFromYAMLIsNil(t *testing.T) {
 	assert.Nil(t, cfg.Service.GetTelemetry())
 }
 
-func TestConfigToMetricsPort(t *testing.T) {
-
+func TestConfigMetricsEndpoint(t *testing.T) {
 	for _, tt := range []struct {
 		desc         string
 		expectedAddr string
@@ -239,7 +238,7 @@ func TestConfigToMetricsPort(t *testing.T) {
 			},
 		},
 		{
-			"bad address",
+			"missing port",
 			"0.0.0.0",
 			8888,
 			Service{
@@ -296,9 +295,9 @@ func TestConfigToMetricsPort(t *testing.T) {
 		},
 	} {
 		t.Run(tt.desc, func(t *testing.T) {
+			logger := logr.Discard()
 			// these are acceptable failures, we return to the collector's default metric port
-			addr, port, err := tt.config.MetricsEndpoint()
-			assert.NoError(t, err)
+			addr, port := tt.config.MetricsEndpoint(logger)
 			assert.Equal(t, tt.expectedAddr, addr)
 			assert.Equal(t, tt.expectedPort, port)
 		})

internal/manifests/collector/container.go

@@ -229,11 +229,7 @@ func getConfigContainerPorts(logger logr.Logger, conf v1beta1.Config) (map[strin
 		}
 	}
 
-	_, 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
-	}
+	_, metricsPort := conf.Service.MetricsEndpoint(logger)
 	ports["metrics"] = corev1.ContainerPort{
 		Name:          "metrics",
 		ContainerPort: metricsPort,

internal/manifests/collector/service.go

@@ -73,7 +73,6 @@ func HeadlessService(params manifests.Params) (*corev1.Service, error) {
 }
 
 func MonitoringService(params manifests.Params) (*corev1.Service, error) {
-
 	name := naming.MonitoringService(params.OtelCol.Name)
 	labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{})
 	labels[monitoringLabel] = valueExists
@@ -84,10 +83,7 @@ func MonitoringService(params manifests.Params) (*corev1.Service, error) {
 		return nil, err
 	}
 
-	_, metricsPort, err := params.OtelCol.Spec.Config.Service.MetricsEndpoint()
-	if err != nil {
-		return nil, err
-	}
+	_, metricsPort := params.OtelCol.Spec.Config.Service.MetricsEndpoint(params.Log)
 
 	return &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{

pkg/collector/upgrade/v0_111_0.go

@@ -18,6 +18,6 @@ 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()
+func upgrade0_111_0(u VersionUpgrade, otelcol *v1beta1.OpenTelemetryCollector) (*v1beta1.OpenTelemetryCollector, error) { //nolint:unparam
+	return otelcol, otelcol.Spec.Config.Service.ApplyDefaults(u.Log)
 }