Skip to content

fix: hardcode image to use params.env#56

Merged
anishasthana merged 2 commits intoopendatahub-io:mainfrom
zdtsw-forking:jira_5816
Apr 7, 2026
Merged

fix: hardcode image to use params.env#56
anishasthana merged 2 commits intoopendatahub-io:mainfrom
zdtsw-forking:jira_5816

Conversation

@zdtsw
Copy link
Copy Markdown
Member

@zdtsw zdtsw commented Apr 6, 2026

Description

ref https://redhat.atlassian.net/browse/INFERENG-5816
need opendatahub-io/opendatahub-operator#3383

we are too late to get this into ODH 3.4 relase cycle
since this wont impact ODH but only RHOAI 3.4, as long as it is merged to main and sync to RHDS by code freeze, we should be fine.
backport fix for 3.4ea2 is already in RHDS build

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features
    • Added a configurable controller image parameter to allow switching the controller version via config.
  • Chores
    • Deployment configuration now generates a stable ConfigMap from environment parameters and injects the configured image into the controller deployment, enabling predictable, environment-specific image updates without code changes.

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 6, 2026

📝 Walkthrough

Walkthrough

The change adds a configMapGenerator in config/openshift/kustomization.yaml that generates a stable-named ConfigMap wva-parameters from params.env (hash suffix disabled). A replacements rule injects data.wva-controller-image from that ConfigMap into the controller-manager Deployment container named manager (spec.template.spec.containers.[name=manager].image). config/openshift/params.env gains a wva-controller-image entry set to ghcr.io/llm-d/llm-d-workload-variant-autoscaler:v0.6-0.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Security and design concerns

  • CWE-426 (Untrusted Search Path / Untrusted Deployment Source): Image URI is sourced from params.env and injected without validation. Action: enforce provenance — keep params.env in version control with restricted write access, or validate image strings during CI (allowlist registries, require digests).
  • CWE-20 (Improper Input Validation): No validation of the wva-controller-image value (format, registry, digest). Action: add schema/regex validation in CI or kustomize pre-apply hooks to reject unexpected formats/tags (e.g., disallow latest, require sha256 digest).
  • Configuration drift / rollout risk: disableNameSuffixHash: true prevents automatic ConfigMap-driven pod restarts on value change. Action: ensure update triggers rollout (e.g., add an annotation on Deployment with configmap checksum, or use Kustomize transformers that update pod template hash).
  • Supply-chain / image integrity: image uses a tag, not a digest. Action: pin to immutable digests in params.env or require image signatures (e.g., cosign) and verify in CI/admission.
  • Missing runtime constraints: no explicit imagePullPolicy, no admission-level registry allowlist evident. Action: enforce registry allowlist and image pull policies via Admission Controller or cluster policy (PodSecurityPolicy replacement / OPA Gatekeeper).
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: replacing hardcoded image configuration to use params.env through Kustomize configMapGenerator and replacements.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
config/openshift/kustomization.yaml (1)

14-15: generatorOptions applies globally to all generators

This disableNameSuffixHash: true is defined at the root level, affecting all current and future configMapGenerator and secretGenerator entries in this file. If other generators are added later, they'll unexpectedly lack hash suffixes, potentially breaking rollout detection.

Move the option inside the specific generator if you want to scope it:

Scoped generator option
 configMapGenerator:
 - envs:
   - params.env
   name: wva-parameters
-generatorOptions:
-  disableNameSuffixHash: true
+  options:
+    disableNameSuffixHash: true
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/openshift/kustomization.yaml` around lines 14 - 15, The
generatorOptions.disableNameSuffixHash is currently set at the root and
therefore globally disables name hashing for all generators; instead, move
disableNameSuffixHash: true into the specific generator block(s) (e.g., inside
the configMapGenerator or secretGenerator entries) so only those generators omit
the hash while others retain rollout-safe suffixes; update the kustomization to
remove the top-level generatorOptions.disableNameSuffixHash and add the option
under the intended configMapGenerator/secretGenerator definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@config/openshift/kustomization.yaml`:
- Around line 10-15: The configMapGenerator references a missing params.env
which causes kustomize to fail; create the missing params.env file containing
the required key (wva-controller-image=<image-reference>) so the wva-parameters
ConfigMap produced by configMapGenerator includes the wva-controller-image entry
used by the replacement rule, or alternatively update documentation to state
that params.env must be provided by the deployer; ensure the filename exactly
matches params.env and that the key name is wva-controller-image to satisfy the
replacement rule that consumes wva-parameters.
- Around line 39-50: The replacements stanza is referencing
data.wva-controller-image from a generated ConfigMap named wva-parameters but
the configMapGenerator relies on a missing params.env, so either add a
params.env in config/openshift containing a line like
wva-controller-image=<your-openshift-image> so the configMapGenerator
(wva-parameters) contains data.wva-controller-image, or remove the broken
configMapGenerator and the replacements: block entirely so the images override
flow (images: in config/manager/kustomization.yaml) does not rely on a
non-existent ConfigMap; update the kustomization entries to ensure the
wva-parameters ConfigMap and the replacements: stanza are consistent with the
presence or absence of params.env and the target data.wva-controller-image.

---

Nitpick comments:
In `@config/openshift/kustomization.yaml`:
- Around line 14-15: The generatorOptions.disableNameSuffixHash is currently set
at the root and therefore globally disables name hashing for all generators;
instead, move disableNameSuffixHash: true into the specific generator block(s)
(e.g., inside the configMapGenerator or secretGenerator entries) so only those
generators omit the hash while others retain rollout-safe suffixes; update the
kustomization to remove the top-level generatorOptions.disableNameSuffixHash and
add the option under the intended configMapGenerator/secretGenerator
definitions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: ff5c3622-abc8-4cfe-a221-b1d830f8b0c3

📥 Commits

Reviewing files that changed from the base of the PR and between 242b310 and 1db74fd.

📒 Files selected for processing (1)
  • config/openshift/kustomization.yaml

Comment thread config/openshift/kustomization.yaml
Comment thread config/openshift/kustomization.yaml
Copy link
Copy Markdown

@vivekk16 vivekk16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zdtsw params.env file is missing. Please add it.

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Copy link
Copy Markdown

@vivekk16 vivekk16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@zdtsw zdtsw requested a review from anishasthana April 7, 2026 05:51
@zdtsw
Copy link
Copy Markdown
Member Author

zdtsw commented Apr 7, 2026

need ack on this one @anishasthana

@zdtsw zdtsw added the odh-3.4.0 label Apr 7, 2026
@anishasthana anishasthana merged commit e11a732 into opendatahub-io:main Apr 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants