Skip to content

fix(clickhouseexporter): respect TLS configuration when cert_file/key_file are missing and add test case (#43911)#44093

Merged
songy23 merged 9 commits into
open-telemetry:mainfrom
harshit-jindal02:fix-clickhouse-server-tls
Nov 18, 2025
Merged

fix(clickhouseexporter): respect TLS configuration when cert_file/key_file are missing and add test case (#43911)#44093
songy23 merged 9 commits into
open-telemetry:mainfrom
harshit-jindal02:fix-clickhouse-server-tls

Conversation

@harshit-jindal02
Copy link
Copy Markdown
Contributor

@harshit-jindal02 harshit-jindal02 commented Nov 6, 2025

Summary

This PR fixes a bug in the ClickHouse exporter where the TLS configuration was ignored
if neither cert_file nor key_file were set — even when a valid ca_file was provided.
As a result, server-side TLS verification setups (CA-only) failed unexpectedly.

Root Cause

In the previous logic inside buildClickHouseOptions(), TLS initialization was conditioned
on both cert_file and key_file being provided.
However, for many ClickHouse deployments, only a ca_file is necessary to validate
the server certificate without using client-side authentication.

This caused the exporter to skip TLS setup entirely when only ca_file was specified.

Fix Implemented

  • Updated buildClickHouseOptions() to initialize TLS even when only a ca_file is set.

  • Ensured server-side TLS verification is correctly configured.

  • Added a new regression test to prevent future regressions:

    TestBuildClickHouseOptions_WithCAFileOnly

This test verifies:

  • TLS initialization occurs when only a ca_file is present.
  • The function handles invalid CA files gracefully without panic.
  • Exporter logic remains stable under all TLS configurations.

Test Plan

Run locally:

cd exporter/clickhouseexporter
go test -v ./...

Expected output:

=== RUN   TestBuildClickHouseOptions_WithCAFileOnly
--- PASS: TestBuildClickHouseOptions_WithCAFileOnly (0.00s)
PASS
ok  	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/clickhouseexporter	0.8s

Behavior Before

  • TLS configuration was ignored when cert_file and key_file were not provided.
  • Exporter silently defaulted to a non-TLS connection even if a ca_file existed.

Behavior After

  • TLS setup is respected when only a ca_file is configured.
  • Server certificate validation now works as expected.
  • Added unit test ensures this behavior remains protected against regressions.

Affected Files

  • exporter/clickhouseexporter/config.go
  • exporter/clickhouseexporter/config_test.go

Related Issue

Fixes: #43911

Additional Notes

This improvement ensures the ClickHouse exporter correctly handles standard TLS verification
scenarios where a root CA file is provided but no client certificates are required.

It brings the exporter in line with expected ClickHouse server security behavior and adds
a regression test to safeguard future changes.

@harshit-jindal02 harshit-jindal02 requested a review from a team as a code owner November 6, 2025 23:28
@github-actions github-actions Bot added the first-time contributor PRs made by new contributors label Nov 6, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 6, 2025

Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib.

Important reminders:

A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better!

SpencerTorres
SpencerTorres approved these changes Nov 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

As long as this doesn't break default configs, should be good. Also add changelog. Thanks!

@harshit-jindal02
Copy link
Copy Markdown
Contributor Author

harshit-jindal02 commented Nov 7, 2025

As long as this doesn't break default configs, should be good. Also add changelog. Thanks!

Added changelog entry as requested.

@pjanotti
Copy link
Copy Markdown
Contributor

pjanotti commented Nov 7, 2025

Thanks for the PR @harshit-jindal02 - don't edit the changelog directly, add the change using chloggen, follow the instructions at https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/a1bbdc152908d8822a0989a5bde4e44a798bd11d/CONTRIBUTING.md#changelog

@pjanotti
Copy link
Copy Markdown
Contributor

pjanotti commented Nov 7, 2025

The tests are going to fail due to the changelog, but I'm approving the workflows to run so we get a run of the new test.

@harshit-jindal02
Copy link
Copy Markdown
Contributor Author

harshit-jindal02 commented Nov 7, 2025

Thanks for the PR @harshit-jindal02 - don't edit the changelog directly, add the change using chloggen, follow the instructions at https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/a1bbdc152908d8822a0989a5bde4e44a798bd11d/CONTRIBUTING.md#changelog

Thanks @pjanotti, I’ve updated and validated the changelog entry as suggested.

@harshit-jindal02
Copy link
Copy Markdown
Contributor Author

harshit-jindal02 commented Nov 13, 2025

Fixed linting issues (os.CreateTemp usage and whitespace).
All checks should now pass on rerun.

@harshit-jindal02
Copy link
Copy Markdown
Contributor Author

harshit-jindal02 commented Nov 14, 2025

Hi @pjanotti,
I ran make generate locally and it produced no changes, and git status shows a clean working tree. That suggests there’s nothing to regenerate for this PR (since the change only affects the ClickHouse exporter config + tests).

The remaining CI failures are coming from:

processor/k8sattributesprocessor

@pjanotti
Copy link
Copy Markdown
Contributor

pjanotti commented Nov 17, 2025 •
edited
Loading

@harshit-jindal02 we need to cleanup CI before merging, so I'm going to update the branch so it runs against the latest. Let's check the status after that.

@pjanotti
Copy link
Copy Markdown
Contributor

pjanotti commented Nov 17, 2025

write /usr/bin/innochecksum: no space left on device

this is not related to this PR, I will get back to it once the space issue is fixed.

@pjanotti pjanotti added the ready to merge Code review completed; ready to merge by maintainers label Nov 17, 2025
@songy23 songy23 merged commit 121a76a into open-telemetry:main Nov 18, 2025
205 checks passed
@github-actions github-actions Bot added this to the next release milestone Nov 18, 2025
@otelbot
Copy link
Copy Markdown
Contributor

otelbot Bot commented Nov 18, 2025

Thank you for your contribution @harshit-jindal02! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help.

This was referenced Jan 21, 2026
Merged
Closed
damemi added a commit to odigos-io/odigos that referenced this pull request Feb 4, 2026
@damemi
CORE-554: Add TLS settings to Clickhouse destination + Update collect…
ea4f3cb 
…or/otel to 141 + Remove deprecated components + Bump k8s min version to 1.21 (#4111)

The clickhouse exporter supports TLS settings similar to otlp:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/945e5a71ef31793ff3280b28c1425086ea5332b6/exporter/clickhouseexporter/README.md#tls

Some users need this to connect to clickhouse, adding them as options in
the destination here

This adds:

* `insecure_skip_verify`
* `ca_file` (using the k8sconfig interface to mount the secret as a
file, similar to how the GCP exporter supports application default
credentials)

The direct string fields (such as CAPem, CertPem, KeyPem) aren't yet
supported in the clickhouse exporter, so it has to be a mounted file.
See
open-telemetry/opentelemetry-collector-contrib#43911 (comment)

---

To do this, it required bumping the collector/otel deps to 136 when TLS
config support was added to clickhouse. This required the following
changes:

This actually needs collector v0.136.0 for these settings from
open-telemetry/opentelemetry-collector-contrib#42581
(open-telemetry/opentelemetry-collector-contrib@d9769f7)

Also needs to remove loki exporter (removed in 131) for 136 🙃
open-telemetry/opentelemetry-collector-contrib#41413,
see
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/v0.130.0/exporter/lokiexporter#deprecation-notice
it's replaced with just otlp. The only destination that actually looks
like it's using the loki exporter is OpsVerse

As well as the opencensus exporter, removed in 133 upstream by
open-telemetry/opentelemetry-collector-contrib#42239

Also routing processor
open-telemetry/opentelemetry-collector-contrib#36616

See previous attempt #3669
(reverted in #3734)

---

Then, it turns out that 136 was bugged and did not have full support for
TLS settings like `insecure_skip_verify`. This was fixed in 141, which
required the following extra changes:

Actually needs collector v141 due to this bug in clickhouse not handling
all tls settings:
open-telemetry/opentelemetry-collector-contrib#43911
fixed in
open-telemetry/opentelemetry-collector-contrib#44093

Remove deprecated carbon exporter support (unmaintained upstream)
open-telemetry/opentelemetry-collector-contrib#44532

another upstream breaking change giving go mod trouble
open-telemetry/opentelemetry-collector#13948

configgrpc update:
open-telemetry/opentelemetry-collector#13996

and now metadata.yaml metrics require stablity levels
open-telemetry/opentelemetry-collector#13756

```
Error: failed loading /app/collector/receivers/odigosebpfreceiver/metadata.yaml: decoding failed due to the following error(s):

'telemetry.metrics[ebpf_memory_pressure_wait_time_total]' missing required field: `stability`
'telemetry.metrics[ebpf_total_bytes_read]' missing required field: `stability`
'telemetry.metrics[ebpf_lost_samples]' missing required field: `stability`
Error: failed loading /app/collector/receivers/odigosebpfreceiver/metadata.yaml: decoding failed due to the following error(s):

'telemetry.metrics[ebpf_memory_pressure_wait_time_total]' missing required field: `stability`
'telemetry.metrics[ebpf_total_bytes_read]' missing required field: `stability`
'telemetry.metrics[ebpf_lost_samples]' missing required field: `stability`
Error: metadata.yaml ordering check failed: [telemetry metrics] keys are not sorted: [odigos_log_data_size odigos_metric_data_size odigos_trace_data_size odigos_accepted_spans odigos_accepted_metric_points odigos_accepted_log_records]
Error: metadata.yaml ordering check failed: [telemetry metrics] keys are not sorted: [odigos_log_data_size odigos_metric_data_size odigos_trace_data_size odigos_accepted_spans odigos_accepted_metric_points odigos_accepted_log_records]
```

This bump also required adding the `endpointslices` permission to the
odiglet service account for the data-collection collector

---

Finally, endpointslices was not GA in k8s 1.20. This PR bumps our
minimum supported k8s version to 1.21. Enterprise update in
odigos-io/odigos-enterprise#2117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pjanotti pjanotti pjanotti approved these changes

@hanjm hanjm Awaiting requested review from hanjm

+2 more reviewers

@SpencerTorres SpencerTorres SpencerTorres approved these changes

@Frapschen Frapschen Frapschen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bogdandrutu bogdandrutu

Labels

exporter/clickhouse first-time contributor PRs made by new contributors ready to merge Code review completed; ready to merge by maintainers

Projects

None yet

Milestone

v0.141.0

Development

Successfully merging this pull request may close these issues.

[exporter/clickhouse] TLS configuration is ignored if neither cert_file nor key_file are set

6 participants

@harshit-jindal02 @pjanotti @SpencerTorres @Frapschen @bogdandrutu @songy23