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

Add handling of environment variables from ConfigMaps and Secrets #3373

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oldium
Copy link
Contributor

@oldium oldium commented Oct 19, 2024

Description:

Added detection and reading of environment variables defined in PODs resources like ConfigMaps and Secrets. Supports both envFrom and valueFrom forms.

Link to tracking Issue(s):

Testing:

Added kubebuilder tests for new code, tested by deploying the generated image to OpenShift cluster.

Documentation:

This is an enhancement, which can be read also as a bug fix (missing feature), so I have not added any documentation.

Implementation notes/questions:

  1. The ConfigMap support is in first commit, the Secret support is in the second commit. They work the same, they both support “if exists” checks and are able to extend the existing value (append) into a newly created (or updated) environment variable.
  2. I was not sure about the Container class naming (or have some common methods prefix?), so please suggest how the main class/file should be named - I can change it easily. The class handles everything regarding environment variables on the container currently.
  3. I was not sure about the usage of namespace name. The sdkInjector.inject method gets one in arguments, but Apache and Nginx auto-instrumentations are ignoring it and taking namespace name from POD definition or from PODs resource map, which looks wrong (at minimum this is suspicious). I took the namespace name from the POD and if not found, I fall back to the one supplied as a parameter to inject method.
  4. I was not sure about the reading of Secrets (second commit) - whether it is secure to handle them fully. The implementation is as full as for ConfigMaps, so the values can be detected (“if exists” checks) and extended (“append when exists”) into a new or updated environment variable (so Secret is converted to env var!). If necessary, I can change the implementation to just recognise them (record the existence) but fail when the value should be extended.

@oldium oldium requested a review from a team as a code owner October 19, 2024 15:40
@oldium oldium force-pushed the main branch 4 times, most recently from 4be1f9d to 506a78c Compare October 19, 2024 18:19
Currently when environment variable is defined in ConfigMap, it is like
being invisible to OpenTelemetry Operator. Improve this by adding a common
class handling the environment variables, which is also able to load
the referenced ConfigMaps.

Signed-off-by: Oldřich Jedlička <[email protected]>
Load also environment variables defined in Secret to make them visible
to OpenTelemetry Operator.

Signed-off-by: Oldřich Jedlička <[email protected]>
@oldium oldium changed the title Add handling of environment variables in ConfigMaps and Secrets Add handling of environment variables from ConfigMaps and Secrets Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant