Skip to content

Added new controller to monitor infra to validate fd usage #1508

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

Draft
vr4manta wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Added new controller to monitor infra to validate fd usage #1508

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
57 changes: 42 additions & 15 deletions cmd/vsphere/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@ import (
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/util/feature"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/component-base/featuregate"
"k8s.io/klog/v2"
ipamv1beta1 "sigs.k8s.io/cluster-api/api/ipam/v1beta1" //nolint:staticcheck
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/config"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/healthz"
Expand All @@ -25,6 +28,7 @@ import (
machinev1 "github.com/openshift/api/machine/v1beta1"
"github.com/openshift/library-go/pkg/config/leaderelection"
"github.com/openshift/library-go/pkg/features"
"github.com/openshift/machine-api-operator/pkg/controller/infrastructurevalidation"
capimachine "github.com/openshift/machine-api-operator/pkg/controller/machine"
"github.com/openshift/machine-api-operator/pkg/controller/vsphere"
machine "github.com/openshift/machine-api-operator/pkg/controller/vsphere"
Expand Down Expand Up @@ -122,7 +126,29 @@ func main() {
LeaseDuration: metav1.Duration{Duration: *leaderElectLeaseDuration},
})

// Setup Scheme for all resources - must be done before creating manager
// so that cache configuration can reference these types
scheme := runtime.NewScheme()

// Register core Kubernetes types (Secret, ConfigMap, etc.)
if err := clientgoscheme.AddToScheme(scheme); err != nil {
klog.Fatalf("unable to add client-go scheme: %v", err)
}

if err := configv1.Install(scheme); err != nil {
klog.Fatalf("unable to add configv1 to scheme: %v", err)
}

if err := machinev1.Install(scheme); err != nil {
klog.Fatalf("unable to add machinev1 to scheme: %v", err)
}

if err := ipamv1beta1.AddToScheme(scheme); err != nil {
klog.Fatalf("unable to add ipamv1beta1 to scheme: %v", err)
}

opts := manager.Options{
Scheme: scheme,
Metrics: server.Options{
BindAddress: *metricsAddress,
},
Expand All @@ -145,6 +171,15 @@ func main() {
klog.Infof("Watching machine-api objects only in namespace %q for reconciliation.", *watchNamespace)
}

// Infrastructure and ClusterOperator are cluster-scoped resources that need to be watched
// regardless of namespace restrictions.
// Machines also need to be watched cluster-wide for infrastructure validation.
opts.Cache.ByObject = map[client.Object]cache.ByObject{
&configv1.Infrastructure{}: {},
&configv1.ClusterOperator{}: {},
&machinev1.Machine{}: {}, // Watch machines cluster-wide for infrastructure validation
}

// Sets feature gates from flags
klog.Infof("Initializing feature gates: %s", strings.Join(defaultMutableGate.KnownFeatures(), ", "))
warnings, err := gateOpts.ApplyTo(defaultMutableGate)
Expand Down Expand Up @@ -182,21 +217,8 @@ func main() {
OpenshiftConfigNamespace: vsphere.OpenshiftConfigManagedNamespace,
})

if err := configv1.Install(mgr.GetScheme()); err != nil {
klog.Fatal(err)
}

if err := machinev1.Install(mgr.GetScheme()); err != nil {
klog.Fatal(err)
}

if err := machinev1.Install(mgr.GetScheme()); err != nil {
klog.Fatal(err)
}

if err := ipamv1beta1.AddToScheme(mgr.GetScheme()); err != nil {
klog.Fatalf("unable to add ipamv1beta1 to scheme: %v", err)
}
// Scheme was already installed before manager creation (see above)
// This allows cache configuration to reference these types

if err := capimachine.AddWithActuator(mgr, machineActuator, defaultMutableGate); err != nil {
klog.Fatal(err)
Expand All @@ -211,6 +233,11 @@ func main() {
os.Exit(1)
}

// Setup Infrastructure validation controller
if err := infrastructurevalidation.Add(mgr, opts); err != nil {
klog.Fatalf("Failed to add infrastructure validation controller: %v", err)
}

if err := mgr.AddReadyzCheck("ping", healthz.Ping); err != nil {
klog.Fatal(err)
}
Expand Down
11 changes: 11 additions & 0 deletions install/0000_30_machine-api-operator_09_rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,17 @@ rules:
- list
- watch

- apiGroups:
- config.openshift.io
resources:
- clusteroperators
- clusteroperators/status
verbs:
- get
- list
- watch
- update
Comment on lines +291 to +300

@coderabbitai coderabbitai Bot Jun 9, 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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Split this rule so update only applies to clusteroperators/status.

The controller reads clusteroperators and writes through Status().Update, but this single rule also grants update on the parent clusteroperators resource because verbs apply to every resource in the list. That is broader cluster-level write access than the new controller needs.

🔐 Suggested RBAC split 
-  - apiGroups:
-      - config.openshift.io
-    resources:
-      - clusteroperators
-      - clusteroperators/status
-    verbs:
-      - get
-      - list
-      - watch
-      - update
+  - apiGroups:
+      - config.openshift.io
+    resources:
+      - clusteroperators
+    verbs:
+      - get
+      - list
+      - watch
+  - apiGroups:
+      - config.openshift.io
+    resources:
+      - clusteroperators/status
+    verbs:
+      - update

As per coding guidelines, "RBAC: least privilege; no cluster-admin for workloads."

📝 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
- apiGroups:
- config.openshift.io
resources:
- clusteroperators
- clusteroperators/status
verbs:
- get
- list
- watch
- update
- apiGroups:
- config.openshift.io
resources:
- clusteroperators
verbs:
- get
- list
- watch
- apiGroups:
- config.openshift.io
resources:
- clusteroperators/status
verbs:
- update
🤖 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 `@install/0000_30_machine-api-operator_09_rbac.yaml` around lines 291 - 300,
Split the single RBAC rule into two rules under apiGroups: config.openshift.io:
one rule that lists resources: clusteroperators with verbs get, list, watch (no
update) to grant read access, and a second rule that lists resource:
clusteroperators/status with verbs update (and optionally get/list/watch if the
controller reads status) so that the update verb only applies to the status
subresource; locate the existing block referencing "clusteroperators" and
"clusteroperators/status" and replace it with these two narrower rules.

Source: Coding guidelines


---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
Expand Down
Loading