[Backend] Keep workflow service account when not default or empty (ku…

…beflow#3435) * [Backend] Keep workflow service account when not default or empty * Fix unit tests * Rename const to be consistent in style
Jeffwan · Dec 9, 2020 · 7c8bcca · 7c8bcca
1 parent 873de2c
commit 7c8bcca
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 19 deletions.
diff --git a/backend/src/apiserver/common/config.go b/backend/src/apiserver/common/config.go
@@ -23,9 +23,10 @@ import (
 )
 
 const (
-	MultiUserMode string = "MULTIUSER"
-	PodNamespace  string = "POD_NAMESPACE"
-	CacheEnabled  string = "CacheEnabled"
+	MultiUserMode                       string = "MULTIUSER"
+	PodNamespace                        string = "POD_NAMESPACE"
+	CacheEnabled                        string = "CacheEnabled"
+	DefaultPipelineRunnerServiceAccount string = "DefaultPipelineRunnerServiceAccount"
 )
 
 func GetStringConfig(configName string) string {

diff --git a/backend/src/apiserver/resource/resource_manager.go b/backend/src/apiserver/resource/resource_manager.go
@@ -39,12 +39,10 @@ import (
 )
 
 const (
-	defaultPipelineRunnerServiceAccountEnvVar = "DefaultPipelineRunnerServiceAccount"
-	defaultPipelineRunnerServiceAccount       = "pipeline-runner"
-	defaultServiceAccount                     = "default-editor"
-	HasDefaultBucketEnvVar                    = "HAS_DEFAULT_BUCKET"
-	ProjectIDEnvVar                           = "PROJECT_ID"
-	DefaultBucketNameEnvVar                   = "BUCKET_NAME"
+	defaultPipelineRunnerServiceAccount = "pipeline-runner"
+	HasDefaultBucketEnvVar              = "HAS_DEFAULT_BUCKET"
+	ProjectIDEnvVar                     = "PROJECT_ID"
+	DefaultBucketNameEnvVar             = "BUCKET_NAME"
 )
 
 type ClientManagerInterface interface {
@@ -278,7 +276,7 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) {
 		return nil, util.Wrap(err, "Failed to verify parameters.")
 	}
 
-	r.setWorkflowServiceAccount(&workflow)
+	r.setDefaultServiceAccount(&workflow)
 
 	// Disable istio sidecar injection
 	workflow.SetAnnotationsToAllTemplates(util.AnnotationKeyIstioSidecarInject, util.AnnotationValueIstioSidecarInjectDisabled)
@@ -524,7 +522,7 @@ func (r *ResourceManager) CreateJob(apiJob *api.Job) (*model.Job, error) {
 		return nil, util.Wrap(err, "Create job failed")
 	}
 
-	r.setWorkflowServiceAccount(&workflow)
+	r.setDefaultServiceAccount(&workflow)
 
 	// Disable istio sidecar injection
 	workflow.SetAnnotationsToAllTemplates(util.AnnotationKeyIstioSidecarInject, util.AnnotationValueIstioSidecarInjectDisabled)
@@ -928,7 +926,7 @@ func (r *ResourceManager) MarkSampleLoaded() error {
 }
 
 func (r *ResourceManager) getDefaultSA() string {
-	return common.GetStringConfigWithDefault(defaultPipelineRunnerServiceAccountEnvVar, defaultPipelineRunnerServiceAccount)
+	return common.GetStringConfigWithDefault(common.DefaultPipelineRunnerServiceAccount, defaultPipelineRunnerServiceAccount)
 }
 
 func (r *ResourceManager) CreatePipelineVersion(apiVersion *api.PipelineVersion, pipelineFile []byte) (*model.PipelineVersion, error) {
@@ -1056,13 +1054,11 @@ func (r *ResourceManager) GetNamespaceFromJobID(jobId string) (string, error) {
 	return job.Namespace, nil
 }
 
-func (r *ResourceManager) setWorkflowServiceAccount(workflow *util.Workflow) {
-	if common.IsMultiUserMode() {
-		if len(workflow.Spec.ServiceAccountName) == 0 || workflow.Spec.ServiceAccountName == defaultPipelineRunnerServiceAccount {
-			// To reserve SDK backward compatibility, the backend currently replaces the serviceaccount in multi-user mode.
-			workflow.SetServiceAccount(defaultServiceAccount)
-		}
-	} else {
+func (r *ResourceManager) setDefaultServiceAccount(workflow *util.Workflow) {
+	workflowServiceAccount := workflow.Spec.ServiceAccountName
+	if len(workflowServiceAccount) == 0 || workflowServiceAccount == defaultPipelineRunnerServiceAccount {
+		// To reserve SDK backward compatibility, the backend only replaces
+		// serviceaccount when it is empty or equal to default value set by SDK.
 		workflow.SetServiceAccount(r.getDefaultSA())
 	}
 }

diff --git a/backend/src/apiserver/server/run_server_test.go b/backend/src/apiserver/server/run_server_test.go
@@ -142,7 +142,9 @@ func TestCreateRun_Unauthorized(t *testing.T) {
 
 func TestCreateRun_Multiuser(t *testing.T) {
 	viper.Set(common.MultiUserMode, "true")
+	viper.Set(common.DefaultPipelineRunnerServiceAccount, "default-editor")
 	defer viper.Set(common.MultiUserMode, "false")
+	defer viper.Set(common.DefaultPipelineRunnerServiceAccount, "pipeline-runner")
 
 	md := metadata.New(map[string]string{common.GoogleIAPUserIdentityHeader: "accounts.google.com:user@google.com"})
 	ctx := metadata.NewIncomingContext(context.Background(), md)