Updated rbac based on the vmfr project #2040
Conversation
WalkthroughAdds VM File Restore manifests, RBAC roles/bindings, sample CRs, and a Makefile target to import/service image-set VMFR manifests; adjusts manager RBAC names and permissions and updates build-must-gather behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml (1)
1-9: Add field documentation or examples to the sample.The spec is empty with only a TODO comment. This sample should include representative field examples or at minimum commented documentation of required fields so users understand how to configure a VirtualMachineFileRestore resource.
Consider adding example fields and inline documentation:
spec: - # TODO(user): Add fields here + # Example fields (customize as needed): + # sourceVM: my-vm + # targetPath: /data + # restoreType: filesystemAlternatively, search the VirtualMachineFileRestore CRD definition to populate this with actual required/recommended fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (34)
Makefile(1 hunks)bundle/manifests/controller-manager_v1_serviceaccount.yaml(1 hunks)bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml(1 hunks)bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml(1 hunks)bundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-operator.clusterserviceversion.yaml(2 hunks)bundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vmfr-controller-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml(1 hunks)bundle/manifests/oadp-vmfr-controller_v1_serviceaccount.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml(1 hunks)config/vm-file-restore-controller_rbac/kustomization.yaml(1 hunks)config/vm-file-restore-controller_rbac/leader_election_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_reader_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/role.yaml(2 hunks)config/vm-file-restore-controller_rbac/role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/service_account.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/samples/oadp_v1alpha1_virtualmachinefilerestore.yamlconfig/vm-file-restore-controller_rbac/metrics_auth_role_binding.yamlbundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yamlbundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yamlbundle/manifests/controller-manager_v1_serviceaccount.yamlbundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yamlconfig/vm-file-restore-controller_rbac/metrics_reader_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yamlconfig/vm-file-restore-controller_rbac/leader_election_role_binding.yamlbundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yamlbundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/kustomization.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yamlconfig/vm-file-restore-controller_rbac/role.yamlconfig/vm-file-restore-controller_rbac/service_account.yamlbundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/role_binding.yamlbundle/manifests/oadp-vmfr-controller-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yamlbundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vmfr-controller_v1_serviceaccount.yamlbundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/metrics_auth_role.yamlbundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/leader_election_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yamlMakefilebundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/samples/kustomization.yamlbundle/manifests/oadp-operator.clusterserviceversion.yaml
🪛 Checkov (3.2.334)
bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🔇 Additional comments (33)
bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: LGTM!The viewer ClusterRole is well-structured with appropriate read-only permissions. The two rules correctly grant access to the
virtualmachinefilerestoresresource and its status subresource, following standard Kubernetes RBAC conventions. Labels and naming are consistent with the oadp-vm-file-restore component.bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-10: Metrics reading permission is appropriately minimal.The ClusterRole grants read-only access to the /metrics endpoint via a nonResourceURL, which is the standard Prometheus metrics endpoint. This follows least privilege. Verify that a corresponding ClusterRoleBinding exists in the bundle.
config/vm-file-restore-controller_rbac/metrics_reader_role.yaml (1)
1-9: Metrics permission is appropriately scoped.The ClusterRole grants minimal read access to the /metrics endpoint, consistent with standard Prometheus metrics exposure. Ensure a corresponding RoleBinding or ClusterRoleBinding is configured to assign this role to the appropriate ServiceAccount.
config/vm-file-restore-controller_rbac/metrics_auth_role.yaml (1)
1-17: Metrics authentication permissions are appropriate but verify binding scope.The ClusterRole correctly grants create-only permissions on tokenreviews and subjectaccessreviews, which are necessary for metrics authentication proxies. Verify that the binding (metrics_auth_role_binding.yaml) correctly scopes this to the intended ServiceAccount and that no additional permissions are granted beyond what's shown here.
config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml (1)
1-15: Sample is well-documented with useful examples.The VirtualMachineBackupsDiscovery sample includes concrete field examples and helpful comments explaining optional fields and supported time formats. This provides good guidance to users.
bundle/manifests/controller-manager_v1_serviceaccount.yaml (1)
1-8: Clarify ServiceAccount naming vs. label inconsistency.The metadata.name is
controller-managerbut the labelapp.kubernetes.io/name: oadp-vm-file-restoresuggests this is specifically the VMFR controller. If there are multiple controllers in the same namespace, this generic name could cause confusion. Consider whether the name should be more specific (e.g.,oadp-vmfr-controller-manager) or clarify the labeling scheme across all related manifests.config/vm-file-restore-controller_rbac/leader_election_role.yaml (1)
1-40: Leader election permissions are appropriate.The Role grants standard leader-election verbs (get, list, watch, create, update, patch, delete) on configmaps, leases, and events, which is the expected permission set for Kubernetes leader election. Ensure a corresponding RoleBinding exists that binds this Role to the correct ServiceAccount.
config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml (1)
1-12: Verify the system namespace is correct for your deployment context.The ClusterRoleBinding binds to the
controller-managerServiceAccount in thesystemnamespace. Confirm this namespace matches where your controller-manager ServiceAccount is actually deployed in your OADP installation (it may beopenshift-adp,oadp-system, or similar depending on your operator's installation pattern).config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml (1)
13-15: Verify ServiceAccount "controller-manager" exists and is deployed to the correct namespace.The RoleBinding references a ServiceAccount named
controller-managerin thesystemnamespace. Ensure this ServiceAccount is created in the bundle manifests and deployed to the correct namespace where the VMFR controller runs (typicallyopenshift-adpor similar, not the defaultsystemnamespace).bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: LGTM!The ClusterRole correctly grants editor permissions on VirtualMachineBackupsDiscovery with full CRUD operations and read-only access to the status subresource, which is the expected editor-level access pattern.
bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml (1)
1-40: LGTM!The Role correctly defines standard Kubernetes leader election permissions. The leases (coordination.k8s.io) and configmaps access with full CRUD operations and events (create/patch) are the expected minimum set for leader election coordination.
bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: LGTM!The ClusterRole correctly grants editor permissions on VirtualMachineFileRestore with the standard pattern of full CRUD operations and read-only status access.
bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml (1)
13-16: Verify ServiceAccount "controller-manager" exists and namespace is correct.This RoleBinding mirrors the one in config/vm-file-restore-controller_rbac/ and references ServiceAccount
controller-managerin thesystemnamespace. Confirm this ServiceAccount is created in bundle manifests and the namespace matches where the VMFR controller is deployed.config/samples/kustomization.yaml (2)
3-4: Verify the new sample YAML files exist and are properly documented.The kustomization adds references to
oadp_v1alpha1_virtualmachinebackupsdiscovery.yamlandoadp_v1alpha1_virtualmachinefilerestore.yaml. Ensure these sample files are present in theconfig/samples/directory and documented appropriately for users getting started with VMFR.
5-7: Verify the removal of DataProtectionApplication and related samples is intentional.The kustomization removes three existing samples (nonadminbackupstoragelocationrequest, nonadminbackupstoragelocation, and dataprotectionapplication). Confirm this refactoring is intentional and that existing documentation and examples are updated accordingly, especially since DataProtectionApplication is a core OADP resource that users rely on for getting started.
bundle/manifests/oadp-vmfr-controller-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml (1)
1-20: Verify the "system" namespace is correct for VMFR controller deployment.The ClusterRoleBinding binds to ServiceAccount
oadp-vmfr-controllerin thesystemnamespace. Confirm this matches the actual deployment namespace for the VMFR controller. OADP components are typically deployed to a dedicated namespace (e.g.,openshift-adporoadp-system), not the defaultsystemnamespace. If the namespace mismatch exists, the binding will fail to attach the role to the controller's ServiceAccount.bundle/manifests/oadp-vmfr-controller_v1_serviceaccount.yaml (1)
1-12: Critical: ServiceAccount name mismatch with RoleBindings.This ServiceAccount is named
oadp-vmfr-controller, but the leader election RoleBindings (in bundle/manifests/leader-election-rolebinding and config/vm-file-restore-controller_rbac/leader_election_role_binding) reference a ServiceAccount namedcontroller-manager. This mismatch will prevent the leader election role from being bound to the actual controller ServiceAccount.Verify and align the ServiceAccount names:
- Either update these RoleBindings to reference
oadp-vmfr-controllerinstead ofcontroller-manager, or- Ensure a separate
controller-managerServiceAccount is created if that is a distinct component.config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml (1)
1-29: Viewer role configuration looks good.The read-only permissions (get, list, watch) are appropriately scoped, and the comment clearly explains this is a helper role for cluster admins. No security concerns.
bundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-142: Controller role permissions are well scoped.The controller role appropriately specifies concrete verbs for each resource group. Permissions are justified for controller operations (e.g., pod management, deployment control, SCC use limited to privileged). The role avoids wildcards and follows least-privilege principles.
bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: Viewer role is properly scoped.Read-only permissions are correctly limited to get, list, watch. No security concerns.
config/vm-file-restore-controller_rbac/service_account.yaml (1)
6-6: Service account rename—verify naming consistency and all references updated.The service account name changed from
oadp-vm-file-restore-controller-managertooadp-vmfr-controller. This deviates from the standard OADP operator naming pattern (<namespace>-controller-manager). Verify that: (1) this shortened naming convention is intentional and consistent across all VMFR-related manifests, and (2) all references in ClusterRoleBindings (subjects), Deployments/StatefulSets (serviceAccountName), and other RBAC resources are updated to the new name.config/vm-file-restore-controller_rbac/kustomization.yaml (1)
1-31: Kustomization structure follows Kubebuilder's standard scaffolding.The RBAC organization is correct and well-commented. All referenced resource files (
leader_election_role.yaml,metrics_auth_role.yaml, admin/editor/viewer roles, etc.) are standard outputs from Kubebuilder's controller-gen and match documented controller scaffolding patterns. The structure appropriately separates core RBAC, leader election permissions, metrics endpoint protection, and cluster management helper roles.config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml (1)
1-33: LGTM—Well-structured editor role for VirtualMachineFileRestore.The role correctly separates permissions for the main resource and its status subresource, following Kubernetes RBAC conventions. The verbs are appropriate for an editor persona.
config/vm-file-restore-controller_rbac/role.yaml (2)
96-103: Verify necessity of privileged SCC access.The addition of
usepermission for the privileged SecurityContextConstraint is narrow and properly scoped to that specific resource. However, verify that this elevated privilege is necessary for VMFR controller functionality.
1-142: Naming convention update is consistent.The role name change from
oadp-vm-file-restore-controller-manager-roletooadp-vmfr-controller-rolealigns with the shorthand consolidation across the VMFR RBAC suite.config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml (1)
1-29: LGTM—Appropriate viewer role with read-only access.The role correctly grants only read-only verbs and properly separates the status subresource. The comments provide good context for cluster administrators.
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml (1)
1-33: LGTM—Well-designed editor role with appropriate permissions.The role correctly grants edit permissions (create, delete, get, list, patch, update, watch) on the VirtualMachineBackupsDiscovery resource while restricting the status subresource to read-only. Consistent with the editor role pattern across the VMFR RBAC suite.
Makefile (2)
937-943: LGTM—Build-must-gather conditional is well-structured.The
SKIP_MUST_GATHERcheck provides a convenient way to skip the expensive must-gather build during local development or CI pipeline runs. The structure is clear and follows shell best practices.
910-935: Verify VMFR image environment variable names in config/manager/manager.yaml.The
update-vmfr-manifeststarget follows the same pattern asupdate-non-admin-manifestsand has good error handling. Verify that the environment variable names targeted by the YQ commands (lines 923-926) match those actually present inconfig/manager/manager.yaml:
RELATED_IMAGE_VM_FILE_RESTORE_CONTROLLERRELATED_IMAGE_VM_FILE_RESTORE_ACCESSRELATED_IMAGE_VM_FILE_RESTORE_SSHRELATED_IMAGE_VM_FILE_RESTORE_BROWSERbundle/manifests/oadp-operator.clusterserviceversion.yaml (1)
175-203: LGTM—CRD samples added with appropriate metadata.The VirtualMachineBackupsDiscovery sample includes helpful spec fields (startTime, endTime, virtualMachineName, virtualMachineNamespace). The VirtualMachineFileRestore sample has a null spec, which is acceptable for basic examples.
config/vm-file-restore-controller_rbac/role_binding.yaml (1)
1-19: Verify service account and role references exist.The binding correctly renames references to align with VMFR controller naming consolidation. Please confirm that the referenced
oadp-vmfr-controllerservice account andoadp-vmfr-controller-roleClusterRole are present in the repository.bundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (2)
11-22: Verify and document the necessity of privileged SCC permissions for VMFR controller.The role grants both
createandupdatepermissions onpods(lines 11–22) andusepermission on theprivilegedSCC (lines 96–103). While backup/restore operators commonly require elevated privileges to function (e.g., mounting volumes, snapshots), this permission combination should be explicitly justified in documentation.Before deploying, confirm:
- Whether
privilegedSCC is the minimum required, or if a less permissive SCC (e.g.,anyuid) suffices- The specific use cases requiring host-level access
- Whether namespace scoping, pod security policies, or network policies are in place as compensating controls
If
privilegedis necessary, document the justification and security review sign-off.
73-83: Critical: Rolebindings management enables privilege escalation.Granting
create,delete,patch,updateverbs onrolebindingsallows a compromised controller to escalate its own privileges or those of other service accounts, directly violating the principle of least privilege.Unless the VMFR controller explicitly requires the ability to create or modify role bindings as part of its core functionality, remove these verbs. If role binding management is necessary, restrict it to specific namespace scopes or consider a separate, tightly scoped ClusterRole for that specific operation. Provide documented justification explaining why these permissions are needed for VMFR functionality.
| resources: | ||
| - namespaces | ||
| - pods | ||
| - secrets |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/bundle/manifests*" -name "*role*" -type fRepository: openshift/oadp-operator
Length of output: 190
🌐 Web query:
OADP operator Velero controller RBAC permissions requirements secrets services routes
💡 Result:
Summary — OADP (Velero via the OADP operator) RBAC, Secrets, Services and Routes (concise)
-
Namespace / ServiceAccount
- Velero runs in the OADP namespace (commonly openshift-adp or velero-*) and uses a Velero ServiceAccount; bind that SA to required Roles/ClusterRoles. [2][3]
-
RBAC (recommended permissions)
- OADP/Velero needs a ClusterRole (or cluster-admin by default) that grants:
- CRUD/list/get on PersistentVolumes, PersistentVolumeClaims, Namespaces (create/list/get), Secrets, ConfigMaps, ServiceAccounts, Roles/RoleBindings, ClusterRoles/ClusterRoleBindings, StorageClasses, VolumeSnapshot* CRs, CustomResourceDefinitions as needed.
- list/read across many API groups (Velero reads many cluster-scoped resources during backup). Example manifests and guidance are in Velero docs (you can restrict further per your backup surface). [1][2]
- Bind the ClusterRole to the Velero ServiceAccount with a ClusterRoleBinding. [1][2]
- OADP/Velero needs a ClusterRole (or cluster-admin by default) that grants:
-
Secrets (names and behavior)
- You must provide object-storage credentials in a default Secret named cloud-credentials (or provider-specific names like cloud-credentials-gcp) in the OADP namespace; if backup and snapshot locations use different credentials, create separate Secrets and reference them appropriately in the DataProtectionApplication CR. A default Secret is required for install (can be empty credentials-velero file if you defer configuration). [2][4]
- Velero creates velero-repo-credentials in the OADP namespace for the backup repository password (you may replace the password before first backup). [2]
-
Services & Routes
- Velero runs as a Deployment and exposes internal Services; OADP installs OpenShift-specific Velero plugins that may create or use Routes for registry access in migration workflows. The openshift-velero-plugin handles image/registry routing and can affect Route host restoration behavior during restores. Restoring Projects/Routes may change host values — the plugin includes logic to strip/adjust host during restore. Monitor Velero/openshift-velero-plugin behavior when restoring Route objects. [4][5]
-
Practical notes / links
- Default install often uses broad cluster permissions; for least-privilege, follow Velero’s “Run Velero more securely” RBAC guidance and craft a ClusterRole that grants only required verbs/resources for your backup/restore scope. Test PV/CSI snapshot and namespace restore workflows after tightening RBAC. [1][2]
Sources
[1] Velero docs — RBAC guidance.
[2] Red Hat / OpenShift docs — OADP install, required Secrets and behavior (openshift-adp namespace, cloud-credentials, velero-repo-credentials).
[3] Red Hat / OpenShift backup & restore docs (version matrix, Velero access).
[4] openshift-velero-plugin (GitHub) — plugin behavior for OpenShift (registry, routes).
[5] Red Hat KB — route restore behavior with OADP / openshift-velero-plugin.
Would you like: (A) example ClusterRole/ClusterRoleBinding and Role/RoleBinding YAML tuned for OADP, or (B) step-by-step commands to create the required Secrets and bind RBAC in your cluster?
🌐 Web query:
Velero OADP ClusterRole least privilege verbs secrets services routes example
💡 Result:
Below is a minimal ClusterRole example for Velero / OADP that follows least-privilege principles for secrets, services, and OpenShift routes. Key ideas: avoid broad verbs on secrets (don’t grant get/list/watch unless absolutely required), grant only the verbs Velero needs for backup/restore operations on services and routes, and keep other rules narrow. Adjust namespaces, serviceAccount, and any additional Velero resources you need.
ClusterRole (example)
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: velero-oadp-minimal
rules:
core resources Velero commonly needs (narrow verbs)
- apiGroups: [""]
resources:- namespaces
- persistentvolumes
- persistentvolumeclaims
verbs: ["get","list","watch"]
- apiGroups: [""]
resources:- services
verbs: ["get","list","watch"]
- services
Secrets: avoid read verbs unless required — here only "create" and "patch" for credential injection (no get/list/watch)
- apiGroups: [""]
resources:- secrets
verbs: ["create","patch","update"]
- secrets
ConfigMaps used by Velero (restricted verbs)
- apiGroups: [""]
resources:- configmaps
verbs: ["get","list","watch","create","update","patch"]
- configmaps
OpenShift routes (route.openshift.io) — only read/list/watch needed for discovery; add update/create only if Velero plugin modifies routes
- apiGroups: ["route.openshift.io"]
resources:- routes
verbs: ["get","list","watch"]
- routes
CRDs Velero operates on (narrowed to specific names where possible)
- apiGroups: ["velero.io"]
resources:- backups
- restores
- backupstoragelocations
- volumesnapshots
verbs: ["get","list","create","update","patch","delete","watch"]
ClusterRoleBinding (bind service account)
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: velero-oadp-minimal-binding
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: velero-oadp-minimal
subjects:
- kind: ServiceAccount
name: velero
namespace: openshift-adp
Notes / recommendations
- If your environment requires Velero to read backup location credentials, you may need to allow "get" on a specific Secret — prefer creating a dedicated Secret in the Velero namespace and grant get only for that Secret (use a Role scoped to that namespace instead of ClusterRole when possible). See guidance on restrictive RBAC for Velero. [1]
- On OpenShift, routes are in the route.openshift.io API group; include only verbs Velero requires (most plugins only need read verbs). [2]
- Consider policy controls (Kyverno/Gatekeeper) that prohibit get/list/watch on Secrets in ClusterRoles to enforce least privilege. [3]
Sources
- Velero documentation — guidance on using restrictive RBAC for Velero. [1]
- OKD / OADP (OpenShift Data Protection) docs — secrets and OADP installation guidance (routing/plugins). [2]
- Kyverno example policy restricting Secrets verbs in Roles/ClusterRoles. [3]
References
[1] Velero docs: Run Velero more securely with restrictive RBAC settings.
[2] OKD OADP docs: Configuring OADP (secrets, routes).
[3] Kyverno policy: restrict-secret-role-verbs (example).
Major: Overly broad permissions on secrets, services, and routes.
While Velero/OADP does require access to these resources for backup/restore operations, the permissions should follow least-privilege principles. Industry best practices and the Velero security documentation recommend:
- Secrets: Grant only
create,patch,update(avoidget,list,watchunless retrieving specific backup location credentials, which should be in a dedicated Secret with namespace-scoped Role) - Services: Grant only
get,list,watch(read-only) - Routes: Grant only
get,list,watch(read-only unless the openshift-velero-plugin explicitly modifies routes)
Full CRUD on these resources is unnecessary and violates least-privilege. Verify the manifest grants only the minimum verbs required and update accordingly.
| - rules: | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - events | ||
| - namespaces | ||
| - pods | ||
| - secrets | ||
| - serviceaccounts | ||
| - services | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - persistentvolumeclaims | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - deployments | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - coordination.k8s.io | ||
| resources: | ||
| - leases | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries | ||
| - virtualmachinefilerestores | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries/finalizers | ||
| - virtualmachinefilerestores/finalizers | ||
| verbs: | ||
| - update | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries/status | ||
| - virtualmachinefilerestores/status | ||
| verbs: | ||
| - get | ||
| - patch | ||
| - update | ||
| - apiGroups: | ||
| - rbac.authorization.k8s.io | ||
| resources: | ||
| - rolebindings | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - route.openshift.io | ||
| resources: | ||
| - routes | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - velero.io | ||
| resources: | ||
| - backups | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: | ||
| - velero.io | ||
| resources: | ||
| - datadownloads | ||
| verbs: | ||
| - get | ||
| - list | ||
| - patch | ||
| - watch | ||
| - apiGroups: | ||
| - velero.io | ||
| - authentication.k8s.io | ||
| resources: | ||
| - downloadrequests | ||
| - tokenreviews | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: | ||
| - velero.io | ||
| - authorization.k8s.io | ||
| resources: | ||
| - restores | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate RBAC rules to avoid redundancy.
The authentication.k8s.io/tokenreviews and authorization.k8s.io/subjectaccessreviews rules appear in two separate blocks within the same clusterPermissions section (lines 873–885 and lines 1071–1081). Kubernetes RBAC will merge these rules, but for clarity and maintainability, consolidate these duplicate rules into a single block.
Apply this diff to remove the duplication:
- apiGroups:
- velero.io
resources:
- '*'
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- - apiGroups:
- - authentication.k8s.io
- resources:
- - tokenreviews
- verbs:
- - create
- - apiGroups:
- - authorization.k8s.io
- resources:
- - subjectaccessreviews
- verbs:
- - create
serviceAccountName: openshift-adp-controller-managerThe first occurrence of these rules (lines 873–885) should be retained, and the second occurrence (lines 1071–1081) should be removed to avoid duplication.
Also applies to: 1071-1081
🤖 Prompt for AI Agents
In bundle/manifests/oadp-operator.clusterserviceversion.yaml around lines
873–885 and 1071–1081, there are duplicate RBAC rule blocks granting
authentication.k8s.io/tokenreviews and
authorization.k8s.io/subjectaccessreviews; retain the first occurrence (lines
873–885) and remove the duplicate block at lines 1071–1081, then update/validate
the surrounding YAML indentation and list commas as needed so the
clusterPermissions section remains syntactically valid.
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace wildcard verbs with explicit permissions (applies to bundle-generated manifests too).
Same as the config source: replace verbs: ['*'] with explicit verbs. If this is auto-generated from the config/ source via kustomize, fixing that source will resolve this automatically.
- apiGroups:
- oadp.openshift.io
resources:
- virtualmachinebackupsdiscoveries
verbs:
- - '*'
+ - create
+ - delete
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 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.
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries | |
| verbs: | |
| - create | |
| - delete | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch |
🤖 Prompt for AI Agents
In
bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
around lines 14-15, the ClusterRole uses a wildcard verb '*' which must be
replaced with an explicit, minimal set of verbs; update the manifest (or the
upstream config/ kustomize source that generates it) to enumerate only the
required verbs such as get, list, watch, create, update, patch, delete (or
whatever specific operations the role actually needs) instead of '*' so the RBAC
policy is explicit and least-privilege.
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinefilerestores | ||
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace wildcard verb with explicit permissions for better security posture.
Using ['*'] grants all verbs on the resource. While admin roles are higher-privilege, explicitly listing verbs (create, delete, deletecollection, get, list, patch, update, watch) provides better visibility and adheres to least-privilege principles. This addresses the static analysis flagging (CKV_K8S_49).
Consider applying this diff to improve the security posture:
- apiGroups:
- oadp.openshift.io
resources:
- virtualmachinefilerestores
verbs:
- - '*'
+ - create
+ - delete
+ - deletecollection
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 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.
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - create | |
| - delete | |
| - deletecollection | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch |
🤖 Prompt for AI Agents
In
bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
around lines 10 to 15, the role currently uses the wildcard verb '*' which
grants all permissions; replace it with an explicit list of verbs to follow
least-privilege principles. Update the verbs array to include: create, delete,
deletecollection, get, list, patch, update, watch (and add any additional
specific verbs required by the controller but avoid '*'), ensuring the YAML
formatting matches existing indentation and list style.
| rules: | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries | ||
| verbs: | ||
| - '*' | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace wildcard verbs with explicit permissions.
Admin roles should specify concrete verbs rather than verbs: ['*'] to follow least-privilege principles. This reduces the attack surface if the role is accidentally assigned.
Apply this diff to replace the wildcard with explicit verbs:
- apiGroups:
- oadp.openshift.io
resources:
- virtualmachinebackupsdiscoveries
verbs:
- - '*'
+ - create
+ - delete
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 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.
| rules: | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries/status | |
| verbs: | |
| - get | |
| rules: | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries | |
| verbs: | |
| - create | |
| - delete | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries/status | |
| verbs: | |
| - get |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🤖 Prompt for AI Agents
In
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml
around lines 15-27, the first rule uses verbs: ['*']; replace this wildcard with
explicit least-privilege verbs such as get, list, watch, create, update, patch,
delete, deletecollection so the role grants concrete actions only; keep the
second rule for resources: virtualmachinebackupsdiscoveries/status with verbs:
[get] unchanged.
| rules: | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinefilerestores | ||
| verbs: | ||
| - '*' | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinefilerestores/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace wildcard verbs with explicit permissions.
Admin roles should specify concrete verbs rather than verbs: ['*']. Replace with: create, delete, get, list, patch, update, watch.
- apiGroups:
- oadp.openshift.io
resources:
- virtualmachinefilerestores
verbs:
- - '*'
+ - create
+ - delete
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 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.
| rules: | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores/status | |
| verbs: | |
| - get | |
| rules: | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - create | |
| - delete | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores/status | |
| verbs: | |
| - get |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🤖 Prompt for AI Agents
In
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml
around lines 15 to 27, the RBAC rule uses verbs: ['*']; replace the wildcard
with explicit verbs to follow least-privilege practice: set verbs to create,
delete, get, list, patch, update, watch for the virtualmachinefilerestores
resource (leave the virtualmachinefilerestores/status rule as is with verbs:
get). Ensure YAML indentation and sequence formatting remain correct when
replacing the verbs value.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml (1)
15-27: Replace wildcard verbs with explicit permissions.Admin roles should specify concrete verbs rather than
verbs: ['*']to follow least-privilege principles. This reduces the attack surface and makes permissions explicit, even for roles intended for cluster admin delegation.Apply this diff to replace the wildcard with explicit verbs:
- apiGroups: - oadp.openshift.io resources: - virtualmachinebackupsdiscoveries verbs: - - '*' + - create + - delete + - get + - list + - patch + - update + - watchbundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
7-22: Restrict permissions on secrets, services, and routes to least-privilege.Granting full CRUD on secrets and routes exceeds least-privilege principles. Secrets should only allow credential injection (create, patch, update), not reads. Routes should typically be read-only (get, list, watch) unless the VMFR controller explicitly modifies routes during restore operations.
If this role is for the VMFR controller, verify that create/delete/patch/update on routes is necessary. Consider tightening to:
- apiGroups: - route.openshift.io resources: - routes verbs: - - create - - delete - get - list - - patch - - update - watchSimilarly, for secrets in lines 7-22:
- apiGroups: - "" resources: - secrets verbs: - - get - - list - create - - watch - patch - updateIf manager-role is for the OADP operator controller (not VMFR-specific), this broader permission set may be intentional. Please clarify the scope and verify necessity.
Also applies to: 85-95
bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
10-21: Replace wildcard verbs with explicit permissions (auto-generated from config source).This manifest is auto-generated from the config/ source. Fixing the wildcard in
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yamlwill automatically resolve this.See the refactor suggestion for the config source file. Once that is fixed and the bundle is regenerated, this file will be updated automatically.
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml (1)
15-27: **** — Wildcard verb flagged in prior review.This issue was previously identified and a specific remediation was provided. Replace
verbs: ['*']with explicit permissions (create, delete, get, list, patch, update, watch) to follow least-privilege principles and address CKV_K8S_49.bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
10-15: **** — Wildcard verb flagged in prior review.This issue was previously identified and a remediation was provided. Replace
verbs: ['*']with explicit permissions to follow least-privilege principles and satisfy CKV_K8S_49. The prior comment includes a complete diff.
🧹 Nitpick comments (1)
bundle/manifests/oadp-operator.clusterserviceversion.yaml (1)
960-973: RBAC rule contains redundant resource list.The oadp.openshift.io rule grants permissions on
'*'(all resources) and then explicitly lists cloudstorages, dataprotectionapplications, and dataprotectiontests. Since'*'already covers all resources, the explicit list is redundant. Consider removing the explicit entries for clarity, or clarify the intent if this is intentionally defensive.- apiGroups: - oadp.openshift.io resources: - '*' - - cloudstorages - - dataprotectionapplications - - dataprotectiontests verbs:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (36)
Makefile(1 hunks)bundle/manifests/controller-manager_v1_serviceaccount.yaml(1 hunks)bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml(1 hunks)bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml(1 hunks)bundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/metrics-auth-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/metrics-auth-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml(1 hunks)bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vmfr-controller-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml(1 hunks)bundle/manifests/oadp-vmfr-controller_v1_serviceaccount.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml(1 hunks)config/vm-file-restore-controller_rbac/kustomization.yaml(1 hunks)config/vm-file-restore-controller_rbac/leader_election_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_reader_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/role.yaml(2 hunks)config/vm-file-restore-controller_rbac/role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/service_account.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml
- config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml
- bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml
- config/vm-file-restore-controller_rbac/kustomization.yaml
- bundle/manifests/oadp-vmfr-controller-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml
- config/vm-file-restore-controller_rbac/role_binding.yaml
- config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml
- bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- config/vm-file-restore-controller_rbac/role.yaml
- bundle/manifests/controller-manager_v1_serviceaccount.yaml
- config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml
- bundle/manifests/oadp-vmfr-controller_v1_serviceaccount.yaml
- bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml
- config/vm-file-restore-controller_rbac/metrics_auth_role.yaml
- config/samples/kustomization.yaml
- bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bundle/manifests/metrics-auth-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yamlbundle/manifests/metrics-auth-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/leader_election_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yamlbundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yamlconfig/vm-file-restore-controller_rbac/metrics_reader_role.yamlbundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/metrics_auth_role_binding.yamlMakefileconfig/vm-file-restore-controller_rbac/service_account.yamlbundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-operator.clusterserviceversion.yamlbundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml
🪛 Checkov (3.2.334)
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🔇 Additional comments (15)
bundle/manifests/oadp-operator.clusterserviceversion.yaml (2)
175-203: New VMFR CRD examples are well-formed.The two new example CRs (VirtualMachineBackupsDiscovery and VirtualMachineFileRestore) are syntactically correct and follow OADP naming conventions. The samples properly demonstrate the specs for the respective kinds with discovery including time windows and VM identifiers, and restore with a minimal null spec.
720-725: New VMFR CRD definitions properly registered.The two new CRD entries for VirtualMachineBackupsDiscovery and VirtualMachineFileRestore are correctly defined with matching apiVersions and kinds. Ensure these CRDs are actually delivered by the operator's deployment process (typically via a Makefile or chart).
Verify that the CRD manifests for virtualmachinebackupsdiscoveries and virtualmachinefilerestores are generated/present in the kube:manifests directory.
bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: Viewer role permissions are appropriate.The ClusterRole correctly restricts viewers to read-only verbs (get, list, watch) on the virtualmachinefilerestore resource and status subresource. Permissions follow least-privilege principles.
config/vm-file-restore-controller_rbac/metrics_reader_role.yaml (1)
1-8: Metrics reader role is properly scoped.The ClusterRole correctly restricts access to the
/metricsendpoint with a read-only verb. This is the appropriate pattern for metrics scraping by monitoring systems.bundle/manifests/metrics-auth-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-17: Metrics auth role permissions are appropriate.The ClusterRole correctly grants only
createverbs ontokenreviewsandsubjectaccessreviews, which is the standard pattern for authentication/authorization proxies (e.g., kube-rbac-proxy). Permissions are minimal and well-scoped.bundle/manifests/metrics-auth-rolebinding_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml (1)
1-13: Binding is properly configured.The ClusterRoleBinding correctly binds the metrics-auth-role to the oadp-vmfr-controller ServiceAccount in the system namespace.
Verify that the ServiceAccount
oadp-vmfr-controllerin thesystemnamespace is created elsewhere in this PR (or already exists in the cluster).config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml (1)
1-12: Binding configuration is correct.The ClusterRoleBinding correctly binds the metrics-auth-role to the oadp-vmfr-controller ServiceAccount. Consistent with the bundle-generated version.
bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: LGTM!New ClusterRole follows best practices with explicit, least-privilege verbs for the editor role. The distinction between writable operations on the resource and read-only access to its status subresource is appropriate.
config/vm-file-restore-controller_rbac/service_account.yaml (1)
1-12: LGTM!ServiceAccount naming changes (from
oadp-vm-file-restore-controller-managertooadp-vmfr-controller) are consistent with VMFR controller naming conventions used throughout the PR and the instance label is updated accordingly.bundle/manifests/oadp-vmfr-controller-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-142: Extensive permissions, verify operational necessity.This ClusterRole grants broad access across multiple API groups. While most permissions appear necessary for VM file restore operations (core resources for pod/secret management, Velero integration, routes for OpenShift), the combination of pod/secret/RBAC management warrants confirmation that all permissions are actively used and required.
Consider documenting which controller features depend on each permission block (especially rolebindings CRUD and route management) to aid future maintainers.
Makefile (2)
910-935: Well-structured target following established pattern.The
update-vmfr-manifeststarget mirrors the existingupdate-non-admin-manifestspattern, making it maintainable and predictable. The sequence of CRD copy, kustomization update, environment variable configuration, and RBAC/sample resource management is sound.Minor note: The four consecutive
yqcommands (lines 923–926) could potentially be consolidated into a single command with multiple filters, but the current clarity-over-brevity approach is fine for occasional maintenance runs.
937-943: LGTM —SKIP_MUST_GATHERenhancement is useful.The conditional logic to honor
SKIP_MUST_GATHERimproves CI/CD flexibility without changing behavior when the flag is unset.config/vm-file-restore-controller_rbac/leader_election_role.yaml (1)
1-40: LGTM!Leader election role is well-scoped with explicit verbs. Event operations are appropriately restricted to create/patch only, and the overall permission set is minimal and necessary for the leader election pattern.
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml (1)
1-33: LGTM!Editor role is appropriately scoped with explicit, least-privilege verbs. The separation between resource operations and read-only status access follows best practices.
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml (1)
1-33: RBAC manifest is well-structured and follows best practices.The ClusterRole correctly defines permissions for an editor-level role:
- Rule 1 grants appropriate CRUD operations (
create,delete,get,list,patch,update,watch) on the core resource, consistent with Kubernetes editor role conventions.- Rule 2 properly restricts status subresource access to read-only (
get), following the best practice of preventing direct status mutation.- Clear documentation comments explain the role's purpose as a helper for cluster admins.
- Metadata labels indicate proper kustomize management and component ownership.
|
/retest |
|
hrm.. time to add a test that enable vmfr here I think |
There was a problem hiding this comment.
@mpryc The deployment fails to create pods because the ServiceAccount name is inconsistent:
- vmfilerestore_controller.go:330 sets ServiceAccountName = "oadp-vm-file-restore-controller-manager"
- But config/vm-file-restore-controller_rbac/service_account.yaml creates a ServiceAccount named oadp-vmfr-controller
This results in the error:
Error creating: pods "oadp-vm-file-restore-controller-manager-..." is forbidden:
serviceaccount "oadp-vm-file-restore-controller-manager" not foundThe RBAC files need to be updated to use oadp-vm-file-restore-controller-manager consistently:
- config/vm-file-restore-controller_rbac/service_account.yaml
- config/vm-file-restore-controller_rbac/role_binding.yaml
- config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml
- config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml
After fixing, run make bundle to regenerate the bundle manifests.
Updated CRDs based on the: migtools/oadp-vm-file-restore#54 Signed-off-by: Michal Pryc <mpryc@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/vm-file-restore-controller_rbac/role_binding.yaml (1)
1-1: Clarify file naming: contains ClusterRoleBinding, not RoleBinding.The filename is
role_binding.yaml, but the content declares akind: ClusterRoleBinding(line 2). Either rename the file tocluster_role_binding.yamlfor clarity, or verify that a RoleBinding (namespace-scoped) was intended.Also applies to: 2-2
♻️ Duplicate comments (5)
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml (1)
15-27: Wildcard verbs flagged in previous review—still unresolved.Previous review comment suggested replacing
verbs: ['*']with explicit verbs. The admin role includes helpful comments explaining its cluster-admin-only intent, but the wildcard remains a security posture gap (CKV_K8S_49). Please address the prior suggestion to replace with explicit verbs or document the rationale for keeping wildcards.bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
14-15: Wildcard verbs flagged in previous review—still unresolved.Previous review suggested replacing
verbs: ['*']with explicit verbs for least-privilege. This bundle manifest lacks the explanatory comments present in the corresponding config file. Please address the prior suggestion or align comments across bundle and config versions.config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml (1)
15-27: Wildcard verbs flagged in previous review—still unresolved.Previous review suggested replacing
verbs: ['*']with explicit verbs. Comments clarify the cluster-admin-only intent, but the wildcard remains a security posture concern (CKV_K8S_49). Please address the prior suggestion.bundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
7-22: ** Overly broad permissions on secrets, services, and routes remain unaddressed.**This concern was raised in a previous review and still applies. The manager-role grants full CRUD on these resources, which violates least-privilege principles:
- Secrets (lines 7–22): Should allow only
create,patch,update(avoidget,list,watchfor general secrets; scope to specific backup-location Secret if credential access is required).- Services (lines 7–22): Should allow only
get,list,watch(read-only).- Routes (lines 84–95): Should allow only
get,list,watch(read-only unless the VM file restore controller or openshift-velero-plugin explicitly requires write access).Please verify against the VMFR controller requirements and restrict permissions accordingly. If the controller genuinely needs broader access, document the business justification in a comment or ADR.
Also applies to: 84-95
bundle/manifests/oadp-operator.clusterserviceversion.yaml (1)
1010-1021: Duplicate RBAC rules for authentication and authorization reviews.The tokenreviews (lines 1010-1015) and subjectaccessreviews (lines 1016-1021) rules appear again at lines 1208-1219 in the third clusterPermissions block. While Kubernetes will merge these, they should be consolidated into a single block to improve maintainability.
The first occurrence (lines 1010-1021) is for
oadp-vm-file-restore-controller-managerand the second (lines 1208-1219) is foropenshift-adp-controller-manager. If only one service account requires these permissions, remove the duplicate. If both do, add a comment explaining why (e.g., both controllers need token review for authentication).Apply this diff to remove the duplicate rules from the third clusterPermissions block (lines 1208–1219):
- apiGroups: - velero.io resources: - '*' verbs: - create - delete - get - list - patch - update - watch - - apiGroups: - - authentication.k8s.io - resources: - - tokenreviews - verbs: - - create - - apiGroups: - - authorization.k8s.io - resources: - - subjectaccessreviews - verbs: - - create serviceAccountName: openshift-adp-controller-managerAlternatively, if both service accounts genuinely need these permissions, add an inline comment explaining the reasoning.
Also applies to: 1208-1219
🧹 Nitpick comments (1)
Makefile (1)
938-944: Minor: Add @ prefix to echo in build-must-gather for quiet mode consistency.Line 941 uses
echowithout@, which prints even in quiet mode. For consistency with other targets and to respect make's verbosity settings, use@echo:build-must-gather: check-go ## Build OADP Must-gather binary must-gather/oadp-must-gather ifeq ($(SKIP_MUST_GATHER),true) - echo "Skipping must-gather build" + @echo "Skipping must-gather build" else cd must-gather && go build -mod=mod -a -o oadp-must-gather cmd/main.go endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (38)
Makefile(1 hunks)bundle/manifests/controller-manager_v1_serviceaccount.yaml(1 hunks)bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml(1 hunks)bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml(1 hunks)bundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-operator.clusterserviceversion.yaml(4 hunks)bundle/manifests/oadp-vm-file-restore-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml(1 hunks)config/samples/kustomization.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml(1 hunks)config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml(1 hunks)config/vm-file-restore-controller_rbac/kustomization.yaml(2 hunks)config/vm-file-restore-controller_rbac/leader_election_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/metrics_reader_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/role.yaml(2 hunks)config/vm-file-restore-controller_rbac/role_binding.yaml(1 hunks)config/vm-file-restore-controller_rbac/service_account.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml(1 hunks)config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- config/vm-file-restore-controller_rbac/virtualmachinefilerestore_editor_role.yaml
- config/vm-file-restore-controller_rbac/virtualmachinefilerestore_viewer_role.yaml
- bundle/manifests/virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundle/manifests/virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
- bundle/manifests/leader-election-rolebinding_rbac.authorization.k8s.io_v1_rolebinding.yaml
- config/samples/oadp_v1alpha1_virtualmachinebackupsdiscovery.yaml
- bundle/manifests/leader-election-role_rbac.authorization.k8s.io_v1_role.yaml
- config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_editor_role.yaml
- config/vm-file-restore-controller_rbac/metrics_reader_role.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
config/vm-file-restore-controller_rbac/metrics_auth_role.yamlconfig/samples/oadp_v1alpha1_virtualmachinefilerestore.yamlbundle/manifests/controller-manager_v1_serviceaccount.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/leader_election_role_binding.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yamlMakefileconfig/vm-file-restore-controller_rbac/service_account.yamlbundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/metrics_auth_role_binding.yamlconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yamlbundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/leader_election_role.yamlbundle/manifests/oadp-operator.clusterserviceversion.yamlbundle/manifests/oadp-vm-file-restore-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/kustomization.yamlbundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yamlbundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/samples/kustomization.yamlconfig/vm-file-restore-controller_rbac/role.yamlconfig/vm-file-restore-controller_rbac/role_binding.yamlbundle/manifests/manager-role_rbac.authorization.k8s.io_v1_clusterrole.yamlconfig/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml
🪛 Checkov (3.2.334)
bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
config/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/virtualmachinebackupsdiscovery-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
config/vm-file-restore-controller_rbac/virtualmachinefilerestore_admin_role.yaml
[medium] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🔇 Additional comments (23)
bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: LGTM!The ClusterRole manifest is well-structured and correctly defines read-only permissions for VM backups discovery resources. The permissions (get, list, watch on the resource and get on its status) are appropriately scoped for a viewer role, following Kubernetes RBAC conventions.
bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: Well-formed RBAC ClusterRole with appropriate editor permissions.The ClusterRole manifest is correctly structured and defines appropriate permissions for an editor role on
virtualmachinefilerestoresresources. The separation of read-write permissions on the main resource and read-only access to/statusfollows Kubernetes RBAC best practices.Verify that this ClusterRole has corresponding ClusterRoleBinding(s) in the PR that bind it to the appropriate subjects (service accounts, users, or groups). Ensure the role is bound only to principals that should have editor access to VM file restore resources.
config/vm-file-restore-controller_rbac/metrics_auth_role.yaml (1)
1-17: LGTM!The metrics auth role uses specific, minimal verbs appropriate for proxy authentication and authorization checks.
bundle/manifests/virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: LGTM!The editor role properly uses explicit, least-privilege verbs for read and write operations.
bundle/manifests/virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: LGTM!The viewer role correctly implements read-only access with minimal, appropriate verbs.
Makefile (1)
910-936: verify namePrefix insertion in kustomization.yaml—sed '1i' may break YAML if file has initial content.The
update-vmfr-manifeststarget correctly sets VMFR image variables and copies manifests, but Line 931 usessed -i '1i namePrefix: oadp-vm-file-restore-'to inject the namePrefix. This assumes the kustomization.yaml has an empty first line; if it already containsapiVersionor another field at line 1, the insertion will corrupt the YAML structure.Recommend validating the VMFR kustomization.yaml format before deployment or switching to
yqfor safer YAML editing.Additionally, Line 941's
echo "Skipping must-gather build"lacks the@prefix, causing it to print even in quiet mode—use@echofor consistency with other targets.config/samples/kustomization.yaml (1)
3-4: New VM File Restore samples added correctly.Integration of the new sample CR types into the kustomization is straightforward and follows the existing pattern. Ensure the corresponding sample YAML files are present.
config/samples/oadp_v1alpha1_virtualmachinefilerestore.yaml (1)
8-9: Sample is properly structured; TODO comment is acceptable.The spec placeholder and TODO comment are appropriate for a sample manifest. Implementers will need to populate the spec based on the CR definition.
config/vm-file-restore-controller_rbac/metrics_auth_role_binding.yaml (1)
1-12: ClusterRoleBinding correctly references metrics-auth-role.Standard RBAC binding structure; binds metrics authentication role to the controller-manager service account in the system namespace.
bundle/manifests/oadp-vm-file-restore-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-10: Metrics reader role is minimally scoped with appropriate permissions.Grants only GET access to the /metrics endpoint as a nonResourceURL. This follows least-privilege principles.
bundle/manifests/controller-manager_v1_serviceaccount.yaml (1)
1-8: ServiceAccount definition is standard and properly labeled.Naming aligns with RBAC bindings referencing controller-manager. Ensure this ServiceAccount is applied to the system namespace when deployed (via kustomization or overlay).
config/vm-file-restore-controller_rbac/leader_election_role.yaml (1)
1-40: Well-structured leader election role following best practices.Uses a namespaced Role (not ClusterRole) with explicit verb enumeration instead of wildcards. Permissions are appropriately scoped to configmaps, leases, and events needed for Kubernetes leader election. No security concerns.
bundle/manifests/metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-10: Duplicate metrics reader role with generic naming.This ClusterRole appears to duplicate the functionality of oadp-vm-file-restore-metrics-reader defined elsewhere, with the same nonResourceURL /metrics GET permission. Consider consolidating or clarifying the intent of both roles if they serve the same purpose.
bundle/manifests/oadp-vm-file-restore-virtualmachinefilerestore-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-23: LGTM. Viewer role is properly scoped with read-only permissions.config/vm-file-restore-controller_rbac/leader_election_role_binding.yaml (1)
1-15: LGTM. RoleBinding properly binds the controller-manager ServiceAccount to the leader-election-role.bundle/manifests/oadp-vm-file-restore-virtualmachinebackupsdiscovery-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
1-27: LGTM. Editor role properly scoped with explicit verbs; status subresource correctly restricted to read-only.config/vm-file-restore-controller_rbac/service_account.yaml (1)
4-7: Verify that RoleBinding and ClusterRoleBinding subjects are updated to reference the new service account name.The service account name was shortened from
oadp-vm-file-restore-controller-managertocontroller-manager, and will be reconstructed asoadp-vm-file-restore-controller-managerby the kustomizationnamePrefix. Ensure all related bindings (e.g.,leader-election-rolebinding, manager role binding) correctly reference the new namecontroller-managerin their subjects.config/vm-file-restore-controller_rbac/role.yaml (1)
96-103: Verify SCC privileged use is required.The addition of
usepermission for theprivilegedSecurityContextConstraint suggests the VM file restore controller or its pods require elevated privileges (e.g., for container mounting or direct device access). Confirm this is necessary and document the business justification in an ADR or inline comment if not obvious.config/vm-file-restore-controller_rbac/kustomization.yaml (1)
1-32: Verify all referenced RBAC resource files exist in the config directory.The kustomization now references 14 RBAC resources. Ensure all files exist in
config/vm-file-restore-controller_rbac/: service_account.yaml, role.yaml, role_binding.yaml, leader_election_role.yaml, leader_election_role_binding.yaml, metrics_auth_role.yaml, metrics_auth_role_binding.yaml, metrics_reader_role.yaml, virtualmachinebackupsdiscovery_admin_role.yaml, virtualmachinebackupsdiscovery_editor_role.yaml, virtualmachinebackupsdiscovery_viewer_role.yaml, virtualmachinefilerestore_admin_role.yaml, virtualmachinefilerestore_editor_role.yaml, and virtualmachinefilerestore_viewer_role.yaml.bundle/manifests/virtualmachinefilerestore-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)
10-15: Replace wildcard verb with explicit permissions to improve security posture.Using
['*']grants all verbs on the resource. Although admin roles are higher-privilege, explicitly listing verbs provides better visibility into the actual permission surface and adheres to least-privilege principles. This also addresses the static analysis flag (CKV_K8S_49).Apply this diff to resolve the issue:
- apiGroups: - oadp.openshift.io resources: - virtualmachinefilerestores verbs: - - '*' + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watchconfig/vm-file-restore-controller_rbac/virtualmachinebackupsdiscovery_viewer_role.yaml (1)
1-29: LGTM!The viewer role is well-documented and grants appropriate read-only access (get, list, watch) to VirtualMachineBackupsDiscovery resources and their status subresource. This follows the aggregated RBAC pattern and aligns with the other admin/editor roles in the PR.
bundle/manifests/oadp-operator.clusterserviceversion.yaml (2)
175-203: New sample CRs are properly formatted.The VirtualMachineBackupsDiscovery and VirtualMachineFileRestore sample manifests are well-structured and demonstrate basic usage patterns. Labels and metadata are consistent with other samples in the manifest.
1387-1419: Verify leader election configuration for VM File Restore controller.New leader election permissions (configmaps, leases, events) are added for
oadp-vm-file-restore-controller-manager(lines 1387–1419). Confirm that the controller is configured to use leader election (e.g., with a--leader-electflag or environment variable) to avoid redundant permissions.
| - apiGroups: | ||
| - security.openshift.io | ||
| resourceNames: | ||
| - privileged | ||
| resources: | ||
| - securitycontextconstraints | ||
| verbs: | ||
| - use |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repository structure
cd /tmp && find . -type f \( -name 'README*' -o -name '*.md' \) -maxdepth 3 2>/dev/null | head -10
git ls-files | grep -E '(README|readme)' | head -5Repository: openshift/oadp-operator
Length of output: 190
🏁 Script executed:
git ls-files | head -30Repository: openshift/oadp-operator
Length of output: 190
🏁 Script executed:
# Check the file in question
cat -n bundle/manifests/oadp-operator.clusterserviceversion.yaml | sed -n '960,980p'Repository: openshift/oadp-operator
Length of output: 190
🏁 Script executed:
# Search for context about VM File Restore and privileged SCC
rg -i "vm file restore|privileged" --type yaml --type md -B 2 -A 2Repository: openshift/oadp-operator
Length of output: 190
🏁 Script executed:
# Look for any documentation or comments explaining SCC requirements
rg -i "securitycontextconstraint|scc" --type yaml -B 3 -A 3 | head -100Repository: openshift/oadp-operator
Length of output: 190
🏁 Script executed:
# Check if there's any documentation folder
git ls-files | grep -i -E '(doc|readme|install|quickstart)'Repository: openshift/oadp-operator
Length of output: 190
🌐 Web query:
OADP OpenShift API Data Protection VM File Restore security requirements
💡 Result:
Summary — key security requirements and considerations for OADP (OpenShift API for Data Protection) VM file-level restore:
-
RBAC and service accounts — ensure OADP/Velero service accounts have required cluster and namespace RBAC to read/write backups, restore CRs, create/delete PVCs and namespaces. [1]
-
Namespace security context / UID/GID mapping — restored namespaces may get different SCC annotations and UID ranges; this can break file ownership inside VMs. Either allow OADP to create the namespace on the destination, remove pre‑existing namespaces before restore, or follow Red Hat's UID/GID-fix steps after restore. [2]
-
Pod Security / SCC / SELinux — workloads and helper pods must run with the SCC and SELinux labels needed to access restored files; file permissions/SELinux contexts inside the VM must permit pre/post hook operations (fsfreeze, scripts). Ensure scripts and files have correct SELinux and UNIX permissions. [3]
-
QEMU guest agent & VM hooks for consistent FS state — for file-level restores you typically need the guest agent and configured pre-freeze/post-thaw hooks (fsfreeze-hook.d) so the filesystem can be quiesced and safe file-level snapshots taken/restored. Scripts must be executable and have proper permissions/SELinux labeling. [3]
-
Backup store credentials & security — protect object-store (S3) credentials/secrets used by Velero/OADP (use Secrets, short-lived credentials where supported such as STS/WIF). Ensure TLS to object storage and least-privilege creds for the BSL. [1][4]
-
Network and transport security — ensure secure, reliable network path to object store (TLS, firewall rules, adequate bandwidth/latency), and open required ports between cluster nodes and storage endpoints. [4]
-
Volume snapshot / CSI prerequisites — underlying CSI driver and snapshot capabilities must be supported and correctly permissioned; restores may require deletion of existing PVCs and creation of new PVCs (RBAC required). [1][5]
-
Restore workflow security checks — OADP/Velero may skip or alter namespace annotations and labels during restore; confirm label selectors, BSL/VolumeSnapshotLocation names and shared credentials across clusters when migrating. [2]
-
Audit, logs, and secrets handling — enable cluster/audit logging for backup/restore operations and rotate/limit access to Velero/OADP secrets. Use restricted access for operators and admins. [1][2]
-
Vendor/platform-specific requirements — follow vendor docs (OpenShift Virtualization, storage plugin) for any additional security requirements (Trident/ONTAP, NetApp, Ceph, etc.). [5]
Sources:
- OpenShift OADP / Velero application backup & restore documentation (OpenShift docs). [1]
- Red Hat OADP FAQ — UID/GID, namespace and restore guidance. [2]
- OADP VM pre-backup hooks and permission/SELinux guidance (Red Hat Developer article). [3]
- OADP recommended network settings and backup-store credential guidance (OpenShift docs). [4]
- OpenShift Virtualization / VM backup & restore guidance (Red Hat Developer). [5]
If you want, I can extract exact RBAC rules, required SCC annotations, sample Secrets (BSL) and example pre-freeze hook scripts with SELinux labels.
🌐 Web query:
OADP operator clusterserviceversion privileged SCC documentation
💡 Result:
Short answer — grant the OADP operator’s service account permission to "use" the privileged SecurityContextConstraints in the operator CSV (install.spec.clusterPermissions) and document why in the CSV description.
How-to (minimal):
-
In the operator ClusterServiceVersion YAML (install.spec of the CSV) add a clusterPermissions entry that allows apiGroup security.openshift.io, resource securitycontextconstraints, resourceName privileged, verbs: ["use"] for the operator serviceAccount. Example snippet:
- rules:
- apiGroups: ["security.openshift.io"]
resources: ["securitycontextconstraints"]
resourceNames: ["privileged"]
verbs: ["use"]
serviceAccountName:
(Place under install.spec.clusterPermissions).
This is the standard method recommended for Operators that need escalated SCCs. [1][2]
- apiGroups: ["security.openshift.io"]
- rules:
-
Ensure the CSV’s spec.description explains why the operator requires the privileged SCC (audit justification). [1][2]
References:
- OpenShift Operators / Developing Operators — managing pod security admission, CSV clusterPermissions example (shows securitycontextconstraints privileged use and CSV description recommendation). [1]
- OADP operator (project repo/README) — OADP is the OpenShift API for Data Protection operator (use this when applying the CSV change for OADP specifically). [2]
Sources:
[1] Red Hat / OpenShift Operators — Developing Operators: Managing pod security admission (CSV clusterPermissions example). (docs.redhat.com)
[2] openshift/oadp-operator GitHub (OADP operator project). (github.com)
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_container_platform/4.16/html/operators/developing-operators?utm_source=openai
- 2: https://github.com/openshift/oadp-operator?utm_source=openai
🌐 Web query:
OADP operator CSV "spec.description" security context constraints VM backup restore documentation
💡 Result:
Short answer — what to put in the OADP CSV "spec.description" and related CSV fields for SCCs plus where to find VM backup/restore docs:
-
CSV spec.description: explicitly state why the operator needs escalated permissions (e.g., to use a SecurityContextConstraints such as "privileged" or to run pods that require specific runAsUser/fsGroup settings) and what actions require them (Velero/kubevirt plugins taking VM snapshots, restoring VM objects, attaching volumes, etc.). This description is required by OLM when an operator requests SCC/use of elevated privileges. [1][2]
-
CSV install spec (required in addition to description):
- Add clusterPermissions that grant the operator service account verbs: use on securitycontextconstraints (resource securitycontextconstraints, apiGroup security.openshift.io) and any other needed SCC names (e.g., "privileged"). Example pattern shown in OpenShift docs. [1][2]
- Set container securityContext in the CSV to document requested capabilities and runAsUser/fsGroup as needed. [1]
-
Why for VM backup/restore: OADP (Velero + kubevirt plugin) may need SCCs when backing up/restoring VM resources, taking/restoring CSI snapshots or using node-level agents (restic/kopia) that require elevated volume or host access. Refer to OADP/Velero & KubeVirt plugin docs for VM-specific behavior and supported backup methods (CSI snapshots, Kopia/Restic, kubevirt plugin notes). [3][4]
Key references:
- OpenShift operator CSV / managing operator SCC and examples (edit CSV description + clusterPermissions for securitycontextconstraints). [1][2]
- OADP (OpenShift API for Data Protection) backup/restore and kubevirt plugin (VM backup/restore behavior and requirements). [3][4]
If you want, I can:
- produce exact YAML snippets for spec.description, clusterPermissions and container securityContext for OADP CSV tailored to OADP/kubevirt; and
- point to the specific OADP CSV in a released OADP package (I can search the operator CSV file for the OADP version you use). Which OADP version/cluster version are you on?
Ensure the CSV description documents why the privileged SCC grant is required.
The privileged SCC permission is necessary for OADP's VM File Restore controller to handle file access and security context restoration during VM backup and restore operations. Per OpenShift Operator guidelines, the CSV spec.description must explicitly justify this escalated privilege grant and explain the specific use cases (Velero/kubevirt plugin VM operations, CSI snapshot handling, volume attachment). This documentation is required by OLM when an operator requests privileged SCC access.
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinebackupsdiscoveries | ||
| verbs: | ||
| - '*' |
There was a problem hiding this comment.
Minimize wildcard verb usage in admin ClusterRole.
Avoid providing wildcard permissions when possible, especially to all resources. Line 15 uses verbs: ['*'] granting all possible actions on virtualmachinebackupsdiscoveries. While admin roles require broad permissions, avoid wildcards when you define verbs or access to resources; wildcards are convenient to apply, but they open up all sorts of unauthorized access holes.
Even if this is intentional for an admin role, explicit enumeration of required verbs improves auditability and reduces attack surface.
Consider replacing the wildcard with specific verbs:
- apiGroups:
- oadp.openshift.io
resources:
- virtualmachinebackupsdiscoveries
verbs:
- - '*'
+ - get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete📝 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.
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinebackupsdiscoveries | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - update | |
| - patch | |
| - delete |
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinefilerestores | ||
| verbs: | ||
| - '*' | ||
| - apiGroups: | ||
| - oadp.openshift.io | ||
| resources: | ||
| - virtualmachinefilerestores/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Minimize wildcard verbs in admin roles.
The admin role uses verbs: ['*'] which overly permits all actions. While admin roles are intended for cluster admins, either add explanatory comments (as in the config version) or replace with explicit verbs (create, delete, get, list, patch, update, watch) to improve clarity and follow least-privilege principles.
Apply this diff to add explanatory comments:
+# This role is intended for cluster admins to manage permissions for users.
+# Grants full permissions ('*') over oadp.openshift.io resources.
+
apiVersion: rbac.authorization.k8s.io/v1Alternatively, replace the wildcard with explicit verbs for stricter least-privilege:
- verbs:
- - '*'
+ verbs:
+ - create
+ - delete
+ - get
+ - list
+ - patch
+ - update
+ - watch📝 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.
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - '*' | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores/status | |
| verbs: | |
| - get | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores | |
| verbs: | |
| - create | |
| - delete | |
| - get | |
| - list | |
| - patch | |
| - update | |
| - watch | |
| - apiGroups: | |
| - oadp.openshift.io | |
| resources: | |
| - virtualmachinefilerestores/status | |
| verbs: | |
| - get |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
| @@ -1,19 +1,15 @@ | |||
| apiVersion: rbac.authorization.k8s.io/v1 | |||
| kind: ClusterRoleBinding | |||
There was a problem hiding this comment.
Generic resource names risk collision with other controllers.
The binding references resources with very generic names: manager-rolebinding, manager-role, and ServiceAccount controller-manager in the system namespace. This significantly increases collision risk if other controllers adopt similar naming. Consider using a namespace-specific or product-specific prefix (e.g., oadp-vm-file-restore-manager-role).
Also applies to: 7-7, 11-11, 14-14
🤖 Prompt for AI Agents
In config/vm-file-restore-controller_rbac/role_binding.yaml around lines 2 (also
lines 7, 11, 14), the manifest uses overly generic resource names
(manager-rolebinding, manager-role, controller-manager in namespace system) that
risk collisions; rename these resources to a product/namespace-specific prefix
(for example oadp-vm-file-restore-manager-rolebinding,
oadp-vm-file-restore-manager-role, and oadp-vm-file-restore-controller-manager
SA in the appropriate namespace), update all references in this file to the new
names, and ensure the ServiceAccount namespace is correct and any related RBAC
manifests are updated to match the new names.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@mpryc: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/override 4.20-e2e-test-cli-aws |
|
@mpryc: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Updated CRDs based on the:
migtools/oadp-vm-file-restore#54