Skip to content

feat(otelcollector): render OTel Collector - #5106

Merged
tianfeng92 merged 1 commit into
tigera:masterfrom
tianfeng92:EV-6862-otel-collector
Aug 19, 2026
Merged

feat(otelcollector): render OTel Collector#5106
tianfeng92 merged 1 commit into
tigera:masterfrom
tianfeng92:EV-6862-otel-collector

Conversation

@tianfeng92

@tianfeng92 tianfeng92 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Render the OTel Collector as a StatefulSet in calico-system, configured via LogCollector.spec.openTelemetry.

The collector receives logs from fluent-bit via OTLP and optionally federates Prometheus metrics, forwarding both to user-configured OTLP endpoints. It is added to the LogCollector CR rather than AdditionalStores because it is operator-managed infrastructure (StatefulSet, ConfigMap, RBAC, certs) with its own lifecycle, not a pointer to an external system.

API

spec:
  openTelemetry:
    logs:
      types: [Flows, DNS, Audit]
    metrics:
      state: Enabled
    exporters:
      - name: datadog
        endpoint: https://otlp.datadoghq.com:4318
        protocol: http
        auth:
          headers:
            - name: DD-API-KEY
              valueFrom:
                secretKeyRef: { name: otlp-datadog-auth, key: api-key }
        tls:
          caConfigMapName: corp-ca
          clientCertSecretName: corp-client-mtls

TLS and auth are configured per exporter rather than once for all of them: this fans out to several external backends, which generally expect different client identities and different trust anchors. tls.clientCertSecretName turns on mutual TLS for that exporter alone; tls.caConfigMapName pins that exporter to the named CA alone — the system roots are not trusted as well, since keeping them would let any public authority satisfy the connection, which is what pinning is for. An exporter that names no CA verifies against the system roots. Verification is never disabled. Header credentials are read from Secrets into environment variables and referenced as ${env:...}, so they never appear in the rendered config.

Exporter CAs are deliberately kept out of the operator's trusted bundle — that bundle backs the receiver's client_ca_file, so folding them in would make them valid signers for inbound client certificates. The collector carries its own named bundle rather than the shared calico-system/tigera-ca-bundle, which the Installation controller owns.

Release Note

Add OpenTelemetry Collector component to export logs and metrics via OTLP.

Test plan

  • Unit tests for render, controller, and the fluent-bit pipeline output
  • Deployed to a cluster: all three log types reaching an OTLP backend, Prometheus federation exporting, collector's own ServiceMonitor scraping, clean startup and teardown
  • Verified the collector is not deployed when the license lacks the feature, and that disabling export removes every resource it owns
  • Full lifecycle on a cluster: enable → logs and metrics flowing → disable → all resources removed → re-enable → data resumes

Exercised on a cluster since: a pinned caConfigMapName renders ca_file with no system roots and is mounted and loaded, and a user-supplied otel-collector-tls reaches fluent-bit's trust bundle and is accepted with no verification errors.

Still unit-tested only: clientCertSecretName and auth.headers against a real TLS-terminating backend — no external OTLP endpoint demanding mutual TLS or a credential header was available to test against.

Follow-up: moving Dex and Guardian onto the shared ParseExternalDestination helper (r3717124352) is going into its own PR.

EV-6862

Comment thread internal/controller/otelcollector_controller.go
Comment thread pkg/render/otelcollector/component.go Outdated
Comment thread pkg/render/otelcollector/component.go Outdated
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch 7 times, most recently from 8124957 to 222f3f4 Compare July 27, 2026 23:24
@tianfeng92 tianfeng92 changed the title feat(otelcollector): render OTel Collector from LogCollector.spec.otelCollector feat(otelcollector): render OTel Collector Jul 28, 2026
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch 2 times, most recently from 6dd36c8 to a8c2d9e Compare July 28, 2026 17:55
Comment thread pkg/controller/otelcollector/controller.go Outdated
Comment thread pkg/controller/otelcollector/controller.go Outdated
Comment thread pkg/render/otelcollector/component.go
Comment thread pkg/render/otelcollector/component.go Outdated
Comment thread pkg/controller/otelcollector/controller.go Outdated
Comment thread pkg/render/otelcollector/collector-config.yaml.template Outdated
Comment thread api/v1/otelcollector_types.go Outdated
Comment thread api/v1/logcollector_types.go Outdated
Comment thread pkg/render/otelcollector/component.go Outdated
Comment thread pkg/render/otelcollector/component.go Outdated
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch from c612900 to fca9047 Compare August 6, 2026 22:12
@tianfeng92
tianfeng92 requested a review from pasanw August 6, 2026 23:02

@pasanw pasanw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments! I let Claude lead the re-review and reviewed everything it said was addressed along with what it flagged - it helped flag some things that weren't fully addressed but it also flagged some new issues (sorry about that!)

I'll let you and @rene-dekker work through them together and decide the best way to proceed.

Comment thread pkg/render/common/networkpolicy/networkpolicy.go
Comment thread pkg/render/otelcollector/component.go
Comment thread pkg/render/logcollector/pipeline.go Outdated
Comment thread pkg/render/monitor/monitor.go
Comment thread pkg/controller/otelcollector/controller.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
@caseydavenport caseydavenport self-assigned this Aug 10, 2026
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch from ba5aeb9 to e8a86d5 Compare August 11, 2026 17:00
@rene-dekker

Copy link
Copy Markdown
Member

Ran Claude Code over this branch as a review pass; it flagged one thing that looks like it would stop the collector from starting at all, so raising it ahead of the smaller notes.

Exporter component IDs may not be valid otelcol types

pkg/render/otelcollector/component.go:76-77:

exporterPrefixHTTP  = "otlp_http"
exporterPrefixGRPC  = "otlp_grpc"

These are used as the component-ID type in two places — the exporters block ({{.Prefix}}/{{.Name}}:, collector-config.yaml.template:50) and every pipeline's exporters: [...] list (ExporterNames, component.go:511) — producing:

exporters:
  otlp_grpc/backend:
    endpoint: ...
service:
  pipelines:
    logs:
      exporters: [otlp_grpc/backend]

In an otelcol config the part before the / has to match a registered component factory type. Upstream those are otlp for the gRPC exporter and otlphttp for the HTTP one — otlp_grpc and otlp_http aren't factory types in any upstream distro. otelcol resolves the type at config-load time and a miss is fatal:

error decoding 'exporters': unknown type: "otlp_grpc" for id "otlp_grpc/backend"

If that applies here, the StatefulSet crash-loops on every install with an exporter configured, the health check never passes, and the TigeraStatus sits on Progressing with nothing pointing at the generated config as the cause.

The render tests (component_test.go:342, :382-404) assert these same prefixes, so they pass either way — they check that the operator renders what the operator intends, not that otelcol accepts it.

What I couldn't verify: the container runs /usr/bin/otelcol from the combined calico image, and the current tigera/calico:latest doesn't contain that binary yet (only calico, calico-ipam, calicoctl, csi-node-driver-registrar), so I couldn't run otelcol components against it. Two questions:

  1. Does the companion build register custom factory types under these names? A factory's type comes from the factory itself, so otlp_grpc/otlp_http would need custom wrapper factories rather than a builder-manifest rename — if that's what's happening, worth a comment in the template saying so, because it reads as a bug.
  2. If not, these want to be otlphttp and otlp.

Either way it'd be good to have one test that feeds a rendered config to the actual collector binary — the current tests can't distinguish the two cases.

Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/logcollector_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
Comment thread api/v1/opentelemetry_types.go Outdated
"github.com/tigera/operator/pkg/render/common/networkpolicy"
rlogcollector "github.com/tigera/operator/pkg/render/logcollector"
"github.com/tigera/operator/pkg/render/monitor"
"github.com/tigera/operator/pkg/render/otelcollector"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need a watch in this controller for the new secret otel-collector-tls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the secrets watch loop in add():

// The collector's serving certificate goes into fluent-bit's trusted
// bundle, so creating or rotating it has to rebuild that bundle.
otelcollector.OpenTelemetryCollectorServerTLSSecretName,

and consumed in Reconcile, where the cert joins fluent-bit's trusted bundle when logs are exported:

if instance.Spec.OpenTelemetry.HasLogs() {
otelCertificate, err := certificateManager.GetCertificate(r.client, otelcollector.OpenTelemetryCollectorServerTLSSecretName, common.OperatorNamespace())
if err != nil {
r.status.SetDegraded(operatorv1.ResourceReadError, "Failed to get the OpenTelemetry Collector certificate", err, reqLogger)
return reconcile.Result{}, err
}
// Absent simply means the otel controller has not minted it yet; it will
// trigger another reconcile here when it does.
if otelCertificate != nil {
extraCerts = append(extraCerts, otelCertificate)
}

@rene-dekker

Copy link
Copy Markdown
Member

Another one from the same Claude Code review pass, much smaller than the last:

Logs.Types accepts duplicates, which render duplicate fluent-bit outputs

api/v1/opentelemetry_types.go:34-38 declares the field with an item-level enum but no uniqueness constraint:

// +optional
Types []OpenTelemetryLogType `json:"types,omitempty"`

The generated CRD (operator.tigera.io_logcollectors.yaml:1110-1123) is a plain type: array with an item enum and no x-kubernetes-list-type: set, so the API server accepts types: [Flows, Flows]. Nothing downstream dedupes it either — otelLogTypes() (logcollector_controller.go:952-957) passes the slice straight through, and pipeline.go:255 iterates it appending one output per entry:

outputs:
  - name: opentelemetry
    match: flows
    ...
  - name: opentelemetry
    match: flows
    ...

fluent-bit runs both instances, so every flow record is shipped to the collector — and on to the customer's OTLP backend — twice. For a usage-priced backend that is a duplicated bill as well as duplicated data. With NonClusterHost configured it's four outputs, since each type already renders a second output for the non-cluster tag.

Fix is a marker:

// +optional
// +listType=set
Types []OpenTelemetryLogType `json:"types,omitempty"`

Exporters already has +listType=map + +listMapKey=name for the same reason, so this looks like an oversight rather than a decision. Note Validate() guards against duplicate exporter names in Go as well, with a comment about the CRD being out of date — if that concern applies here too, the same belt-and-braces dedupe would be worth adding for log types.

@caseydavenport caseydavenport left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these comments are just me scolding Claude for being way too verbose in its code comments....

Comment thread api/v1/logcollector_types.go
Comment thread pkg/common/validation/enterprise/otelcollector/validation.go
Comment thread pkg/controller/logcollector/logcollector_controller.go Outdated
Comment thread pkg/controller/logcollector/logcollector_controller.go Outdated
Comment thread pkg/controller/logcollector/logcollector_controller.go Outdated
Comment thread pkg/controller/otelcollector/controller.go Outdated
Comment thread pkg/render/common/networkpolicy/networkpolicy.go Outdated
Comment thread pkg/render/monitor/monitor.go Outdated
Comment thread pkg/render/otelcollector/component.go Outdated
Comment thread pkg/render/otelcollector/component.go Outdated
@danudey danudey modified the milestones: v1.44.0, v1.45.0 Aug 17, 2026
Copilot AI lite review requested due to automatic review settings August 17, 2026 21:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR introduces an operator-managed OpenTelemetry Collector for exporting Calico Enterprise logs/metrics via OTLP, and integrates it with fluent-bit and the monitoring stack.

Changes:

  • Add a new otelcollector render component that emits the Collector StatefulSet, config, RBAC, TLS/auth material aggregation, and NetworkPolicy.
  • Add an OpenTelemetry Collector controller plus LogCollector/Monitor controller wiring to deploy/teardown the collector and its ServiceMonitor safely.
  • Extend the LogCollector API/CRDs with spec.openTelemetry and related validation and fluent-bit OTLP output rendering.

Reviewed changes

Copilot reviewed 24 out of 26 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
pkg/render/otelcollector/suite_test.go Adds Ginkgo suite for otelcollector render tests.
pkg/render/otelcollector/component_test.go Comprehensive render tests for Collector objects/config/TLS/auth/networkpolicy behavior.
pkg/render/otelcollector/component.go New Collector render component (StatefulSet/Service/RBAC/config/TLS/auth aggregation/NetworkPolicy).
pkg/render/otelcollector/collector-config.yaml.template Collector config template for receivers/exporters/processors/pipelines.
pkg/render/otelcollector.go Centralizes Collector identity constants to avoid import cycles (used by monitor render).
pkg/render/monitor/monitor_test.go Adds tests for Collector ServiceMonitor lifecycle and updates expected delete counts.
pkg/render/monitor/monitor.go Conditionally renders Collector ServiceMonitor and Prometheus egress rule.
pkg/render/logcollector/pipeline.go Adds fluent-bit OTLP outputs targeting the in-cluster Collector Service.
pkg/render/logcollector/logcollector.go Extends fluent-bit render config to include OpenTelemetry export toggles and log type selection.
pkg/render/logcollector/fluentbit_test.go Adds tests verifying OTLP outputs rendered per selected log type (+ non-cluster-host variants).
pkg/render/common/networkpolicy/networkpolicy.go Adds endpoint parsing & entity rule building for tighter egress policies (host/ip/service matching).
pkg/imports/crds/operator/operator.tigera.io_logcollectors.yaml CRD schema updates for new spec.openTelemetry fields.
pkg/imports/crds/operator/operator.tigera.io_installations.yaml CRD schema field reordering for calico host path fields (unrelated but included).
pkg/controller/otelcollector/otelcollector_suite_test.go Adds Ginkgo suite for otelcollector controller tests.
pkg/controller/otelcollector/otelcollector_controller_test.go Adds controller tests covering deploy/teardown, license gating, validation, and exporter material fetch.
pkg/controller/otelcollector/controller.go New controller that reconciles the Collector based on LogCollector spec + license + dependencies.
pkg/controller/monitor/monitor_controller_test.go Adds controller tests ensuring ServiceMonitor follows collector Service presence.
pkg/controller/monitor/monitor_controller.go Watches LogCollector + Collector Service and threads OpenTelemetryEnabled into monitor render config.
pkg/controller/logcollector/logcollector_controller_test.go Adds test ensuring fluent-bit trusts BYO Collector server cert when OTLP log export enabled.
pkg/controller/logcollector/logcollector_controller.go Extends fluent-bit trusted bundle + config to support OTLP output to Collector and trust its cert.
pkg/common/validation/enterprise/otelcollector/validation.go Adds validation hook for Collector StatefulSet container resource overrides.
pkg/common/common.go Introduces OpenTelemetryCollectorFeature constant for license gating.
internal/controller/otelcollector_controller.go Wires the new controller into the internal controller scaffolding.
internal/controller/controllers.go Registers the new OpenTelemetry controller with the manager.
api/v1/logcollector_types.go Adds spec.openTelemetry API types, validation, and helper methods (Deployable, env name mapping, etc.).
Files not reviewed (1)
  • api/v1/zz_generated.deepcopy.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/render/otelcollector/collector-config.yaml.template
Comment thread api/v1/logcollector_types.go
Comment thread api/v1/logcollector_types.go
Comment thread pkg/render/monitor/monitor.go
Comment thread pkg/render/otelcollector/component.go
Comment thread pkg/render/otelcollector/component.go
Comment thread pkg/render/otelcollector/component.go
Copilot AI review requested due to automatic review settings August 17, 2026 21:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)

pkg/render/monitor/monitor.go:1512

  • The OTEL collector ServiceMonitor is gated on OpenTelemetryEnabled && !LicenseExpired, but this Prometheus egress rule is gated only on OpenTelemetryEnabled. That breaks the stated invariant that the ServiceMonitor and reachability rule appear together (and can leave an allow rule behind during license-expired teardown). Gate this rule on the same condition as the ServiceMonitor (e.g., also require !cfg.LicenseExpired).
	if cfg.OpenTelemetryEnabled {
		// Pairs with serviceMonitorOpenTelemetryCollector; without it the scrape
		// is blocked by the namespace default-deny and just reports up=0.
		egressRules = append(egressRules, v3.Rule{
			Action:      v3.Allow,
			Protocol:    &networkpolicy.TCPProtocol,
			Destination: networkpolicy.CreateServiceSelectorEntityRule(render.OpenTelemetryCollectorNamespace, render.OpenTelemetryCollectorName),
		})
	}

pkg/render/otelcollector/component.go:304

  • If an exporter has auth headers but ExporterAuthSecrets does not include the referenced Secret, this code silently skips it and can result in rendering env-var references without creating otel-collector-exporter-auth at all (pod will fail due to missing Secret / missing keys). Instead of continue, treat missing referenced Secrets/keys as a rendering error (or, at minimum, ensure the aggregated Secret is still created whenever any auth headers exist so the pod doesn’t reference a non-existent Secret).
	data := map[string][]byte{}
	for _, exp := range c.cfg.OpenTelemetry.Exporters {
		for _, h := range exp.AuthHeaders() {
			src := c.cfg.ExporterAuthSecrets[h.ValueFrom.SecretKeyRef.Name]
			if src == nil {
				continue
			}
			data[operatorv1.HeaderEnvName(exp.Name, h.Name)] = src.Data[h.ValueFrom.SecretKeyRef.Key]
		}
	}
	if len(data) == 0 {
		return nil
	}

pkg/render/otelcollector/component_test.go:112

  • These render tests exercise configurations that are now explicitly rejected by OpenTelemetrySpec.Validate() (e.g., exporters configured but no logs types and metrics disabled), which means the controller will never deploy the collector for these specs. To keep tests aligned with real deployability, update them to include at least one data source (logs or metrics) or convert them into validation-focused tests that assert Validate() rejects the config.
		Entry("no logs, no metrics",
			&otelcollector.Configuration{
				Installation: &operatorv1.InstallationSpec{KubernetesProvider: operatorv1.ProviderGKE},
				OpenTelemetry: &operatorv1.OpenTelemetrySpec{
					Exporters: []operatorv1.OpenTelemetryExporter{{Name: "backend", Endpoint: "https://otlp.example.com:4317"}},
				},
			},
			7, 4,
		),

pkg/render/otelcollector/collector-config.yaml.template:51

  • The exporter endpoint value is emitted as an unquoted YAML scalar. Valid URLs can include characters like # (fragment) which YAML treats as a comment delimiter, producing broken config (endpoint: https://...#frag would be truncated). Quote the endpoint (and consider YAML-escaping) to ensure all valid https URLs render correctly.
  {{.Prefix}}/{{.Name}}:
    endpoint: {{.Endpoint}}

Copilot AI review requested due to automatic review settings August 17, 2026 22:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)

api/v1/logcollector_types.go:581

  • These fields are marked +optional but their JSON tags omit omitempty. That causes them to serialize as null/empty even when unset, which is inconsistent with the rest of the API types and can lead to noisy diffs and less predictable patch/merge behavior. Update the tags to json:\"affinity,omitempty\" and json:\"tolerations,omitempty\" (and consider doing the same for any other optional fields in the new OTel StatefulSet override structs that lack omitempty).
	Affinity *corev1.Affinity `json:"affinity"`

api/v1/logcollector_types.go:599

  • These fields are marked +optional but their JSON tags omit omitempty. That causes them to serialize as null/empty even when unset, which is inconsistent with the rest of the API types and can lead to noisy diffs and less predictable patch/merge behavior. Update the tags to json:\"affinity,omitempty\" and json:\"tolerations,omitempty\" (and consider doing the same for any other optional fields in the new OTel StatefulSet override structs that lack omitempty).
	Tolerations []corev1.Toleration `json:"tolerations"`

pkg/render/monitor/monitor.go:1512

  • The collector ServiceMonitor is rendered only when OpenTelemetryEnabled && !LicenseExpired, but this Prometheus egress rule is rendered whenever OpenTelemetryEnabled is true (even if LicenseExpired is true). That can leave an unnecessary allow-rule behind after license expiration, potentially allowing Prometheus to reach any service named otel-collector in calico-system if such a Service exists. To keep behavior consistent and minimize exposure, gate this rule on the same condition as the ServiceMonitor (e.g., also require !cfg.LicenseExpired) or ensure OpenTelemetryEnabled is forced false in the controller when the license is expired.
	if cfg.OpenTelemetryEnabled {
		// Pairs with serviceMonitorOpenTelemetryCollector; without it the scrape
		// is blocked by the namespace default-deny and just reports up=0.
		egressRules = append(egressRules, v3.Rule{
			Action:      v3.Allow,
			Protocol:    &networkpolicy.TCPProtocol,
			Destination: networkpolicy.CreateServiceSelectorEntityRule(render.OpenTelemetryCollectorNamespace, render.OpenTelemetryCollectorName),
		})
	}

pkg/render/otelcollector/component_test.go:747

  • This test uses a bare host:port endpoint (first.example.com:4317), but the new API/CRD validation in OpenTelemetrySpec.Validate() requires exporter endpoints to start with https://. That mismatch can confuse future readers and makes it harder to reason about what inputs are actually supported. Consider updating the test to use https://first.example.com:4317 (and/or adding a dedicated test that explicitly covers the 'stale CRD / bare host' fallback behavior where appropriate).
		It("should list multiple exporters in pipelines", func() {
			cfg := &otelcollector.Configuration{
				Installation: defaultInstallation,
				OpenTelemetry: &operatorv1.OpenTelemetrySpec{
					Logs: &operatorv1.OpenTelemetryLogs{Types: []operatorv1.OpenTelemetryLogType{operatorv1.OpenTelemetryFlowLog}},
					Exporters: []operatorv1.OpenTelemetryExporter{
						{Name: "first", Endpoint: "first.example.com:4317"},
						{Name: "second", Endpoint: "https://second.example.com", Protocol: operatorv1.OpenTelemetryProtocolHTTP},
					},
				},

Comment thread pkg/render/otelcollector/component.go
Copilot AI review requested due to automatic review settings August 17, 2026 22:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)

pkg/render/otelcollector/component.go:312

  • Within the renderer, missing ExporterAuthSecrets[...] entries are silently skipped when building the aggregated Secret, but exporterAuthEnv() still unconditionally renders EnvVars for all configured auth headers. If the caller forgets to populate ExporterAuthSecrets (or it’s partially populated), this can render a pod that references Secret keys that were never created (hard failure at runtime). To make the component internally consistent, either: (a) only emit EnvVars for headers that were actually inserted into the aggregated Secret, or (b) treat missing Secret/key data as a render-time error (preferred), so the controller can degrade instead of producing a broken workload.
func (c *component) exporterAuthSecret() client.Object {
	data := map[string][]byte{}
	for _, exp := range c.cfg.OpenTelemetry.Exporters {
		for _, h := range exp.AuthHeaders() {
			src := c.cfg.ExporterAuthSecrets[h.ValueFrom.SecretKeyRef.Name]
			if src == nil {
				continue
			}
			data[operatorv1.HeaderEnvName(exp.Name, h.Name)] = src.Data[h.ValueFrom.SecretKeyRef.Key]
		}
	}
	if len(data) == 0 {
		return nil
	}
	return &corev1.Secret{
		TypeMeta:   metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"},
		ObjectMeta: metav1.ObjectMeta{Name: OpenTelemetryCollectorExporterAuthName, Namespace: OpenTelemetryCollectorNamespace},
		Data:       data,
	}
}

pkg/render/otelcollector/component.go:166

  • OpenTelemetryCollector stores cfg directly and multiple methods dereference fields like cfg.Installation / cfg.OpenTelemetry without guarding. If a future call site passes nil (or partial config), this will panic at render time. Add explicit validation here (e.g., return an error when cfg == nil or required sub-structs are nil) to fail fast and keep rendering errors actionable.
func OpenTelemetryCollector(cfg *Configuration) (render.Component, error) {
	c := &component{cfg: cfg}
	conf, err := c.collectorConfig()
	if err != nil {
		return nil, err
	}
	c.renderedConf = conf
	return c, nil
}

pkg/render/logcollector/pipeline.go:271

  • The OTEL collector namespace is already centralized in render.OpenTelemetryCollectorNamespace, but this code uses common.CalicoNamespace. Even if they currently match, using the shared render constant reduces the chance of drift and keeps all OTEL identity details sourced from the same place.
				"host":         fmt.Sprintf("%s.%s.svc", render.OpenTelemetryCollectorName, common.CalicoNamespace),
				"port":         render.OpenTelemetryCollectorOTLPHTTPPort,

Comment thread pkg/render/otelcollector/collector-config.yaml.template
Comment thread api/v1/logcollector_types.go
Copilot AI review requested due to automatic review settings August 17, 2026 23:07
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch from 6d28c48 to 0d29bf0 Compare August 17, 2026 23:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (5)

api/v1/logcollector_types.go:325

  • Validate() dereferences s without guarding against a nil receiver. Elsewhere (e.g., Deployable, HasLogs) the API deliberately supports calling methods on a nil *OpenTelemetrySpec via short-circuiting, so adding an early if s == nil { ... } guard prevents future nil-pointer panics if Validate() is ever invoked directly on a nil spec.
func (s *OpenTelemetrySpec) Validate() error {
	// Every pipeline we render fans out to all exporters, so with none configured
	// each pipeline would be exporter-less.
	if len(s.Exporters) == 0 {
		return fmt.Errorf("at least one exporter must be configured in spec.openTelemetry.exporters")
	}

pkg/render/otelcollector/component.go:312

  • exporterAuthSecret() silently skips missing ExporterAuthSecrets entries (src == nil). However, exporterAuthEnv() still renders EnvVars for every header in the spec, which can produce a pod that fails to start (Secret/key missing) or behaves unpredictably if the aggregate Secret is partially populated. Consider making this fail-fast (return an error from render) when referenced auth secrets/keys are missing, or (less ideally) only emitting env vars for headers that are actually present in the aggregated Secret so rendering stays internally consistent.
func (c *component) exporterAuthSecret() client.Object {
	data := map[string][]byte{}
	for _, exp := range c.cfg.OpenTelemetry.Exporters {
		for _, h := range exp.AuthHeaders() {
			src := c.cfg.ExporterAuthSecrets[h.ValueFrom.SecretKeyRef.Name]
			if src == nil {
				continue
			}
			data[operatorv1.HeaderEnvName(exp.Name, h.Name)] = src.Data[h.ValueFrom.SecretKeyRef.Key]
		}
	}
	if len(data) == 0 {
		return nil
	}
	return &corev1.Secret{
		TypeMeta:   metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"},
		ObjectMeta: metav1.ObjectMeta{Name: OpenTelemetryCollectorExporterAuthName, Namespace: OpenTelemetryCollectorNamespace},
		Data:       data,
	}
}

pkg/render/otelcollector/component_test.go:751

  • This test uses a bare host:port endpoint (first.example.com:4317) even though the new CRD schema and OpenTelemetrySpec.Validate() require https://.... To keep tests aligned with the supported API contract (and avoid codifying an invalid configuration as 'expected'), update the endpoint to an https:// URL (or explicitly document and isolate this as a 'stale CRD compatibility' case, similar to the dedicated test for endpoints without ports).
		It("should list multiple exporters in pipelines", func() {
			cfg := &otelcollector.Configuration{
				Installation: defaultInstallation,
				OpenTelemetry: &operatorv1.OpenTelemetrySpec{
					Logs: &operatorv1.OpenTelemetryLogs{Types: []operatorv1.OpenTelemetryLogType{operatorv1.OpenTelemetryFlowLog}},
					Exporters: []operatorv1.OpenTelemetryExporter{
						{Name: "first", Endpoint: "first.example.com:4317"},
						{Name: "second", Endpoint: "https://second.example.com", Protocol: operatorv1.OpenTelemetryProtocolHTTP},
					},
				},
			}

pkg/render/logcollector/pipeline.go:271

  • The Fluent Bit OTLP output composes the collector Service FQDN using common.CalicoNamespace. Since this PR introduces render.OpenTelemetryCollectorNamespace explicitly to centralize collector identity and avoid drift, it would be more robust to use that constant here as well (so the Service namespace cannot diverge from the collector render constants).
			cfg.Pipeline.Outputs = append(cfg.Pipeline.Outputs, map[string]interface{}{
				"name":         "opentelemetry",
				"match":        match,
				"host":         fmt.Sprintf("%s.%s.svc", render.OpenTelemetryCollectorName, common.CalicoNamespace),
				"port":         render.OpenTelemetryCollectorOTLPHTTPPort,

pkg/render/monitor/monitor.go:1512

  • The collector ServiceMonitor is gated on cfg.OpenTelemetryEnabled && !cfg.LicenseExpired, but the paired Prometheus egress rule is currently gated only on cfg.OpenTelemetryEnabled. If the license is expired (or other conditions prevent the ServiceMonitor from being rendered) this can leave behind an unnecessary allow-egress rule. Consider gating this rule on the same condition as the ServiceMonitor (e.g., also checking !cfg.LicenseExpired) to keep the two resources coupled as intended.
	if cfg.OpenTelemetryEnabled {
		// Pairs with serviceMonitorOpenTelemetryCollector; without it the scrape
		// is blocked by the namespace default-deny and just reports up=0.
		egressRules = append(egressRules, v3.Rule{
			Action:      v3.Allow,
			Protocol:    &networkpolicy.TCPProtocol,
			Destination: networkpolicy.CreateServiceSelectorEntityRule(render.OpenTelemetryCollectorNamespace, render.OpenTelemetryCollectorName),
		})
	}

Add spec.openTelemetry to LogCollector, and render an OpenTelemetry Collector
StatefulSet in calico-system from it. Fluent Bit pushes flow, DNS and audit
logs to the collector's OTLP receiver over mTLS, stamping service.name per log
type so the selection is a source-side filter rather than a classification step
in the collector. Metrics come from federating tigera-prometheus through its
authn-proxy, so the collector inherits every ServiceMonitor, including ones
added later. Both pipelines fan out to the exporters the user configures.

Exporters carry their own TLS and auth: a CA ConfigMap, a client keypair for
mutual TLS, and header credentials read from Secrets. Credentials reach the
collector as environment variables, so they never appear in the rendered
ConfigMap, and a pinned CA is trusted alone rather than alongside the system
roots. Endpoints must be https - there is no path that disables TLS or skips
verification.

The collector is not owned by a CR that gets deleted, so the component renders
for removal as well as creation: one render call, with cfg.Disabled deciding
which. Turning the feature off, losing the licensed feature and license expiry
all take that path, rather than leaving a collector forwarding telemetry after
the entitlement lapses.

Replicas are pinned to 1. Each replica would federate the same targets and
duplicate every series downstream; scaling out needs a target allocator,
anti-affinity and a PDB first.

The monitor controller renders the collector's ServiceMonitor and the
Prometheus egress rule that reaches it, gated on the collector's Service so it
cannot select a workload that was never created.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tianfeng92
tianfeng92 force-pushed the EV-6862-otel-collector branch from 0d29bf0 to 4691f2a Compare August 18, 2026 17:17
@tianfeng92
tianfeng92 merged commit 5615c1a into tigera:master Aug 19, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants