Inconsistency in config for TLS vs Auth

[bogdandrutu]

It seems to be very confusing in confighttp and configgrpc that TLS is a special configuration embedded into the main config, and "Auth" is separated.

We need to look into this and confirm the right design/configuration for TLS and other Authentications. Another inconsistency that I remember is the KafkaReceiver/Exporter where these options are combined under an "authentication" sub-layer.

Investigate how other projects are dealing with this and try to use the "standard" across projects (Prometheus, Kafka, Grafana, K8S, etc.).

[alolita]

@jpkrohling can you please take a look at this issue and share your design input for auth configs.

[jpkrohling]

Looking at it now, I believe that it should indeed be squashed, like TLS. It did make sense in the past to be conservative about this and group all auth options under auth, but I don't foresee any other option within that config option.

[mxiamxia]

It seems there is no consistent design pattern among these projects for TLS and Auth configurations. I found Prometheus and Grafna try to group TLS and Auth configs together, but k8s Kubelet separates them out in the configuration when doing API server authenticating.
Eg,

1. Prometheus configuration groups TLS and Auth secrets together in one tls_config structure. Here is Prom auth reference doc and code.
2. Grafana uses key value pairs in its ini config file. It also tries to group TLS and Auth secrets entries in the same section within configurations. See example.

However, in Kubelet Configuration

• Kubelet TLS config is set under KubeletConfiguration and Authentication config is set in a separate config - KubeletAuthentication.

We can combine TLS and Authenticator in confighttp and configgrpc as one config. But it seems will only be useful for OLTP receiver/exporter or common cases. Because there are so many authentication protocols and custom authentication options, we can't have a generic TLS/Authentication config or custom authenticator extension that could satisfy the different auth use cases in different receivers/exporters. Take Kafka exporter as example, it supports multiple authentication protocols including basic plain text auth, SASL and Kerberos, we can't apply TLS/Authentication config directly from confighttp and configgrpc for Kafka components. it has to define its own customized authentication config in Kafka components.

I'd like to help on this issue. pls let me know if you want me help on anything. @jpkrohling @bogdandrutu @alolita

[jpkrohling]

The authenticator options will still be part of the extension's own configuration. confighttp and configgrpc would include a configauth with squash mode, similar to what they do with configtls. At least, that's how I understand the original concern.

[mxiamxia]

Agree that we should place TLS and authenticator in the same config level to reduce the confusing. We can either simply squash configauth in confighttp and configgrpc or combine them under a new auth config structure. I listed these 2 option examples below, pls let me know which one is preferred.

Option#1 - simply squash configauth

otlp:
  endpoint: "1.2.3.4:1234"
  timeout: 10s
  ca_file: ca.pem
  cert_file: cert.pem
  key_file: key.pem
  authenticator: oAuth      

Option#2 - put both TLS and Authenticator under a new auth

otlp:
  endpoint: "1.2.3.4:1234"
  timeout: 10s
  auth:
    ca_file: ca.pem
    cert_file: cert.pem
    key_file: key.pem
    authenticator: oAuth

[Aneurysm9]

I think that either auth should be squashed (as in #1 above) or that tls should not, like this:

otlp:
  endpoint: "1.2.3.4:1234"
  timeout: 10s
  tls:
    ca_file: ca.pem
    cert_file: cert.pem
    key_file: key.pem
  auth:
    authenticator: oAuth

[alolita]

Thanks Anthony. I agree having TLS and Auth as siblings would provide more flexibility. You can have Auth with TLS but TLS can standalone without Auth.

[alolita]

@bogdandrutu @tigrannajaryan @jpkrohling Can you weigh in on what your final recommendation is? Based on that we will file a PR.

[mxiamxia]

All these options will introduce the break changes. Option#1 has the minimal impact since Authenticator is newer that has not been commonly applied yet compare to tls.

[jpkrohling]

I prefer the non-squashed versions, as it makes the context clear: we know that key_file is related to TLS. For the auth part, I don't have a strong opinion for the current implementation, as the only property inside it is authenticator.

[alolita]

@bogdandrutu looks like non-squashed version is the way to do - can you weigh in with your comments so that we can make progress.

[tigrannajaryan]

I think that either auth should be squashed (as in #1 above) or that tls should not, like this:

otlp:
  endpoint: "1.2.3.4:1234"
  timeout: 10s
  tls:
    ca_file: ca.pem
    cert_file: cert.pem
    key_file: key.pem
  auth:
    authenticator: oAuth

This looks better to me. I don't like the squashed version since it is not clear what keys are about (what is key_file?)
It is unfortunate that this is going to be a breaking change but I think we need too fix it.

[mxiamxia]

I think that either auth should be squashed (as in #1 above) or that tls should not, like this:

otlp:
  endpoint: "1.2.3.4:1234"
  timeout: 10s
  tls:
    ca_file: ca.pem
    cert_file: cert.pem
    key_file: key.pem
  auth:
    authenticator: oAuth

This looks better to me. I don't like the squashed version since it is not clear what keys are about (what is key_file?)
It is unfortunate that this is going to be a breaking change but I think we need too fix it.

Thanks @tigrannajaryan. Has sent the PR above (#4063) for this option. Please help review it.

[alolita]

@tigrannajaryan can you please help review and merge?

[bogdandrutu]

@jpkrohling @tigrannajaryan @mxiamxia we need to make sure we are consistent with Server side correct? See https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/confighttp/confighttp.go#L143. I think personally both should be "tls"

[jpkrohling]

Agree, consistency FTW!

[bogdandrutu]

@mxiamxia please think about #4063 (comment)

[tigrannajaryan]

@mxiamxia please close this when you consider it done.

[mxiamxia]

@mxiamxia please close this when you consider it done.

Hi @tigrannajaryan , I'm fine to close this issue and also take a look the comment you put in PR #4104 which is the existing behavior not related to this config inconsistency.