Merge pull request kubevirt#10438 from lyarwood/instancetype-view-clu…

…sterrole

rbac: Add instancetype.kubevirt.io:view ClusterRole

manifests/generated/operator-csv.yaml.in

@@ -1233,6 +1233,15 @@ spec:
           - get
           - list
           - watch
+        - apiGroups:
+          - instancetype.kubevirt.io
+          resources:
+          - virtualmachineclusterinstancetypes
+          - virtualmachineclusterpreferences
+          verbs:
+          - get
+          - list
+          - watch
         - apiGroups:
           - authentication.k8s.io
           resources:

manifests/generated/rbac-operator.authorization.k8s.yaml.in

@@ -1173,6 +1173,15 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - instancetype.kubevirt.io
+  resources:
+  - virtualmachineclusterinstancetypes
+  - virtualmachineclusterpreferences
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - authentication.k8s.io
   resources:

pkg/virt-operator/kubevirt_test.go

@@ -85,9 +85,9 @@ const (
 
 	NAMESPACE = "kubevirt-test"
 
-	resourceCount = 75
+	resourceCount = 76
 	patchCount    = 50
-	updateCount   = 26
+	updateCount   = 27
 )
 
 type KubeVirtTestData struct {
@@ -2425,7 +2425,7 @@ var _ = Describe("KubeVirt Operator", func() {
 			Expect(kvTestData.totalAdds).To(Equal(resourceCount - expectedUncreatedResources + expectedTemporaryResources))
 
 			Expect(kvTestData.controller.stores.ServiceAccountCache.List()).To(HaveLen(4))
-			Expect(kvTestData.controller.stores.ClusterRoleCache.List()).To(HaveLen(8))
+			Expect(kvTestData.controller.stores.ClusterRoleCache.List()).To(HaveLen(9))
 			Expect(kvTestData.controller.stores.ClusterRoleBindingCache.List()).To(HaveLen(6))
 			Expect(kvTestData.controller.stores.RoleCache.List()).To(HaveLen(5))
 			Expect(kvTestData.controller.stores.RoleBindingCache.List()).To(HaveLen(5))

pkg/virt-operator/resource/generate/rbac/cluster.go

@@ -55,6 +55,7 @@ func GetAllCluster() []runtime.Object {
 		newAdminClusterRole(),
 		newEditClusterRole(),
 		newViewClusterRole(),
+		newInstancetypeViewClusterRole(),
 	}
 }
 
@@ -631,3 +632,28 @@ func newViewClusterRole() *rbacv1.ClusterRole {
 		},
 	}
 }
+func newInstancetypeViewClusterRole() *rbacv1.ClusterRole {
+	return &rbacv1.ClusterRole{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: VersionNamev1,
+			Kind:       "ClusterRole",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "instancetype.kubevirt.io:view",
+		},
+		Rules: []rbacv1.PolicyRule{
+			{
+				APIGroups: []string{
+					GroupNameInstancetype,
+				},
+				Resources: []string{
+					instancetype.ClusterPluralResourceName,
+					instancetype.ClusterPluralPreferenceResourceName,
+				},
+				Verbs: []string{
+					"get", "list", "watch",
+				},
+			},
+		},
+	}
+}

tests/access_test.go

@@ -39,6 +39,7 @@ import (
 	"kubevirt.io/api/core"
 
 	v1 "kubevirt.io/api/core/v1"
+	instancetypeapi "kubevirt.io/api/instancetype"
 	pool "kubevirt.io/api/pool"
 	"kubevirt.io/api/snapshot/v1alpha1"
 )
@@ -164,12 +165,13 @@ var _ = Describe("[rfe_id:500][crit:high][arm64][vendor:[email protected]][level
 	var k8sClient string
 	var authClient *authClientV1.AuthorizationV1Client
 
-	doSarRequest := func(group string, resource string, subresource string, namespace string, role string, verb string, expected bool) {
+	doSarRequest := func(group string, resource string, subresource string, namespace string, role string, verb string, expected, clusterWide bool) {
 		roleToUser := map[string]string{
-			"view":    testsuite.ViewServiceAccountName,
-			"edit":    testsuite.EditServiceAccountName,
-			"admin":   testsuite.AdminServiceAccountName,
-			"default": "default",
+			"view":              testsuite.ViewServiceAccountName,
+			"instancetype:view": testsuite.ViewInstancetypeServiceAccountName,
+			"edit":              testsuite.EditServiceAccountName,
+			"admin":             testsuite.AdminServiceAccountName,
+			"default":           "default",
 		}
 		userName, exists := roleToUser[role]
 		Expect(exists).To(BeTrue(), fmt.Sprintf("role %s is not defined", role))
@@ -178,22 +180,26 @@ var _ = Describe("[rfe_id:500][crit:high][arm64][vendor:[email protected]][level
 		sar.Spec = authv1.SubjectAccessReviewSpec{
 			User: user,
 			ResourceAttributes: &authv1.ResourceAttributes{
-				Namespace:   namespace,
 				Verb:        verb,
 				Group:       group,
 				Version:     v1.GroupVersion.Version,
 				Resource:    resource,
 				Subresource: subresource,
 			},
 		}
+		if !clusterWide {
+			sar.Spec.ResourceAttributes.Namespace = namespace
+		}
+
 		result, err := authClient.SubjectAccessReviews().Create(context.Background(), sar, metav1.CreateOptions{})
 		Expect(err).ToNot(HaveOccurred())
-		Expect(result.Status.Allowed).To(Equal(expected), fmt.Sprintf("access check for user '%v' on resource '%v' with subresource '%v' for verb '%v' should have returned '%v'.",
+		Expect(result.Status.Allowed).To(Equal(expected), fmt.Sprintf("access check for user '%v' on resource '%v' with subresource '%v' for verb '%v' should have returned '%v'. Gave reason %s.",
 			user,
 			resource,
 			subresource,
 			verb,
 			expected,
+			result.Status.Reason,
 		))
 	}
 
@@ -207,84 +213,142 @@ var _ = Describe("[rfe_id:500][crit:high][arm64][vendor:[email protected]][level
 	})
 
 	Describe("With default kubevirt service accounts", func() {
-		DescribeTable("should verify permissions on resources are correct for view, edit, and admin", func(group string, resource string, accessRights ...rights) {
+		DescribeTable("should verify permissions on resources are correct for view, edit, and admin", func(group string, resource string, clusterWide bool, accessRights ...rights) {
 			namespace := testsuite.GetTestNamespace(nil)
 			for _, accessRight := range accessRights {
 				for _, entry := range accessRight.list() {
 					By(fmt.Sprintf("verifying sa %s for verb %s on resource %s", entry.role, entry.verb, resource))
-					doSarRequest(group, resource, "", namespace, entry.role, entry.verb, entry.allowed)
+					doSarRequest(group, resource, "", namespace, entry.role, entry.verb, entry.allowed, clusterWide)
 				}
 			}
 		},
 			Entry("[test_id:526]given a vmi",
 				core.GroupName,
 				"virtualmachineinstances",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:527]given a vm",
 				core.GroupName,
 				"virtualmachines",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("given a vmpool",
 				pool.GroupName,
 				"virtualmachinepools",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:528]given a vmi preset",
 				core.GroupName,
 				"virtualmachineinstancepresets",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:529][crit:low]given a vmi replica set",
 				core.GroupName,
 				"virtualmachineinstancereplicasets",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:3230]given a vmi migration",
 				core.GroupName,
 				"virtualmachineinstancemigrations",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:5243]given a vmsnapshot",
 				v1alpha1.SchemeGroupVersion.Group,
 				"virtualmachinesnapshots",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 
 			Entry("[test_id:5244]given a vmsnapshotcontent",
 				v1alpha1.SchemeGroupVersion.Group,
 				"virtualmachinesnapshotcontents",
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
 				denyAllFor("default")),
 			Entry("[test_id:5245]given a vmsrestore",
 				v1alpha1.SchemeGroupVersion.Group,
 				"virtualmachinerestores",
+				false,
+				allowAllFor("admin"),
+				denyDeleteCollectionFor("edit"),
+				denyModificationsFor("view"),
+				denyAllFor("instancetype:view"),
+				denyAllFor("default")),
+			Entry("[test_id:TODO]given a virtualmachineinstancetype",
+				instancetypeapi.GroupName,
+				instancetypeapi.PluralResourceName,
+				false,
 				allowAllFor("admin"),
 				denyDeleteCollectionFor("edit"),
 				denyModificationsFor("view"),
+				// instancetype:view only provides access to the cluster-scoped resources
+				denyAllFor("instancetype:view"),
+				denyAllFor("default")),
+			Entry("[test_id:TODO]given a virtualmachinepreference",
+				instancetypeapi.GroupName,
+				instancetypeapi.PluralPreferenceResourceName,
+				false,
+				allowAllFor("admin"),
+				denyDeleteCollectionFor("edit"),
+				denyModificationsFor("view"),
+				// instancetype:view only provides access to the cluster-scoped resources
+				denyAllFor("instancetype:view"),
+				denyAllFor("default")),
+			Entry("[test_id:TODO]given a virtualmachineclusterinstancetype",
+				instancetypeapi.GroupName,
+				instancetypeapi.ClusterPluralResourceName,
+				// only ClusterRoles bound with a ClusterRoleBinding should have access
+				true,
+				denyAllFor("admin"),
+				denyAllFor("edit"),
+				denyAllFor("view"),
+				denyModificationsFor("instancetype:view"),
+				denyAllFor("default")),
+			Entry("[test_id:TODO]given a virtualmachineclusterpreference",
+				instancetypeapi.GroupName,
+				instancetypeapi.ClusterPluralResourceName,
+				// only ClusterRoles bound with a ClusterRoleBinding should have access
+				true,
+				denyAllFor("admin"),
+				denyAllFor("edit"),
+				denyAllFor("view"),
+				denyModificationsFor("instancetype:view"),
 				denyAllFor("default")),
 		)
 
@@ -293,7 +357,7 @@ var _ = Describe("[rfe_id:500][crit:high][arm64][vendor:[email protected]][level
 			for _, accessRight := range accessRights {
 				for _, entry := range accessRight.list() {
 					By(fmt.Sprintf("verifying sa %s for verb %s on resource %s on subresource %s", entry.role, entry.verb, resource, subresource))
-					doSarRequest(v1.SubresourceGroupName, resource, subresource, namespace, entry.role, entry.verb, entry.allowed)
+					doSarRequest(v1.SubresourceGroupName, resource, subresource, namespace, entry.role, entry.verb, entry.allowed, false)
 				}
 			}
 		},

tests/testsuite/serviceaccount.go

@@ -21,6 +21,9 @@ package testsuite
 
 import (
 	"context"
+	"fmt"
+
+	. "github.com/onsi/ginkgo/v2"
 
 	"kubevirt.io/kubevirt/tests/framework/kubevirt"
 
@@ -36,10 +39,17 @@ const (
 	AdminServiceAccountName                   = "kubevirt-admin-test-sa"
 	EditServiceAccountName                    = "kubevirt-edit-test-sa"
 	ViewServiceAccountName                    = "kubevirt-view-test-sa"
+	ViewInstancetypeServiceAccountName        = "kubevirt-instancetype-view-test-sa"
 	SubresourceServiceAccountName             = "kubevirt-subresource-test-sa"
 	SubresourceUnprivilegedServiceAccountName = "kubevirt-subresource-test-unprivileged-sa"
 )
 
+// As our tests run in parallel we need to ensure each worker creates a
+// unique clusterRoleBinding to avoid cleaning up anothers prematurely
+func getClusterRoleBindingName(saName string) string {
+	return fmt.Sprintf("%s-%d", saName, GinkgoParallelProcess())
+}
+
 func createServiceAccounts() {
 	createServiceAccount(AdminServiceAccountName)
 	createRoleBinding(AdminServiceAccountName, "kubevirt.io:admin")
@@ -50,6 +60,9 @@ func createServiceAccounts() {
 	createServiceAccount(ViewServiceAccountName)
 	createRoleBinding(ViewServiceAccountName, "kubevirt.io:view")
 
+	createServiceAccount(ViewInstancetypeServiceAccountName)
+	createClusterRoleBinding(ViewInstancetypeServiceAccountName, "instancetype.kubevirt.io:view")
+
 	createServiceAccount(SubresourceServiceAccountName)
 	createSubresourceRole(SubresourceServiceAccountName)
 
@@ -60,6 +73,7 @@ func cleanupServiceAccounts() {
 	cleanupServiceAccount(AdminServiceAccountName)
 	cleanupServiceAccount(EditServiceAccountName)
 	cleanupServiceAccount(ViewServiceAccountName)
+	cleanupServiceAccount(ViewInstancetypeServiceAccountName)
 	cleanupServiceAccount(SubresourceServiceAccountName)
 	cleanupServiceAccount(SubresourceUnprivilegedServiceAccountName)
 }
@@ -96,6 +110,36 @@ func createServiceAccount(saName string) {
 	}
 }
 
+func createClusterRoleBinding(saName string, clusterRole string) {
+	virtCli := kubevirt.Client()
+
+	clusterRoleBinding := rbacv1.ClusterRoleBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: getClusterRoleBindingName(saName),
+			Labels: map[string]string{
+				util.KubevirtIoTest: saName,
+			},
+		},
+		RoleRef: rbacv1.RoleRef{
+			Kind:     "ClusterRole",
+			Name:     clusterRole,
+			APIGroup: "rbac.authorization.k8s.io",
+		},
+		Subjects: []rbacv1.Subject{
+			{
+				Kind:      "ServiceAccount",
+				Name:      saName,
+				Namespace: GetTestNamespace(nil),
+			},
+		},
+	}
+
+	_, err := virtCli.RbacV1().ClusterRoleBindings().Create(context.Background(), &clusterRoleBinding, metav1.CreateOptions{})
+	if !k8serrors.IsAlreadyExists(err) {
+		util.PanicOnError(err)
+	}
+}
+
 func createRoleBinding(saName string, clusterRole string) {
 	virtCli := kubevirt.Client()
 
@@ -199,4 +243,9 @@ func cleanupServiceAccount(saName string) {
 	if !k8serrors.IsNotFound(err) {
 		util.PanicOnError(err)
 	}
+
+	err = virtCli.RbacV1().ClusterRoleBindings().Delete(context.Background(), getClusterRoleBindingName(saName), metav1.DeleteOptions{})
+	if !k8serrors.IsNotFound(err) {
+		util.PanicOnError(err)
+	}
 }