Update otelcontribcol to latest #1391

tiffanny29631 · 2024-08-13T23:20:34Z

Updating otelcontribcol to latest to include fix of open-telemetry/opentelemetry-collector-contrib#35006 and other essential security updates in components like Prometheus exporter.

The upgrade includes a breaking change of defaulting to local host instead of 0.0.0.0, the feature is now stable and feature gate has been removed, Config Sync should explicitly set endpoint to host 0.0.0.0 to expose port for external pod access. This applies to the metric receiver opencensus, and the exporter prometheus.

google-oss-prow · 2024-08-13T23:20:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from tiffanny29631. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karlkfi · 2024-08-13T23:34:08Z

Why id this safe to re-apply now?

tiffanny29631 · 2024-08-13T23:37:12Z

Why id this safe to re-apply now?

Are you referring to the upstream fix or our release schedule?

karlkfi · 2024-08-14T18:26:28Z

I mean, if we reverted it before, why is it now safe to reapply? What changed?

tiffanny29631 · 2024-08-14T20:09:44Z

I mean, if we reverted it before, why is it now safe to reapply? What changed?

To ensure correct behavior, we've included a no_op_label in the metricstransform processor's label_set filter (explained in the description). This is a temporary solution until the upstream fix is available, but we need to update to the latest otelcontribcol regardless.

sdowell · 2024-08-14T20:13:12Z

I mean, if we reverted it before, why is it now safe to reapply? What changed?

To ensure correct behavior, we've included a no_op_label in the metricstransform processor's label_set filter (explained in the description). This is a temporary solution until the upstream fix is available, but we need to update to the latest otelcontribcol regardless.

This isn't really a revert then, since new changes were added to the original commit. Suggest just creating a new commit instead of reapplying.

we need to update to the latest otelcontribcol regardless.

why?

tiffanny29631 · 2024-08-14T20:17:07Z

Re @sdowell While not currently anticipated, there's a possibility that future vulnerability fixes need dependency upgrades that are incompatible with current version we use (0.103). In such cases, the version upgrade process will be similar. Otelcontribcol versions beyond 0.104.0 will introduce breaking changes.

I could start a new PR.

tiffanny29631 · 2024-08-23T21:49:26Z

Recreated the commit and message.

This change updates otelcoontribcol to latest and modifies to fit breaking changes from 0.104.0 and 0.106.0. Breaking change in 0.104.0 https://github.com/open-telemetry/opentelemetry-collector-contrib/releases/tag/v0.104.0 Breaking change in 0.106.0 open-telemetry/opentelemetry-collector-contrib#34430 - Localhost is now the default setting, while otel-agent and otel-collector require 0.0.0.0, so the feature gate has been removed. - The format of the environment variable was updated to meet the new syntax requirements. The otel-agent ConfigMap was split between the reconciler and controllers, ensuring that sync-related labels are only applied to reconcilers. - A `no_op_label` has been added to ensure that the aggregation in the metricstransform processor filters on all metric labels. This is a temporary workaround until a permanent fix is implemented upstream.

Also specifying host 0.0.0.0 instead of the new default localhost.

For all receivers and health_check. Adding networkpolicy for ingress on essential ports.

- port 13133: health_check for readiness check. Apply to otel-agent in reconciler-manager and reconcilers; Otel-collector. - port 55678: opencensus receiver for otel-collector to receive metric from other pods Fix test ConfigMap.

Only keep networkpolicy for otel-collector

e2e/testdata/otel-collector/otel-cm-full-gcm.yaml

karlkfi · 2024-12-19T21:45:32Z

pkg/metrics/otel.go

-            label_set: []
+            # Using a no_op_label to get around issue in the upstream
+            # https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/34430
+            label_set: [no_op_label]


Does setting label_set to nil or removing it work?

Looks like it. Maybe clarify the comment?

karlkfi · 2024-12-19T21:47:59Z

manifests/otel-agent-cm.yaml

    exporters:
      opencensus:
        endpoint: otel-collector.config-management-monitoring:55678
        tls:
          insecure: true
    processors:
-      # Attributes processor adds custom configsync metric labels to applicable


Why did these attributes get removed from the agent config?

It seems to be replaced by manifests/otel-agent-reconciler-cm.yaml

karlkfi · 2024-12-19T21:49:17Z

manifests/templates/reconciler-manager.yaml

@@ -81,6 +81,7 @@ spec:
        ports:
        - containerPort: 55678 # Default OpenCensus receiver port.
        - containerPort: 8888  # Metrics.
+        - containerPort: 13133 # Health check


Is this the otel-agent readiness probe port?

tiffanny29631 · 2024-12-19T22:07:46Z

/blocked open-telemetry/opentelemetry-collector-contrib#34844

Generated files (go.mod etc.) were removed, which would break AutoVC integration. No affect to the continuous build of the internal mirror so far.

A CI can be added to restore the generated files to a new branch, along with base image update for CVE scan purpose.

janetkuo · 2024-12-19T23:04:26Z

manifests/templates/reconciler-manager-configmap.yaml

           configMap:
-             name: otel-agent


Is this ConfigMap still used anywhere?

janetkuo · 2024-12-19T23:04:45Z

manifests/otel-agent-cm.yaml

    exporters:
      opencensus:
        endpoint: otel-collector.config-management-monitoring:55678
        tls:
          insecure: true
    processors:
-      # Attributes processor adds custom configsync metric labels to applicable


It seems to be replaced by manifests/otel-agent-reconciler-cm.yaml

janetkuo · 2024-12-19T23:26:40Z

@code-review-assist review

code-review-assist

This pull request updates the otelcontribcol version and introduces several important changes to address a breaking change in the default endpoint behavior. The upgrade to v0.115.0-gke.0 defaults the opencensus receiver and prometheus exporter endpoints to localhost, requiring explicit configuration for external access. The PR description effectively communicates this change and its implications. The subsequent modifications to set the opencensus endpoint to 0.0.0.0:55678, the prometheus endpoint to 0.0.0.0:8675, and the health_check endpoint to 0.0.0.0:13133 are necessary and correctly implemented.

The introduction of the otel-agent-reconciler ConfigMap is a valuable addition. The accompanying comments explaining the rationale for the changes, including the removal of the attributes processor in the otel-agent ConfigMap and the use of no_op_label in the metric pipelines, enhance code clarity and maintainability. The comprehensive updates to the test files ensure consistency and validate the implemented changes.

To further improve this PR, I recommend the following:

README Update: Document the new otel-agent-reconciler ConfigMap, its purpose, and the changes to the default endpoints. This will aid users in understanding the modifications and configuring their deployments accordingly.
Version Bump Justification: While the PR description mentions the breaking change, it's beneficial to explicitly state the reason for the version bump to v0.115.0-gke.0 in the commit message or PR description. This helps track changes and provides valuable context for future maintainers.
NetworkPolicy Documentation: Add a comment to the NetworkPolicy in manifests/templates/otel-collector.yaml explaining its purpose and the ports it opens. This enhances readability and maintainability.
ConfigMap Clarification: Add a comment to test/kustomization/expected.yaml clarifying the relationship between the updated otel-agent-config.yaml and the otel-agent-reconciler-config.yaml.
Upstream Issue Link: Include a link to the upstream issue ([processor/metricstransform] Aggregation skipped when label_set is empty but not nil open-telemetry/opentelemetry-collector-contrib#34430) related to the use of no_op_label directly in the comments within pkg/metrics/otel.go. This facilitates quick access to the relevant context.

code-review-assist · 2024-12-19T23:27:33Z

Makefile

@@ -89,7 +89,7 @@ COSIGN := $(BIN_DIR)/cosign
 GIT_SYNC_VERSION := v4.3.0-gke.4__linux_amd64
 GIT_SYNC_IMAGE_NAME := gcr.io/config-management-release/git-sync:$(GIT_SYNC_VERSION)

-OTELCONTRIBCOL_VERSION := v0.103.0-gke.6
+OTELCONTRIBCOL_VERSION := v0.115.0-gke.0


Document the reason for updating to v0.115.0-gke.0 (e.g., bug fixes, security updates, feature additions). This helps track changes and understand the context of the update.

code-review-assist · 2024-12-19T23:27:33Z

manifests/otel-agent-cm.yaml

      batch:
      # Populate resource attributes from OTEL_RESOURCE_ATTRIBUTES env var and
      # the GCE metadata service, if available.
      resourcedetection:


Since the attributes processor is removed, delete these comments to maintain clarity and prevent confusion.

Suggested change

batch:

# Populate resource attributes from OTEL_RESOURCE_ATTRIBUTES env var and

# the GCE metadata service, if available.

resourcedetection:

# Populate resource attributes from OTEL_RESOURCE_ATTRIBUTES env var and

# the GCE metadata service, if available.

resourcedetection:

detectors: [env, gcp]

code-review-assist · 2024-12-19T23:27:33Z

manifests/templates/otel-collector.yaml

+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: allow-port-ingress
+  namespace: config-management-monitoring
+spec:
+  podSelector:
+    matchLabels:
+      app: opentelemetry
+      component: otel-collector
+  ingress:
+  - from:
+    - namespaceSelector: {}
+    ports:
+    - protocol: TCP
+      port: 55678


Adding a NetworkPolicy is a good security practice. Document its purpose (allowing ingress traffic on port 55678) for better maintainability.

code-review-assist · 2024-12-19T23:27:33Z

pkg/metrics/otel.go

+            # Using a no_op_label to get around issue in the upstream
+            # https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/34430


Include a direct link to the upstream issue: open-telemetry/opentelemetry-collector-contrib#34430 for easier reference.

code-review-assist · 2024-12-19T23:27:33Z

test/kustomization/expected.yaml

  otel-agent-config.yaml: |
    receivers:
      opencensus:
+        endpoint: 0.0.0.0:55678
+    exporters:
+      opencensus:
+        endpoint: otel-collector.config-management-monitoring:55678
+        tls:
+          insecure: true
+    processors:
+      batch:
+      # Populate resource attributes from OTEL_RESOURCE_ATTRIBUTES env var and
+      # the GCE metadata service, if available.
+      resourcedetection:
+        detectors: [env, gcp]
+    extensions:
+      health_check:
+        endpoint: 0.0.0.0:13133
+    service:
+      extensions: [health_check]
+      pipelines:
+        metrics:
+          receivers: [opencensus]
+          processors: [batch, resourcedetection]
+          exporters: [opencensus]
+      telemetry:
+        logs:
+          level: "INFO"


Clarify the purpose of this otel-agent-config.yaml and its relationship to otel-agent-reconciler-config.yaml. For example, explain that this config is used for the main otel-agent, while the reconciler config is used for the otel-agent-reconciler.

tiffanny29631 requested review from karlkfi and sdowell August 13, 2024 23:20

google-oss-prow bot requested a review from nan-yu August 13, 2024 23:20

google-oss-prow bot added the size/L label Aug 13, 2024

tiffanny29631 force-pushed the otel branch from 23c6316 to 41c1470 Compare August 16, 2024 19:33

tiffanny29631 changed the title ~~Reapply "Bump otelcontribcol version to v0.106.0-gke.2 (#1369)" (#1376)~~ Update otelcontribcol to 0.106.0-gke.2 Aug 16, 2024

tiffanny29631 force-pushed the otel branch from 41c1470 to 89a5757 Compare December 16, 2024 22:32

tiffanny29631 changed the title ~~Update otelcontribcol to 0.106.0-gke.2~~ Update otelcontribcol to latest Dec 16, 2024

tiffanny29631 marked this pull request as draft December 16, 2024 22:33

google-oss-prow bot added the do-not-merge/work-in-progress label Dec 16, 2024

tiffanny29631 added 7 commits December 17, 2024 01:28

Change otelcontribcol to v0.115.0-gke.0

317fa6f

Remove deprecated feature gate UseLocalHostAsDefaultHost

240e3cc

Also specifying host 0.0.0.0 instead of the new default localhost.

Explicitly bind to endpoint 0.0.0.0

d58128b

For all receivers and health_check. Adding networkpolicy for ingress on essential ports.

Apply change to resource-group-manifest

a12bfe6

Only expose necessary ingress

e21b59a

- port 13133: health_check for readiness check. Apply to otel-agent in reconciler-manager and reconcilers; Otel-collector. - port 55678: opencensus receiver for otel-collector to receive metric from other pods Fix test ConfigMap.

Cleanup

d98f4b4

Only keep networkpolicy for otel-collector

tiffanny29631 force-pushed the otel branch from b0e4cb5 to 05b8d99 Compare December 17, 2024 01:29

tiffanny29631 marked this pull request as ready for review December 17, 2024 01:46

google-oss-prow bot removed the do-not-merge/work-in-progress label Dec 17, 2024

Update expected.yaml

111f6bb

tiffanny29631 force-pushed the otel branch from 05b8d99 to 111f6bb Compare December 17, 2024 20:37

karlkfi reviewed Dec 19, 2024

View reviewed changes

e2e/testdata/otel-collector/otel-cm-full-gcm.yaml Show resolved Hide resolved

karlkfi reviewed Dec 19, 2024

View reviewed changes

janetkuo reviewed Dec 19, 2024

View reviewed changes

code-review-assist bot reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update otelcontribcol to latest #1391

Update otelcontribcol to latest #1391

tiffanny29631 commented Aug 13, 2024 •

edited

Loading

google-oss-prow bot commented Aug 13, 2024

karlkfi commented Aug 13, 2024

tiffanny29631 commented Aug 13, 2024

karlkfi commented Aug 14, 2024

tiffanny29631 commented Aug 14, 2024

sdowell commented Aug 14, 2024

tiffanny29631 commented Aug 14, 2024

tiffanny29631 commented Aug 23, 2024

karlkfi Dec 19, 2024

karlkfi Dec 19, 2024

karlkfi Dec 19, 2024

janetkuo Dec 19, 2024

karlkfi Dec 19, 2024

tiffanny29631 commented Dec 19, 2024

janetkuo Dec 19, 2024

janetkuo Dec 19, 2024

janetkuo commented Dec 19, 2024

code-review-assist bot left a comment

code-review-assist bot Dec 19, 2024

code-review-assist bot Dec 19, 2024

code-review-assist bot Dec 19, 2024

code-review-assist bot Dec 19, 2024

code-review-assist bot Dec 19, 2024

		# Using a no_op_label to get around issue in the upstream
		# https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/34430

Update otelcontribcol to latest #1391

Are you sure you want to change the base?

Update otelcontribcol to latest #1391

Conversation

tiffanny29631 commented Aug 13, 2024 • edited Loading

google-oss-prow bot commented Aug 13, 2024

karlkfi commented Aug 13, 2024

tiffanny29631 commented Aug 13, 2024

karlkfi commented Aug 14, 2024

tiffanny29631 commented Aug 14, 2024

sdowell commented Aug 14, 2024

tiffanny29631 commented Aug 14, 2024

tiffanny29631 commented Aug 23, 2024

karlkfi Dec 19, 2024

Choose a reason for hiding this comment

karlkfi Dec 19, 2024

Choose a reason for hiding this comment

karlkfi Dec 19, 2024

Choose a reason for hiding this comment

janetkuo Dec 19, 2024

Choose a reason for hiding this comment

karlkfi Dec 19, 2024

Choose a reason for hiding this comment

tiffanny29631 commented Dec 19, 2024

janetkuo Dec 19, 2024

Choose a reason for hiding this comment

janetkuo Dec 19, 2024

Choose a reason for hiding this comment

janetkuo commented Dec 19, 2024

code-review-assist bot left a comment

Choose a reason for hiding this comment

code-review-assist bot Dec 19, 2024

Choose a reason for hiding this comment

code-review-assist bot Dec 19, 2024

Choose a reason for hiding this comment

code-review-assist bot Dec 19, 2024

Choose a reason for hiding this comment

code-review-assist bot Dec 19, 2024

Choose a reason for hiding this comment

code-review-assist bot Dec 19, 2024

Choose a reason for hiding this comment

tiffanny29631 commented Aug 13, 2024 •

edited

Loading