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

Setting JAVA_TOOL_OPTIONS via configmap is not possible (replaced,ignored) #1814

Closed
5V715 opened this issue Jun 7, 2023 · 15 comments · Fixed by #3379
Closed

Setting JAVA_TOOL_OPTIONS via configmap is not possible (replaced,ignored) #1814

5V715 opened this issue Jun 7, 2023 · 15 comments · Fixed by #3379
Assignees
Labels
area:auto-instrumentation Issues for auto-instrumentation enhancement New feature or request help wanted Extra attention is needed

Comments

@5V715
Copy link

5V715 commented Jun 7, 2023

when using the operator in combination with ENVVAR from ConfigMap the original value of JAVA_TOOL_OPTIONS is ignored and therefore lost. see the append yaml and comments.

Containers:
  some-app:
    Container ID: xxx
    Image: xxx
    Image ID: xxx
    Port: 8080/TCP
    Host Port: 0/TCP
    ...
    Environment Variables from:
      some-app-config  ConfigMap  Optional: false # (<--) this also contains a value for JAVA_TOOL_OPTIONS
    Environment:
      AWS_METADATA_SERVICE_TIMEOUT: 5
      AWS_METADATA_SERVICE_NUM_ATTEMPTS: 20
      JAVA_TOOL_OPTIONS: -javaagent:/otel-auto-instrumentation/javaagent.jar # <-- replaces the old value from config map
@pavolloffay pavolloffay added area:auto-instrumentation Issues for auto-instrumentation help wanted Extra attention is needed enhancement New feature or request labels Jun 7, 2023
@pavolloffay
Copy link
Member

Correct, the implementation at the moment does not check env vars defined in the config map.

No objections on supporting it.

@pavolloffay
Copy link
Member

Actually in this case I am not sure what would be the behaviour. I would say the operator should make changes to the config map - the problem would be to deal with reverting those changes.

Maybe it could copy the env var value from the config map if the modification is needed?

@5V715
Copy link
Author

5V715 commented Jun 7, 2023

copying and setting it directly sounds cleaner to me. no changes to the config map needed. it's expressive and visible.

@5V715 5V715 changed the title Setting JAVA_TOOL_OPTIONS via configmap is not possible working (replaced,ignored) Setting JAVA_TOOL_OPTIONS via configmap is not possible (replaced,ignored) Jun 7, 2023
@KitSutliff
Copy link
Contributor

Here’s a proposed implementation and a quick question. With a little more info I would be happy to knock this out.

Proposed implementation:

  1. When the operator receives a pod we will check for config maps that reference the given pod
  2. If a config map is present operator will look for JAVA_TOOL_OPTS env var
  3. I there is a JAVA_TOOL_OPTS env var in the ConfigMap, that will be used to populate the JAVA_TOOL_OPTS env var in the container

Should the value form the ConfigMap overwrite the value form the container or should it be appended in some way?

@pavolloffay
Copy link
Member

The operator should copy the value from the CM and override it.

@KitSutliff
Copy link
Contributor

Awesome, thank you for the clarification! I'll start knocking this out.

@pavolloffay
Copy link
Member

Perfect, can we make it generic enough and reuse for other/all variables?

We should as well document the behavior and that these env vars cannot be hardcoded in the image #1884

@shazada
Copy link

shazada commented Jul 26, 2023

Any update on this?

@yuriolisa
Copy link
Contributor

#1393, I believe is requesting the same approach.

@yuriolisa yuriolisa self-assigned this Aug 7, 2023
@jaronoff97
Copy link
Contributor

Let's maybe discuss solutions here in our next SIG meeting. I don't initially love the idea of us reading values from arbitrary configmaps, i think that expands the permissions the operator needs and seems a bit unsafe, I can be persuaded otherwise.

@daguero-technisys
Copy link

Any update on this?

@pavolloffay
Copy link
Member

The issue hasn't been resolved yet. This still needs a proposal that will be accepted.

@daguero-technisys
Copy link

daguero-technisys commented Oct 17, 2023

I think that a possible solution for this case will be make a merge between the configmap variable value and the operator variable value, before inject JAVA_TOOL_OPTIONS variable value in each pod with the auto-instrumentation. Somethings like that:

JAVA_TOOL_OPTIONS="$JAVA_TOOL_OPTIONS -javaagent:/otel-auto-instrumentation/javaagent.jar"

What are you thinking about this proposal?

@pavolloffay
Copy link
Member

it could work, but we would need to test it

@oldium
Copy link
Contributor

oldium commented Oct 19, 2024

I prepared a fix for this in #3373 - to read POD's env variables defined in ConfigMaps and Secrets.

With my fix the JAVA_TOOL_OPTIONS value is correctly merged - OTEL's -javaagent is appended to the existing value.

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 help wanted Extra attention is needed
Projects
None yet
8 participants