Skip to content

Commit 12de3c3

Browse files
arnongilboaarnongilboa
committed
Simplify DataImportCron ServiceAccount authorization
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. Signed-off-by: Arnon Gilboa <[email protected]>
1 parent d5ae91f commit 12de3c3

File tree

11 files changed

+218
-21
lines changed

11 files changed

+218
-21
lines changed

‎api/openapi-spec/swagger.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4400,6 +4400,10 @@
44004400
"type": "string",
44014401
"default": ""
44024402
},
4403+
"serviceAccountName": {
4404+
"description": "ServiceAccountName is the name of the ServiceAccount for creating DataVolumes.",
4405+
"type": "string"
4406+
},
44034407
"template": {
44044408
"description": "Template specifies template for the DVs to be created",
44054409
"default": {},

‎pkg/apis/core/v1beta1/openapi_generated.go‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/controller/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ go_library(
4040
"//vendor/github.com/pkg/errors:go_default_library",
4141
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
4242
"//vendor/github.com/robfig/cron/v3:go_default_library",
43+
"//vendor/k8s.io/api/authorization/v1:go_default_library",
4344
"//vendor/k8s.io/api/batch/v1:go_default_library",
4445
"//vendor/k8s.io/api/core/v1:go_default_library",
4546
"//vendor/k8s.io/api/networking/v1:go_default_library",
@@ -109,6 +110,7 @@ go_test(
109110
"//vendor/github.com/openshift/api/security/v1:go_default_library",
110111
"//vendor/github.com/pkg/errors:go_default_library",
111112
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
113+
"//vendor/k8s.io/api/authorization/v1:go_default_library",
112114
"//vendor/k8s.io/api/batch/v1:go_default_library",
113115
"//vendor/k8s.io/api/core/v1:go_default_library",
114116
"//vendor/k8s.io/api/networking/v1:go_default_library",

‎pkg/controller/dataimportcron-conditions.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,13 @@ import (
3030
)
3131

3232
const (
33-
noDigest = "NoDigest"
34-
noImport = "NoImport"
35-
outdated = "Outdated"
36-
scheduled = "ImportScheduled"
37-
inProgress = "ImportProgressing"
38-
upToDate = "UpToDate"
33+
noDigest = "NoDigest"
34+
noImport = "NoImport"
35+
notAuthorized = "NotAuthorized"
36+
outdated = "Outdated"
37+
scheduled = "ImportScheduled"
38+
inProgress = "ImportProgressing"
39+
upToDate = "UpToDate"
3940
)
4041

4142
func updateDataImportCronCondition(cron *cdiv1.DataImportCron, conditionType cdiv1.DataImportCronConditionType, status corev1.ConditionStatus, message, reason string) {

‎pkg/controller/dataimportcron-controller.go‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/pkg/errors"
3434
cronexpr "github.com/robfig/cron/v3"
3535

36+
authorizationv1 "k8s.io/api/authorization/v1"
3637
batchv1 "k8s.io/api/batch/v1"
3738
corev1 "k8s.io/api/core/v1"
3839
storagev1 "k8s.io/api/storage/v1"
@@ -873,7 +874,6 @@ func (r *DataImportCronReconciler) createImportDataVolume(ctx context.Context, d
873874
if err != nil {
874875
return err
875876
}
876-
dataImportCron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: digest}}
877877

878878
sources := []client.Object{&snapshotv1.VolumeSnapshot{}, &corev1.PersistentVolumeClaim{}}
879879
for _, src := range sources {
@@ -886,18 +886,72 @@ func (r *DataImportCronReconciler) createImportDataVolume(ctx context.Context, d
886886
return err
887887
}
888888
// If source exists don't create DV
889+
dataImportCron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: digest}}
889890
return nil
890891
}
891892
}
892893

893894
dv := r.newSourceDataVolume(dataImportCron, dvName)
895+
if allowed, err := r.authorizeCloneDataVolume(dataImportCron, dv); err != nil {
896+
return err
897+
} else if !allowed {
898+
updateDataImportCronCondition(dataImportCron, cdiv1.DataImportCronProgressing, corev1.ConditionFalse,
899+
"Not authorized to create DataVolume", notAuthorized)
900+
return nil
901+
}
894902
if err := r.client.Create(ctx, dv); err != nil && !k8serrors.IsAlreadyExists(err) {
895903
return err
896904
}
905+
dataImportCron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: digest}}
897906

898907
return nil
899908
}
900909

910+
func (r *DataImportCronReconciler) authorizeCloneDataVolume(dataImportCron *cdiv1.DataImportCron, dv *cdiv1.DataVolume) (bool, error) {
911+
if !isPvcSource(dataImportCron) {
912+
return true, nil
913+
}
914+
saName := "default"
915+
if dataImportCron.Spec.ServiceAccountName != nil {
916+
saName = *dataImportCron.Spec.ServiceAccountName
917+
}
918+
if resp, err := dv.AuthorizeSA(dv.Namespace, dv.Name, &authProxy{r.client}, dataImportCron.Namespace, saName); err != nil {
919+
return false, err
920+
} else if !resp.Allowed {
921+
r.log.Info("Not authorized to create DataVolume", "cron", dataImportCron.Name, "reason", resp.Reason)
922+
return false, nil
923+
}
924+
925+
return true, nil
926+
}
927+
928+
type authProxy struct {
929+
client client.Client
930+
}
931+
932+
func (p *authProxy) CreateSar(sar *authorizationv1.SubjectAccessReview) (*authorizationv1.SubjectAccessReview, error) {
933+
if err := p.client.Create(context.TODO(), sar); err != nil {
934+
return nil, err
935+
}
936+
return sar, nil
937+
}
938+
939+
func (p *authProxy) GetNamespace(name string) (*corev1.Namespace, error) {
940+
ns := &corev1.Namespace{}
941+
if err := p.client.Get(context.TODO(), types.NamespacedName{Name: name}, ns); err != nil {
942+
return nil, err
943+
}
944+
return ns, nil
945+
}
946+
947+
func (p *authProxy) GetDataSource(namespace, name string) (*cdiv1.DataSource, error) {
948+
das := &cdiv1.DataSource{}
949+
if err := p.client.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: name}, das); err != nil {
950+
return nil, err
951+
}
952+
return das, nil
953+
}
954+
901955
func (r *DataImportCronReconciler) handleStorageClassChange(ctx context.Context, dataImportCron *cdiv1.DataImportCron, desiredStorageClass string) error {
902956
digest, ok := dataImportCron.Annotations[AnnSourceDesiredDigest]
903957
if !ok {

‎pkg/controller/dataimportcron-controller_test.go‎

Lines changed: 97 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/pkg/errors"
3434
"github.com/prometheus/client_golang/prometheus"
3535

36+
authorizationv1 "k8s.io/api/authorization/v1"
3637
batchv1 "k8s.io/api/batch/v1"
3738
corev1 "k8s.io/api/core/v1"
3839
storagev1 "k8s.io/api/storage/v1"
@@ -911,18 +912,86 @@ var _ = Describe("All DataImportCron Tests", func() {
911912
Expect(err.Error()).To(ContainSubstring("not found"))
912913
})
913914

914-
It("Should succeed with existing source PVC", func() {
915-
pvc := newPVC("test-pvc")
915+
It("Should create DataVolume when source PVC is in the same namespace", func() {
916+
sourcePvc := newPVC("source-pvc", metav1.NamespaceDefault)
916917
cron := newDataImportCron(cronName)
917918
cron.Spec.Template.Spec.Source = &cdiv1.DataVolumeSource{
918919
PVC: &cdiv1.DataVolumeSourcePVC{
919-
Name: pvc.Name,
920-
Namespace: pvc.Namespace,
920+
Name: sourcePvc.Name,
921+
Namespace: sourcePvc.Namespace,
921922
},
922923
}
923-
reconciler = createDataImportCronReconciler(cron, pvc)
924+
925+
reconciler = createDataImportCronReconciler(cron, sourcePvc,
926+
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: sourcePvc.Namespace}})
927+
setFakeSarClient(reconciler, false)
924928
_, err := reconciler.Reconcile(context.TODO(), cronReq)
925929
Expect(err).ToNot(HaveOccurred())
930+
931+
err = reconciler.client.Get(context.TODO(), cronKey, cron)
932+
Expect(err).ToNot(HaveOccurred())
933+
934+
imports := cron.Status.CurrentImports
935+
Expect(imports).ToNot(BeEmpty())
936+
dvName := imports[0].DataVolumeName
937+
Expect(dvName).ToNot(BeEmpty())
938+
dv := &cdiv1.DataVolume{}
939+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
940+
Expect(err).ToNot(HaveOccurred())
941+
})
942+
943+
It("Should create DataVolume with cross-namespace source PVC when ServiceAccount is authorized", func() {
944+
sourcePvc := newPVC("source-pvc", "source-ns")
945+
cron := newDataImportCron(cronName)
946+
cron.Spec.Template.Spec.Source = &cdiv1.DataVolumeSource{
947+
PVC: &cdiv1.DataVolumeSourcePVC{
948+
Name: sourcePvc.Name,
949+
Namespace: sourcePvc.Namespace,
950+
},
951+
}
952+
953+
reconciler = createDataImportCronReconciler(cron, sourcePvc,
954+
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: sourcePvc.Namespace}})
955+
setFakeSarClient(reconciler, true)
956+
_, err := reconciler.Reconcile(context.TODO(), cronReq)
957+
Expect(err).ToNot(HaveOccurred())
958+
959+
err = reconciler.client.Get(context.TODO(), cronKey, cron)
960+
Expect(err).ToNot(HaveOccurred())
961+
962+
imports := cron.Status.CurrentImports
963+
Expect(imports).ToNot(BeEmpty())
964+
dvName := imports[0].DataVolumeName
965+
Expect(dvName).ToNot(BeEmpty())
966+
dv := &cdiv1.DataVolume{}
967+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
968+
Expect(err).ToNot(HaveOccurred())
969+
})
970+
971+
It("Should not create DataVolume with cross-namespace source PVC when ServiceAccount is not authorized", func() {
972+
sourcePvc := newPVC("source-pvc", "source-ns")
973+
cron := newDataImportCron(cronName)
974+
cron.Spec.Template.Spec.Source = &cdiv1.DataVolumeSource{
975+
PVC: &cdiv1.DataVolumeSourcePVC{
976+
Name: sourcePvc.Name,
977+
Namespace: sourcePvc.Namespace,
978+
},
979+
}
980+
981+
reconciler = createDataImportCronReconciler(cron, sourcePvc,
982+
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: sourcePvc.Namespace}})
983+
setFakeSarClient(reconciler, false)
984+
_, err := reconciler.Reconcile(context.TODO(), cronReq)
985+
Expect(err).ToNot(HaveOccurred())
986+
987+
err = reconciler.client.Get(context.TODO(), cronKey, cron)
988+
Expect(err).ToNot(HaveOccurred())
989+
990+
cronCond := FindDataImportCronConditionByType(cron, cdiv1.DataImportCronProgressing)
991+
Expect(cronCond).ToNot(BeNil())
992+
Expect(cronCond.ConditionState.Message).To(Equal("Not authorized to create DataVolume"))
993+
Expect(cronCond.ConditionState.Reason).To(Equal("NotAuthorized"))
994+
Expect(cron.Status.CurrentImports).To(BeEmpty())
926995
})
927996

928997
It("Should succeed garbage collecting old version DVs", func() {
@@ -1601,22 +1670,43 @@ func createDataImportCronReconcilerWithoutConfig(objects ...runtime.Object) *Dat
16011670
return r
16021671
}
16031672

1673+
func setFakeSarClient(reconciler *DataImportCronReconciler, allowed bool) {
1674+
client := &fakeSarClient{Client: reconciler.client, allowed: allowed}
1675+
reconciler.client = client
1676+
reconciler.uncachedClient = client
1677+
}
1678+
1679+
type fakeSarClient struct {
1680+
client.Client
1681+
allowed bool
1682+
}
1683+
1684+
func (s *fakeSarClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
1685+
if sar, ok := obj.(*authorizationv1.SubjectAccessReview); ok {
1686+
sar.GenerateName = "test-sar"
1687+
sar.Status.Allowed = s.allowed
1688+
return s.Client.Create(ctx, sar, opts...)
1689+
}
1690+
return s.Client.Create(ctx, obj, opts...)
1691+
}
1692+
16041693
func newDataImportCronWithImageStream(dataImportCronName, taggedImageStreamName string) *cdiv1.DataImportCron {
16051694
cron := newDataImportCron(dataImportCronName)
16061695
cron.Spec.Template.Spec.Source.Registry.ImageStream = &taggedImageStreamName
16071696
cron.Spec.Template.Spec.Source.Registry.URL = nil
16081697
return cron
16091698
}
16101699

1611-
func newPVC(name string) *corev1.PersistentVolumeClaim {
1700+
func newPVC(name, namespace string) *corev1.PersistentVolumeClaim {
16121701
return &corev1.PersistentVolumeClaim{
16131702
ObjectMeta: metav1.ObjectMeta{
16141703
Name: name,
1615-
Namespace: metav1.NamespaceDefault,
1704+
Namespace: namespace,
16161705
UID: types.UID(metav1.NamespaceDefault + "-" + name),
16171706
},
16181707
}
16191708
}
1709+
16201710
func newImageStream(name string) *imagev1.ImageStream {
16211711
return &imagev1.ImageStream{
16221712
TypeMeta: metav1.TypeMeta{APIVersion: imagev1.SchemeGroupVersion.String()},

‎pkg/operator/resources/cluster/controller.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,19 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
117117
"delete",
118118
},
119119
},
120+
{
121+
APIGroups: []string{
122+
"",
123+
},
124+
Resources: []string{
125+
"namespaces",
126+
},
127+
Verbs: []string{
128+
"get",
129+
"list",
130+
"watch",
131+
},
132+
},
120133
{
121134
APIGroups: []string{
122135
"",
@@ -271,6 +284,17 @@ func getControllerClusterPolicyRules() []rbacv1.PolicyRule {
271284
"update",
272285
},
273286
},
287+
{
288+
APIGroups: []string{
289+
"authorization.k8s.io",
290+
},
291+
Resources: []string{
292+
"subjectaccessreviews",
293+
},
294+
Verbs: []string{
295+
"create",
296+
},
297+
},
274298
}
275299
}
276300

‎pkg/operator/resources/crds_generated.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,10 @@ type DataImportCronSpec struct {
596596
// RetentionPolicy specifies whether the created DataVolumes and DataSources are retained when their DataImportCron is deleted. Default is RatainAll.
597597
// +optional
598598
RetentionPolicy *DataImportCronRetentionPolicy `json:"retentionPolicy,omitempty"`
599+
// ServiceAccountName is the name of the ServiceAccount for creating DataVolumes.
600+
// +optional
601+
// +kubebuilder:validation:MinLength=1
602+
ServiceAccountName *string `json:"serviceAccountName,omitempty"`
599603
}
600604

601605
// DataImportCronGarbageCollect represents the DataImportCron garbage collection mode

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

Lines changed: 8 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)