Add namespace predicate and fix RBAC for HostLease controller - #3
Conversation
WalkthroughThis PR extends the HostLeaseReconciler to operate within a single configurable Kubernetes namespace. The controller now reads Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/hostlease_controller.go`:
- Around line 307-313: NamespacePredicate currently returns a predicate that
only matches objects whose namespace equals the provided namespace string, which
silently disables reconciliation when HOSTLEASE_NAMESPACE is empty; change
NamespacePredicate to detect an empty namespace and either (a) return a
predicate that always allows all objects (e.g., use
predicate.LabelChangedPredicate/TruePredicate equivalent) so the controller
watches all namespaces when namespace == "", or (b) alternatively validate the
env earlier by adding a check in NewHostLeaseReconciler to fail startup with a
clear error if the namespace is required; update NamespacePredicate (and/or
NewHostLeaseReconciler) accordingly and add a unit/test or startup log to make
the behavior explicit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8afc9d5-fe59-4704-bdd7-b15fe02c24c9
📒 Files selected for processing (5)
cmd/main.goconfig/rbac/role.yamlinternal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/controller/hostlease_names.go
163e718 to
b2ac229
Compare
tzumainn
left a comment
tzumainn
left a comment
There was a problem hiding this comment.
Thanks for making this change! Two comments:
- I think you may want to update
config/manager/manager.yamlsimilar to https://github.com/osac-project/osac-operator/blob/main/config/manager/manager.yaml#L77-L80; that should ensure that a deployment uses the non-default namespace - I'd move
EnvHostLeaseNamespaceout ofhostlease_names.goand intocmd/main.go. It has nothing to do with the controller, and is only used bymain.go.
b2ac229 to
39a65c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/hostlease_controller_test.go (1)
107-113: ⚡ Quick winAssert namespace behavior in constructor tests.
These tests now pass a namespace but only verify interval. Add assertions for
r.Namespace(provided value and empty-value default path) so the new scoping behavior is covered.Targeted test additions
It("should use the provided recheck interval when positive", func() { r := NewHostLeaseReconciler(nil, testScheme, mockIronic, "test-ns", 30*time.Second) + Expect(r.Namespace).To(Equal("test-ns")) Expect(r.RecheckInterval).To(Equal(30 * time.Second)) }) It("should use the default recheck interval when zero", func() { r := NewHostLeaseReconciler(nil, testScheme, mockIronic, "test-ns", 0) + Expect(r.Namespace).To(Equal("test-ns")) Expect(r.RecheckInterval).To(Equal(DefaultRecheckInterval)) }) + + It("should use default namespace when namespace is empty", func() { + r := NewHostLeaseReconciler(nil, testScheme, mockIronic, "", 0) + Expect(r.Namespace).To(Equal(defaultHostLeaseNamespace)) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/hostlease_controller_test.go` around lines 107 - 113, The constructor tests currently only verify RecheckInterval; update both cases that call NewHostLeaseReconciler to also assert the reconciler's Namespace field: when calling NewHostLeaseReconciler(nil, testScheme, mockIronic, "test-ns", 30*time.Second) expect r.Namespace to equal "test-ns", and when calling it with an empty namespace string (0 interval case) expect r.Namespace to equal the default/empty behavior (e.g., DefaultNamespace or "" depending on constructor semantics) so the new scoping behavior is covered; reference NewHostLeaseReconciler and r.Namespace when adding these assertions and keep existing RecheckInterval checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/manager/manager.yaml`:
- Around line 68-72: The manifest currently hard-sets HOSTLEASE_NAMESPACE via
env.fieldRef to metadata.namespace, forcing the manager into the Pod namespace;
remove this env entry so the manager's constructor fallback can determine the
intended HostLease scope. Locate the env block that defines HOSTLEASE_NAMESPACE
(the env -> - name: HOSTLEASE_NAMESPACE -> valueFrom -> fieldRef -> fieldPath:
metadata.namespace) and delete that whole env variable (or replace it with an
explicit empty/default value if you need an explicit unset), leaving the manager
code to read configuration or fall back as implemented.
---
Nitpick comments:
In `@internal/controller/hostlease_controller_test.go`:
- Around line 107-113: The constructor tests currently only verify
RecheckInterval; update both cases that call NewHostLeaseReconciler to also
assert the reconciler's Namespace field: when calling
NewHostLeaseReconciler(nil, testScheme, mockIronic, "test-ns", 30*time.Second)
expect r.Namespace to equal "test-ns", and when calling it with an empty
namespace string (0 interval case) expect r.Namespace to equal the default/empty
behavior (e.g., DefaultNamespace or "" depending on constructor semantics) so
the new scoping behavior is covered; reference NewHostLeaseReconciler and
r.Namespace when adding these assertions and keep existing RecheckInterval
checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2364f045-854e-49d7-9ea2-5d197fe41c5c
📒 Files selected for processing (6)
cmd/main.goconfig/manager/manager.yamlconfig/rbac/role.yamlinternal/controller/hostlease_controller.gointernal/controller/hostlease_controller_test.gointernal/controller/hostlease_names.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/hostlease_names.go
🚧 Files skipped from review as they are similar to previous changes (2)
- config/rbac/role.yaml
- internal/controller/hostlease_controller.go
| env: | ||
| - name: HOSTLEASE_NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace |
There was a problem hiding this comment.
Avoid hard-binding HostLease scope to the manager Pod namespace.
Line 69 forces HOSTLEASE_NAMESPACE to metadata.namespace (system here), which bypasses constructor fallback and can silently exclude HostLeases if they live in another intended namespace.
Suggested adjustment
- env:
- - name: HOSTLEASE_NAMESPACE
- valueFrom:
- fieldRef:
- fieldPath: metadata.namespace
+ # Keep unset to use controller default, or set explicitly via overlay/config.
+ env: []📝 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.
| env: | |
| - name: HOSTLEASE_NAMESPACE | |
| valueFrom: | |
| fieldRef: | |
| fieldPath: metadata.namespace | |
| # Keep unset to use controller default, or set explicitly via overlay/config. | |
| env: [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/manager/manager.yaml` around lines 68 - 72, The manifest currently
hard-sets HOSTLEASE_NAMESPACE via env.fieldRef to metadata.namespace, forcing
the manager into the Pod namespace; remove this env entry so the manager's
constructor fallback can determine the intended HostLease scope. Locate the env
block that defines HOSTLEASE_NAMESPACE (the env -> - name: HOSTLEASE_NAMESPACE
-> valueFrom -> fieldRef -> fieldPath: metadata.namespace) and delete that whole
env variable (or replace it with an explicit empty/default value if you need an
explicit unset), leaving the manager code to read configuration or fall back as
implemented.
There was a problem hiding this comment.
Following the pattern here https://github.com/osac-project/osac-operator/blob/main/config/manager/manager.yaml#L77-L80. HostLeases in other namespaces should be excluded as intended.
There was a problem hiding this comment.
@DanNiESh, understood — thanks for the context. Using the Kubernetes Downward API to inject metadata.namespace as an env var is a well-established pattern (also used by the Operator SDK, OLM, and others) to namespace-scope a controller to its own deployment namespace. Since the explicit design goal is for the controller to only reconcile HostLeases in its own namespace (mirroring the osac-operator pattern), the current implementation is correct. I'll withdraw my earlier concern.
✏️ Learnings added
Learnt from: DanNiESh
Repo: osac-project/host-management-openstack PR: 3
File: config/manager/manager.yaml:68-72
Timestamp: 2026-05-12T14:21:18.642Z
Learning: In `config/manager/manager.yaml` of the `host-management-openstack` project, the `HOSTLEASE_NAMESPACE` env var is intentionally set via Kubernetes Downward API (`fieldRef: fieldPath: metadata.namespace`) to scope the HostLease controller to its own Pod namespace. This mirrors the pattern used by `osac-operator`. Excluding HostLeases in other namespaces is the intended behavior — do not flag this as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Done! |
| mgr.GetClient(), | ||
| mgr.GetScheme(), | ||
| ironicClient, | ||
| os.Getenv(envHostLeaseNamespace), |
There was a problem hiding this comment.
The HOSTLEASE_NAMESPACE is set in config/manager/manager.yaml. Does that guarantee that the environment variable is always set?
There was a problem hiding this comment.
On the one hand, osac-operator has a default. On the other hand, I've never thought the default made much sense, since we never use it. I guess there could be an argument for having defaults in an environment where we only expect to install a single OSAC?
| return obj.GetNamespace() == namespace | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Using a namespace predicate works, but we can do better. See larsks@af76bdc, which shows an alternative approach that is more efficient in terms of memory usage and api event processing. If that seems like a reasonable approach we can update this PR.
There was a problem hiding this comment.
I think watching namespace using cache is reasonble.
Use controller-runtime cache configuration to restrict the HostLease watch to a single namespace (from HOSTLEASE_NAMESPACE env var) instead of using a namespace predicate. This reduces memory usage and event notifications since the controller no longer watches HostLease resources across all namespaces. Also adds RBAC for pods and configures the HOSTLEASE_NAMESPACE env var in the manager deployment via downward API. Co-Authored-By: Lars Kellogg-Stedman <lars@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
39a65c7 to
31236ea
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/main.go (1)
186-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hard-failing when
HOSTLEASE_NAMESPACEis unset; use the documented default.Line 186 to Line 190 exits the process on empty env var, but this PR’s stated behavior is to default to
osac-hostleases. Current behavior can cause avoidable startup failures when env injection is misconfigured.Suggested patch
-import ( +import ( "crypto/tls" "flag" - "fmt" "os" "path/filepath" @@ -const envHostLeaseNamespace = "HOSTLEASE_NAMESPACE" +const ( + envHostLeaseNamespace = "HOSTLEASE_NAMESPACE" + defaultHostLeaseNamespace = "osac-hostleases" +) @@ controllerNamespace := os.Getenv(envHostLeaseNamespace) if controllerNamespace == "" { - setupLog.Error(nil, fmt.Sprintf("%s environment variable must be set", envHostLeaseNamespace)) - os.Exit(1) + controllerNamespace = defaultHostLeaseNamespace + setupLog.Info("HOSTLEASE_NAMESPACE not set; using default namespace", "namespace", controllerNamespace) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/main.go` around lines 186 - 190, The code currently exits when controllerNamespace := os.Getenv(envHostLeaseNamespace) is empty; instead, set controllerNamespace to the documented default "osac-hostleases" if the env var is unset and log a warning rather than calling setupLog.Error(nil, ...) and os.Exit(1); update the logic around controllerNamespace and the envHostLeaseNamespace usage so it assigns "osac-hostleases" when empty and emits a clear warning via setupLog (e.g., setupLog.Info or setupLog.Warn) mentioning the default being used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmd/main.go`:
- Around line 186-190: The code currently exits when controllerNamespace :=
os.Getenv(envHostLeaseNamespace) is empty; instead, set controllerNamespace to
the documented default "osac-hostleases" if the env var is unset and log a
warning rather than calling setupLog.Error(nil, ...) and os.Exit(1); update the
logic around controllerNamespace and the envHostLeaseNamespace usage so it
assigns "osac-hostleases" when empty and emits a clear warning via setupLog
(e.g., setupLog.Info or setupLog.Warn) mentioning the default being used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70a987e1-c0f1-450c-881b-379091e03fca
📒 Files selected for processing (4)
cmd/main.goconfig/manager/manager.yamlconfig/rbac/role.yamlinternal/controller/hostlease_controller_test.go
✅ Files skipped from review due to trivial changes (2)
- config/manager/manager.yaml
- internal/controller/hostlease_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- config/rbac/role.yaml
|
Squashed @larsks commit into this commit. |
3 participants
Summary
matching the pattern used by
osac-operator. The namespace is configured via theHOSTLEASE_NAMESPACEenvironment variable, defaulting to
osac-hostleaseswhen not set.required for finalizer management.
Summary by CodeRabbit
New Features
Chores