simplify controller logic + lock down cr

Author: ehearne-redhat

manifests/03-rbac-role-cluster.yaml

@@ -153,12 +153,20 @@ rules:
     resources:
       - serviceaccounts
     verbs:
-      - get
-      - list
-      - delete
       - create
-      - update
+      - list
       - watch
+      - get
+  - apiGroups:
+      - ""
+    resources:
+      - serviceaccounts
+    verbs:
+      - update
+      - delete
+    resourceNames:
+      - console
+      - downloads
 ---
 kind: ClusterRole
 apiVersion: rbac.authorization.k8s.io/v1

pkg/api/api.go

@@ -56,18 +56,16 @@ const (
 	DefaultIngressController   = "default"
 	IngressControllerNamespace = "openshift-ingress-operator"
 
-	OAuthClientName                                 = OpenShiftConsoleName
-	OpenShiftConsoleDeploymentName                  = OpenShiftConsoleName
-	OpenShiftConsoleDownloadsDeploymentName         = DownloadsResourceName
-	OpenShiftConsoleDownloadsPDBName                = DownloadsResourceName
-	OpenShiftConsoleDownloadsRouteName              = DownloadsResourceName
-	OpenShiftConsoleNamespace                       = TargetNamespace
-	OpenShiftConsolePDBName                         = OpenShiftConsoleName
-	OpenShiftConsoleRouteName                       = OpenShiftConsoleName
-	OpenShiftConsoleServiceName                     = OpenShiftConsoleName
-	OpenShiftConsoleDownloadsServiceAccountName     = OpenShiftConsoleDownloadsDeploymentName
-	OpenshiftConsoleServiceAccountName              = OpenShiftConsoleDeploymentName
-	RedirectContainerTargetPort                     = RedirectContainerPort
-	OpenShiftConsoleSASyncControllerSuffix          = "ConsoleServiceAccountSyncController"
-	OpenshiftConsoleDownloadsSASyncControllerPrefix = "DownloadsServiceAccountSyncController"
+	OAuthClientName                             = OpenShiftConsoleName
+	OpenShiftConsoleDeploymentName              = OpenShiftConsoleName
+	OpenShiftConsoleDownloadsDeploymentName     = DownloadsResourceName
+	OpenShiftConsoleDownloadsPDBName            = DownloadsResourceName
+	OpenShiftConsoleDownloadsRouteName          = DownloadsResourceName
+	OpenShiftConsoleNamespace                   = TargetNamespace
+	OpenShiftConsolePDBName                     = OpenShiftConsoleName
+	OpenShiftConsoleRouteName                   = OpenShiftConsoleName
+	OpenShiftConsoleServiceName                 = OpenShiftConsoleName
+	OpenShiftConsoleDownloadsServiceAccountName = OpenShiftConsoleDownloadsDeploymentName
+	OpenShiftConsoleServiceAccountName          = OpenShiftConsoleDeploymentName
+	RedirectContainerTargetPort                 = RedirectContainerPort
 )

pkg/console/controllers/serviceaccounts/controller.go

@@ -3,6 +3,8 @@ package serviceaccounts
 import (
 	"context"
 	"fmt"
+	"reflect"
+	"strings"
 	"time"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -11,13 +13,15 @@ import (
 	operatorinformerv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1"
 	operatorlistersv1 "github.com/openshift/client-go/operator/listers/operator/v1"
 
+	"github.com/openshift/console-operator/bindata"
 	"github.com/openshift/console-operator/pkg/api"
 	"github.com/openshift/console-operator/pkg/console/controllers/util"
 	"github.com/openshift/console-operator/pkg/console/status"
-	serviceaccountssub "github.com/openshift/console-operator/pkg/console/subresource/serviceaccount"
+	subresourceutil "github.com/openshift/console-operator/pkg/console/subresource/util"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceread"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 
 	corev1 "k8s.io/api/core/v1"
@@ -31,6 +35,7 @@ import (
 
 type ServiceAccountSyncController struct {
 	serviceAccountName string
+	conditionType      string
 	operatorClient     v1helpers.OperatorClient
 	// configs
 	consoleOperatorLister operatorlistersv1.ConsoleLister
@@ -54,13 +59,12 @@ func NewServiceAccountSyncController(
 	serviceAccountName string,
 	// controllerName,
 	controllerName string,
-	// controllerSuffix
-	controllerSuffix string,
 ) factory.Controller {
 	configV1Informers := configInformer.Config().V1()
 
 	ctrl := &ServiceAccountSyncController{
 		serviceAccountName: serviceAccountName,
+		conditionType:      fmt.Sprintf("%sServiceAccountSync", controllerName),
 		// configs
 		operatorClient:        operatorClient,
 		consoleOperatorLister: operatorConfigInformer.Lister(),
@@ -81,32 +85,32 @@ func NewServiceAccountSyncController(
 		serviceAccountNameFilter,
 		serviceAccountInformer.Informer(),
 	).ResyncEvery(time.Minute).WithSync(ctrl.Sync).
-		ToController(controllerName, recorder.WithComponentSuffix(controllerSuffix))
+		ToController(fmt.Sprintf("%sServiceAccountController", strings.Title(controllerName)), recorder.WithComponentSuffix(fmt.Sprintf("%s-service-account-controller", controllerName)))
 }
 
 func (c *ServiceAccountSyncController) Sync(ctx context.Context, controllerContext factory.SyncContext) error {
 	operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get console operator config %s: %w", api.ConfigResourceName, err)
 	}
 	operatorConfigCopy := operatorConfig.DeepCopy()
 
 	switch operatorConfigCopy.Spec.ManagementState {
 	case operatorv1.Managed:
-		klog.V(4).Infoln("console is in a managed state: syncing service account")
+		klog.V(4).Infoln("console is in a managed state: syncing serviceaccount")
 	case operatorv1.Unmanaged:
-		klog.V(4).Infoln("console is in an unmanaged state: skipping service account sync")
+		klog.V(4).Infoln("console is in an unmanaged state: skipping serviceaccount sync")
 		return nil
 	case operatorv1.Removed:
-		klog.V(4).Infoln("console is in a removed state: removing synced service account")
+		klog.V(4).Infoln("console is in a removed state: removing synced serviceaccount")
 		return c.removeServiceAccount(ctx)
 	default:
 		return fmt.Errorf("unknown state: %v", operatorConfigCopy.Spec.ManagementState)
 	}
 	statusHandler := status.NewStatusHandler(c.operatorClient)
 
-	_, _, serviceAccountErr := c.SyncServiceAccount(ctx, operatorConfigCopy, controllerContext)
-	statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedApply", serviceAccountErr))
+	serviceAccountErr := c.SyncServiceAccount(ctx, operatorConfigCopy, controllerContext)
+	statusHandler.AddConditions(status.HandleProgressingOrDegraded(c.conditionType, "FailedApply", serviceAccountErr))
 	if serviceAccountErr != nil {
 		return statusHandler.FlushAndReturn(serviceAccountErr)
 	}
@@ -122,25 +126,52 @@ func (c *ServiceAccountSyncController) removeServiceAccount(ctx context.Context)
 	return err
 }
 
-func (c *ServiceAccountSyncController) SyncServiceAccount(ctx context.Context, operatorConfigCopy *operatorv1.Console, controllerContext factory.SyncContext) (*corev1.ServiceAccount, bool, error) {
-	requiredServiceAccount, err := serviceaccountssub.DefaultServiceAccountFactory(c.serviceAccountName, operatorConfigCopy)
+func (c *ServiceAccountSyncController) SyncServiceAccount(ctx context.Context, operatorConfigCopy *operatorv1.Console, controllerContext factory.SyncContext) error {
+	serviceAccount, err := c.DefaultServiceAccount(operatorConfigCopy)
+
 	if err != nil {
-		return nil, false, err
+		return err
 	}
 
-	// if the object has one or more ownerRef objects, then we must
-	// ensure that their controller attribute is set to false.
-	// Only one ownerRef.controller == true .
-	// https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#owner-references-in-object-specifications
-
-	for oR := range requiredServiceAccount.OwnerReferences {
-		falseBool := false
-		requiredServiceAccount.OwnerReferences[oR].Controller = &falseBool
+	// check for service account existence
+
+	existingServiceAccount, err := c.serviceAccountClient.ServiceAccounts(serviceAccount.Namespace).Get(ctx, serviceAccount.Name, metav1.GetOptions{})
+
+	if err == nil {
+		for _, oR := range existingServiceAccount.OwnerReferences {
+			// mark ownerref for deletion. we cannot have multiple owner refs
+			// https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourcemerge/object_merger.go#L214-L219
+			if reflect.DeepEqual(oR, *subresourceutil.OwnerRefFrom(operatorConfigCopy)) {
+				continue // we want to keep this controller=true
+			}
+			removalRef := oR.DeepCopy()
+			removalRef.UID = removalRef.UID + "-"
+			serviceAccount.OwnerReferences = append(serviceAccount.OwnerReferences, *removalRef)
+		}
+	} else if !apierrors.IsNotFound(err) {
+		return fmt.Errorf("failed to get existing service account %s: %w", c.serviceAccountName, err)
 	}
 
-	return resourceapply.ApplyServiceAccount(ctx,
+	_, _, err = resourceapply.ApplyServiceAccount(ctx,
 		c.serviceAccountClient,
 		controllerContext.Recorder(),
-		requiredServiceAccount,
+		serviceAccount,
+	)
+
+	if err != nil {
+		return fmt.Errorf("failed to apply service account %s: %w", c.serviceAccountName, err)
+	}
+
+	return nil
+}
+
+func (c *ServiceAccountSyncController) DefaultServiceAccount(cr *operatorv1.Console) (*corev1.ServiceAccount, error) {
+	serviceAccount := resourceread.ReadServiceAccountV1OrDie(
+		bindata.MustAsset(fmt.Sprintf("assets/serviceaccounts/%s-sa.yaml", c.serviceAccountName)),
 	)
+	if serviceAccount.Name == "" {
+		return nil, fmt.Errorf("No service account found for name %v .", c.serviceAccountName)
+	}
+	serviceAccount.SetOwnerReferences([]metav1.OwnerReference{*subresourceutil.OwnerRefFrom(cr)})
+	return serviceAccount, nil
 }

pkg/console/starter/starter.go

@@ -338,9 +338,8 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
 		kubeInformersNamespaced.Core().V1().ServiceAccounts(), // ServiceAccount
 
 		recorder,
-		api.OpenshiftConsoleServiceAccountName,
+		api.OpenShiftConsoleServiceAccountName,
 		api.OpenShiftConsoleName, // controller name
-		api.OpenShiftConsoleSASyncControllerSuffix,
 	)
 
 	downloadsServiceAccountController := serviceaccounts.NewServiceAccountSyncController(
@@ -357,7 +356,6 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
 
 		api.OpenShiftConsoleDownloadsServiceAccountName,
 		api.DownloadsResourceName,
-		api.OpenshiftConsoleDownloadsSASyncControllerPrefix,
 	)
 
 	downloadsDeploymentController := downloadsdeployment.NewDownloadsDeploymentSyncController(