Merge pull request #3009 from chrischdi/pr-svs-drop-svcacct-cm

🌱 supervisor: drop ProviderServiceAccount ConfigMap

apis/vmware/v1beta1/vspherecluster_types.go

@@ -29,6 +29,8 @@ const (
 
 	// ProviderServiceAccountFinalizer allows ServiceAccountReconciler to clean up service accounts
 	// resources associated with VSphereCluster from the SERVICE_ACCOUNTS_CM (service accounts ConfigMap).
+	//
+	// Deprecated: ProviderServiceAccountFinalizer will be removed in a future release.
 	ProviderServiceAccountFinalizer = "providerserviceaccount.vmware.infrastructure.cluster.x-k8s.io"
 )
 

config/supervisor/kustomization.yaml

@@ -17,9 +17,6 @@ resources:
   - crd/bases/vmware.infrastructure.cluster.x-k8s.io_providerserviceaccounts.yaml
   - ./webhook
 
-patchesStrategicMerge:
-  - add-configmap-env-vars.yaml
-
 vars:
   - name: CERTIFICATE_NAMESPACE # namespace of the certificate CR
     objref:

controllers/serviceaccount_controller.go

@@ -19,9 +19,7 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"os"
 	"reflect"
-	"strconv"
 	"time"
 
 	"github.com/pkg/errors"
@@ -183,14 +181,15 @@ func (r *ServiceAccountReconciler) Reconcile(ctx context.Context, req reconcile.
 		}
 	}()
 
-	if !vsphereCluster.DeletionTimestamp.IsZero() {
-		return ctrl.Result{}, r.reconcileDelete(ctx, clusterContext)
+	// Always remove the deprecated finalizer if it exists.
+	// This code needs to be kept for a while to cleanup the finalizer.
+	//nolint:staticcheck
+	if controllerutil.ContainsFinalizer(clusterContext.VSphereCluster, vmwarev1.ProviderServiceAccountFinalizer) {
+		controllerutil.RemoveFinalizer(clusterContext.VSphereCluster, vmwarev1.ProviderServiceAccountFinalizer) //nolint:staticcheck
 	}
 
-	// Add finalizer first if not set to avoid the race condition between init and delete.
-	// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
-	if !controllerutil.ContainsFinalizer(clusterContext.VSphereCluster, vmwarev1.ProviderServiceAccountFinalizer) {
-		controllerutil.AddFinalizer(clusterContext.VSphereCluster, vmwarev1.ProviderServiceAccountFinalizer)
+	// Nothing to do when vsphereCluster is getting deleted.
+	if !vsphereCluster.DeletionTimestamp.IsZero() {
 		return ctrl.Result{}, nil
 	}
 
@@ -215,30 +214,6 @@ func (r *ServiceAccountReconciler) Reconcile(ctx context.Context, req reconcile.
 	})
 }
 
-// reconcileDelete handles delete events for ProviderServiceAccounts.
-func (r *ServiceAccountReconciler) reconcileDelete(ctx context.Context, clusterCtx *vmwarecontext.ClusterContext) error {
-	log := ctrl.LoggerFrom(ctx)
-
-	pSvcAccounts, err := r.getProviderServiceAccounts(ctx, clusterCtx)
-	if err != nil {
-		return errors.Wrap(err, "failed to get ProviderServiceAccounts")
-	}
-
-	for _, pSvcAccount := range pSvcAccounts {
-		// Note: We have to use := here to not overwrite log & ctx outside the for loop.
-		log := log.WithValues("ProviderServiceAccount", klog.KRef(pSvcAccount.Namespace, pSvcAccount.Name))
-		ctx := ctrl.LoggerInto(ctx, log)
-
-		// Delete entries for configmap with serviceaccount
-		if err := r.deleteServiceAccountConfigMap(ctx, pSvcAccount); err != nil {
-			return errors.Wrapf(err, "failed to delete ServiceAccount %s from ServiceAccounts ConfigMap", pSvcAccount.Name)
-		}
-	}
-
-	controllerutil.RemoveFinalizer(clusterCtx.VSphereCluster, vmwarev1.ProviderServiceAccountFinalizer)
-	return nil
-}
-
 // reconcileNormal handles create and update events for ProviderServiceAccounts.
 func (r *ServiceAccountReconciler) reconcileNormal(ctx context.Context, guestClusterCtx *vmwarecontext.GuestClusterContext) (_ reconcile.Result, reterr error) {
 	defer func() {
@@ -282,27 +257,22 @@ func (r *ServiceAccountReconciler) ensureProviderServiceAccounts(ctx context.Con
 			return errors.Wrapf(err, "failed to ensure ServiceAccount %s", pSvcAccount.Name)
 		}
 
-		// 2. Add ServiceAccount to ServiceAccounts ConfigMap
-		if err := r.ensureServiceAccountConfigMap(ctx, pSvcAccount); err != nil {
-			return errors.Wrapf(err, "failed to add ServiceAccount %s to ServiceAccounts ConfigMap", pSvcAccount.Name)
-		}
-
-		// 3. Ensure secret of ServiceAccountToken type for the ServiceAccount
+		// 2. Ensure secret of ServiceAccountToken type for the ServiceAccount
 		if err := r.ensureServiceAccountSecret(ctx, pSvcAccount); err != nil {
 			return errors.Wrapf(err, "failed to ensure ServiceAcountToken secret %s", getServiceAccountSecretName(pSvcAccount))
 		}
 
-		// 4. Ensure the associated Role for the ServiceAccount
+		// 3. Ensure the associated Role for the ServiceAccount
 		if err := r.ensureRole(ctx, pSvcAccount); err != nil {
 			return errors.Wrapf(err, "failed to ensure Role for ServiceAccount %s", pSvcAccount.Name)
 		}
 
-		// 5. Ensure the associated RoleBinding for the ServiceAccount
+		// 4. Ensure the associated RoleBinding for the ServiceAccount
 		if err := r.ensureRoleBinding(ctx, pSvcAccount); err != nil {
 			return errors.Wrapf(err, "failed to ensure RoleBinding for ServiceAccount %s", pSvcAccount.Name)
 		}
 
-		// 6. Sync the ServiceAccount secret to the workload cluster
+		// 5. Sync the ServiceAccount secret to the workload cluster
 		if err := r.syncServiceAccountSecret(ctx, guestClusterCtx, pSvcAccount); err != nil {
 			return errors.Wrapf(err, "failed to sync secret for ProviderServiceAccount %s to workload cluster", pSvcAccount.Name)
 		}
@@ -520,64 +490,6 @@ func (r *ServiceAccountReconciler) syncServiceAccountSecret(ctx context.Context,
 	return err
 }
 
-func (r *ServiceAccountReconciler) getConfigMapAndBuffer(ctx context.Context) (*corev1.ConfigMap, *corev1.ConfigMap, error) {
-	configMap := &corev1.ConfigMap{}
-
-	if err := r.Client.Get(ctx, GetCMNamespaceName(), configMap); err != nil {
-		return nil, nil, err
-	}
-
-	configMapBuffer := &corev1.ConfigMap{}
-	configMapBuffer.Name = configMap.Name
-	configMapBuffer.Namespace = configMap.Namespace
-	return configMapBuffer, configMap, nil
-}
-
-func (r *ServiceAccountReconciler) deleteServiceAccountConfigMap(ctx context.Context, svcAccount vmwarev1.ProviderServiceAccount) error {
-	log := ctrl.LoggerFrom(ctx)
-
-	svcAccountName := getSystemServiceAccountFullName(svcAccount)
-	configMapBuffer, configMap, err := r.getConfigMapAndBuffer(ctx)
-	if err != nil {
-		return err
-	}
-	if valid, exist := configMap.Data[svcAccountName]; !exist || valid != strconv.FormatBool(true) {
-		// Service account name is not in the config map
-		return nil
-	}
-	log.Info("Patching ServiceAccounts ConfigMap to ensure ServiceAccount is deleted")
-	_, err = controllerutil.CreateOrPatch(ctx, r.Client, configMapBuffer, func() error {
-		configMapBuffer.Data = configMap.Data
-		delete(configMapBuffer.Data, svcAccountName)
-		return nil
-	})
-	return err
-}
-
-func (r *ServiceAccountReconciler) ensureServiceAccountConfigMap(ctx context.Context, svcAccount vmwarev1.ProviderServiceAccount) error {
-	log := ctrl.LoggerFrom(ctx)
-
-	svcAccountName := getSystemServiceAccountFullName(svcAccount)
-	configMapBuffer, configMap, err := r.getConfigMapAndBuffer(ctx)
-	if err != nil {
-		return err
-	}
-	if configMap.Data == nil {
-		configMap.Data = map[string]string{}
-	}
-	if valid, exist := configMap.Data[svcAccountName]; exist && valid == strconv.FormatBool(true) {
-		// Service account name is already in the config map
-		return nil
-	}
-	log.Info("Patching ServiceAccounts ConfigMap to ensure ServiceAccount is added")
-	_, err = controllerutil.CreateOrPatch(ctx, r.Client, configMapBuffer, func() error {
-		configMapBuffer.Data = configMap.Data
-		configMapBuffer.Data[svcAccountName] = "true"
-		return nil
-	})
-	return err
-}
-
 func (r *ServiceAccountReconciler) getProviderServiceAccounts(ctx context.Context, clusterCtx *vmwarecontext.ClusterContext) ([]vmwarev1.ProviderServiceAccount, error) {
 	var pSvcAccounts []vmwarev1.ProviderServiceAccount
 
@@ -617,22 +529,6 @@ func getServiceAccountSecretName(pSvcAccount vmwarev1.ProviderServiceAccount) st
 	return fmt.Sprintf("%s-secret", pSvcAccount.Name)
 }
 
-func getSystemServiceAccountFullName(pSvcAccount vmwarev1.ProviderServiceAccount) string {
-	return fmt.Sprintf("%s.%s.%s", systemServiceAccountPrefix, getServiceAccountNamespace(pSvcAccount), getServiceAccountName(pSvcAccount))
-}
-
-func getServiceAccountNamespace(pSvcAccount vmwarev1.ProviderServiceAccount) string {
-	return pSvcAccount.Namespace
-}
-
-// GetCMNamespaceName gets capi valid modifier configmap metadata.
-func GetCMNamespaceName() types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: os.Getenv("SERVICE_ACCOUNTS_CM_NAMESPACE"),
-		Name:      os.Getenv("SERVICE_ACCOUNTS_CM_NAME"),
-	}
-}
-
 // secretToVSphereCluster is a mapper function used to enqueue reconcile.Request objects.
 // It accepts a Secret object owned by the controller and fetches the service account
 // that contains the token and creates a reconcile.Request for the vmwarev1.VSphereCluster object.

controllers/serviceaccount_controller_intg_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package controllers
 
 import (
-	"os"
 	"reflect"
 
 	"github.com/google/uuid"
@@ -42,11 +41,6 @@ var _ = Describe("ProviderServiceAccount controller integration tests", func() {
 
 	BeforeEach(func() {
 		intCtx = helpers.NewIntegrationTestContextWithClusters(ctx, testEnv.Manager.GetClient())
-		testSystemSvcAcctCM := "test-system-svc-acct-cm"
-		cfgMap := getSystemServiceAccountsConfigMap(intCtx.VSphereCluster.Namespace, testSystemSvcAcctCM)
-		Expect(intCtx.Client.Create(ctx, cfgMap)).To(Succeed())
-		_ = os.Setenv("SERVICE_ACCOUNTS_CM_NAMESPACE", intCtx.VSphereCluster.Namespace)
-		_ = os.Setenv("SERVICE_ACCOUNTS_CM_NAME", testSystemSvcAcctCM)
 	})
 
 	AfterEach(func() {

controllers/serviceaccount_controller_suite_test.go

@@ -35,10 +35,8 @@ import (
 )
 
 const (
-	testTargetNS        = "test-pvcsi-system"
-	testTargetSecret    = "test-pvcsi-secret" //nolint:gosec //Non-production code.
-	testSystemSvcAcctNs = "test-system-svc-acct-namespace"
-	testSystemSvcAcctCM = "test-system-svc-acct-cm"
+	testTargetNS     = "test-pvcsi-system"
+	testTargetSecret = "test-pvcsi-secret" //nolint:gosec //Non-production code.
 
 	testSecretToken = "ZXlKaGJHY2lPaUpTVXpJMU5pSXNJbXRwWkNJNklp" //nolint:gosec //Non-production code.
 )
@@ -228,19 +226,6 @@ func getTestProviderServiceAccount(namespace string, vSphereCluster *vmwarev1.VS
 	return pSvcAccount
 }
 
-func getSystemServiceAccountsConfigMap(namespace, name string) *corev1.ConfigMap {
-	return &corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: namespace,
-			Name:      name,
-		},
-		Data: map[string]string{
-			"system-account-1": "true",
-			"system-account-2": "true",
-		},
-	}
-}
-
 func getTestRoleWithGetPod(namespace, name string) *rbacv1.Role {
 	return &rbacv1.Role{
 		ObjectMeta: metav1.ObjectMeta{

controllers/serviceaccount_controller_unit_test.go

@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"os"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -72,10 +71,7 @@ func unitTestsReconcileNormal() {
 			namespace = capiutil.RandomString(6)
 			obj := fake.NewVSphereCluster(namespace)
 			vsphereCluster = &obj
-			_ = os.Setenv("SERVICE_ACCOUNTS_CM_NAMESPACE", testSystemSvcAcctNs)
-			_ = os.Setenv("SERVICE_ACCOUNTS_CM_NAME", testSystemSvcAcctCM)
 			initObjects = []client.Object{
-				getSystemServiceAccountsConfigMap(testSystemSvcAcctNs, testSystemSvcAcctCM),
 				getTestProviderServiceAccount(namespace, vsphereCluster, false),
 			}
 		})

test/e2e/config/vsphere.yaml

@@ -283,8 +283,6 @@ variables:
   EXP_NODE_ANTI_AFFINITY: "true"
   CAPI_DIAGNOSTICS_ADDRESS: ":8080"
   CAPI_INSECURE_DIAGNOSTICS: "true"
-  SERVICE_ACCOUNTS_CM_NAME: ""
-  SERVICE_ACCOUNTS_CM_NAMESPACE: ""
 
 intervals:
   default/wait-controllers: ["5m", "10s"]