Skip to content

Add namespace predicate and fix RBAC for HostLease controller #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tzumainn merged 1 commit into from
May 13, 2026
+65 −30
Merged

Add namespace predicate and fix RBAC for HostLease controller #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package main
import (
"crypto/tls"
"flag"
"fmt"
"os"
"path/filepath"

Expand All @@ -30,7 +31,9 @@ import (
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/certwatcher"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
"sigs.k8s.io/controller-runtime/pkg/metrics/filters"
Expand All @@ -43,6 +46,8 @@ import (
// +kubebuilder:scaffold:imports
)

const envHostLeaseNamespace = "HOSTLEASE_NAMESPACE"

var (
scheme = runtime.NewScheme()
setupLog = ctrl.Log.WithName("setup")
Expand Down Expand Up @@ -178,13 +183,28 @@ func main() {
})
}

controllerNamespace := os.Getenv(envHostLeaseNamespace)
if controllerNamespace == "" {
setupLog.Error(nil, fmt.Sprintf("%s environment variable must be set", envHostLeaseNamespace))
os.Exit(1)
}

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
Metrics: metricsServerOptions,
WebhookServer: webhookServer,
HealthProbeBindAddress: probeAddr,
LeaderElection: enableLeaderElection,
LeaderElectionID: "6f134d35.osac.openshift.io",
Cache: cache.Options{
ByObject: map[client.Object]cache.ByObject{
&v1alpha1.HostLease{}: {
Namespaces: map[string]cache.Config{
controllerNamespace: {},
},
},
},
},
// LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily
// when the Manager ends. This requires the binary to immediately end when the
// Manager is stopped, otherwise, this setting is unsafe. Setting this significantly
Expand Down
5 changes: 5 additions & 0 deletions config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ spec:
- --health-probe-bind-address=:8081
image: controller:latest
name: manager
env:
- name: HOSTLEASE_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
Comment on lines +68 to +72

@coderabbitai coderabbitai Bot May 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

@DanNiESh DanNiESh May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coderabbitai coderabbitai Bot May 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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.

ports: []
securityContext:
allowPrivilegeEscalation: false
Expand Down
5 changes: 4 additions & 1 deletion config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ rules:
verbs: ["get", "list", "watch"]
- apiGroups: ["osac.openshift.io"]
resources: ["hostleases"]
verbs: ["get", "list", "watch"]
verbs: ["get", "list", "watch", "update", "patch"]
- apiGroups: ["osac.openshift.io"]
resources: ["hostleases/status"]
verbs: ["get", "update", "patch"]
- apiGroups: ["osac.openshift.io"]
resources: ["hostleases/finalizers"]
verbs: ["update", "patch"]
65 changes: 36 additions & 29 deletions internal/controller/hostlease_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

const (
testPowerOff = "power off"
testPowerOn = "power on"
testNodeID = "node-1"
testNamespace = "default"
)

// mockIronicClient implements ironic.NodeClient for testing.
type mockIronicClient struct {
getNodeFunc func(ctx context.Context, nodeID string) (*nodes.Node, error)
Expand All @@ -53,7 +60,7 @@ func (m *mockIronicClient) GetNode(ctx context.Context, nodeID string) (*nodes.N
if m.getNodeFunc != nil {
return m.getNodeFunc(ctx, nodeID)
}
return &nodes.Node{PowerState: "power off"}, nil
return &nodes.Node{PowerState: testPowerOff}, nil
}

func (m *mockIronicClient) SetPowerState(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand Down Expand Up @@ -112,7 +119,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "",
HostClass: "openstack",
HostClass: hostClass,
},
}
Expect(reconciler.validateOpenStackHost(hostLease, log)).To(BeFalse())
Expand All @@ -121,7 +128,7 @@ var _ = Describe("HostLeaseReconciler", func() {
It("should return false when HostClass does not match", func() {
hostLease := &v1alpha1.HostLease{
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-1",
ExternalHostID: testNodeID,
HostClass: "other",
},
}
Expand All @@ -131,8 +138,8 @@ var _ = Describe("HostLeaseReconciler", func() {
It("should return true when ExternalHostID and HostClass are valid", func() {
hostLease := &v1alpha1.HostLease{
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-1",
HostClass: "openstack",
ExternalHostID: testNodeID,
HostClass: hostClass,
},
}
Expect(reconciler.validateOpenStackHost(hostLease, log)).To(BeTrue())
Expand All @@ -149,15 +156,15 @@ var _ = Describe("HostLeaseReconciler", func() {
ctx = context.Background()
hostLease = &v1alpha1.HostLease{
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-1",
HostClass: "openstack",
ExternalHostID: testNodeID,
HostClass: hostClass,
},
}
})

It("should power on when desired on and currently off", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power off"}
node := &nodes.Node{PowerState: testPowerOff}

var calledTarget ironic.TargetPowerState
mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand All @@ -172,7 +179,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should power off when desired off and currently on", func() {
hostLease.Spec.PoweredOn = boolPtr(false)
node := &nodes.Node{PowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOn}

var calledTarget ironic.TargetPowerState
mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand All @@ -187,7 +194,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should not call SetPowerState when power state already matches (on)", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOn}

called := false
mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand All @@ -202,7 +209,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should not call SetPowerState when power state already matches (off)", func() {
hostLease.Spec.PoweredOn = boolPtr(false)
node := &nodes.Node{PowerState: "power off"}
node := &nodes.Node{PowerState: testPowerOff}

called := false
mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand All @@ -217,15 +224,15 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should not be called when PoweredOn is nil (guarded by Reconcile)", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOn}

err := reconciler.reconcilePower(ctx, hostLease, node, log)
Expect(err).NotTo(HaveOccurred())
})

It("should skip SetPowerState when node is transitioning", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power off", TargetPowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOff, TargetPowerState: testPowerOn}

called := false
mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
Expand All @@ -240,7 +247,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should return error when SetPowerState fails on power on", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power off"}
node := &nodes.Node{PowerState: testPowerOff}

mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
return errors.New("ironic API error")
Expand All @@ -253,7 +260,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should return error when SetPowerState fails on power off", func() {
hostLease.Spec.PoweredOn = boolPtr(false)
node := &nodes.Node{PowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOn}

mockIronic.setPowerStateFunc = func(ctx context.Context, nodeID string, target ironic.TargetPowerState) error {
return errors.New("ironic API error")
Expand All @@ -271,7 +278,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-delete",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{hostLeaseFinalizer},
DeletionTimestamp: &now,
},
Expand Down Expand Up @@ -324,7 +331,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-delete-non-openstack",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{hostLeaseFinalizer},
DeletionTimestamp: &now,
},
Expand Down Expand Up @@ -360,7 +367,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-delete-openstack-no-externalid",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{hostLeaseFinalizer},
DeletionTimestamp: &now,
},
Expand Down Expand Up @@ -397,7 +404,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-add-finalizer",
Namespace: "default",
Namespace: testNamespace,
},
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-finalizer",
Expand Down Expand Up @@ -437,13 +444,13 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-sample",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{
hostLeaseFinalizer,
},
},
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-1",
ExternalHostID: testNodeID,
HostClass: hostClass,
PoweredOn: nil,
},
Expand Down Expand Up @@ -494,7 +501,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-managed",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{
hostLeaseFinalizer,
},
Expand Down Expand Up @@ -550,7 +557,7 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease := &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-requeue",
Namespace: "default",
Namespace: testNamespace,
Finalizers: []string{
hostLeaseFinalizer,
},
Expand Down Expand Up @@ -679,11 +686,11 @@ var _ = Describe("HostLeaseReconciler", func() {
hostLease = &v1alpha1.HostLease{
ObjectMeta: metav1.ObjectMeta{
Name: "hostlease-sync",
Namespace: "default",
Namespace: testNamespace,
},
Spec: v1alpha1.HostLeaseSpec{
ExternalHostID: "node-1",
HostClass: "openstack",
ExternalHostID: testNodeID,
HostClass: hostClass,
},
}
reconciler.Client = fake.NewClientBuilder().
Expand All @@ -706,7 +713,7 @@ var _ = Describe("HostLeaseReconciler", func() {
})

It("should set phase to Ready and PowerSynced to True when node is on", func() {
node := &nodes.Node{PowerState: "power on"}
node := &nodes.Node{PowerState: testPowerOn}
err := reconciler.syncHostLeaseStatus(context.Background(), hostLease, node, nil, log)
Expect(err).NotTo(HaveOccurred())

Expand All @@ -721,7 +728,7 @@ var _ = Describe("HostLeaseReconciler", func() {
})

It("should set phase to Ready and PowerSynced to True when node is off", func() {
node := &nodes.Node{PowerState: "power off"}
node := &nodes.Node{PowerState: testPowerOff}
err := reconciler.syncHostLeaseStatus(context.Background(), hostLease, node, nil, log)
Expect(err).NotTo(HaveOccurred())

Expand All @@ -737,7 +744,7 @@ var _ = Describe("HostLeaseReconciler", func() {

It("should set phase to Progressing when power state does not match desired", func() {
hostLease.Spec.PoweredOn = boolPtr(true)
node := &nodes.Node{PowerState: "power off"}
node := &nodes.Node{PowerState: testPowerOff}
err := reconciler.syncHostLeaseStatus(context.Background(), hostLease, node, nil, log)
Expect(err).NotTo(HaveOccurred())

Expand Down
Loading