fix: only create and use service account if needed

There are currently two circumstances that require a service account: 1. user specified image pull secret 2. backend needs it, e.g. for an OpenShift SecurityContextConstraint Signed-off-by: Nick Mitchell <[email protected]>
aavarghese · Aug 15, 2024 · 7bdf698 · 7bdf698
1 parent f6bbc83
commit 7bdf698
Show file tree

Hide file tree

Showing 21 changed files with 73 additions and 48 deletions.
diff --git a/charts/minio/templates/job.yaml b/charts/minio/templates/job.yaml
@@ -14,7 +14,6 @@ metadata:
 spec:
   restartPolicy: OnFailure
   terminationGracePeriodSeconds: 0
-  serviceAccountName: {{ .Values.rbac.serviceaccount }}
   containers:
     {{- include "containers/minio" . | indent 4 }}
 

diff --git a/charts/shell/templates/pod.yaml b/charts/shell/templates/pod.yaml
@@ -12,7 +12,11 @@ metadata:
     app.kubernetes.io/managed-by: lunchpail.io
 spec:
   restartPolicy: OnFailure
+
+  {{- if .Values.rbac.serviceaccount }}
   serviceAccountName: {{ .Values.rbac.serviceaccount }}
+  {{- end }}
+
   {{- include "prestop/spec/pod" . | indent 2 }}
 
   {{- if .Values.securityContext }}

diff --git a/charts/templates/rbac.yaml b/charts/templates/rbac.yaml
@@ -1,10 +1,11 @@
-# TODO run controller should create this on demand?
+{{- if .Values.rbac.serviceaccount }}
 apiVersion: v1
 kind: ServiceAccount
 metadata:
-  name: {{ .Values.global.rbac.serviceaccount }}
+  name: {{ .Values.rbac.serviceaccount }}
   namespace: {{ .Values.namespace.user }}
 {{- if .Values.global.jaas.ips }}
 imagePullSecrets:
   - name: {{ .Values.global.jaas.ips }}
 {{- end }}
+{{- end }}
diff --git a/charts/templates/scc.yaml b/charts/templates/scc.yaml
@@ -1,4 +1,4 @@
-{{- if (eq .Values.global.type "oc")}}
+{{- if eq .Values.global.type "oc"}}
 kind: SecurityContextConstraints
 apiVersion: security.openshift.io/v1
 metadata:
@@ -31,5 +31,5 @@ supplementalGroups:
 volumes:
 - '*'
 users:
-- system:serviceaccount:{{ .Values.namespace.user }}:{{ .Values.global.rbac.serviceaccount }}
+- system:serviceaccount:{{ .Values.namespace.user }}:{{ .Values.rbac.serviceaccount }}
 {{- end }}
diff --git a/charts/workerpool/templates/job.yaml b/charts/workerpool/templates/job.yaml
@@ -38,7 +38,11 @@ spec:
       {{- end }}
 
       terminationGracePeriodSeconds: 5 # give time for the preStop in the container
+
+      {{- if .Values.rbac.serviceaccount }}
       serviceAccountName: {{ .Values.rbac.serviceaccount }}
+      {{- end }}
+
       volumes:
         {{- if .Values.volumes }}
         {{- .Values.volumes | b64dec | fromJsonArray | toYaml | nindent 8 }}

diff --git a/charts/workstealer/templates/job.yaml b/charts/workstealer/templates/job.yaml
@@ -16,7 +16,6 @@ metadata:
 spec:
   restartPolicy: OnFailure
   terminationGracePeriodSeconds: 0
-  serviceAccountName: {{ .Values.rbac.serviceaccount }}
   containers:
     {{- include "containers/workstealer" . | indent 4 }}
 

diff --git a/pkg/be/backend.go b/pkg/be/backend.go
@@ -16,7 +16,7 @@ type Backend interface {
 	Ok() error
 
 	// Overrides to values used by linker.Configure
-	Values() ([]string, error)
+	Values() (platform.Values, error)
 
 	// Bring up the linked application
 	Up(linked ir.Linked) error

diff --git a/pkg/be/ibmcloud/values.go b/pkg/be/ibmcloud/values.go
@@ -1,5 +1,7 @@
 package ibmcloud
 
-func (backend Backend) Values() ([]string, error) {
-	return []string{}, nil
+import "lunchpail.io/pkg/be/platform"
+
+func (backend Backend) Values() (platform.Values, error) {
+	return platform.Values{}, nil
 }
diff --git a/pkg/be/kubernetes/values.go b/pkg/be/kubernetes/values.go
@@ -8,31 +8,36 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8s "k8s.io/client-go/kubernetes"
+
+	"lunchpail.io/pkg/be/platform"
 )
 
-func openshiftSpecificValues(clientset *k8s.Clientset) ([]string, error) {
+func openshiftSpecificValues(clientset *k8s.Clientset) (platform.Values, error) {
+	var values platform.Values
+
 	namespaces, err := clientset.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{})
 	if err != nil {
-		return nil, err
+		return values, err
 	}
 
 	openshiftIdx := slices.IndexFunc(namespaces.Items, func(ns corev1.Namespace) bool { return strings.Contains(ns.Name, "openshift") })
 	if openshiftIdx >= 0 {
-		return []string{"global.type=oc"}, nil
+		values.Kv = append(values.Kv, "global.type=oc")
+		values.NeedsServiceAccount = true
 	}
 
-	return []string{}, nil
+	return values, nil
 }
 
-func (backend Backend) Values() ([]string, error) {
+func (backend Backend) Values() (platform.Values, error) {
 	clientset, _, err := Client()
 	if err != nil {
-		return nil, err
+		return platform.Values{}, err
 	}
 
 	openshiftValues, err := openshiftSpecificValues(clientset)
 	if err != nil {
-		return nil, err
+		return platform.Values{}, err
 	}
 
 	return openshiftValues, nil

diff --git a/pkg/be/platform/values.go b/pkg/be/platform/values.go
@@ -0,0 +1,9 @@
+package platform
+
+type Values struct {
+	// list of key=value pairs
+	Kv []string
+
+	// Do we need a Kubernetes service account?
+	NeedsServiceAccount bool
+}
diff --git a/pkg/fe/compile.go b/pkg/fe/compile.go
@@ -47,7 +47,7 @@ func Compile(backend be.Backend, opts CompileOptions) (ir.Linked, error) {
 		fmt.Fprintf(os.Stderr, "Using internal S3 port %d\n", internalS3Port)
 	}
 
-	yamlValues, dashdashSetValues, dashdashSetFileValues, queueSpec, err := linker.Configure(compilationName, runname, namespace, templatePath, internalS3Port, backend, opts.ConfigureOptions)
+	yamlValues, dashdashSetValues, dashdashSetFileValues, queueSpec, serviceAccount, err := linker.Configure(compilationName, runname, namespace, templatePath, internalS3Port, backend, opts.ConfigureOptions)
 	if err != nil {
 		return ir.Linked{}, err
 	}
@@ -57,7 +57,7 @@ func Compile(backend be.Backend, opts CompileOptions) (ir.Linked, error) {
 		return ir.Linked{}, err
 	} else if hlir, err := parser.Parse(yaml); err != nil {
 		return ir.Linked{}, err
-	} else if llir, err := transformer.Lower(compilationName, runname, namespace, hlir, queueSpec, opts.ConfigureOptions.CompilationOptions, opts.Verbose); err != nil {
+	} else if llir, err := transformer.Lower(compilationName, runname, namespace, hlir, queueSpec, serviceAccount, opts.ConfigureOptions.CompilationOptions, opts.Verbose); err != nil {
 		return ir.Linked{}, err
 	} else {
 		return ir.Linked{

diff --git a/pkg/fe/linker/configure.go b/pkg/fe/linker/configure.go
@@ -17,14 +17,14 @@ type ConfigureOptions struct {
 	Verbose            bool
 }
 
-func Configure(appname, runname, namespace, templatePath string, internalS3Port int, backend be.Backend, opts ConfigureOptions) (string, []string, []string, queue.Spec, error) {
+func Configure(appname, runname, namespace, templatePath string, internalS3Port int, backend be.Backend, opts ConfigureOptions) (string, []string, []string, queue.Spec, string, error) {
 	if opts.Verbose {
 		fmt.Fprintf(os.Stderr, "Stage directory %s\n", templatePath)
 	}
 
 	shrinkwrappedOptions, err := compilation.RestoreOptions(templatePath)
 	if err != nil {
-		return "", nil, nil, queue.Spec{}, err
+		return "", nil, nil, queue.Spec{}, "", err
 	} else {
 		if opts.CompilationOptions.Namespace == "" {
 			opts.CompilationOptions.Namespace = shrinkwrappedOptions.Namespace
@@ -62,17 +62,17 @@ func Configure(appname, runname, namespace, templatePath string, internalS3Port
 
 	queueSpec, err := queue.ParseFlag(opts.CompilationOptions.Queue, runname, internalS3Port)
 	if err != nil {
-		return "", nil, nil, queue.Spec{}, err
+		return "", nil, nil, queue.Spec{}, "", err
 	}
 
 	imagePullSecretName, dockerconfigjson, ipsErr := imagePullSecret(opts.CompilationOptions.ImagePullSecret)
 	if ipsErr != nil {
-		return "", nil, nil, queue.Spec{}, ipsErr
+		return "", nil, nil, queue.Spec{}, "", ipsErr
 	}
 
 	user, err := user.Current()
 	if err != nil {
-		return "", nil, nil, queue.Spec{}, err
+		return "", nil, nil, queue.Spec{}, "", err
 	}
 
 	// the app.kubernetes.io/part-of label value
@@ -93,11 +93,19 @@ func Configure(appname, runname, namespace, templatePath string, internalS3Port
 		queueSpec.Bucket = queueSpec.Name
 	}
 
+	backendValues, err := backend.Values()
+	if err != nil {
+		return "", nil, nil, queue.Spec{}, "", err
+	}
+
+	serviceAccount := runname
+	if !backendValues.NeedsServiceAccount && imagePullSecretName == "" {
+		serviceAccount = ""
+	}
+
 	yaml := fmt.Sprintf(`
 global:
   dockerHost: %s # dockerHost (1)
-  rbac:
-    serviceaccount: %s # runname (2)
   jaas:
     ips: %s # imagePullSecretName (3)
     dockerconfigjson: %s # dockerconfigjson (4)
@@ -115,7 +123,7 @@ uid: %s # user.Uid (11)
 mcad:
   enabled: false
 rbac:
-  serviceaccount: %s # runname (12)
+  serviceaccount: %s # serviceAccount (12)
 image:
   registry: %s # imageRegistry (13)
   repo: %s # imageRepo (14)
@@ -141,7 +149,6 @@ lunchpail_internal:
     sleep_before_exit: %s # sleepBeforeExit (28)
 `,
 		opts.CompilationOptions.DockerHost,      // (1)
-		runname,                                 // (2)
 		imagePullSecretName,                     // (3)
 		dockerconfigjson,                        // (4)
 		systemNamespace,                         // (5)
@@ -152,7 +159,7 @@ lunchpail_internal:
 		internalS3Port,                           // (9)
 		user.Username,                            // (10)
 		user.Uid,                                 // (11)
-		runname,                                  // (12)
+		serviceAccount,                           // (12)
 		imageRegistry,                            // (13)
 		imageRepo,                                // (14)
 		lunchpail.Version(),                      // (15)
@@ -171,20 +178,15 @@ lunchpail_internal:
 		os.Getenv("LUNCHPAIL_SLEEP_BEFORE_EXIT"), // (28)
 	)
 
-	backendValues, err := backend.Values()
-	if err != nil {
-		return "", nil, nil, queue.Spec{}, err
-	}
-
 	if opts.Verbose {
 		fmt.Fprintf(os.Stderr, "shrinkwrap app values=%s\n", yaml)
 		fmt.Fprintf(os.Stderr, "shrinkwrap app overrides=%v\n", opts.CompilationOptions.OverrideValues)
 		fmt.Fprintf(os.Stderr, "shrinkwrap app file overrides=%v\n", opts.CompilationOptions.OverrideFileValues)
 		fmt.Fprintf(os.Stderr, "shrinkwrap backend overrides=%v\n", backendValues)
 	}
 
-	overrides := slices.Concat(opts.CompilationOptions.OverrideValues, backendValues)
+	overrides := slices.Concat(opts.CompilationOptions.OverrideValues, backendValues.Kv)
 	fileOverrides := opts.CompilationOptions.OverrideFileValues // Note: no backend value support here
 
-	return yaml, overrides, fileOverrides, queueSpec, nil
+	return yaml, overrides, fileOverrides, queueSpec, serviceAccount, nil
 }
diff --git a/pkg/fe/transformer/api/dispatch/parametersweep/lower.go b/pkg/fe/transformer/api/dispatch/parametersweep/lower.go
@@ -20,6 +20,7 @@ func Lower(compilationName, runname, namespace string, sweep hlir.ParameterSweep
 		namespace,
 		app,
 		queueSpec,
+		"", // no service account needed
 		opts,
 		verbose,
 	)

diff --git a/pkg/fe/transformer/api/dispatch/s3/lower.go b/pkg/fe/transformer/api/dispatch/s3/lower.go
@@ -20,6 +20,7 @@ func Lower(compilationName, runname, namespace string, s3 hlir.ProcessS3Objects,
 		namespace,
 		app,
 		queueSpec,
+		"", // no service account needed
 		opts,
 		verbose,
 	)

diff --git a/pkg/fe/transformer/api/minio/lower.go b/pkg/fe/transformer/api/minio/lower.go
@@ -37,7 +37,6 @@ func Lower(compilationName, runname, namespace string, model hlir.AppModel, queu
 		"namespace.user=" + namespace,
 		"lunchpail=lunchpail",
 		"mcad.enabled=false",
-		"rbac.serviceaccount=" + runname,
 		"image.registry=" + lunchpail.ImageRegistry,
 		"image.repo=" + lunchpail.ImageRepo,
 		"image.version=" + lunchpail.Version(),

diff --git a/pkg/fe/transformer/api/shell/lower.go b/pkg/fe/transformer/api/shell/lower.go
@@ -15,7 +15,7 @@ import (
 	"lunchpail.io/pkg/util"
 )
 
-func Lower(compilationName, runname, namespace string, app hlir.Application, queueSpec queue.Spec, opts compilation.Options, verbose bool) (llir.Component, error) {
+func Lower(compilationName, runname, namespace string, app hlir.Application, queueSpec queue.Spec, serviceAccount string, opts compilation.Options, verbose bool) (llir.Component, error) {
 	component := ""
 	switch app.Spec.Role {
 	case "dispatcher":
@@ -79,7 +79,7 @@ func Lower(compilationName, runname, namespace string, app hlir.Application, que
 		"taskqueue.prefixPath=" + api.QueuePrefixPath(queueSpec, runname),
 		"mcad.enabled=false",
 		"rbac.runAsRoot=false",
-		"rbac.serviceaccount=" + runname,
+		"rbac.serviceaccount=" + serviceAccount,
 		"securityContext=" + securityContext,
 		"containerSecurityContext=" + containerSecurityContext,
 		"workdir.cm.data=" + workdirCmData,

diff --git a/pkg/fe/transformer/api/workerpool/lower.go b/pkg/fe/transformer/api/workerpool/lower.go
@@ -16,7 +16,7 @@ import (
 	"lunchpail.io/pkg/util"
 )
 
-func Lower(compilationName, runname, namespace string, app hlir.Application, pool hlir.WorkerPool, queueSpec queue.Spec, opts compilation.Options, verbose bool) (llir.Component, error) {
+func Lower(compilationName, runname, namespace string, app hlir.Application, pool hlir.WorkerPool, queueSpec queue.Spec, serviceAccount string, opts compilation.Options, verbose bool) (llir.Component, error) {
 	// name of worker pods/deployment = run_name-pool_name
 	releaseName := strings.TrimSuffix(
 		util.Truncate(
@@ -94,7 +94,7 @@ func Lower(compilationName, runname, namespace string, app hlir.Application, poo
 		"startupDelay=" + strconv.Itoa(startupDelay),
 		"mcad.enabled=false",
 		"rbac.runAsRoot=false",
-		"rbac.serviceaccount=" + runname,
+		"rbac.serviceaccount=" + serviceAccount,
 		"securityContext=" + securityContext,
 		"containerSecurityContext=" + containerSecurityContext,
 		"workdir.cm.data=" + workdirCmData,

diff --git a/pkg/fe/transformer/api/workerpool/lowerall.go b/pkg/fe/transformer/api/workerpool/lowerall.go
@@ -10,7 +10,7 @@ import (
 )
 
 // HLIR -> LLIR for []hlir.WorkerPool
-func LowerAll(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, opts compilation.Options, verbose bool) ([]llir.Component, error) {
+func LowerAll(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, serviceAccount string, opts compilation.Options, verbose bool) ([]llir.Component, error) {
 	components := []llir.Component{}
 
 	app, found := model.GetApplicationByRole(hlir.WorkerRole)
@@ -19,7 +19,7 @@ func LowerAll(compilationName, runname, namespace string, model hlir.AppModel, q
 	}
 
 	for _, pool := range model.WorkerPools {
-		if component, err := Lower(compilationName, runname, namespace, app, pool, queueSpec, opts, verbose); err != nil {
+		if component, err := Lower(compilationName, runname, namespace, app, pool, queueSpec, serviceAccount, opts, verbose); err != nil {
 			return components, err
 		} else {
 			components = append(components, component)

diff --git a/pkg/fe/transformer/api/workstealer/lower.go b/pkg/fe/transformer/api/workstealer/lower.go
@@ -29,7 +29,6 @@ func Lower(compilationName, runname, namespace string, app hlir.Application, que
 		"namespace.user=" + namespace,
 		"lunchpail=lunchpail",
 		"mcad.enabled=false",
-		"rbac.serviceaccount=" + runname,
 		"image.registry=" + lunchpail.ImageRegistry,
 		"image.repo=" + lunchpail.ImageRepo,
 		"image.version=" + lunchpail.Version(),

diff --git a/pkg/fe/transformer/application.go b/pkg/fe/transformer/application.go
@@ -10,7 +10,7 @@ import (
 )
 
 // HLIR -> LLIR for []hlir.Application
-func lowerApplications(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, opts compilation.Options, verbose bool) ([]llir.Component, error) {
+func lowerApplications(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, serviceAccount string, opts compilation.Options, verbose bool) ([]llir.Component, error) {
 	components := []llir.Component{}
 
 	for _, r := range model.Applications {
@@ -22,7 +22,7 @@ func lowerApplications(compilationName, runname, namespace string, model hlir.Ap
 				components = append(components, component)
 			}
 		default:
-			if component, err := shell.Lower(compilationName, runname, namespace, r, queueSpec, opts, verbose); err != nil {
+			if component, err := shell.Lower(compilationName, runname, namespace, r, queueSpec, serviceAccount, opts, verbose); err != nil {
 				return components, err
 			} else {
 				components = append(components, component)

diff --git a/pkg/fe/transformer/lower.go b/pkg/fe/transformer/lower.go
@@ -13,13 +13,13 @@ import (
 )
 
 // HLIR -> LLIR
-func Lower(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, opts compilation.Options, verbose bool) (llir.LLIR, error) {
+func Lower(compilationName, runname, namespace string, model hlir.AppModel, queueSpec queue.Spec, serviceAccount string, opts compilation.Options, verbose bool) (llir.LLIR, error) {
 	minio, err := minio.Lower(compilationName, runname, namespace, model, queueSpec, opts, verbose)
 	if err != nil {
 		return llir.LLIR{}, err
 	}
 
-	apps, err := lowerApplications(compilationName, runname, namespace, model, queueSpec, opts, verbose)
+	apps, err := lowerApplications(compilationName, runname, namespace, model, queueSpec, serviceAccount, opts, verbose)
 	if err != nil {
 		return llir.LLIR{}, err
 	}
@@ -29,7 +29,7 @@ func Lower(compilationName, runname, namespace string, model hlir.AppModel, queu
 		return llir.LLIR{}, err
 	}
 
-	pools, err := workerpool.LowerAll(compilationName, runname, namespace, model, queueSpec, opts, verbose)
+	pools, err := workerpool.LowerAll(compilationName, runname, namespace, model, queueSpec, serviceAccount, opts, verbose)
 	if err != nil {
 		return llir.LLIR{}, err
 	}