Repository: vmware-tanzu/tanzu-framework (language: go)

Issue:
CPI and CSI config controller should add RBAC rules required for CAPV Provider Service Account to use
**Bug description**
CAPV creates a Role (with the permissions listed above), a ServiceAccount and binds the two using a RoleBinding to a particular namespace. CAPV controller manager ClustreRole needs to have the permissions specified in the ProviderServiceAccount.spec.Rules before it is able to create that Role object. Hence on startup, (ideally) the tanzu addons manager should create a ClusterRole with the label [capv.infrastucture.cluster.x-k8s.io/aggregate-to-manager](http://capv.infrastucture.cluster.x-k8s.io/aggregate-to-manager): "true" and the necessary permissions.
That should enable the reconciliation of the ProviderSvcAccount successfully.

**Affected product area (please put an X in all that apply)**

- [ ] APIs
- [x] Addons
- [ ] CLI
- [ ] Docs
- [ ] IAM
- [ ] Installation
- [ ] Plugin
- [ ] Security
- [ ] Test and Release
- [ ] User Experience

**Expected behavior**

**Steps to reproduce the bug**

**Version** (include the SHA if the version is not obvious)

**Environment where the bug was observed (cloud, OS, etc)**

**Relevant Debug Output (Logs, manifests, etc)**



Discussion / hints:

Fix patch:
diff --git a/addons/controllers/cpi/vspherecpiconfig_controller.go b/addons/controllers/cpi/vspherecpiconfig_controller.go
index 7a6a63c154..137fd3107e 100644
--- a/addons/controllers/cpi/vspherecpiconfig_controller.go
+++ b/addons/controllers/cpi/vspherecpiconfig_controller.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/go-logr/logr"
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -36,6 +37,29 @@ type VSphereCPIConfigReconciler struct {
 	Scheme *runtime.Scheme
 }
 
+var providerServiceAccountRBACRules = []rbacv1.PolicyRule{
+	{
+		Verbs:     []string{"get", "create", "update", "patch", "delete"},
+		APIGroups: []string{"vmoperator.vmware.com"},
+		Resources: []string{"virtualmachineservices", "virtualmachineservices/status"},
+	},
+	{
+		Verbs:     []string{"get", "list"},
+		APIGroups: []string{"vmoperator.vmware.com"},
+		Resources: []string{"virtualmachines", "virtualmachines/status"},
+	},
+	{
+		Verbs:     []string{"get", "create", "update", "list", "patch", "delete", "watch"},
+		APIGroups: []string{"nsx.vmware.com"},
+		Resources: []string{"ippools", "ippools/status"},
+	},
+	{
+		Verbs:     []string{"get", "create", "update", "list", "patch", "delete"},
+		APIGroups: []string{"nsx.vmware.com"},
+		Resources: []string{"routesets", "routesets/status"},
+	},
+}
+
 //+kubebuilder:rbac:groups=cpi.tanzu.vmware.com,resources=vspherecpiconfigs,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=cpi.tanzu.vmware.com,resources=vspherecpiconfigs/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=vmware.infrastructure.cluster.x-k8s.io,resources=providerserviceaccounts,verbs=get;create;list;watch;update;patch
@@ -149,7 +173,17 @@ func (r *VSphereCPIConfigReconciler) reconcileVSphereCPIConfigNormal(ctx context
 
 	// deploy the provider service account for paravirtual mode
 	if *cpiConfig.Spec.VSphereCPI.Mode == VSphereCPIParavirtualMode {
-		r.Log.Info("Create or update provider serviceAccount for VSphere CPI")
+		// create an aggregated cluster role RBAC that will be inherited by CAPV (https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles)
+		// CAPV needs to hold these rules before it can grant it to serviceAccount for CPI
+		_, err := controllerutil.CreateOrPatch(ctx, r.Client, constants.CAPVAggregatedClusterRole, func() error {
+			constants.CAPVAggregatedClusterRole.Rules = providerServiceAccountRBACRules
+			return nil
+		})
+		if err != nil {
+			r.Log.Error(err, "Error creating or patching cluster role", "name", constants.ProviderServiceAccountAggregatedClusterRole)
+			return err
+		}
+
 		vsphereClusters := &capvvmwarev1beta1.VSphereClusterList{}
 		labelMatch, err := labels.NewRequirement(clusterapiv1beta1.ClusterLabelName, selection.Equals, []string{cluster.Name})
 		if err != nil {
diff --git a/addons/controllers/cpi/vspherecpiconfig_utils.go b/addons/controllers/cpi/vspherecpiconfig_utils.go
index 80e334b5e5..9c1a6acef8 100644
--- a/addons/controllers/cpi/vspherecpiconfig_utils.go
+++ b/addons/controllers/cpi/vspherecpiconfig_utils.go
@@ -12,7 +12,6 @@ import (
 
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
-	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -339,29 +338,8 @@ func (r *VSphereCPIConfigReconciler) mapCPIConfigToDataValues(ctx context.Contex
 // mapCPIConfigToProviderServiceAccountSpec maps CPIConfig and cluster to the corresponding service account spec
 func (r *VSphereCPIConfigReconciler) mapCPIConfigToProviderServiceAccountSpec(vsphereCluster *capvvmwarev1beta1.VSphereCluster) capvvmwarev1beta1.ProviderServiceAccountSpec {
 	return capvvmwarev1beta1.ProviderServiceAccountSpec{
-		Ref: &v1.ObjectReference{Name: vsphereCluster.Name, Namespace: vsphereCluster.Namespace},
-		Rules: []rbacv1.PolicyRule{
-			{
-				Verbs:     []string{"get", "create", "update", "patch", "delete"},
-				APIGroups: []string{"vmoperator.vmware.com"},
-				Resources: []string{"virtualmachineservices", "virtualmachineservices/status"},
-			},
-			{
-				Verbs:     []string{"get", "list"},
-				APIGroups: []string{"vmoperator.vmware.com"},
-				Resources: []string{"virtualmachines", "virtualmachines/status"},
-			},
-			{
-				Verbs:     []string{"get", "create", "update", "list", "patch", "delete", "watch"},
-				APIGroups: []string{"nsx.vmware.com"},
-				Resources: []string{"ippools", "ippools/status"},
-			},
-			{
-				Verbs:     []string{"get", "create", "update", "list", "patch", "delete"},
-				APIGroups: []string{"nsx.vmware.com"},
-				Resources: []string{"routesets", "routesets/status"},
-			},
-		},
+		Ref:              &v1.ObjectReference{Name: vsphereCluster.Name, Namespace: vsphereCluster.Namespace},
+		Rules:            providerServiceAccountRBACRules,
 		TargetNamespace:  ProviderServiceAccountSecretNamespace,
 		TargetSecretName: ProviderServiceAccountSecretName,
 	}
diff --git a/addons/controllers/csi/vspherecsiconfig_controller.go b/addons/controllers/csi/vspherecsiconfig_controller.go
index 060b2a4719..4ede1cee5b 100644
--- a/addons/controllers/csi/vspherecsiconfig_controller.go
+++ b/addons/controllers/csi/vspherecsiconfig_controller.go
@@ -10,8 +10,9 @@ import (
 
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
-	yaml "gopkg.in/yaml.v3"
+	"gopkg.in/yaml.v3"
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -39,6 +40,39 @@ type VSphereCSIConfigReconciler struct {
 	Scheme *runtime.Scheme
 }
 
+var providerServiceAccountRBACRules = []rbacv1.PolicyRule{
+	{
+		APIGroups: []string{"vmoperator.vmware.com"},
+		Resources: []string{"virtualmachines"},
+		Verbs:     []string{"get", "list", "watch", "update", "patch"},
+	},
+	{
+		APIGroups: []string{"cns.vmware.com"},
+		Resources: []string{"cnsvolumemetadatas", "cnsfileaccessconfigs"},
+		Verbs:     []string{"get", "list", "watch", "update", "create", "delete"},
+	},
+	{
+		APIGroups: []string{"cns.vmware.com"},
+		Resources: []string{"cnscsisvfeaturestates"},
+		Verbs:     []string{"get", "list", "watch"},
+	},
+	{
+		APIGroups: []string{""},
+		Resources: []string{"persistentvolumeclaims"},
+		Verbs:     []string{"get", "list", "watch", "update", "create", "delete"},
+	},
+	{
+		APIGroups: []string{""},
+		Resources: []string{"persistentvolumeclaims/status"},
+		Verbs:     []string{"get", "update", "patch"},
+	},
+	{
+		APIGroups: []string{""},
+		Resources: []string{"events"},
+		Verbs:     []string{"list"},
+	},
+}
+
 //+kubebuilder:rbac:groups=csi.tanzu.vmware.com,resources=vspherecsiconfigs,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=csi.tanzu.vmware.com,resources=vspherecsiconfigs/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=csi.tanzu.vmware.com,resources=vspherecsiconfigs/finalizers,verbs=update
@@ -183,6 +217,18 @@ func (r *VSphereCSIConfigReconciler) reconcileVSphereCSIConfigNormal(ctx context
 
 	// deploy the provider service account for paravirtual mode
 	if csiCfg.Spec.VSphereCSI.Mode == VSphereCSIParavirtualMode {
+		// create an aggregated cluster role RBAC that will be inherited by CAPV (https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles)
+		// CAPV needs to hold these rules before it can grant it to serviceAccount for CSI
+		_, err := controllerutil.CreateOrPatch(ctx, r.Client, constants.CAPVAggregatedClusterRole, func() error {
+			constants.CAPVAggregatedClusterRole.Rules = providerServiceAccountRBACRules
+			return nil
+		})
+
+		if err != nil {
+			r.Log.Error(err, "Error creating or patching cluster role", "name", constants.ProviderServiceAccountAggregatedClusterRole)
+			return ctrl.Result{}, err
+		}
+
 		serviceAccount := &capvvmwarev1beta1.ProviderServiceAccount{}
 		vsphereCluster, err := r.getVsphereCluster(ctx, cluster)
 		if err != nil {
diff --git a/addons/controllers/csi/vspherecsiconfig_utils.go b/addons/controllers/csi/vspherecsiconfig_utils.go
index 504642e4c0..216df71352 100644
--- a/addons/controllers/csi/vspherecsiconfig_utils.go
+++ b/addons/controllers/csi/vspherecsiconfig_utils.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
-	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -172,39 +171,8 @@ func (r *VSphereCSIConfigReconciler) mapCSIConfigToProviderServiceAccount(vspher
 			Namespace: vsphereCluster.Namespace,
 		},
 		Spec: capvvmwarev1beta1.ProviderServiceAccountSpec{
-			Ref: &v1.ObjectReference{Name: vsphereCluster.Name, Namespace: vsphereCluster.Namespace},
-			Rules: []rbacv1.PolicyRule{
-				{
-					APIGroups: []string{"vmoperator.vmware.com"},
-					Resources: []string{"virtualmachines"},
-					Verbs:     []string{"get", "list", "watch", "update", "patch"},
-				},
-				{
-					APIGroups: []string{"cns.vmware.com"},
-					Resources: []string{"cnsvolumemetadatas", "cnsfileaccessconfigs"},
-					Verbs:     []string{"get", "list", "watch", "update", "create", "delete"},
-				},
-				{
-					APIGroups: []string{"cns.vmware.com"},
-					Resources: []string{"cnscsisvfeaturestates"},
-					Verbs:     []string{"get", "list", "watch"},
-				},
-				{
-					APIGroups: []string{""},
-					Resources: []string{"persistentvolumeclaims"},
-					Verbs:     []string{"get", "list", "watch", "update", "create", "delete"},
-				},
-				{
-					APIGroups: []string{""},
-					Resources: []string{"persistentvolumeclaims/status"},
-					Verbs:     []string{"get", "update", "patch"},
-				},
-				{
-					APIGroups: []string{""},
-					Resources: []string{"events"},
-					Verbs:     []string{"list"},
-				},
-			},
+			Ref:              &v1.ObjectReference{Name: vsphereCluster.Name, Namespace: vsphereCluster.Namespace},
+			Rules:            providerServiceAccountRBACRules,
 			TargetNamespace:  "vmware-system-csi",
 			TargetSecretName: "pvcsi-provider-creds",
 		},
diff --git a/addons/pkg/constants/constants.go b/addons/pkg/constants/constants.go
index 2cbe6817a2..b3966733ba 100644
--- a/addons/pkg/constants/constants.go
+++ b/addons/pkg/constants/constants.go
@@ -8,6 +8,8 @@ import (
 	"reflect"
 	"time"
 
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterapiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
@@ -197,7 +199,26 @@ const (
 	// AddCBMissingFieldsAnnotationKey is the annotation key used by ClusterBootstrap webhook to implement its defaulting
 	// logic
 	AddCBMissingFieldsAnnotationKey = "tkg.tanzu.vmware.com/add-missing-fields-from-tkr"
+
+	// ProviderServiceAccountAggregatedClusterRole is the name of ClusterRole created by controllers that use ProviderServiceAccount
+	ProviderServiceAccountAggregatedClusterRole = "tanzu-addons-manager-providerserviceaccount-aggregatedrole"
+
+	// CAPVClusterRoleAggregationRuleLabelSelectorKey is the label selector key used by aggregation rule in CAPV ClusterRole
+	CAPVClusterRoleAggregationRuleLabelSelectorKey = "capv.infrastucture.cluster.x-k8s.io/aggregate-to-manager"
+
+	// CAPVClusterRoleAggregationRuleLabelSelectorValue is the label selector value used by aggregation rule in CAPV ClusterRole
+	CAPVClusterRoleAggregationRuleLabelSelectorValue = "true"
 )
 
 // ClusterKind is the Kind for cluster-api Cluster object
 var ClusterKind = reflect.TypeOf(clusterapiv1beta1.Cluster{}).Name()
+
+// CAPVAggregatedClusterRole is the cluster role to assign permissions to capv provider
+var CAPVAggregatedClusterRole = &rbacv1.ClusterRole{
+	ObjectMeta: metav1.ObjectMeta{
+		Name: ProviderServiceAccountAggregatedClusterRole,
+		Labels: map[string]string{
+			CAPVClusterRoleAggregationRuleLabelSelectorKey: CAPVClusterRoleAggregationRuleLabelSelectorValue,
+		},
+	},
+}


Test patch:
diff --git a/addons/controllers/vspherecpiconfig_controller_test.go b/addons/controllers/vspherecpiconfig_controller_test.go
index 50c1e8f30e..8d2b632116 100644
--- a/addons/controllers/vspherecpiconfig_controller_test.go
+++ b/addons/controllers/vspherecpiconfig_controller_test.go
@@ -13,6 +13,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	capvv1beta1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
@@ -301,6 +302,23 @@ var _ = Describe("VSphereCPIConfig Reconciler", func() {
 			})
 		})
 
+		It("Should reconcile aggregated cluster role", func() {
+			clusterRole := &rbacv1.ClusterRole{}
+			Eventually(func() bool {
+				key := client.ObjectKey{
+					Name: constants.ProviderServiceAccountAggregatedClusterRole,
+				}
+				if err := k8sClient.Get(ctx, key, clusterRole); err != nil {
+					return false
+				}
+				Expect(clusterRole.Labels).To(Equal(map[string]string{
+					constants.CAPVClusterRoleAggregationRuleLabelSelectorKey: constants.CAPVClusterRoleAggregationRuleLabelSelectorValue,
+				}))
+				Expect(clusterRole.Rules).To(HaveLen(5))
+				return true
+			})
+		})
+
 		When("Headless service and endpoints already exists", func() {
 			var svc *v1.Service
 			BeforeEach(func() {
diff --git a/addons/controllers/vspherecsiconfig_controller_test.go b/addons/controllers/vspherecsiconfig_controller_test.go
index 23add57aba..adc2016e94 100644
--- a/addons/controllers/vspherecsiconfig_controller_test.go
+++ b/addons/controllers/vspherecsiconfig_controller_test.go
@@ -12,6 +12,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	capvvmwarev1beta1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
 	clusterapiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -314,5 +315,22 @@ var _ = Describe("VSphereCSIConfig Reconciler", func() {
 				return true
 			})
 		})
+
+		It("Should reconcile aggregated cluster role", func() {
+			clusterRole := &rbacv1.ClusterRole{}
+			Eventually(func() bool {
+				key := client.ObjectKey{
+					Name: constants.ProviderServiceAccountAggregatedClusterRole,
+				}
+				if err := k8sClient.Get(ctx, key, clusterRole); err != nil {
+					return false
+				}
+				Expect(clusterRole.Labels).To(Equal(map[string]string{
+					constants.CAPVClusterRoleAggregationRuleLabelSelectorKey: constants.CAPVClusterRoleAggregationRuleLabelSelectorValue,
+				}))
+				Expect(clusterRole.Rules).To(HaveLen(6))
+				return true
+			})
+		})
 	})
 })