-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Incorrect/misleading Documentation for receiver/kafkametrics TLS Configuration #37776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
@enrico-agile thanks for opening this. I've recently been doing some refactoring across the Kafka components, including receiver/kafkametrics, and I also found the auth bits in general and the TLS part specifically to be confusing. I'm thinking of making a couple of changes:
So your example config above would look like: receivers:
kafkametrics:
brokers:
- broker0:9092
- broker1:9092
- broker3:9093
protocol_version: 2.0.0
scrapers:
- brokers
- topics
- consumers
tls: {} # enable TLS with defaults, i.e. verify server cert & DNS
auth:
sasl:
mechanism: PLAIN
username: username
password: password As far as documentation goes, I would be inclined to mostly remove it and refer readers to the TLS config docs, like the OTLP receiver does -- see https://github.com/open-telemetry/opentelemetry-collector/tree/main/receiver/otlpreceiver#advanced-configuration How does this sound to you? |
Thanks @axw for addressing this! |
#39115 should fix this |
#### Description Deprecates `auth::tls` and introduce `tls` to be consistent with other components, such as the OTLP receiver and exporter. Until we remove it altogether, `auth::tls` will continue to be honoured only if `tls` is unspecified. Why? Two reasons: - TLS is primarily used for encryption. It might also be for client authentication, but that's less common. - Consistency with the majority of other components using configtls.ClientConfig. Almost all of them put it at the top-level. Put together, I think this makes for a clearer structure. Having consistency makes it easier to understand the configuration, particularly if you're already familiar with other components. Moving out of "auth" will also make it easier to understand if you're not using TLS for auth. #### Link to tracking issue Fixes open-telemetry#37776 #### Testing Unit tests pass. Added tests covering the new deprecated config handling. #### Documentation Simplified READMEs by just pointing to the configtls docs, reducing opportunity for drift.
#### Description Deprecates `auth::tls` and introduce `tls` to be consistent with other components, such as the OTLP receiver and exporter. Until we remove it altogether, `auth::tls` will continue to be honoured only if `tls` is unspecified. Why? Two reasons: - TLS is primarily used for encryption. It might also be for client authentication, but that's less common. - Consistency with the majority of other components using configtls.ClientConfig. Almost all of them put it at the top-level. Put together, I think this makes for a clearer structure. Having consistency makes it easier to understand the configuration, particularly if you're already familiar with other components. Moving out of "auth" will also make it easier to understand if you're not using TLS for auth. #### Link to tracking issue Fixes open-telemetry#37776 #### Testing Unit tests pass. Added tests covering the new deprecated config handling. #### Documentation Simplified READMEs by just pointing to the configtls docs, reducing opportunity for drift.
#### Description Deprecates `auth::tls` and introduce `tls` to be consistent with other components, such as the OTLP receiver and exporter. Until we remove it altogether, `auth::tls` will continue to be honoured only if `tls` is unspecified. Why? Two reasons: - TLS is primarily used for encryption. It might also be for client authentication, but that's less common. - Consistency with the majority of other components using configtls.ClientConfig. Almost all of them put it at the top-level. Put together, I think this makes for a clearer structure. Having consistency makes it easier to understand the configuration, particularly if you're already familiar with other components. Moving out of "auth" will also make it easier to understand if you're not using TLS for auth. #### Link to tracking issue Fixes open-telemetry#37776 #### Testing Unit tests pass. Added tests covering the new deprecated config handling. #### Documentation Simplified READMEs by just pointing to the configtls docs, reducing opportunity for drift.
Component(s)
receiver/kafkametrics
Describe the issue you're reporting
Today I was trying to configure the kafkametrics receiver to monitor a cluster that is using TLS w/ plain_text authentication.
In the end I've managed to make it work but I've admittedly lost a lot of time trying to configure it.
The documentation currently states:
auth.tls.ca_file
: path to the CA cert. For a client this verifies the server certificate. Should only be used if insecure is set to trueauth.tls.insecure
: (default = false) Disable verifying the server's certificate chain and host name (InsecureSkipVerify in the tls config)However, after reviewing the code (here and here), I found discrepancies:
insecure
: Does not disable certificate verification, it actually disables TLS entirelyca_file
: The statement Should only be used if insecure is set to true is misleading - In reality, configuring a ca_file implicitly neutralizes an insecure=true, enabling TLS even if insecure is set to falseinsecure_skip_verify
: This is the actual parameter that disables certificate verification, but it is undocumentedAdditionally, the way the configuration is structured is a little confusing. There are valid Kafka configurations with plain_text authentication (like my case) or even Kerberos authentication that require TLS encryption. The only way to enable TLS in these scenarios is to pass a non-nil
auth.tls
configuration, as shown in my example configuration below, even if I'm not actually using mTLS authn.The following configuration successfully enabled monitoring in my setup:
The text was updated successfully, but these errors were encountered: