Merge branch 'main' into chore/improving-set-targets

Author: nicolastakashi

.chloggen/revert-3379-otel-configmap.yaml .chloggen/fix-metrics-service-address-env-var.yaml

@@ -2,18 +2,17 @@
 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: auto-instrumentation
+component: operator
 
 # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
-note: Reverts PR 3379 which inadvertently broke users setting JAVA_TOOL_OPTIONS
+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: [3463]
+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: |
-  Reverts a previous PR which was causing JAVA_TOOL_OPTIONS to not be overriden when
-  set by users. This was resulting in application crashloopbackoffs for users relying
-  on java autoinstrumentation.
+  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).

.chloggen/fix-prometheus-rule-file.yaml

@@ -37,6 +37,7 @@ jobs:
             ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-java
           tags: |
             type=match,pattern=v(.*),group=1,value=v${{ env.VERSION }}
+            type=semver,pattern={{major}},value=v${{ env.VERSION }}
 
       - name: Set up QEMU
         uses: docker/setup-qemu-action@v3

.chloggen/scrape_config_probe.yaml

@@ -491,11 +491,16 @@ CHLOGGEN ?= $(LOCALBIN)/chloggen
 GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint
 CHAINSAW ?= $(LOCALBIN)/chainsaw
 
-KUSTOMIZE_VERSION ?= v5.0.3
-CONTROLLER_TOOLS_VERSION ?= v0.16.1
+# renovate: datasource=go depName=sigs.k8s.io/kustomize/kustomize/v5
+KUSTOMIZE_VERSION ?= v5.5.0
+# renovate: datasource=go depName=sigs.k8s.io/controller-tools/cmd/controller-gen
+CONTROLLER_TOOLS_VERSION ?= v0.16.5
+# renovate: datasource=go depName=github.com/golangci/golangci-lint/cmd/golangci-lint
 GOLANGCI_LINT_VERSION ?= v1.57.2
-KIND_VERSION ?= v0.20.0
-CHAINSAW_VERSION ?= v0.2.8
+# renovate: datasource=go depName=sigs.k8s.io/kind
+KIND_VERSION ?= v0.25.0
+# renovate: datasource=go depName=github.com/kyverno/chainsaw
+CHAINSAW_VERSION ?= v0.2.12
 
 .PHONY: install-tools
 install-tools: kustomize golangci-lint kind controller-gen envtest crdoc kind operator-sdk chainsaw

.chloggen/service-extension.yaml

@@ -72,12 +72,16 @@ This will create an OpenTelemetry Collector instance named `simplest`, exposing
 
 The `config` node holds the `YAML` that should be passed down as-is to the underlying OpenTelemetry Collector instances. Refer to the [OpenTelemetry Collector](https://github.com/open-telemetry/opentelemetry-collector) documentation for a reference of the possible entries.
 
-> 🚨 **NOTE:** At this point, the Operator does _not_ validate the contents of the configuration file: if the configuration is invalid, the instance will still be created but the underlying OpenTelemetry Collector might crash.
+> 🚨 **NOTE:** At this point, the Operator does _not_ validate the whole contents of the configuration file: if the configuration is invalid, the instance might still be created but the underlying OpenTelemetry Collector might crash.
 
 > 🚨 **Note:** For private GKE clusters, you will need to either add a firewall rule that allows master nodes access to port `9443/tcp` on worker nodes, or change the existing rule that allows access to port `80/tcp`, `443/tcp` and `10254/tcp` to also allow access to port `9443/tcp`. More information can be found in the [Official GCP Documentation](https://cloud.google.com/load-balancing/docs/tcp/setting-up-tcp#config-hc-firewall). See the [GKE documentation](https://cloud.google.com/kubernetes-engine/docs/how-to/private-clusters#add_firewall_rules) on adding rules and the [Kubernetes issue](https://github.com/kubernetes/kubernetes/issues/79739) for more detail.
 
-The Operator does examine the configuration file to discover configured receivers and their ports. If it finds receivers with ports, it creates a pair of kubernetes services, one headless, exposing those ports within the cluster. The headless service contains a `service.beta.openshift.io/serving-cert-secret-name` annotation that will cause OpenShift to create a secret containing a certificate and key. This secret can be mounted as a volume and the certificate and key used in those receivers' TLS configurations.
+The Operator does examine the configuration file for a few purposes:
 
+- To discover configured receivers and their ports. If it finds receivers with ports, it creates a pair of kubernetes services, one headless, exposing those ports within the cluster. If the port is using environment variable expansion or cannot be parsed, an error will be returned. The headless service contains a `service.beta.openshift.io/serving-cert-secret-name` annotation that will cause OpenShift to create a secret containing a certificate and key. This secret can be mounted as a volume and the certificate and key used in those receivers' TLS configurations.
+
+- To check if Collector observability is enabled (controlled by `spec.observability.metrics.enableMetrics`). In this case, a Service and ServiceMonitor/PodMonitor are created for the Collector instance. As a consequence, if the metrics service address contains an invalid port or uses environment variable expansion for the port, an error will be returned. A workaround for the environment variable case is to set `enableMetrics` to `false` and manually create the previously mentioned objects with the correct port if you need them.
+ 
 ### Upgrades
 
 As noted above, the OpenTelemetry Collector format is continuing to evolve. However, a best-effort attempt is made to upgrade all managed `OpenTelemetryCollector` resources.
@@ -534,9 +538,10 @@ apiVersion: opentelemetry.io/v1alpha1
 kind: Instrumentation
 metadata:
   name: my-instrumentation
-  apache:
+spec:
+  apacheHttpd:
     image: your-customized-auto-instrumentation-image:apache-httpd
-    version: 2.2
+    version: "2.2"
     configPath: /your-custom-config-path
     attrs:
       - name: ApacheModuleOtelMaxQueueSize
@@ -556,6 +561,7 @@ apiVersion: opentelemetry.io/v1alpha1
 kind: Instrumentation
 metadata:
   name: my-instrumentation
+spec:
   nginx:
     image: your-customized-auto-instrumentation-image:nginx # if custom instrumentation image is needed
     configFile: /my/custom-dir/custom-nginx.conf
@@ -725,14 +731,16 @@ EOF
 
 ### Configure resource attributes with annotations
 
-This example shows a pod configuration with OpenTelemetry annotations using the `resource.opentelemetry.io/` prefix. These annotations can be used to add resource attributes to data produced by OpenTelemetry instrumentation.
+This example shows a pod configuration with OpenTelemetry annotations using the `resource.opentelemetry.io/` prefix. 
+These annotations can be used to add resource attributes to data produced by OpenTelemetry instrumentation.
 
 ```yaml
 apiVersion: v1
 kind: Pod
 metadata:
   name: example-pod
   annotations:
+    # this is just an example, you can create any resource attributes you need
     resource.opentelemetry.io/service.name: "my-service"
     resource.opentelemetry.io/service.version: "1.0.0"
     resource.opentelemetry.io/environment: "production"
@@ -750,7 +758,6 @@ The following labels are supported:
 - `app.kubernetes.io/name` becomes `service.name`
 - `app.kubernetes.io/version` becomes `service.version`
 - `app.kubernetes.io/part-of` becomes `service.namespace`
-- `app.kubernetes.io/instance` becomes `service.instance.id`
 
 ```yaml
 apiVersion: v1
@@ -761,7 +768,6 @@ metadata:
     app.kubernetes.io/name: "my-service"
     app.kubernetes.io/version: "1.0.0"
     app.kubernetes.io/part-of: "shop"
-    app.kubernetes.io/instance: "my-service-123"
 spec:
   containers:
   - name: main-container
@@ -794,6 +800,40 @@ The priority for setting resource attributes is as follows (first found wins):
 This priority is applied for each resource attribute separately, so it is possible to set some attributes via
 annotations and others via labels.
 
+### How resource attributes are calculated from the pod's metadata
+
+The following resource attributes are calculated from the pod's metadata.
+
+#### How `service.name` is calculated
+
+Choose the first value found: 
+
+- `pod.annotation[resource.opentelemetry.io/service.name]`
+- `if (config[useLabelsForResourceAttributes]) pod.label[app.kubernetes.io/name]`
+- `k8s.depleyment.name`
+- `k8s.replicaset.name`
+- `k8s.statefulset.name`
+- `k8s.daemonset.name`
+- `k8s.cronjob.name`
+- `k8s.job.name`
+- `k8s.pod.name`
+- `k8s.container.name`
+
+#### How `service.version` is calculated
+
+Choose the first value found:
+
+- `pod.annotation[resource.opentelemetry.io/service.version]`
+- `if (cfg[useLabelsForResourceAttributes]) pod.label[app.kubernetes.io/version]`
+- `if (contains(container docker image tag, '/') == false) container docker image tag`
+
+#### How `service.instance.id` is calculated
+
+Choose the first value found:
+                                   
+- `pod.annotation[resource.opentelemetry.io/service.instance.id]`
+- `concat([k8s.namespace.name, k8s.pod.name, k8s.container.name], '.')`
+
 ## Contributing and Developing
 
 Please see [CONTRIBUTING.md](CONTRIBUTING.md).
@@ -802,7 +842,6 @@ In addition to the [core responsibilities](https://github.com/open-telemetry/com
 
 Approvers ([@open-telemetry/operator-approvers](https://github.com/orgs/open-telemetry/teams/operator-approvers)):
 
-- [Benedikt Bongartz](https://github.com/frzifus), Red Hat
 - [Tyler Helmuth](https://github.com/TylerHelmuth), Honeycomb
 - [Yuri Oliveira Sa](https://github.com/yuriolisa), Red Hat
 - [Israel Blancas](https://github.com/iblancasa), Red Hat
@@ -818,6 +857,7 @@ Emeritus Approvers:
 
 Maintainers ([@open-telemetry/operator-maintainers](https://github.com/orgs/open-telemetry/teams/operator-maintainers)):
 
+- [Benedikt Bongartz](https://github.com/frzifus), Red Hat
 - [Jacob Aronoff](https://github.com/jaronoff97), Lightstep
 - [Mikołaj Świątek](https://github.com/swiatekm), Elastic
 - [Pavol Loffay](https://github.com/pavolloffay), Red Hat

.github/workflows/publish-autoinstrumentation-java.yaml

@@ -44,10 +44,10 @@ The operator should be released within a week after the [OpenTelemetry collector
 
 | Version  | Release manager |
 |----------|-----------------|
-| v0.115.0 | @TylerHelmuth   |
 | v0.116.0 | @jaronoff97     |
 | v0.117.0 | @iblancasa      |
 | v0.118.0 | @frzifus        |
 | v0.119.0 | @yuriolisa      |
 | v0.120.0 | @pavolloffay    |
 | v0.121.0 | @swiatekm       |
+| v0.122.0 | @TylerHelmuth   |

CHANGELOG.md

@@ -152,7 +152,6 @@ type Defaults struct {
 	//   - `app.kubernetes.io/name` becomes `service.name`
 	//   - `app.kubernetes.io/version` becomes `service.version`
 	//   - `app.kubernetes.io/part-of` becomes `service.namespace`
-	//   - `app.kubernetes.io/instance` becomes `service.instance.id`
 	UseLabelsForResourceAttributes bool `json:"useLabelsForResourceAttributes,omitempty"`
 }
 

Makefile

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

README.md

@@ -18,8 +18,8 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"net"
 	"reflect"
+	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -269,7 +269,7 @@ func (c *Config) getEnvironmentVariablesForComponentKinds(logger logr.Logger, co
 
 // 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 +427,60 @@ type Service struct {
 	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)
+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", "${env:POD_IP}", or "${POD_IP}".
+// In cases which the port itself is a variable, i.e. "${env:POD_IP}:${env:PORT}", this returns an error. This happens
+// because the port is used to generate Service objects and mappings.
+func (s *Service) MetricsEndpoint(logger logr.Logger) (string, int32, error) {
+	telemetry := s.GetTelemetry()
+	if telemetry == nil || telemetry.Metrics.Address == "" {
+		return defaultServiceHost, defaultServicePort, nil
+	}
+
+	// The regex below matches on strings that end with a colon followed by the environment variable expansion syntax.
+	// So it should match on strings ending with: ":${env:POD_IP}" or ":${POD_IP}".
+	const portEnvVarRegex = `:\${[env:]?.*}$`
+	isPortEnvVar := regexp.MustCompile(portEnvVarRegex).MatchString(telemetry.Metrics.Address)
+	if isPortEnvVar {
+		errMsg := fmt.Sprintf("couldn't determine metrics port from configuration: %s",
+			telemetry.Metrics.Address)
+		logger.Info(errMsg)
+		return "", 0, fmt.Errorf(errMsg)
+	}
+
+	// The regex below matches on strings that end with a colon followed by 1 or more numbers (representing the port).
+	const explicitPortRegex = `:(\d+$)`
+	explicitPortMatches := regexp.MustCompile(explicitPortRegex).FindStringSubmatch(telemetry.Metrics.Address)
+	if len(explicitPortMatches) <= 1 {
+		return telemetry.Metrics.Address, defaultServicePort, nil
+	}
+
+	port, err := strconv.ParseInt(explicitPortMatches[1], 10, 32)
 	if err != nil {
+		errMsg := fmt.Sprintf("couldn't determine metrics port from configuration: %s",
+			telemetry.Metrics.Address)
+		logger.Info(errMsg, "error", err)
 		return "", 0, err
 	}
 
-	if host == "" {
-		host = defaultAddr
-	}
-
-	return host, int32(i64), nil
+	host, _, _ := strings.Cut(telemetry.Metrics.Address, explicitPortMatches[0])
+	return host, int32(port), nil
 }
 
 // ApplyDefaults inserts configuration defaults if it has not been set.
-func (s *Service) ApplyDefaults() error {
-	telemetryAddr, telemetryPort, err := s.MetricsEndpoint()
+func (s *Service) ApplyDefaults(logger logr.Logger) error {
+	telemetryAddr, telemetryPort, err := s.MetricsEndpoint(logger)
 	if err != nil {
 		return err
 	}
+
 	tm := &AnyConfig{
 		Object: map[string]interface{}{
 			"metrics": map[string]interface{}{