Skip to content
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

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

Open
pavolloffay opened this issue Oct 25, 2024 · 6 comments
Labels
area:auto-instrumentation Issues for auto-instrumentation enhancement New feature or request

Comments

@pavolloffay
Copy link
Member

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:

  • OTEL_ env vars - it respects what user configures in pod spec
  • runtime config e.g. JAVA_TOOL_OPTIONS it appends required config by the operator.

Describe the solution you'd like

There are a couple of possible solutions

Read configmap and secret in pod webhook

#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
Copy link
Contributor

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
Copy link

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

@oldium
Copy link
Contributor

oldium commented Oct 31, 2024

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
Copy link
Contributor

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
Copy link
Contributor

oldium commented Oct 31, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:auto-instrumentation Issues for auto-instrumentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants