Skip to content

fix: replace namespace selector in servicemonitor to match deployment#42

Merged
github-actions[bot] merged 2 commits intoopendatahub-io:mainfrom
zdtsw-forking:fix_3.4_ea2
Mar 27, 2026
Merged

fix: replace namespace selector in servicemonitor to match deployment#42
github-actions[bot] merged 2 commits intoopendatahub-io:mainfrom
zdtsw-forking:fix_3.4_ea2

Conversation

@zdtsw
Copy link
Copy Markdown
Member

@zdtsw zdtsw commented Mar 27, 2026

namespace

Description

ref red-hat-data-services/rhods-operator#22298
ref llm-d#941

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

  • Chores

    • Updated controller deployment so it no longer passes a runtime namespace flag by default, making it observe all namespaces unless explicitly restricted.
    • Ensured metrics monitoring picks up the controller’s configured namespace automatically.
  • Documentation

    • Clarified that multi-namespace (watch-all) is the default for the provided deployment and updated troubleshooting guidance for single-namespace configuration.

namespace

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

coderabbitai Bot commented Mar 27, 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: dac5d8e7-5b27-43f3-b7d8-fb9eee471e67

📥 Commits

Reviewing files that changed from the base of the PR and between af3a266 and f2dc438.

📒 Files selected for processing (2)
  • docs/user-guide/namespace-management.md
  • docs/user-guide/troubleshooting.md
✅ Files skipped from review due to trivial changes (2)
  • docs/user-guide/namespace-management.md
  • docs/user-guide/troubleshooting.md

📝 Walkthrough

Walkthrough

Removed the --watch-namespace=$(POD_NAMESPACE) container arg from the controller-manager Deployment and added a Kustomize replacements entry that copies metadata.namespace from the Deployment named controller-manager into spec.namespaceSelector.matchNames[0] of the ServiceMonitor named controller-manager-metrics-monitor. Documentation updates declare that the default kustomize overlay at config/default deploys the controller in multi-namespace (all namespaces) mode by default and update troubleshooting guidance to reflect that omission of --watch-namespace means the controller watches all namespaces.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Critical Issues

  • Behavioral scope change (watch-all namespaces): Removing --watch-namespace=$(POD_NAMESPACE) expands the controller’s observed scope to all namespaces by default. Verify RBAC and privilege boundaries; unchecked scope expansion is an access control risk (CWE-269, Improper Privilege Management). Action: confirm ClusterRole/RoleBindings and ensure only intended privileges are granted.

  • Potential improper access control: Broadened watch scope can expose resources the controller should not touch. Audit authorization rules and admission controls to ensure the controller cannot escalate or modify unintended objects (CWE-284, Improper Access Control). Action: restrict RBAC rules or reintroduce explicit namespace scoping where required.

  • Implicit Kustomize coupling and drift risk: The new replacement statically binds ServiceMonitor namespace to the Deployment namespace at build time; if namespaces diverge post-install, metrics scraping may break silently. Action: document the coupling in repository README/overlay docs and consider runtime validation (e.g., admission/CI checks) to detect mismatch.

🚥 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 primary change: replacing the namespace selector in ServiceMonitor to match the deployment namespace, which aligns with the kustomization.yaml additions and manager.yaml modification.

✏️ 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

@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
Kindly update the troubleshooting.md and namespace-management.md in docs/user-guide clarifying that the default kustomize deployment now watches all namespaces, and --watch-namespace is only needed if you want to restrict to a single namespace.

- --leader-elect=true
- --health-probe-bind-address=:8081
- --watch-namespace=$(POD_NAMESPACE)
# Leader election timeout configuration (optional - defaults shown below)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is ok for downstream, for upstream, the default is to watch just the namespace

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

zdtsw commented Mar 27, 2026

/lgtm /approve

@zdtsw Kindly update the troubleshooting.md and namespace-management.md in docs/user-guide clarifying that the default kustomize deployment now watches all namespaces, and --watch-namespace is only needed if you want to restrict to a single namespace.

updated

@github-actions github-actions Bot added the lgtm label Mar 27, 2026
@zdtsw
Copy link
Copy Markdown
Member Author

zdtsw commented Mar 27, 2026

ok, lets keep this only for ODH and RHDS for now
i've updated docs as well, and will bring this to RHDS ea2 for now

@github-actions github-actions Bot merged commit 1d19622 into opendatahub-io:main Mar 27, 2026
16 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