Use a naive approach to parse port before env var expansion

Author: douglascamata

.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

@@ -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

@@ -427,16 +427,18 @@ type Service struct {
 	Pipelines map[string]*Pipeline `json:"pipelines" yaml:"pipelines"`
 }
 
+const defaultServicePort int32 = 8888
+
 // 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
+		return defaultAddr, defaultServicePort, 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
+		return defaultAddr, defaultServicePort, nil
 	} else if netErr != nil {
 		return "", 0, netErr
 	}
@@ -452,6 +454,26 @@ func (s *Service) MetricsEndpoint() (string, int32, error) {
 	return host, int32(i64), nil
 }
 
+// NaivePort attempts gets the 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}".
+func (s *Service) NaivePort() (int32, error) {
+	telemetry := s.GetTelemetry()
+	if telemetry == nil {
+		return defaultServicePort, nil
+	}
+	splitAddress := strings.Split(telemetry.Metrics.Address, ":")
+	if len(splitAddress) == 1 {
+		return defaultServicePort, nil
+	}
+	port, err := strconv.Atoi(splitAddress[len(splitAddress)-1])
+	if err != nil {
+		return 0, err
+	}
+	return int32(port), nil
+}
+
 // ApplyDefaults inserts configuration defaults if it has not been set.
 func (s *Service) ApplyDefaults() error {
 	telemetryAddr, telemetryPort, err := s.MetricsEndpoint()

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