Merge pull request #704 from jakobmoellerdev/remove-service-accounts

chore!: remove service account spec and impersonation

Author: k8s-ci-robot

api/v1alpha1/resourcegraphdefinition_types.go

@@ -16,13 +16,7 @@ package v1alpha1
 import (
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	runtime "k8s.io/apimachinery/pkg/runtime"
-)
-
-const (
-	// DefaultServiceAccountKey is the key to use for the default service account
-	// in the serviceAccounts map.
-	DefaultServiceAccountKey = "*"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
 // ResourceGraphDefinitionSpec defines the desired state of ResourceGraphDefinition
@@ -37,13 +31,6 @@ type ResourceGraphDefinitionSpec struct {
 	//
 	// +kubebuilder:validation:Optional
 	Resources []*Resource `json:"resources,omitempty"`
-	// ServiceAccount configuration for controller impersonation.
-	// Key is the namespace, value is the service account name to use.
-	// Special key "*" defines the default service account for any
-	// namespace not explicitly mapped.
-	//
-	// +kubebuilder:validation:Optional
-	DefaultServiceAccounts map[string]string `json:"defaultServiceAccounts,omitempty"`
 }
 
 // Schema represents the attributes that define an instance of
@@ -198,7 +185,7 @@ func (o *ResourceGraphDefinition) SetConditions(conditions []Condition) {
 	o.Status.Conditions = conditions
 }
 
-//+kubebuilder:object:root=true
+// +kubebuilder:object:root=true
 
 // ResourceGraphDefinitionList contains a list of ResourceGraphDefinition
 type ResourceGraphDefinitionList struct {

api/v1alpha1/zz_generated.deepcopy.go

@@ -60,15 +60,6 @@ spec:
             description: ResourceGraphDefinitionSpec defines the desired state of
               ResourceGraphDefinition
             properties:
-              defaultServiceAccounts:
-                additionalProperties:
-                  type: string
-                description: |-
-                  ServiceAccount configuration for controller impersonation.
-                  Key is the namespace, value is the service account name to use.
-                  Special key "*" defines the default service account for any
-                  namespace not explicitly mapped.
-                type: object
               resources:
                 description: The resources that are part of the resourcegraphdefinition.
                 items:

config/crd/bases/kro.run_resourcegraphdefinitions.yaml

@@ -60,15 +60,6 @@ spec:
             description: ResourceGraphDefinitionSpec defines the desired state of
               ResourceGraphDefinition
             properties:
-              defaultServiceAccounts:
-                additionalProperties:
-                  type: string
-                description: |-
-                  ServiceAccount configuration for controller impersonation.
-                  Key is the namespace, value is the service account name to use.
-                  Special key "*" defines the default service account for any
-                  namespace not explicitly mapped.
-                type: object
               resources:
                 description: The resources that are part of the resourcegraphdefinition.
                 items:

helm/crds/kro.run_resourcegraphdefinitions.yaml

@@ -21,15 +21,12 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
-	"github.com/prometheus/client_golang/prometheus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/client-go/dynamic"
 	ctrl "sigs.k8s.io/controller-runtime"
 
-	"github.com/kro-run/kro/api/v1alpha1"
 	kroclient "github.com/kro-run/kro/pkg/client"
 	"github.com/kro-run/kro/pkg/graph"
 	"github.com/kro-run/kro/pkg/metadata"
@@ -88,8 +85,6 @@ type Controller struct {
 	// reconcileConfig holds the configuration parameters for the reconciliation
 	// process.
 	reconcileConfig ReconcileConfig
-	// defaultServiceAccounts is a map of service accounts to use for controller impersonation.
-	defaultServiceAccounts map[string]string
 }
 
 // NewController creates a new Controller instance.
@@ -100,17 +95,15 @@ func NewController(
 	rgd *graph.Graph,
 	clientSet kroclient.SetInterface,
 	restMapper meta.RESTMapper,
-	defaultServiceAccounts map[string]string,
 	instanceLabeler metadata.Labeler,
 ) *Controller {
 	return &Controller{
-		log:                    log,
-		gvr:                    gvr,
-		clientSet:              clientSet,
-		rgd:                    rgd,
-		instanceLabeler:        instanceLabeler,
-		reconcileConfig:        reconcileConfig,
-		defaultServiceAccounts: defaultServiceAccounts,
+		log:             log,
+		gvr:             gvr,
+		clientSet:       clientSet,
+		rgd:             rgd,
+		instanceLabeler: instanceLabeler,
+		reconcileConfig: reconcileConfig,
 	}
 }
 
@@ -145,17 +138,10 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) error {
 		return fmt.Errorf("failed to create instance sub-resources labeler: %w", err)
 	}
 
-	// If possible, use a service account to create the execution client
-	// TODO(a-hilaly): client caching
-	executionClient, err := c.getExecutionClient(namespace)
-	if err != nil {
-		return fmt.Errorf("failed to create execution client: %w", err)
-	}
-
 	instanceGraphReconciler := &instanceGraphReconciler{
 		log:                         log,
 		gvr:                         c.gvr,
-		client:                      executionClient,
+		client:                      c.clientSet.Dynamic(),
 		restMapper:                  c.clientSet.RESTMapper(),
 		runtime:                     rgRuntime,
 		instanceLabeler:             c.instanceLabeler,
@@ -177,98 +163,3 @@ func getNamespaceName(req ctrl.Request) (string, string) {
 	}
 	return namespace, name
 }
-
-// errorCategory helps classify different types of impersonation errors
-type errorCategory string
-
-const (
-	errorConfigCreate errorCategory = "config_create"
-	errorInvalidSA    errorCategory = "invalid_sa"
-	errorClientCreate errorCategory = "client_create"
-	errorPermissions  errorCategory = "permissions"
-)
-
-// getExecutionClient determines the execution client to use for the instance.
-// If the instance is created in a namespace of which a service account is specified,
-// the execution client will be created using the service account. If no service account
-// is specified for the namespace, the default client will be used.
-func (c *Controller) getExecutionClient(namespace string) (dynamic.Interface, error) {
-	// if no service accounts are specified, use the default client
-	if len(c.defaultServiceAccounts) == 0 {
-		c.log.V(1).Info("no service accounts configured, using default client")
-		return c.clientSet.Dynamic(), nil
-	}
-
-	timer := prometheus.NewTimer(impersonationDuration.WithLabelValues(namespace, ""))
-	defer timer.ObserveDuration()
-
-	// Check for namespace specific service account
-	if sa, ok := c.defaultServiceAccounts[namespace]; ok {
-		userName, err := getServiceAccountUserName(namespace, sa)
-		if err != nil {
-			c.handleImpersonateError(namespace, sa, err)
-			return nil, fmt.Errorf("invalid service account configuration: %w", err)
-		}
-
-		pivotedClient, err := c.clientSet.WithImpersonation(userName)
-		if err != nil {
-			c.handleImpersonateError(namespace, sa, err)
-			return nil, fmt.Errorf("failed to create impersonated client: %w", err)
-		}
-
-		impersonationTotal.WithLabelValues(namespace, sa, "success").Inc()
-		return pivotedClient.Dynamic(), nil
-	}
-
-	// Check for default service account (marked by "*")
-	if defaultSA, ok := c.defaultServiceAccounts[v1alpha1.DefaultServiceAccountKey]; ok {
-		userName, err := getServiceAccountUserName(namespace, defaultSA)
-		if err != nil {
-			c.handleImpersonateError(namespace, defaultSA, err)
-			return nil, fmt.Errorf("invalid default service account configuration: %w", err)
-		}
-
-		pivotedClient, err := c.clientSet.WithImpersonation(userName)
-		if err != nil {
-			c.handleImpersonateError(namespace, defaultSA, err)
-			return nil, fmt.Errorf("failed to create impersonated client with default SA: %w", err)
-		}
-
-		impersonationTotal.WithLabelValues(namespace, defaultSA, "success").Inc()
-		return pivotedClient.Dynamic(), nil
-	}
-
-	impersonationTotal.WithLabelValues(namespace, "", "default").Inc()
-	// Fallback to the default client
-	return c.clientSet.Dynamic(), nil
-}
-
-// handleImpersonateError logs the error and records the error in the metrics
-func (c *Controller) handleImpersonateError(namespace, sa string, err error) {
-	var category errorCategory
-	switch {
-	case strings.Contains(err.Error(), "forbidden"):
-		category = errorPermissions
-	case strings.Contains(err.Error(), "cannot get token"):
-		category = errorConfigCreate
-	default:
-		category = errorClientCreate
-	}
-	recordImpersonateError(namespace, sa, category)
-	c.log.Error(
-		err,
-		"failed to create impersonated client",
-		"namespace", namespace,
-		"serviceAccount", sa,
-		"errorCategory", category,
-	)
-}
-
-// getServiceAccountUserName builds the impersonate service account user name.
-// The format of the user name is "system:serviceaccount:<namespace>:<serviceaccount>"
-func getServiceAccountUserName(namespace, serviceAccount string) (string, error) {
-	if namespace == "" || serviceAccount == "" {
-		return "", fmt.Errorf("namespace and service account must be provided")
-	}
-	return fmt.Sprintf("system:serviceaccount:%s:%s", namespace, serviceAccount), nil
-}

pkg/controller/instance/controller.go

@@ -57,10 +57,6 @@ var (
 	)
 )
 
-func recordImpersonateError(namespace, sa string, category errorCategory) {
-	impersonationErrors.WithLabelValues(namespace, sa, string(category)).Inc()
-}
-
 func init() {
 	metrics.Registry.MustRegister(
 		impersonationTotal,

pkg/controller/instance/metrics.go

@@ -74,8 +74,7 @@ func (r *ResourceGraphDefinitionReconciler) reconcileResourceGraphDefinition(
 
 	// Setup and start microcontroller
 	gvr := processedRGD.Instance.GetGroupVersionResource()
-	controller := r.setupMicroController(gvr, processedRGD,
-		rgd.Spec.DefaultServiceAccounts, graphExecLabeler)
+	controller := r.setupMicroController(gvr, processedRGD, graphExecLabeler)
 
 	log.V(1).Info("reconciling resource graph definition micro controller")
 	// TODO: the context that is passed here is tied to the reconciliation of the rgd, we might need to make
@@ -100,7 +99,6 @@ func (r *ResourceGraphDefinitionReconciler) setupLabeler(rgd *v1alpha1.ResourceG
 func (r *ResourceGraphDefinitionReconciler) setupMicroController(
 	gvr schema.GroupVersionResource,
 	processedRGD *graph.Graph,
-	defaultSVCs map[string]string,
 	labeler metadata.Labeler,
 ) *instancectrl.Controller {
 	instanceLogger := r.instanceLogger.WithName(fmt.Sprintf("%s-controller", gvr.Resource)).WithValues(
@@ -120,7 +118,6 @@ func (r *ResourceGraphDefinitionReconciler) setupMicroController(
 		processedRGD,
 		r.clientSet,
 		r.clientSet.RESTMapper(),
-		defaultSVCs,
 		labeler,
 	)
 }