feat: use velero SA and add bound-sa-token volume for KDM controller - #2349
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe KubeVirt datamover now uses the ChangesKubeVirt datamover STS authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KubeVirtDatamoverController
participant KubeVirtDatamoverDeployment
participant OpenShiftSTS
KubeVirtDatamoverController->>KubeVirtDatamoverDeployment: Create deployment with velero service account
KubeVirtDatamoverController->>KubeVirtDatamoverDeployment: Mount projected token at /var/run/secrets/openshift/serviceaccount
KubeVirtDatamoverDeployment->>OpenShiftSTS: Use openshift-audience token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/kubevirt_datamover_controller_test.go`:
- Around line 952-963: The volume assertions in both test blocks should validate
the ServiceAccountTokenProjection fields, not only that Projected is non-nil.
Extend the checks around hasBoundSATokenVolume to require Audience "openshift",
ExpirationSeconds 3600, and Path "token", while preserving the existing
volume-name and projected-source requirements.
- Around line 976-986: The test currently verifies the bound service-account
mount only when no existing containers are provided, leaving the
existing-manager reconciliation path uncovered. Add or update a test case with
an existing “manager” container, run reconciliation, then apply the same
volume-mount assertions to the updated manager container, including name, mount
path, and read-only status.
In `@internal/controller/kubevirt_datamover_controller.go`:
- Around line 267-271: The existing-manager reconciliation path does not add the
projected STS token mount. In
internal/controller/kubevirt_datamover_controller.go:267-271, update the
existing manager container branch to upsert the bound-sa-token mount at the
configured path with read-only access, reusing the same mount definition as
newly created managers. In
internal/controller/kubevirt_datamover_controller_test.go:976-986, extend the
existing manager container test to assert that this mount is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71e04d4e-08bd-462d-9fee-c80fe3bc4a9d
📒 Files selected for processing (3)
bundle/manifests/oadp-operator.clusterserviceversion.yamlinternal/controller/kubevirt_datamover_controller.gointernal/controller/kubevirt_datamover_controller_test.go
| // Verify bound-sa-token projected volume is present on pod spec | ||
| hasBoundSATokenVolume := false | ||
| for _, v := range volumes { | ||
| if v.Name == "bound-sa-token" && v.VolumeSource.Projected != nil { | ||
| hasBoundSATokenVolume = true | ||
| break | ||
| } | ||
| } | ||
| if !hasBoundSATokenVolume { | ||
| t.Error("expected bound-sa-token projected volume on pod spec") | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the token projection fields.
The new checks only require VolumeSource.Projected != nil. They do not verify ServiceAccountTokenProjection. Assert Audience == "openshift", ExpirationSeconds == 3600, and Path == "token" in both test blocks. A wrong token file contract would pass these tests and break STS authentication.
Also applies to: 1252-1274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/kubevirt_datamover_controller_test.go` around lines 952 -
963, The volume assertions in both test blocks should validate the
ServiceAccountTokenProjection fields, not only that Projected is non-nil. Extend
the checks around hasBoundSATokenVolume to require Audience "openshift",
ExpirationSeconds 3600, and Path "token", while preserving the existing
volume-name and projected-source requirements.
|
|
||
| hasBoundSATokenMount := false | ||
| for _, vm := range container.VolumeMounts { | ||
| if vm.Name == "bound-sa-token" && vm.MountPath == "/var/run/secrets/openshift/serviceaccount" && vm.ReadOnly { | ||
| hasBoundSATokenMount = true | ||
| break | ||
| } | ||
| } | ||
| if !hasBoundSATokenMount { | ||
| t.Error("expected bound-sa-token volumeMount on container") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the existing-manager reconciliation path.
This assertion is guarded by len(tt.existingContainers) == 0. The existing-manager test can pass without the bound-sa-token mount. Add a case that verifies the mount after updating an existing manager container.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/kubevirt_datamover_controller_test.go` around lines 976 -
986, The test currently verifies the bound service-account mount only when no
existing containers are provided, leaving the existing-manager reconciliation
path uncovered. Add or update a test case with an existing “manager” container,
run reconciliation, then apply the same volume-mount assertions to the updated
manager container, including name, mount path, and read-only status.
72e3610 to
d9f4514
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bundle/manifests/oadp-operator.clusterserviceversion.yaml (1)
1252-1371: 🔒 Security & Privacy | 🔵 TrivialDocument the KubeVirt DataMover RBAC blast radius.
The controller uses
serviceAccountName: "velero";oadp-kubevirt-datamover-controller-manageris the Deployment name, not a ServiceAccount reference. Document theveleroServiceAccount’s wildcard cluster-wide permissions and the STS trust requirement in the security review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bundle/manifests/oadp-operator.clusterserviceversion.yaml` around lines 1252 - 1371, Update the security review documentation associated with the KubeVirt DataMover RBAC manifest to identify serviceAccountName "velero" as the controller identity, not the oadp-kubevirt-datamover-controller-manager Deployment. Document that this ServiceAccount has wildcard cluster-wide permissions and explicitly record the required STS trust relationship.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bundle/manifests/oadp-operator.clusterserviceversion.yaml`:
- Around line 1252-1371: Update the security review documentation associated
with the KubeVirt DataMover RBAC manifest to identify serviceAccountName
"velero" as the controller identity, not the
oadp-kubevirt-datamover-controller-manager Deployment. Document that this
ServiceAccount has wildcard cluster-wide permissions and explicitly record the
required STS trust relationship.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0b7f748-80e2-4b91-8327-5699d4dd8c92
📒 Files selected for processing (7)
bundle/manifests/oadp-operator.clusterserviceversion.yamlconfig/kubevirt-datamover-controller_rbac/kustomization.yamlconfig/kubevirt-datamover-controller_rbac/leader_election_role_binding.yamlconfig/kubevirt-datamover-controller_rbac/metrics_auth_role_binding.yamlconfig/kubevirt-datamover-controller_rbac/role_binding.yamlinternal/controller/kubevirt_datamover_controller.gointernal/controller/kubevirt_datamover_controller_test.go
💤 Files with no reviewable changes (1)
- config/kubevirt-datamover-controller_rbac/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/kubevirt_datamover_controller.go
- internal/controller/kubevirt_datamover_controller_test.go
STS Testing PassedTested end-to-end on an AWS STS cluster (OCP 4.22, CCO Manual mode) with a cirros VM backup.
Full test report: migtools/kubevirt-datamover-controller#24 (comment) |
|
/retest |
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
Didn't we just get suggested by the Telco team to use a separate service account for RBAC between different controllers?
There was a problem hiding this comment.
This PR slightly contradicts #2318
but it does pass https://redhat-best-practices-for-k8s.github.io/guide/#k8s-best-practices-automount-services-for-pods https://github.com/redhat-best-practices-for-k8s/certsuite/blob/main/CATALOG.md#access-control-pod-automount-service-account-token
But the KubeVirt Data Mover is highly coupled to using Velero Backup, unlike the CLI Server, so this may be okay.
|
/cherry-pick oadp-1.6 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of 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. |
|
need rebase: #2352 merged |
openshift-ci
Bot
added
the
needs-rebase
On OpenShift STS clusters, pods authenticate to AWS using projected service account tokens via the Cloud Credential Operator. The KDM controller deployment was using a separate service account that had no IAM trust configured, and was missing the bound-sa-token projected volume needed for STS authentication. Switch the KDM controller deployment to use the velero service account (which already has IAM trust on STS clusters) and add the bound-sa-token projected volume and mount. KDM is always deployed by OADP and never standalone, so a separate SA is unnecessary. Update kustomize configs to bind KDM RBAC permissions to the velero SA. Fixes: migtools/kubevirt-datamover-controller#24 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
d9f4514 to
0aa7038
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
removed
the
needs-rebase
|
/lgtm |
|
@shubham-pampattiwar: all tests passed! 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, sseago 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 |
|
@kaovilai: new pull request created: #2359 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. |
Resolving the CSV merge conflict against upstream's velero-SA RBAC restructuring (PR openshift#2349) by taking upstream's side lost a small generated permission block (events create/patch). make bundle regenerates it correctly. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
4 participants
Summary
oadp-kubevirt-datamover-controller-managerSA toveleroSA, since KDM is always deployed by OADP and the velero SA already has IAM trust configured on STS clustersbound-sa-tokenprojected volume and mount to the KDM controller deployment for STS authenticationserviceAccountNameentries to bind KDM RBAC permissions to theveleroSATest plan
go test ./internal/controller/ -run "TestEnsureKubevirtDatamoverRequiredSpecs|TestBuildKubevirtDatamoverDeployment")veleroSACompanion PR: migtools/kubevirt-datamover-controller#158 - adds bound-sa-token volume to datamover pod
Parent issue: migtools/kubevirt-datamover-controller#25
Fixes: migtools/kubevirt-datamover-controller#24
🤖 Generated with Claude Code
Summary by CodeRabbit