ESO-334: Adds implementation logic for Environment Variables to support customizations at install time#106
ESO-334: Adds implementation logic for Environment Variables to support customizations at install time#106siddhibhor-56 wants to merge 4 commits intoopenshift:mainfrom
Conversation
WalkthroughAdds unexported container-name constants; extends asset→(component,container) mapping; implements env var merging and applies OverrideEnv to matched containers and initContainers; updates getComponentNameFromAsset signature; expands unit and e2e tests and updates govulncheck ignore list. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: siddhibhor-56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@siddhibhor-56: This pull request references ESO-334 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
siddhibhor-56
force-pushed
the
env-var
branch
from
6c970a0 to
f7c5e38
Compare
siddhibhor-56
force-pushed
the
env-var
branch
from
f7c5e38 to
484a1d5
Compare
|
@siddhibhor-56: This pull request references ESO-334 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@siddhibhor-56: This pull request references ESO-334 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/controller/external_secrets/deployments.go`:
- Around line 676-688: The current code applies OverrideEnv to every init
container (deployment.Spec.Template.Spec.InitContainers), risking secret
leakage; change the second loop to only merge when the init container's Name
matches the target containerName (i.e., check InitContainers[j].Name ==
containerName before calling mergeEnvVars) or remove applying OverrideEnv to
init containers entirely if the intent is to only target the component
container; update any related comments to reflect the new behavior.
| // Apply OverrideEnv if set | ||
| if len(i.OverrideEnv) > 0 { | ||
| for j := range deployment.Spec.Template.Spec.Containers { | ||
| if deployment.Spec.Template.Spec.Containers[j].Name == containerName { | ||
| mergeEnvVars(&deployment.Spec.Template.Spec.Containers[j], i.OverrideEnv) | ||
| break | ||
| } | ||
| } | ||
| // Apply to all init containers in the deployment without name filtering, | ||
| for j := range deployment.Spec.Template.Spec.InitContainers { | ||
| mergeEnvVars(&deployment.Spec.Template.Spec.InitContainers[j], i.OverrideEnv) | ||
| } | ||
| } |
There was a problem hiding this comment.
OverrideEnv is applied to all init containers—potential secret leakage.
OverrideEnv is documented as applying to the component container; pushing the same env into every init container can expose secrets or alter init behavior unexpectedly. Consider restricting to the target container, or explicitly extending the API/CRD docs and validating this behavior.
🛠️ Option: restrict overrideEnv to the target container only
- // Apply to all init containers in the deployment without name filtering,
- for j := range deployment.Spec.Template.Spec.InitContainers {
- mergeEnvVars(&deployment.Spec.Template.Spec.InitContainers[j], i.OverrideEnv)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Apply OverrideEnv if set | |
| if len(i.OverrideEnv) > 0 { | |
| for j := range deployment.Spec.Template.Spec.Containers { | |
| if deployment.Spec.Template.Spec.Containers[j].Name == containerName { | |
| mergeEnvVars(&deployment.Spec.Template.Spec.Containers[j], i.OverrideEnv) | |
| break | |
| } | |
| } | |
| // Apply to all init containers in the deployment without name filtering, | |
| for j := range deployment.Spec.Template.Spec.InitContainers { | |
| mergeEnvVars(&deployment.Spec.Template.Spec.InitContainers[j], i.OverrideEnv) | |
| } | |
| } | |
| // Apply OverrideEnv if set | |
| if len(i.OverrideEnv) > 0 { | |
| for j := range deployment.Spec.Template.Spec.Containers { | |
| if deployment.Spec.Template.Spec.Containers[j].Name == containerName { | |
| mergeEnvVars(&deployment.Spec.Template.Spec.Containers[j], i.OverrideEnv) | |
| break | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/controller/external_secrets/deployments.go` around lines 676 - 688, The
current code applies OverrideEnv to every init container
(deployment.Spec.Template.Spec.InitContainers), risking secret leakage; change
the second loop to only merge when the init container's Name matches the target
containerName (i.e., check InitContainers[j].Name == containerName before
calling mergeEnvVars) or remove applying OverrideEnv to init containers entirely
if the intent is to only target the component container; update any related
comments to reflect the new behavior.
There was a problem hiding this comment.
@siddhibhor-56 I think we should consider, though I had suggested we consider init containers.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/e2e_test.go`:
- Around line 316-333: The current test flattens env vars into envMap (envMap,
deployment.Spec.Template.Spec.Containers,
deployment.Spec.Template.Spec.InitContainers) which can false-positive if
overrides appear in different containers; change the assertion to check
per-container: ensure at least one primary container from
deployment.Spec.Template.Spec.Containers contains all entries from
config.OverrideEnv (use a loop to verify each expectedEnv.Name/Value exists in
that container's Env slice and assert success if any container satisfies all),
and separately assert that every init container in
deployment.Spec.Template.Spec.InitContainers contains all entries from
config.OverrideEnv; keep using g.Expect with descriptive messages referencing
deploymentName and the specific container when failing.
🧹 Nitpick comments (1)
pkg/controller/external_secrets/deployments_test.go (1)
1536-1547: Harden ValueFrom assertions in mergeEnvVars tests.Right now the test only checks that
ValueFromis non-nil; it won’t catch mismatchedSecretKeyRefor leftoverValue. Tightening this improves test correctness for the new behavior.🔧 Proposed test tightening
- if expected.ValueFrom != nil { - if actual.ValueFrom == nil { - t.Errorf("mergeEnvVars() env var %s expected ValueFrom but got Value", expected.Name) - } - } else if actual.Value != expected.Value { + if expected.ValueFrom != nil { + if actual.ValueFrom == nil || !reflect.DeepEqual(actual.ValueFrom, expected.ValueFrom) { + t.Errorf("mergeEnvVars() env var %s ValueFrom mismatch. got %+v, want %+v", expected.Name, actual.ValueFrom, expected.ValueFrom) + } + if actual.Value != "" { + t.Errorf("mergeEnvVars() env var %s should not set Value when ValueFrom is used", expected.Name) + } + } else if actual.Value != expected.Value { t.Errorf("mergeEnvVars() env var %s = %v, want %v", expected.Name, actual.Value, expected.Value) }
| // Collect env vars from all containers and initContainers | ||
| envMap := make(map[string]string) | ||
| for _, container := range deployment.Spec.Template.Spec.Containers { | ||
| for _, env := range container.Env { | ||
| envMap[env.Name] = env.Value | ||
| } | ||
| } | ||
| for _, initContainer := range deployment.Spec.Template.Spec.InitContainers { | ||
| for _, env := range initContainer.Env { | ||
| envMap[env.Name] = env.Value | ||
| } | ||
| } | ||
|
|
||
| // Verify all expected env vars are present | ||
| for _, expectedEnv := range config.OverrideEnv { | ||
| g.Expect(envMap).To(HaveKeyWithValue(expectedEnv.Name, expectedEnv.Value), | ||
| "%s should have env var %s=%s", deploymentName, expectedEnv.Name, expectedEnv.Value) | ||
| } |
There was a problem hiding this comment.
Strengthen env-var assertions to avoid false positives.
The test currently merges env vars from all containers and initContainers into a single map. This can pass even if only one container has the overrides, which doesn’t validate “primary container + all init containers.” Validate per-container instead (at least one primary container must have all override envs; every init container must have them).
Suggested fix
- // Collect env vars from all containers and initContainers
- envMap := make(map[string]string)
- for _, container := range deployment.Spec.Template.Spec.Containers {
- for _, env := range container.Env {
- envMap[env.Name] = env.Value
- }
- }
- for _, initContainer := range deployment.Spec.Template.Spec.InitContainers {
- for _, env := range initContainer.Env {
- envMap[env.Name] = env.Value
- }
- }
-
- // Verify all expected env vars are present
- for _, expectedEnv := range config.OverrideEnv {
- g.Expect(envMap).To(HaveKeyWithValue(expectedEnv.Name, expectedEnv.Value),
- "%s should have env var %s=%s", deploymentName, expectedEnv.Name, expectedEnv.Value)
- }
+ hasAll := func(envs []corev1.EnvVar) bool {
+ envMap := make(map[string]string, len(envs))
+ for _, env := range envs {
+ envMap[env.Name] = env.Value
+ }
+ for _, expectedEnv := range config.OverrideEnv {
+ if val, ok := envMap[expectedEnv.Name]; !ok || val != expectedEnv.Value {
+ return false
+ }
+ }
+ return true
+ }
+
+ // Ensure at least one primary container has all override envs
+ primaryMatched := false
+ for _, container := range deployment.Spec.Template.Spec.Containers {
+ if hasAll(container.Env) {
+ primaryMatched = true
+ break
+ }
+ }
+ g.Expect(primaryMatched).To(BeTrue(),
+ "%s should have override env vars on the primary container", deploymentName)
+
+ // Ensure all init containers have override envs
+ for _, initContainer := range deployment.Spec.Template.Spec.InitContainers {
+ g.Expect(hasAll(initContainer.Env)).To(BeTrue(),
+ "%s init container should have override env vars", deploymentName)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Collect env vars from all containers and initContainers | |
| envMap := make(map[string]string) | |
| for _, container := range deployment.Spec.Template.Spec.Containers { | |
| for _, env := range container.Env { | |
| envMap[env.Name] = env.Value | |
| } | |
| } | |
| for _, initContainer := range deployment.Spec.Template.Spec.InitContainers { | |
| for _, env := range initContainer.Env { | |
| envMap[env.Name] = env.Value | |
| } | |
| } | |
| // Verify all expected env vars are present | |
| for _, expectedEnv := range config.OverrideEnv { | |
| g.Expect(envMap).To(HaveKeyWithValue(expectedEnv.Name, expectedEnv.Value), | |
| "%s should have env var %s=%s", deploymentName, expectedEnv.Name, expectedEnv.Value) | |
| } | |
| hasAll := func(envs []corev1.EnvVar) bool { | |
| envMap := make(map[string]string, len(envs)) | |
| for _, env := range envs { | |
| envMap[env.Name] = env.Value | |
| } | |
| for _, expectedEnv := range config.OverrideEnv { | |
| if val, ok := envMap[expectedEnv.Name]; !ok || val != expectedEnv.Value { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| // Ensure at least one primary container has all override envs | |
| primaryMatched := false | |
| for _, container := range deployment.Spec.Template.Spec.Containers { | |
| if hasAll(container.Env) { | |
| primaryMatched = true | |
| break | |
| } | |
| } | |
| g.Expect(primaryMatched).To(BeTrue(), | |
| "%s should have override env vars on the primary container", deploymentName) | |
| // Ensure all init containers have override envs | |
| for _, initContainer := range deployment.Spec.Template.Spec.InitContainers { | |
| g.Expect(hasAll(initContainer.Env)).To(BeTrue(), | |
| "%s init container should have override env vars", deploymentName) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/e2e_test.go` around lines 316 - 333, The current test flattens env
vars into envMap (envMap, deployment.Spec.Template.Spec.Containers,
deployment.Spec.Template.Spec.InitContainers) which can false-positive if
overrides appear in different containers; change the assertion to check
per-container: ensure at least one primary container from
deployment.Spec.Template.Spec.Containers contains all entries from
config.OverrideEnv (use a loop to verify each expectedEnv.Name/Value exists in
that container's Env slice and assert success if any container satisfies all),
and separately assert that every init container in
deployment.Spec.Template.Spec.InitContainers contains all entries from
config.OverrideEnv; keep using g.Expect with descriptive messages referencing
deploymentName and the specific container when failing.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@siddhibhor-56: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
Everything else LGTM.
| controllerContainerName = "external-secrets" | ||
| webhookContainerName = "webhook" | ||
| certControllerContainerName = "cert-controller" | ||
| bitwardenContainerName = "bitwarden-sdk-server" |
There was a problem hiding this comment.
This doesn't look right. These consts can be defined in this file along with others after line 77 with proper comment.
| // Apply OverrideEnv if set | ||
| if len(i.OverrideEnv) > 0 { | ||
| for j := range deployment.Spec.Template.Spec.Containers { | ||
| if deployment.Spec.Template.Spec.Containers[j].Name == containerName { | ||
| mergeEnvVars(&deployment.Spec.Template.Spec.Containers[j], i.OverrideEnv) | ||
| break | ||
| } | ||
| } | ||
| // Apply to all init containers in the deployment without name filtering, | ||
| for j := range deployment.Spec.Template.Spec.InitContainers { | ||
| mergeEnvVars(&deployment.Spec.Template.Spec.InitContainers[j], i.OverrideEnv) | ||
| } | ||
| } |
There was a problem hiding this comment.
@siddhibhor-56 I think we should consider, though I had suggested we consider init containers.
| { | ||
| ComponentName: "ExternalSecretsCoreController", | ||
| OverrideEnv: []corev1.EnvVar{ | ||
| {Name: "GOMAXPROCS", Value: "4"}, |
There was a problem hiding this comment.
This is go well-known env var, should we be including it in the test environment?
| // Collect env vars from all containers and initContainers | ||
| envMap := make(map[string]string) | ||
| for _, container := range deployment.Spec.Template.Spec.Containers { | ||
| for _, env := range container.Env { | ||
| envMap[env.Name] = env.Value | ||
| } | ||
| } | ||
| for _, initContainer := range deployment.Spec.Template.Spec.InitContainers { | ||
| for _, env := range initContainer.Env { | ||
| envMap[env.Name] = env.Value | ||
| } | ||
| } | ||
|
|
||
| // Verify all expected env vars are present | ||
| for _, expectedEnv := range config.OverrideEnv { | ||
| g.Expect(envMap).To(HaveKeyWithValue(expectedEnv.Name, expectedEnv.Value), | ||
| "%s should have env var %s=%s", deploymentName, expectedEnv.Name, expectedEnv.Value) | ||
| } |
3 participants
Uh oh!
There was an error while loading. Please reload this page.