Skip to content

fix: avoid maas-controller env value + valueFrom kustomize merge#657

Open
jland-redhat wants to merge 2 commits intoopendatahub-io:mainfrom
jland-redhat:cache-fix
Open

fix: avoid maas-controller env value + valueFrom kustomize merge#657
jland-redhat wants to merge 2 commits intoopendatahub-io:mainfrom
jland-redhat:cache-fix

Conversation

@jland-redhat
Copy link
Copy Markdown
Contributor

@jland-redhat jland-redhat commented Apr 1, 2026

Suggested title: fix(deploy): avoid maas-controller env value + valueFrom kustomize merge

Description

Kubernetes rejects EnvVar entries that set both value and valueFrom. The ODH overlay patched METADATA_CACHE_TTL and AUTHZ_CACHE_TTL with valueFrom (ConfigMap) while the base Deployment still had literal value: "60". Strategic merge merges by env name and kept both fields.
This change:

  • Base (deployment/base/maas-controller/manager/manager.yaml): Removes the TTL entries from env and sets TTLs via literal manager flags (--metadata-cache-ttl=60, --authz-cache-ttl=60) so standalone kustomize build / kubectl apply -k deployment/base/maas-controller/default still applies valid TTLs (unlike leaving $(METADATA_CACHE_TTL) in args with no env, which breaks flag parsing).
  • ODH (deployment/overlays/odh/kustomization.yaml): Extends the maas-controller patch with a full args list that uses $(METADATA_CACHE_TTL) / $(AUTHZ_CACHE_TTL) and env that only uses valueFrom to maas-parameters, matching params.env keys (metadata-cache-ttl, authz-cache-ttl).
    Non-ODH overlays inherit the base literal TTL args; no duplicate overlay patches were added.

How Has This Been Tested?

Not tested yet; validation in progress.
Planned checks:

  • kustomize build deployment/overlays/odh and confirm maas-controller env for METADATA_CACHE_TTL / AUTHZ_CACHE_TTL have only valueFrom (no value).
  • kustomize build deployment/base/maas-controller/default (and other overlays that include maas-controller) and confirm TTLs appear as literal 60 in args where expected.
  • Optional: apply the ODH bundle in a test cluster and confirm the Deployment is accepted and the controller starts.

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

  • Chores
    • Updated deployment configuration for the MAAS controller manager: replaced templated cache TTLs with fixed values (60s), added explicit runtime arguments for leader election and health/metrics probes, and made configuration entries for cache TTLs required. These changes adjust runtime behavior and configuration sourcing for the manager component.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 1, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jland-redhat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Apr 1, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: aa5d8465-c11c-432a-a79e-6e480d6243e0

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9a89a and e2a88b6.

📒 Files selected for processing (1)
  • deployment/overlays/odh/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • deployment/overlays/odh/kustomization.yaml

📝 Walkthrough

Walkthrough

The PR updates Kubernetes manifests for the maas-controller. In the base Deployment the manager container's cache TTL args were changed from templated env references to hardcoded --metadata-cache-ttl=60 and --authz-cache-ttl=60, and the corresponding METADATA_CACHE_TTL/AUTHZ_CACHE_TTL env entries were removed. The kustomize overlay adds an explicit args list to the manager container (leader election, health probe bind address, gateway/namespace, MAAS API/subscription namespaces, cluster audience, and cache TTLs) and changes configMapKeyRef.optional for the TTL keys from true to false.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Security Findings

  • CWE-15: External Control of System or Configuration Setting — The base manifest hardcodes cache TTLs while the overlay reintroduces env-driven TTL values. Action: consolidate to a single authoritative source (prefer overlay values) and remove contradictory hardcoded defaults from base or document precedence clearly in deployment docs.

  • CWE-703: Improper Check or Handling of Exceptional Conditions — Changing configMapKeyRef.optional from true to false makes those ConfigMap keys required; missing keys will cause pod creation failures. Action: ensure the ConfigMap with required keys is created in all target environments, or provide safe defaults and readiness/validation checks to avoid crash-on-missing-config.

  • Operational risk (configuration drift) — Duplicate/conflicting configuration across base and overlay increases risk of unexpected runtime behavior. Action: choose one pattern (config-driven or default-in-base) and update manifests and CI/deployment checks to enforce it.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the core issue: avoiding a Kubernetes EnvVar merge conflict caused by both value and valueFrom fields being present in the same environment variable.

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


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@deployment/overlays/odh/kustomization.yaml`:
- Around line 64-73: The METADATA_CACHE_TTL and AUTHZ_CACHE_TTL environment
variable ConfigMap references are marked optional:true causing empty
substitutions and invalid container args (--metadata-cache-ttl= /
--authz-cache-ttl=); change the ConfigMapKeyRef entries that supply
METADATA_CACHE_TTL and AUTHZ_CACHE_TTL to optional:false so the Pod will fail to
create (CreateContainerConfigError) when those keys are missing and
deployment-time validation enforces valid TTL args for the container.
🪄 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: b6384a07-00c8-4fdb-a0ff-5099aa10e5d0

📥 Commits

Reviewing files that changed from the base of the PR and between 409b0a7 and 2d9a89a.

📒 Files selected for processing (2)
  • deployment/base/maas-controller/manager/manager.yaml
  • deployment/overlays/odh/kustomization.yaml

@jland-redhat
Copy link
Copy Markdown
Contributor Author

Validated:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant