Simplify DataImportCron ServiceAccount authorization #3970

arnongilboa · 2025-12-03T10:40:48Z

What this PR does / why we need it:
Add ServiceAccountName to DataImportCron spec, replacing CreatedBy which was added in #3946.

In case of DataImportCron with PVC source, the controller checks the ServiceAccount is authorized to clone the source PVC.

Release note:

Simplify DataImportCron ServiceAccount authorization

kubevirt-bot · 2025-12-03T10:41:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign aglitke for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akalenyu · 2025-12-03T12:05:36Z

/test pull-cdi-linter
/test pull-containerized-data-importer-e2e-nfs
unrelated issues with ci

akalenyu · 2025-12-03T12:19:01Z

@arnongilboa real linter issues in there

akalenyu

skimmed through, looks good, will try to properly go over this later and find releases containing this API

Just a note, this might've been easier to review if it was split to

revert commit
reintroduce with new api

arnongilboa · 2025-12-03T13:09:03Z

skimmed through, looks good, will try to properly go over this later and find releases containing this API

Just a note, this might've been easier to review if it was split to

revert commit

reintroduce with new api

sure, fixing.

…ubevirt#3946)" This reverts commit b578f2a. Signed-off-by: Arnon Gilboa <[email protected]>

In case of DataImportCron with PVC source, the controller checks the namespace default ServiceAccount is authorized to clone the source PVC. This is a minimally partial backport of kubevirt#3970. Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2025-12-03T19:59:49Z

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

+	// ServiceAccountName is the name of the ServiceAccount for creating DataVolumes.
 	// +optional
-	CreatedBy *string `json:"createdBy,omitempty"`
+	ServiceAccountName string `json:"serviceAccountName,omitempty"`


Should this be a pointer?

In PodSpec it's ServiceAccountName string. Afaik this is the standard choice in Kubernetes, as the zero value ("") represents the "not set" state when combined with the omitempty tag.

Pod spec has been around for a long time, likely predating API conventions

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Maybe make it a pointer and add: +kubebuilder:validation:MinLength=1

mhenriks · 2025-12-03T20:08:09Z

pkg/controller/dataimportcron-controller.go

-
-	var userInfo authenticationv1.UserInfo
-	if err := json.Unmarshal([]byte(*createdBy), &userInfo); err != nil {
+	if resp, err := dv.AuthorizeSA(dv.Namespace, dv.Name, r, dataImportCron.Namespace, dataImportCron.Spec.ServiceAccountName); err != nil {


Don't pass unset ServiceAccountName. See: https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm/vm.go#L490-L495

Sure, I'll use "default" in this case, but note that in your link if somehow vol.ServiceAccount.ServiceAccountName == "" it will also be used instead of "default".

pretty sure you can't have a serviceaccount volume with empty sa name

arnongilboa · 2025-12-04T11:30:05Z

/retest

akalenyu

Looks good, some comments, but the approach looks ok for me

akalenyu · 2025-12-04T14:28:47Z

tests/dataimportcron_test.go

 		})
 	})
+
+	Context("DataImportCron controller authorization when cloning from PVC source", func() {


bit of a shame to use functional tests for this, it's all just a bunch of APIs that could be faked

Agree. I just recycled the tests of the previous PR which involved the webhooks, so I felt func tests are appropriate. I'll sure change to utests.

akalenyu · 2025-12-04T14:30:28Z

pkg/controller/dataimportcron-controller.go

+	return true, nil
+}
+
+func (r *DataImportCronReconciler) CreateSar(sar *authorizationv1.SubjectAccessReview) (*authorizationv1.SubjectAccessReview, error) {


I think it'll read nicer if there was a separate struct for AuthorizationHelperProxy impl

Sure, I'll fix.

Add ServiceAccountName to DataImportCron spec, replacing CreatedBy which was added in kubevirt#3946. In case of DataImportCron with PVC source, the controller checks the ServiceAccount is authorized to clone the source PVC. Signed-off-by: Arnon Gilboa <[email protected]>

kubevirt-bot · 2025-12-07T17:27:42Z

@arnongilboa: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cdi-linter	`12de3c3`	link	false	`/test pull-cdi-linter`

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. I understand the commands that are listed here.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 3, 2025

kubevirt-bot requested review from akalenyu and awels December 3, 2025 10:41

kubevirt-bot added the size/L label Dec 3, 2025

akalenyu reviewed Dec 3, 2025

View reviewed changes

arnongilboa force-pushed the dic_sa branch from 3191362 to 278b72b Compare December 3, 2025 14:00

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 3, 2025

Revert "Authorize DataImportCron PVC clone based on creator UserInfo (k…

d5ae91f

…ubevirt#3946)" This reverts commit b578f2a. Signed-off-by: Arnon Gilboa <[email protected]>

arnongilboa force-pushed the dic_sa branch from 278b72b to afe85e4 Compare December 3, 2025 14:03

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Dec 3, 2025

arnongilboa force-pushed the dic_sa branch from afe85e4 to c72679a Compare December 3, 2025 14:36

arnongilboa mentioned this pull request Dec 3, 2025

[release-v1.63] Authorize default SA on DataImportCron PVC source #3971

Open

arnongilboa mentioned this pull request Dec 3, 2025

[release-v1.62] Authorize default SA on DataImportCron PVC source #3972

Open

mhenriks reviewed Dec 3, 2025

View reviewed changes

arnongilboa force-pushed the dic_sa branch 2 times, most recently from 5d5b676 to 32acdbf Compare December 4, 2025 09:08

akalenyu reviewed Dec 4, 2025

View reviewed changes

arnongilboa force-pushed the dic_sa branch from 32acdbf to 12de3c3 Compare December 7, 2025 16:21

kubevirt-bot added size/XL and removed size/L labels Dec 7, 2025

Simplify DataImportCron ServiceAccount authorization #3970

Are you sure you want to change the base?

Simplify DataImportCron ServiceAccount authorization #3970

Uh oh!

Conversation

arnongilboa commented Dec 3, 2025

Uh oh!

kubevirt-bot commented Dec 3, 2025

Uh oh!

akalenyu commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akalenyu commented Dec 3, 2025

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

arnongilboa commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa commented Dec 4, 2025

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubevirt-bot commented Dec 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akalenyu commented Dec 3, 2025 •

edited

Loading