tls configuration options update

[codeboten]

The TLS options for the schema and the collector are inconsistent. The schema currently includes the following options:

tls:
  certificate_file:
  client_key_file:
  client_certificate_file:
  insecure:

The collector supports the following:

tls:
  insecure: false
  ca_file: server.crt
  cert_file: client.crt
  key_file: client.key
  min_version: "1.1"
  max_version: "1.2"

I'd like to propose updating the name certificate_file to ca_file to make it clear this is the certificate authority certificate and removing the client_ prefix for the other two options. If that's acceptable, I will then propose updating the Collector's configuration to support certificate_file instead of cert_file, bringing the two configurations closer together.

[marcalff]

As of now we have:

tls:
  certificate_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE}
  client_key_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY}
  client_certificate_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE}

Currently:

• the collector is already used, there are a lot of existing configurations in the wild
• the file configuration yaml is new, not used yet

Unless the collector naming is somehow flawed and needs to change for other reasons, I would prefer to align on the already established names, to limit impact to a minimum (here, none) for everybody.

This leads to:

tls:
  ca_file:
  key_file:
  cert_file:
  insecure:

While this sounds good because the file config yaml attributes are aligned with the collector, this creates more issues:

tls:
  ca_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE}
  key_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY}
  cert_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE}

It is very confusing to map a variable named OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE to ca_file, a lot of people will use it with cert_file instead, causing chaos.

Because the existing environment variables from the spec can not be changed, I think the best is to stick with the current names and do nothing, so back to:

tls:
  certificate_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE}
  client_key_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY}
  client_certificate_file: ${env:OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE}

Decreasing the difference between:

• yaml names in file configuration
• yaml names in the collector

causes a side effect of increasing the difference between:

• yaml names in file configuration
• environment variables names in the spec.

so this is running in circles, there is no clear winning solution.

[marcalff]

@codeboten

After more time thinking about this:

• I am not opposed to naming changes, only concerned about the impact for users
• There is a tradeoff between better consistency in the long term (so some changes are needed), and a smother transition from environment variables to file configuration in the short term (so less changes are needed)

Now, the whole point of file configuration is to get away from environment variables, it would be ironic to still be constrained by the existing environment variables names once the transition is complete.

As long as the transition is sufficiently documented, which is already the case with sdk-migration-config.yaml, we can change names before the first 1.0.0 release then, in tls or elsewhere if needed.

Leaning towards:

tls:
  ca_file:
  key_file:
  cert_file:
  insecure:

to avoid un necessary changes in the collector.

[codeboten]

to avoid un necessary changes in the collector.

I ended up leaning towards mostly not making changes in the collector with the proposal:

tls:
  ca_file
  key_file
  certificate_file

I'm not sure if cert_file vs certificate_file makes much of a difference in terms of clarity, but i do think calling out the CA file specifically in the config is important. Adding an alias for certificate_file to cert_file isn't a lot of work, but i agree that if it's not too beneficial, it might not be worth doing.