CORE-554: Add TLS settings to Clickhouse destination + Update collector/otel to 141 + Remove deprecated components + Bump k8s min version to 1.21#4111
Conversation
8567006 to
e135186
Compare
| return common.ClickhouseDestinationType | ||
| } | ||
|
|
||
| func clickhouseTlsConfig(dest ExporterConfigurer) GenericMap { |
There was a problem hiding this comment.
I see we check if clickhouseTlsEnabled is true, but we don't check that all keys exist. Isn't it prone to misconfiguration?
There was a problem hiding this comment.
Good point you don't need to set all of the keys (just having tls=true is enough for the minimum) but from what I could find it sounds like certPem and keyPem always need to be provided together if they are used for mTLS. updated
e135186 to
6e08959
Compare
|
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 |
|
If we are bumping the collector I'd consider bumping to a newer version. |
@RonFed for this I'm just trying to bump it to the minimum for the feature I need. We can do more incremental bumps this way and have fewer huge PRs with lots of changes. wdyt about that approach? |
7fe94ae to
ec0ccf8
Compare
That makes sense. On the other side, since this is quite a pain to do, if we have the opportunity - maybe it is something worth to consider. There are also performance improvements that we can gain that are not necessarily related to new features. |
ec0ccf8 to
c08e16e
Compare
| componentProps: | ||
| required: false | ||
| tooltip: 'Skip TLS certificate verification' | ||
| renderCondition: ['CLICKHOUSE_TLS_ENABLED', '==', 'true'] |
There was a problem hiding this comment.
should be rendered only when there is a certificate? or should be the opposite?
There was a problem hiding this comment.
@RonFed wdym? This is the same as the otlphttp skip verify setting
odigos/destinations/data/otlphttp.yaml
Lines 183 to 190 in 0c278ae
There was a problem hiding this comment.
@RonFed ah I think I get what you mean. Only show the insecure skip verify if no cert is provided? Yeah I would like to do that, but from what I can tell right now the render conditions don't allow multiple conditionals which would be nice. Talked to Ben about that and we could use that in some other destinations too, but for now I think it's fine
| tlsConfig := GenericMap{ | ||
| "insecure": false, | ||
| } | ||
| if caPem, ok := dest.GetConfig()[clickhouseCaPem]; ok && caPem != "" { |
There was a problem hiding this comment.
should we validate the ca format here to be a valid one?
There was a problem hiding this comment.
added a function to validate the ca format
c9b8b16 to
cc62af6
Compare
|
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 |
f4d2833 to
4422338
Compare
There was a problem hiding this comment.
Since all the k8s v1.20 tests are failing I guess something in the 141 bump caused issues in that version.
Looking at the EndpointSlice docs it seems it became stable on v1.21 - I guess this might have something to do with the tests failures.
@damemi Do you know which component has this requirement? this open-telemetry/opentelemetry-collector-contrib#44079 seems like one case that might cause this
Looking at the parent issue (open-telemetry/opentelemetry-collector-contrib#43891) it seems like
if that's the case then it might be a problem either blocking us from updating otel or forcing us to drop 1.20. looking into it if endpointslices were stable in 1.21 then they should have at least been beta (on by default) in 1.20 🤔 |
|
@RonFed it does seem to be the issue (ran 1.20 test locally): |
65eb76a to
391fde6
Compare
536273c to
34704f9
Compare
|
the changes lgtm, seems like all the 1.21 tests are failing though |
a8b73ea to
7e43290
Compare
7e43290 to
a8bde6b
Compare
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_verifyca_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
This bump also required adding the
endpointslicespermission to the odiglet service account for the data-collection collectorFinally, endpointslices was not GA in k8s 1.20. This PR bumps our minimum supported k8s version to 1.21. Enterprise update in https://github.com/odigos-io/odigos-enterprise/pull/2117