open-telemetry / opentelemetry-operator

Kubernetes Operator for OpenTelemetry Collector
Apache License 2.0
1.2k stars 439 forks source link

Support defining auto-instrumentation configuration in configmap, secret for all key-value pairs #3392

Open pavolloffay opened 1 week ago

pavolloffay commented 1 week ago

Component(s)

No response

Is your feature request related to a problem? Please describe.

https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables

apiVersion: v1
kind: Pod
metadata:
  name: dapi-test-pod
spec:
  containers:
    - name: test-container
      image: registry.k8s.io/busybox
      command: [ "/bin/sh", "-c", "env" ]
      envFrom:
      - configMapRef:
          name: special-config
  restartPolicy: Never

The above configuration mounts all configmap/secret keys as env vars to pod. The OTEL operator's pod webhook does not know which keys are mounted and therefore it will not respect them (e.g. OTEL_ ) or it can even crash the app (it overrides JAVA_TOOL_OPTIONS).

The current behavior of the operator is:

Describe the solution you'd like

There are a couple of possible solutions

Read configmap and secret in pod webhook

https://github.com/open-telemetry/opentelemetry-operator/pull/3373

The most simple implementation could read just the keys of the configmaps/secretes that are mounted as envFrom.configMapRef without the key. The operator could put all the keys in a map and if they env var needs to be modified, modify it with new env var and substitution e.g. JAVA_TOOL_OPTIONS: $(JAVA_TOOL_OPTIONS) new-val

The downside of this approach is per impact on the webhook - it will make additional API calls during the pod startup.

Use custom env vars

We could build custom agents or extension modules that would accept a config file or custom env vars set by the operator and do the resolution at runtime.

Describe alternatives you've considered

No response

Additional context

It was discussed at SIG meeting https://docs.google.com/document/d/1Unbs2qp_j5kp8FfL_lRH-ld7i5EOQpsq0I4djkOOSL4/edit?tab=t.0#heading=h.j3a542wavlt6

swiatekm commented 1 week ago

In my opinion, the long-term solution to this is to avoid defining any environment variables at all in the application container, and instead write our configuration to a file. A configuration schema for otel SDKs has recently been finalized, and we'll be able to use that once SDKs actually implement support. The problem then disappears, and users can set whatever they want, however they want.

For platform-specific variables unrelated to Otel that we need to modify, we'll either need to require users to set them directly (not via envFrom), or find a different way to inject the otel agent.

It's possible to do a PoC for this in Java by using a properties file as per https://opentelemetry.io/docs/zero-code/java/agent/configuration/#configuration-file. If anyone wants to have a stab at that, be my guest.

sergeykad commented 5 days ago

Custom samplers require ConfigMaps due to their declarative configuration model. https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/samplers

oldium commented 1 day ago

In my opinion, the long-term solution to this is to avoid defining any environment variables at all in the application container, and instead write our configuration to a file. A configuration schema for otel SDKs has recently been finalized, and we'll be able to use that once SDKs actually implement support. The problem then disappears, and users can set whatever they want, however they want.

This looks a way to go, but still the user can set env var OTEL_EXPERIMENTAL_CONFIG_FILE (users are doing it already), in which case the auto-instrumentation's definition gets into conflict with this - and overriding this env var is even more challenging than with JAVA_TOOL_OPTIONS, because it (currently) allows to specify only a single file, not a list of configuration files.

For platform-specific variables unrelated to Otel that we need to modify, we'll either need to require users to set them directly (not via envFrom), or find a different way to inject the otel agent.

If we do not want to read ConfigMaps/Secrets, then K8s should do it. This can be done either in the init container (with all envs copied from app container), or in an app container by overriding the entrypoint. The init container would need to prepare the overridden env vars for the app contianer somehow.

swiatekm commented 1 day ago

This looks a way to go, but still the user can set env var OTEL_EXPERIMENTAL_CONFIG_FILE (users are doing it already), in which case the auto-instrumentation's definition gets into conflict with this - and overriding this env var is even more challenging than with JAVA_TOOL_OPTIONS, because it (currently) allows to specify only a single file, not a list of configuration files.

Setting OTEL_EXPERIMENTAL_CONFIG_FILE means that you want to configure the SDK on your own, with the operator only ensuring it's installed in the app container. For that use case, I think we could add a flag on the Instrumentation CRD itself.

If we do not want to read ConfigMaps/Secrets, then K8s should do it. This can be done either in the init container (with all envs copied from app container), or in an app container by overriding the entrypoint. The init container would need to prepare the overridden env vars for the app contianer somehow.

It depends on what the platform allows. The operator can only set environment variables in the app container at build time, so at runtime this information would have to be persisted in some other way, most likely via a file. I'm far from an expert on injecting agents into the JVM - if anyone knows how this could be accomplished without getting in the way of what the user wants, please chime in and let us know.

oldium commented 1 day ago

This looks a way to go, but still the user can set env var OTEL_EXPERIMENTAL_CONFIG_FILE (users are doing it already), in which case the auto-instrumentation's definition gets into conflict with this - and overriding this env var is even more challenging than with JAVA_TOOL_OPTIONS, because it (currently) allows to specify only a single file, not a list of configuration files.

Setting OTEL_EXPERIMENTAL_CONFIG_FILE means that you want to configure the SDK on your own, with the operator only ensuring it's installed in the app container. For that use case, I think we could add a flag on the Instrumentation CRD itself.

Ok, so if the user wants to override the values, he will need to do this via env vars, which are referenced from the config file.

If we do not want to read ConfigMaps/Secrets, then K8s should do it. This can be done either in the init container (with all envs copied from app container), or in an app container by overriding the entrypoint. The init container would need to prepare the overridden env vars for the app contianer somehow.

It depends on what the platform allows. The operator can only set environment variables in the app container at build time, so at runtime this information would have to be persisted in some other way, most likely via a file. I'm far from an expert on injecting agents into the JVM - if anyone knows how this could be accomplished without getting in the way of what the user wants, please chime in and let us know.

Yes, changing command line arguments without changing the entrypoint is challenging.

swiatekm commented 1 day ago

Ok, so if the user wants to override the values, he will need to do this via env vars, which are referenced from the config file.

We don't need to reference env variables in the config file for this to work. I believe all the SDKs already give env variables a higher priority.