From fa966967122cdfacfb316e1746c707b19675eb29 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Mon, 15 Jul 2019 11:35:27 +0900 Subject: [PATCH 01/13] Support AutomountServiceAccountToken --- api/openapi-spec/swagger.json | 8 +++++++ .../workflow/v1alpha1/openapi_generated.go | 14 +++++++++++ pkg/apis/workflow/v1alpha1/types.go | 8 +++++++ .../v1alpha1/zz_generated.deepcopy.go | 10 ++++++++ workflow/controller/workflowpod.go | 16 +++++++++---- workflow/controller/workflowpod_test.go | 24 +++++++++++++++++++ 6 files changed, 75 insertions(+), 5 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index df852a66e895..707852f3963c 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -817,6 +817,10 @@ "description": "Location in which all files related to the step will be stored (logs, artifacts, etc...). Can be overridden by individual items in Outputs. If omitted, will use the default artifact repository location configured in the controller, appended with the \u003cworkflowname\u003e/\u003cnodename\u003e in the key.", "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ArtifactLocation" }, + "automountServiceAccountToken": { + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + "type": "boolean" + }, "container": { "description": "Container is the main container image to run in the pod", "$ref": "#/definitions/io.k8s.api.core.v1.Container" @@ -1160,6 +1164,10 @@ "description": "Arguments contain the parameters and artifacts sent to the workflow entrypoint Parameters are referencable globally using the 'workflow' variable prefix. e.g. {{workflow.parameters.myparam}}", "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.Arguments" }, + "automountServiceAccountToken": { + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + "type": "boolean" + }, "dnsConfig": { "description": "PodDNSConfig defines the DNS parameters of a pod in addition to those generated from DNSPolicy.", "$ref": "#/definitions/io.k8s.api.core.v1.PodDNSConfig" diff --git a/pkg/apis/workflow/v1alpha1/openapi_generated.go b/pkg/apis/workflow/v1alpha1/openapi_generated.go index bc3bd4d5f5b4..3eeb03f0365c 100644 --- a/pkg/apis/workflow/v1alpha1/openapi_generated.go +++ b/pkg/apis/workflow/v1alpha1/openapi_generated.go @@ -1710,6 +1710,13 @@ func schema_pkg_apis_workflow_v1alpha1_Template(ref common.ReferenceCallback) co Format: "", }, }, + "automountServiceAccountToken": { + SchemaProps: spec.SchemaProps{ + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + Type: []string{"boolean"}, + Format: "", + }, + }, "hostAliases": { SchemaProps: spec.SchemaProps{ Description: "HostAliases is an optional list of hosts and IPs that will be injected into the pod spec", @@ -2140,6 +2147,13 @@ func schema_pkg_apis_workflow_v1alpha1_WorkflowSpec(ref common.ReferenceCallback Format: "", }, }, + "automountServiceAccountToken": { + SchemaProps: spec.SchemaProps{ + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + Type: []string{"boolean"}, + Format: "", + }, + }, "volumes": { SchemaProps: spec.SchemaProps{ Description: "Volumes is a list of volumes that can be mounted by containers in a workflow.", diff --git a/pkg/apis/workflow/v1alpha1/types.go b/pkg/apis/workflow/v1alpha1/types.go index ea8d64b9ba29..d4df77631e0c 100644 --- a/pkg/apis/workflow/v1alpha1/types.go +++ b/pkg/apis/workflow/v1alpha1/types.go @@ -86,6 +86,10 @@ type WorkflowSpec struct { // ServiceAccountName is the name of the ServiceAccount to run all pods of the workflow as. ServiceAccountName string `json:"serviceAccountName,omitempty"` + // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. + // +optional + AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` + // Volumes is a list of volumes that can be mounted by containers in a workflow. Volumes []apiv1.Volume `json:"volumes,omitempty"` @@ -264,6 +268,10 @@ type Template struct { // ServiceAccountName to apply to workflow pods ServiceAccountName string `json:"serviceAccountName,omitempty"` + // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. + // +optional + AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` + // HostAliases is an optional list of hosts and IPs that will be injected into the pod spec HostAliases []apiv1.HostAlias `json:"hostAliases,omitempty"` diff --git a/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go index c59bbc6621cb..4dbe88fa63a6 100644 --- a/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go @@ -853,6 +853,11 @@ func (in *Template) DeepCopyInto(out *Template) { *out = new(int32) **out = **in } + if in.AutomountServiceAccountToken != nil { + in, out := &in.AutomountServiceAccountToken, &out.AutomountServiceAccountToken + *out = new(bool) + **out = **in + } if in.HostAliases != nil { in, out := &in.HostAliases, &out.HostAliases *out = make([]v1.HostAlias, len(*in)) @@ -988,6 +993,11 @@ func (in *WorkflowSpec) DeepCopyInto(out *WorkflowSpec) { } } in.Arguments.DeepCopyInto(&out.Arguments) + if in.AutomountServiceAccountToken != nil { + in, out := &in.AutomountServiceAccountToken, &out.AutomountServiceAccountToken + *out = new(bool) + **out = **in + } if in.Volumes != nil { in, out := &in.Volumes, &out.Volumes *out = make([]v1.Volume, len(*in)) diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 489f9b102ea2..bf491fb88bf7 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -90,6 +90,7 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont woc.log.Debugf("Creating Pod: %s (%s)", nodeName, nodeID) tmpl = tmpl.DeepCopy() wfSpec := woc.wf.Spec.DeepCopy() + tmplServiceAccountName := woc.wf.Spec.ServiceAccountName if tmpl.ServiceAccountName != "" { tmplServiceAccountName = tmpl.ServiceAccountName @@ -112,14 +113,19 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont }, }, Spec: apiv1.PodSpec{ - RestartPolicy: apiv1.RestartPolicyNever, - Volumes: woc.createVolumes(), - ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds, - ServiceAccountName: tmplServiceAccountName, - ImagePullSecrets: woc.wf.Spec.ImagePullSecrets, + RestartPolicy: apiv1.RestartPolicyNever, + Volumes: woc.createVolumes(), + ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds, + ServiceAccountName: tmplServiceAccountName, + AutomountServiceAccountToken: woc.wf.Spec.AutomountServiceAccountToken, + ImagePullSecrets: woc.wf.Spec.ImagePullSecrets, }, } + if tmpl.AutomountServiceAccountToken != nil { + pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken + } + if woc.wf.Spec.HostNetwork != nil { pod.Spec.HostNetwork = *woc.wf.Spec.HostNetwork } diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index b603f09cb727..053aa4a00cc7 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -100,6 +100,30 @@ func TestTmplServiceAccount(t *testing.T) { assert.Equal(t, pod.Spec.ServiceAccountName, "tmpl") } +// TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. +func TestWFLevelAutomountServiceAccountToken(t *testing.T) { + woc := newWoc() + automountServiceAccountToken := false + woc.wf.Spec.AutomountServiceAccountToken = &automountServiceAccountToken + woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") + podName := getPodName(woc.wf) + pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) + assert.Nil(t, err) + assert.Equal(t, *pod.Spec.AutomountServiceAccountToken, false) +} + +// TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. +func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { + woc := newWoc() + automountServiceAccountToken := false + woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &automountServiceAccountToken + woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") + podName := getPodName(woc.wf) + pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) + assert.Nil(t, err) + assert.Equal(t, *pod.Spec.AutomountServiceAccountToken, false) +} + // TestImagePullSecrets verifies the ability to carry forward imagePullSecrets from workflow.spec func TestImagePullSecrets(t *testing.T) { woc := newWoc() From 94d28283c8cdf442c343e2b67353099b506476ac Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Mon, 15 Jul 2019 23:09:02 +0900 Subject: [PATCH 02/13] Disallow automountServiceAccountToken in resource templates --- api/openapi-spec/swagger.json | 4 ++-- .../workflow/v1alpha1/openapi_generated.go | 4 ++-- pkg/apis/workflow/v1alpha1/types.go | 2 ++ workflow/controller/workflowpod.go | 21 ++++++++++--------- workflow/validate/validate.go | 3 +++ 5 files changed, 20 insertions(+), 14 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 707852f3963c..7ea8c61ca213 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -818,7 +818,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ArtifactLocation" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. It cannot be set in resource templates.", "type": "boolean" }, "container": { @@ -1165,7 +1165,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.Arguments" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. It is ignored in resource templates.", "type": "boolean" }, "dnsConfig": { diff --git a/pkg/apis/workflow/v1alpha1/openapi_generated.go b/pkg/apis/workflow/v1alpha1/openapi_generated.go index 3eeb03f0365c..0a48021257f8 100644 --- a/pkg/apis/workflow/v1alpha1/openapi_generated.go +++ b/pkg/apis/workflow/v1alpha1/openapi_generated.go @@ -1712,7 +1712,7 @@ func schema_pkg_apis_workflow_v1alpha1_Template(ref common.ReferenceCallback) co }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. It cannot be set in resource templates.", Type: []string{"boolean"}, Format: "", }, @@ -2149,7 +2149,7 @@ func schema_pkg_apis_workflow_v1alpha1_WorkflowSpec(ref common.ReferenceCallback }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. It is ignored in resource templates.", Type: []string{"boolean"}, Format: "", }, diff --git a/pkg/apis/workflow/v1alpha1/types.go b/pkg/apis/workflow/v1alpha1/types.go index d4df77631e0c..2fc885b19252 100644 --- a/pkg/apis/workflow/v1alpha1/types.go +++ b/pkg/apis/workflow/v1alpha1/types.go @@ -87,6 +87,7 @@ type WorkflowSpec struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. + // It is ignored in resource templates. // +optional AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` @@ -269,6 +270,7 @@ type Template struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. + // It cannot be set in resource templates. // +optional AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index bf491fb88bf7..b56a0bda4bd1 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -113,19 +113,14 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont }, }, Spec: apiv1.PodSpec{ - RestartPolicy: apiv1.RestartPolicyNever, - Volumes: woc.createVolumes(), - ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds, - ServiceAccountName: tmplServiceAccountName, - AutomountServiceAccountToken: woc.wf.Spec.AutomountServiceAccountToken, - ImagePullSecrets: woc.wf.Spec.ImagePullSecrets, + RestartPolicy: apiv1.RestartPolicyNever, + Volumes: woc.createVolumes(), + ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds, + ServiceAccountName: tmplServiceAccountName, + ImagePullSecrets: woc.wf.Spec.ImagePullSecrets, }, } - if tmpl.AutomountServiceAccountToken != nil { - pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken - } - if woc.wf.Spec.HostNetwork != nil { pod.Spec.HostNetwork = *woc.wf.Spec.HostNetwork } @@ -151,6 +146,12 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont } if tmpl.GetType() != wfv1.TemplateTypeResource { + if woc.wf.Spec.AutomountServiceAccountToken != nil { + pod.Spec.AutomountServiceAccountToken = woc.wf.Spec.AutomountServiceAccountToken + } else if tmpl.AutomountServiceAccountToken != nil { + pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken + } + // we do not need the wait container for resource templates because // argoexec runs as the main container and will perform the job of // annotating the outputs or errors, making the wait container redundant. diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index 48853e957e88..a61e05c3f0c5 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -334,6 +334,9 @@ func validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error { default: return errors.Errorf(errors.CodeBadRequest, "templates.%s.resource.action must be either get, create, apply, delete or replace", tmpl.Name) } + if tmpl.AutomountServiceAccountToken != nil { + return errors.Errorf(errors.CodeBadRequest, "templates.%s.automountServiceAccountToken can not be set in resource templates", tmpl.Name) + } // Try to unmarshal the given manifest. obj := unstructured.Unstructured{} err := yaml.Unmarshal([]byte(tmpl.Resource.Manifest), &obj) From 9d122352eed5678fb07cfda13d0dc4c3f541b72f Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Mon, 15 Jul 2019 23:54:55 +0900 Subject: [PATCH 03/13] Support ServiceAccountTokenName for init/wait containers --- workflow/common/common.go | 2 ++ workflow/config/config.go | 3 +++ workflow/controller/workflowpod.go | 25 ++++++++++++++++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/workflow/common/common.go b/workflow/common/common.go index 68909a755877..9b38aa5f2312 100644 --- a/workflow/common/common.go +++ b/workflow/common/common.go @@ -116,6 +116,8 @@ const ( KubeConfigDefaultMountPath = "/kube/config" KubeConfigDefaultVolumeName = "kubeconfig" + ServiceAccountTokenMountPath = "/var/run/secrets/kubernetes.io/serviceaccount" + ServiceAccountTokenVolumeName = "exec-sa-token" SecretVolMountPath = "/argo/secret" ) diff --git a/workflow/config/config.go b/workflow/config/config.go index 4409faecbbde..4c5c374ff60b 100644 --- a/workflow/config/config.go +++ b/workflow/config/config.go @@ -26,6 +26,9 @@ type WorkflowControllerConfig struct { // KubeConfig specifies a kube config file for the wait & init containers KubeConfig *KubeConfig `json:"kubeConfig,omitempty"` + // ServiceAccountTokenName specifies a service account token name for the wait & init containers. + ServiceAccountTokenName string `json:"serviceAccountTokenName,omitempty"` + // ContainerRuntimeExecutor specifies the container runtime interface to use, default is docker ContainerRuntimeExecutor string `json:"containerRuntimeExecutor,omitempty"` diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index b56a0bda4bd1..2aeaf79d794e 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -151,6 +151,9 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont } else if tmpl.AutomountServiceAccountToken != nil { pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken } + if woc.controller.Config.KubeConfig == nil && woc.controller.Config.ServiceAccountTokenName == "" { + return nil, errors.Errorf(errors.CodeBadRequest, "automountServiceAccountToken cannot be set because the controller is not configured with kubeConfig nor serviceAccountTokenName") + } // we do not need the wait container for resource templates because // argoexec runs as the main container and will perform the job of @@ -401,6 +404,16 @@ func (woc *wfOperationCtx) createVolumes() []apiv1.Volume { }, }) } + if woc.controller.Config.ServiceAccountTokenName != "" { + volumes = append(volumes, apiv1.Volume{ + Name: common.ServiceAccountTokenVolumeName, + VolumeSource: apiv1.VolumeSource{ + Secret: &apiv1.SecretVolumeSource{ + SecretName: woc.controller.Config.ServiceAccountTokenName, + }, + }, + }) + } switch woc.controller.Config.ContainerRuntimeExecutor { case common.ContainerRuntimeExecutorKubelet, common.ContainerRuntimeExecutorK8sAPI, common.ContainerRuntimeExecutorPNS: return volumes @@ -436,15 +449,21 @@ func (woc *wfOperationCtx) newExecContainer(name string) *apiv1.Container { if name == "" { name = common.KubeConfigDefaultVolumeName } - exec.VolumeMounts = []apiv1.VolumeMount{{ + exec.VolumeMounts = append(exec.VolumeMounts, apiv1.VolumeMount{ Name: name, MountPath: path, ReadOnly: true, SubPath: woc.controller.Config.KubeConfig.SecretKey, - }, - } + }) exec.Args = append(exec.Args, "--kubeconfig="+path) } + if woc.controller.Config.ServiceAccountTokenName != "" { + exec.VolumeMounts = append(exec.VolumeMounts, apiv1.VolumeMount{ + Name: common.ServiceAccountTokenVolumeName, + MountPath: common.ServiceAccountTokenMountPath, + ReadOnly: true, + }) + } return &exec } From 3a87b5abea26ab6a0a766ca97033025e6d2cf3de Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 16 Jul 2019 00:03:07 +0900 Subject: [PATCH 04/13] Add a test for ServiceAccountTokenName --- workflow/controller/workflowpod_test.go | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index 053aa4a00cc7..144ebdb46b4f 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -465,6 +465,32 @@ func TestOutOfCluster(t *testing.T) { } } +// TestServiceAccountTokenName verifies a volume and volumeMount of mounting a given service account token. +func TestServiceAccountTokenName(t *testing.T) { + verifyServiceAccountTokenVolume := func(ctr apiv1.Container, volName, mountPath string) { + for _, vol := range ctr.VolumeMounts { + if vol.Name == volName && vol.MountPath == mountPath { + return + } + } + t.Fatalf("%v does not have serviceAccountToken mounted properly (name: %s, mountPath: %s)", ctr, volName, mountPath) + } + + woc := newWoc() + woc.controller.Config.ServiceAccountTokenName = "foo" + + woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") + podName := getPodName(woc.wf) + pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) + + assert.NoError(t, err) + assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[1].Name) + assert.Equal(t, "foo", pod.Spec.Volumes[1].VolumeSource.Secret.SecretName) + + waitCtr := pod.Spec.Containers[0] + verifyServiceAccountTokenVolume(waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") +} + // TestPriority verifies the ability to carry forward priorityClassName and priority. func TestPriority(t *testing.T) { priority := int32(15) From 2a47ec850b20dca159c862d9326c16c47fa251c5 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 16 Jul 2019 00:17:37 +0900 Subject: [PATCH 05/13] Fix wrong condition of AutomountServiceAccountToken check --- workflow/controller/workflowpod.go | 6 ++++-- workflow/controller/workflowpod_test.go | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 2aeaf79d794e..9ec43739d562 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -151,8 +151,10 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont } else if tmpl.AutomountServiceAccountToken != nil { pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken } - if woc.controller.Config.KubeConfig == nil && woc.controller.Config.ServiceAccountTokenName == "" { - return nil, errors.Errorf(errors.CodeBadRequest, "automountServiceAccountToken cannot be set because the controller is not configured with kubeConfig nor serviceAccountTokenName") + if pod.Spec.AutomountServiceAccountToken != nil && !*pod.Spec.AutomountServiceAccountToken { + if woc.controller.Config.KubeConfig == nil && woc.controller.Config.ServiceAccountTokenName == "" { + return nil, errors.Errorf(errors.CodeBadRequest, "automountServiceAccountToken cannot be set because the controller is not configured with kubeConfig nor serviceAccountTokenName") + } } // we do not need the wait container for resource templates because diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index 144ebdb46b4f..577639c9769c 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -103,6 +103,7 @@ func TestTmplServiceAccount(t *testing.T) { // TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. func TestWFLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() + woc.controller.Config.ServiceAccountTokenName = "foo" automountServiceAccountToken := false woc.wf.Spec.AutomountServiceAccountToken = &automountServiceAccountToken woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") @@ -115,6 +116,7 @@ func TestWFLevelAutomountServiceAccountToken(t *testing.T) { // TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() + woc.controller.Config.ServiceAccountTokenName = "foo" automountServiceAccountToken := false woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &automountServiceAccountToken woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") From 7601f695962474237bb22fffe60714ac514f3f74 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 16 Jul 2019 00:45:34 +0900 Subject: [PATCH 06/13] Fix lint error --- workflow/common/common.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/workflow/common/common.go b/workflow/common/common.go index 9b38aa5f2312..e1f132564fbb 100644 --- a/workflow/common/common.go +++ b/workflow/common/common.go @@ -114,11 +114,11 @@ const ( // LocalVarPodName is a step level variable that references the name of the pod LocalVarPodName = "pod.name" - KubeConfigDefaultMountPath = "/kube/config" - KubeConfigDefaultVolumeName = "kubeconfig" + KubeConfigDefaultMountPath = "/kube/config" + KubeConfigDefaultVolumeName = "kubeconfig" ServiceAccountTokenMountPath = "/var/run/secrets/kubernetes.io/serviceaccount" ServiceAccountTokenVolumeName = "exec-sa-token" - SecretVolMountPath = "/argo/secret" + SecretVolMountPath = "/argo/secret" ) // GlobalVarWorkflowRootTags is a list of root tags in workflow which could be used for variable reference From 0de424f2c0324a412d9cf59cf164d383972a4b6d Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 23 Jul 2019 22:15:03 +0900 Subject: [PATCH 07/13] Implement ExecutorServiceAccountTokenName --- pkg/apis/workflow/v1alpha1/types.go | 16 +++-- workflow/config/config.go | 3 - workflow/controller/operator.go | 2 +- workflow/controller/workflowpod.go | 85 ++++++++++++++--------- workflow/controller/workflowpod_test.go | 89 +++++++++++++++---------- workflow/validate/validate.go | 22 ++++-- workflow/validate/validate_test.go | 87 ++++++++++++++++++++++++ 7 files changed, 224 insertions(+), 80 deletions(-) diff --git a/pkg/apis/workflow/v1alpha1/types.go b/pkg/apis/workflow/v1alpha1/types.go index 2fc885b19252..f901681f22c6 100644 --- a/pkg/apis/workflow/v1alpha1/types.go +++ b/pkg/apis/workflow/v1alpha1/types.go @@ -86,11 +86,13 @@ type WorkflowSpec struct { // ServiceAccountName is the name of the ServiceAccount to run all pods of the workflow as. ServiceAccountName string `json:"serviceAccountName,omitempty"` - // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. - // It is ignored in resource templates. - // +optional + // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. + // ExecutorServiceAccountTokenName must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` + // ExecutorServiceAccountTokenName specifies a service account token name for executor containers. + ExecutorServiceAccountTokenName string `json:"executorServiceAccountTokenName,omitempty"` + // Volumes is a list of volumes that can be mounted by containers in a workflow. Volumes []apiv1.Volume `json:"volumes,omitempty"` @@ -269,11 +271,13 @@ type Template struct { // ServiceAccountName to apply to workflow pods ServiceAccountName string `json:"serviceAccountName,omitempty"` - // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted. - // It cannot be set in resource templates. - // +optional + // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. + // ExecutorServiceAccountTokenName must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` + // ExecutorServiceAccountTokenName specifies a service account token name for executor containers. + ExecutorServiceAccountTokenName string `json:"executorServiceAccountTokenName,omitempty"` + // HostAliases is an optional list of hosts and IPs that will be injected into the pod spec HostAliases []apiv1.HostAlias `json:"hostAliases,omitempty"` diff --git a/workflow/config/config.go b/workflow/config/config.go index 4c5c374ff60b..4409faecbbde 100644 --- a/workflow/config/config.go +++ b/workflow/config/config.go @@ -26,9 +26,6 @@ type WorkflowControllerConfig struct { // KubeConfig specifies a kube config file for the wait & init containers KubeConfig *KubeConfig `json:"kubeConfig,omitempty"` - // ServiceAccountTokenName specifies a service account token name for the wait & init containers. - ServiceAccountTokenName string `json:"serviceAccountTokenName,omitempty"` - // ContainerRuntimeExecutor specifies the container runtime interface to use, default is docker ContainerRuntimeExecutor string `json:"containerRuntimeExecutor,omitempty"` diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index 9b354a96b275..d2a0aff5e836 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -1599,7 +1599,7 @@ func (woc *wfOperationCtx) executeResource(nodeName string, tmpl *wfv1.Template, tmpl.Resource.Manifest = string(bytes) } - mainCtr := woc.newExecContainer(common.MainContainerName) + mainCtr := woc.newExecContainer(common.MainContainerName, tmpl) mainCtr.Command = []string{"argoexec", "resource", tmpl.Resource.Action} mainCtr.VolumeMounts = []apiv1.VolumeMount{ volumeMountPodMetadata, diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 9ec43739d562..824ca536ef96 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -91,11 +91,6 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont tmpl = tmpl.DeepCopy() wfSpec := woc.wf.Spec.DeepCopy() - tmplServiceAccountName := woc.wf.Spec.ServiceAccountName - if tmpl.ServiceAccountName != "" { - tmplServiceAccountName = tmpl.ServiceAccountName - } - mainCtr.Name = common.MainContainerName pod := &apiv1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -116,7 +111,6 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont RestartPolicy: apiv1.RestartPolicyNever, Volumes: woc.createVolumes(), ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds, - ServiceAccountName: tmplServiceAccountName, ImagePullSecrets: woc.wf.Spec.ImagePullSecrets, }, } @@ -145,18 +139,12 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont return nil, err } - if tmpl.GetType() != wfv1.TemplateTypeResource { - if woc.wf.Spec.AutomountServiceAccountToken != nil { - pod.Spec.AutomountServiceAccountToken = woc.wf.Spec.AutomountServiceAccountToken - } else if tmpl.AutomountServiceAccountToken != nil { - pod.Spec.AutomountServiceAccountToken = tmpl.AutomountServiceAccountToken - } - if pod.Spec.AutomountServiceAccountToken != nil && !*pod.Spec.AutomountServiceAccountToken { - if woc.controller.Config.KubeConfig == nil && woc.controller.Config.ServiceAccountTokenName == "" { - return nil, errors.Errorf(errors.CodeBadRequest, "automountServiceAccountToken cannot be set because the controller is not configured with kubeConfig nor serviceAccountTokenName") - } - } + err = woc.setupServiceAccount(pod, tmpl) + if err != nil { + return nil, err + } + if tmpl.GetType() != wfv1.TemplateTypeResource { // we do not need the wait container for resource templates because // argoexec runs as the main container and will perform the job of // annotating the outputs or errors, making the wait container redundant. @@ -283,13 +271,13 @@ func substitutePodParams(pod *apiv1.Pod, globalParams map[string]string, tmpl *w } func (woc *wfOperationCtx) newInitContainer(tmpl *wfv1.Template) apiv1.Container { - ctr := woc.newExecContainer(common.InitContainerName) + ctr := woc.newExecContainer(common.InitContainerName, tmpl) ctr.Command = []string{"argoexec", "init"} return *ctr } func (woc *wfOperationCtx) newWaitContainer(tmpl *wfv1.Template) (*apiv1.Container, error) { - ctr := woc.newExecContainer(common.WaitContainerName) + ctr := woc.newExecContainer(common.WaitContainerName, tmpl) ctr.Command = []string{"argoexec", "wait"} switch woc.controller.Config.ContainerRuntimeExecutor { case common.ContainerRuntimeExecutorPNS: @@ -406,16 +394,6 @@ func (woc *wfOperationCtx) createVolumes() []apiv1.Volume { }, }) } - if woc.controller.Config.ServiceAccountTokenName != "" { - volumes = append(volumes, apiv1.Volume{ - Name: common.ServiceAccountTokenVolumeName, - VolumeSource: apiv1.VolumeSource{ - Secret: &apiv1.SecretVolumeSource{ - SecretName: woc.controller.Config.ServiceAccountTokenName, - }, - }, - }) - } switch woc.controller.Config.ContainerRuntimeExecutor { case common.ContainerRuntimeExecutorKubelet, common.ContainerRuntimeExecutorK8sAPI, common.ContainerRuntimeExecutorPNS: return volumes @@ -424,7 +402,7 @@ func (woc *wfOperationCtx) createVolumes() []apiv1.Volume { } } -func (woc *wfOperationCtx) newExecContainer(name string) *apiv1.Container { +func (woc *wfOperationCtx) newExecContainer(name string, tmpl *wfv1.Template) *apiv1.Container { exec := apiv1.Container{ Name: name, Image: woc.controller.executorImage(), @@ -459,7 +437,13 @@ func (woc *wfOperationCtx) newExecContainer(name string) *apiv1.Container { }) exec.Args = append(exec.Args, "--kubeconfig="+path) } - if woc.controller.Config.ServiceAccountTokenName != "" { + executorServiceAccountTokenName := "" + if tmpl.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName + } else if woc.wf.Spec.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = woc.wf.Spec.ExecutorServiceAccountTokenName + } + if executorServiceAccountTokenName != "" { exec.VolumeMounts = append(exec.VolumeMounts, apiv1.VolumeMount{ Name: common.ServiceAccountTokenVolumeName, MountPath: common.ServiceAccountTokenMountPath, @@ -844,6 +828,45 @@ func (woc *wfOperationCtx) addArchiveLocation(pod *apiv1.Pod, tmpl *wfv1.Templat return nil } +// setupServiceAccount sets up service account and token. +func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Template) error { + if tmpl.ServiceAccountName != "" { + pod.Spec.ServiceAccountName = tmpl.ServiceAccountName + } else if woc.wf.Spec.ServiceAccountName != "" { + pod.Spec.ServiceAccountName = woc.wf.Spec.ServiceAccountName + } + + var automountServiceAccountToken *bool + if tmpl.AutomountServiceAccountToken != nil { + automountServiceAccountToken = tmpl.AutomountServiceAccountToken + } else if woc.wf.Spec.AutomountServiceAccountToken != nil { + automountServiceAccountToken = woc.wf.Spec.AutomountServiceAccountToken + } + if automountServiceAccountToken != nil && !*automountServiceAccountToken { + pod.Spec.AutomountServiceAccountToken = automountServiceAccountToken + } + + executorServiceAccountTokenName := "" + if tmpl.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName + } else if woc.wf.Spec.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = woc.wf.Spec.ExecutorServiceAccountTokenName + } + if executorServiceAccountTokenName != "" { + pod.Spec.Volumes = append(pod.Spec.Volumes, apiv1.Volume{ + Name: common.ServiceAccountTokenVolumeName, + VolumeSource: apiv1.VolumeSource{ + Secret: &apiv1.SecretVolumeSource{ + SecretName: executorServiceAccountTokenName, + }, + }, + }) + } else if automountServiceAccountToken != nil && !*automountServiceAccountToken { + return errors.Errorf(errors.CodeBadRequest, "executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + } + return nil +} + // addScriptStagingVolume sets up a shared staging volume between the init container // and main container for the purpose of holding the script source code for script templates func addScriptStagingVolume(pod *apiv1.Pod) { diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index 577639c9769c..daeee72f3d2d 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -75,9 +75,9 @@ func TestScriptTemplateWithVolume(t *testing.T) { assert.Equal(t, node.Phase, wfv1.NodePending) } -// TestServiceAccount verifies the ability to carry forward the service account name +// TestWFLevelServiceAccount verifies the ability to carry forward the service account name // for the pod from workflow.spec.serviceAccountName. -func TestServiceAccount(t *testing.T) { +func TestWFLevelServiceAccount(t *testing.T) { woc := newWoc() woc.wf.Spec.ServiceAccountName = "foo" woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") @@ -103,9 +103,9 @@ func TestTmplServiceAccount(t *testing.T) { // TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. func TestWFLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() - woc.controller.Config.ServiceAccountTokenName = "foo" - automountServiceAccountToken := false - woc.wf.Spec.AutomountServiceAccountToken = &automountServiceAccountToken + falseValue := false + woc.wf.Spec.AutomountServiceAccountToken = &falseValue + woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) @@ -116,9 +116,11 @@ func TestWFLevelAutomountServiceAccountToken(t *testing.T) { // TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() - woc.controller.Config.ServiceAccountTokenName = "foo" - automountServiceAccountToken := false - woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &automountServiceAccountToken + trueValue := true + falseValue := false + woc.wf.Spec.AutomountServiceAccountToken = &trueValue + woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &falseValue woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) @@ -126,6 +128,51 @@ func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { assert.Equal(t, *pod.Spec.AutomountServiceAccountToken, false) } +// verifyServiceAccountTokenVolumeMount is a helper function to verify service account token volume in a container. +func verifyServiceAccountTokenVolumeMount(t *testing.T, ctr apiv1.Container, volName, mountPath string) { + for _, vol := range ctr.VolumeMounts { + if vol.Name == volName && vol.MountPath == mountPath { + return + } + } + t.Fatalf("%v does not have serviceAccountToken mounted properly (name: %s, mountPath: %s)", ctr, volName, mountPath) +} + +// TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. +func TestWFLevelExecutorServiceAccountTokenName(t *testing.T) { + woc := newWoc() + woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + + woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") + podName := getPodName(woc.wf) + pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) + + assert.NoError(t, err) + assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[2].Name) + assert.Equal(t, "foo", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) + + waitCtr := pod.Spec.Containers[0] + verifyServiceAccountTokenVolumeMount(t, waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") +} + +// TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. +func TestTmplLevelExecutorServiceAccountTokenName(t *testing.T) { + woc := newWoc() + woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + woc.wf.Spec.Templates[0].ExecutorServiceAccountTokenName = "tmpl" + + woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") + podName := getPodName(woc.wf) + pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) + + assert.NoError(t, err) + assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[2].Name) + assert.Equal(t, "tmpl", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) + + waitCtr := pod.Spec.Containers[0] + verifyServiceAccountTokenVolumeMount(t, waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") +} + // TestImagePullSecrets verifies the ability to carry forward imagePullSecrets from workflow.spec func TestImagePullSecrets(t *testing.T) { woc := newWoc() @@ -467,32 +514,6 @@ func TestOutOfCluster(t *testing.T) { } } -// TestServiceAccountTokenName verifies a volume and volumeMount of mounting a given service account token. -func TestServiceAccountTokenName(t *testing.T) { - verifyServiceAccountTokenVolume := func(ctr apiv1.Container, volName, mountPath string) { - for _, vol := range ctr.VolumeMounts { - if vol.Name == volName && vol.MountPath == mountPath { - return - } - } - t.Fatalf("%v does not have serviceAccountToken mounted properly (name: %s, mountPath: %s)", ctr, volName, mountPath) - } - - woc := newWoc() - woc.controller.Config.ServiceAccountTokenName = "foo" - - woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], "") - podName := getPodName(woc.wf) - pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) - - assert.NoError(t, err) - assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[1].Name) - assert.Equal(t, "foo", pod.Spec.Volumes[1].VolumeSource.Secret.SecretName) - - waitCtr := pod.Spec.Containers[0] - verifyServiceAccountTokenVolume(waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") -} - // TestPriority verifies the ability to carry forward priorityClassName and priority. func TestPriority(t *testing.T) { priority := int32(15) diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index a61e05c3f0c5..199222c99f6b 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -160,7 +160,7 @@ func (ctx *wfValidationCtx) validateTemplate(tmpl *wfv1.Template, args wfv1.Argu case wfv1.TemplateTypeDAG: err = ctx.validateDAG(scope, tmpl) default: - err = validateLeaf(scope, tmpl) + err = ctx.validateLeaf(scope, tmpl) } if err != nil { return err @@ -302,7 +302,7 @@ func validateNonLeaf(tmpl *wfv1.Template) error { return nil } -func validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error { +func (ctx *wfValidationCtx) validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error { tmplBytes, err := json.Marshal(tmpl) if err != nil { return errors.InternalWrapError(err) @@ -334,9 +334,6 @@ func validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error { default: return errors.Errorf(errors.CodeBadRequest, "templates.%s.resource.action must be either get, create, apply, delete or replace", tmpl.Name) } - if tmpl.AutomountServiceAccountToken != nil { - return errors.Errorf(errors.CodeBadRequest, "templates.%s.automountServiceAccountToken can not be set in resource templates", tmpl.Name) - } // Try to unmarshal the given manifest. obj := unstructured.Unstructured{} err := yaml.Unmarshal([]byte(tmpl.Resource.Manifest), &obj) @@ -352,6 +349,21 @@ func validateLeaf(scope map[string]interface{}, tmpl *wfv1.Template) error { if tmpl.Parallelism != nil { return errors.Errorf(errors.CodeBadRequest, "templates.%s.parallelism is only valid for steps and dag templates", tmpl.Name) } + var automountServiceAccountToken *bool + if tmpl.AutomountServiceAccountToken != nil { + automountServiceAccountToken = tmpl.AutomountServiceAccountToken + } else if ctx.wf.Spec.AutomountServiceAccountToken != nil { + automountServiceAccountToken = ctx.wf.Spec.AutomountServiceAccountToken + } + executorServiceAccountTokenName := "" + if tmpl.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName + } else if ctx.wf.Spec.ExecutorServiceAccountTokenName != "" { + executorServiceAccountTokenName = ctx.wf.Spec.ExecutorServiceAccountTokenName + } + if automountServiceAccountToken != nil && !*automountServiceAccountToken && executorServiceAccountTokenName == "" { + return errors.Errorf(errors.CodeBadRequest, "templates.%s.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false", tmpl.Name) + } return nil } diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go index 811527492153..14a69747639a 100644 --- a/workflow/validate/validate_test.go +++ b/workflow/validate/validate_test.go @@ -1427,3 +1427,90 @@ func TestInvalidResourceWorkflow(t *testing.T) { err = ValidateWorkflow(wf, ValidateOpts{}) assert.Error(t, err, "templates.whalesay.resource.action must be either get, create, apply, delete or replace") } + +var validAutomountServiceAccountTokenUseWfLevel = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: valid-automount-service-account-token-use-wf-level- +spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: alpine:latest + - name: per-tmpl-automount + container: + image: alpine:latest + automountServiceAccountToken: true + executorServiceAccountTokenName: "" + automountServiceAccountToken: false + executorServiceAccountTokenName: foo +` + +var validAutomountServiceAccountTokenUseTmplLevel = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: valid-automount-service-account-token-use-tmpl-level- +spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: alpine:latest + executorServiceAccountTokenName: foo + automountServiceAccountToken: false +` + +var invalidAutomountServiceAccountTokenUseWfLevel = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: invalid-automount-service-account-token-use-wf-level- +spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: alpine:latest + automountServiceAccountToken: false +` + +var invalidAutomountServiceAccountTokenUseTmplLevel = ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + generateName: invalid-automount-service-account-token-use-tmpl-level- +spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: alpine:latest + automountServiceAccountToken: false +` + +// TestAutomountServiceAccountTokenUse verifies an error against a workflow of an invalid automountServiceAccountToken use. +func TestAutomountServiceAccountTokenUse(t *testing.T) { + { + wf := unmarshalWf(validAutomountServiceAccountTokenUseWfLevel) + err := ValidateWorkflow(wf, ValidateOpts{}) + assert.NoError(t, err) + } + { + wf := unmarshalWf(validAutomountServiceAccountTokenUseTmplLevel) + err := ValidateWorkflow(wf, ValidateOpts{}) + assert.NoError(t, err) + } + { + wf := unmarshalWf(invalidAutomountServiceAccountTokenUseWfLevel) + err := ValidateWorkflow(wf, ValidateOpts{}) + assert.EqualError(t, err, "templates.whalesay.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + } + { + wf := unmarshalWf(invalidAutomountServiceAccountTokenUseTmplLevel) + err := ValidateWorkflow(wf, ValidateOpts{}) + assert.EqualError(t, err, "templates.whalesay.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + } +} From f32ffcdcbabb297205353990f29051e85253de2b Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 23 Jul 2019 22:38:52 +0900 Subject: [PATCH 08/13] Fix unexpected volume mounts overwritten --- workflow/controller/operator.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/workflow/controller/operator.go b/workflow/controller/operator.go index d2a0aff5e836..cbca3eb2579a 100644 --- a/workflow/controller/operator.go +++ b/workflow/controller/operator.go @@ -1601,9 +1601,6 @@ func (woc *wfOperationCtx) executeResource(nodeName string, tmpl *wfv1.Template, mainCtr := woc.newExecContainer(common.MainContainerName, tmpl) mainCtr.Command = []string{"argoexec", "resource", tmpl.Resource.Action} - mainCtr.VolumeMounts = []apiv1.VolumeMount{ - volumeMountPodMetadata, - } _, err = woc.createWorkflowPod(nodeName, *mainCtr, tmpl) if err != nil { return woc.initializeNode(nodeName, wfv1.NodeTypePod, tmpl.Name, boundaryID, wfv1.NodeError, err.Error()) From 2dc27281b966d19d4938e802de2160cbb7ad0d75 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 6 Aug 2019 19:47:05 +0900 Subject: [PATCH 09/13] Remove test fields --- examples/hello-world.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/examples/hello-world.yaml b/examples/hello-world.yaml index f4593690032d..51a0994ad12c 100644 --- a/examples/hello-world.yaml +++ b/examples/hello-world.yaml @@ -10,5 +10,3 @@ spec: image: docker/whalesay:latest command: [cowsay] args: ["hello world"] - automountServiceAccountToken: false - executorServiceAccountTokenName: argo-token-9rkvd From ef19bb2bdfbd8c97eff295c641cc856c93158e25 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Wed, 7 Aug 2019 23:25:21 +0900 Subject: [PATCH 10/13] Support ExecutorServiceAccountName instead of ExecutorServiceAccountTokenName --- Gopkg.lock | 81 ++++++++++++------- api/openapi-spec/swagger.json | 12 +-- manifests/base/kustomization.yaml | 8 +- .../workflow-controller-clusterrole.yaml | 10 +++ manifests/install.yaml | 25 +++++- manifests/namespace-install.yaml | 14 +++- .../workflow-controller-role.yaml | 10 +++ .../workflow/v1alpha1/openapi_generated.go | 12 +-- pkg/apis/workflow/v1alpha1/workflow_types.go | 12 +-- test/util/serviceaccount.go | 28 +++++++ workflow/common/serviceaccount.go | 72 +++++++++++++++++ workflow/common/serviceaccount_test.go | 42 ++++++++++ workflow/controller/workflowpod.go | 32 ++++---- workflow/controller/workflowpod_test.go | 35 +++++--- workflow/validate/validate.go | 16 ++-- workflow/validate/validate_test.go | 10 +-- 16 files changed, 322 insertions(+), 97 deletions(-) create mode 100644 test/util/serviceaccount.go create mode 100644 workflow/common/serviceaccount.go create mode 100644 workflow/common/serviceaccount_test.go diff --git a/Gopkg.lock b/Gopkg.lock index b0ceced1e7c9..63746d869184 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -44,25 +44,6 @@ pruneopts = "" revision = "de5bf2ad457846296e2031421a34e2568e304e35" -[[projects]] - branch = "master" - digest = "1:a74730e052a45a3fab1d310fdef2ec17ae3d6af16228421e238320846f2aaec8" - name = "github.com/alecthomas/template" - packages = [ - ".", - "parse", - ] - pruneopts = "" - revision = "a0175ee3bccc567396460bf5acd36800cb10c49c" - -[[projects]] - branch = "master" - digest = "1:8483994d21404c8a1d489f6be756e25bfccd3b45d65821f25695577791a08e68" - name = "github.com/alecthomas/units" - packages = ["."] - pruneopts = "" - revision = "2efee857e7cfd4f3d0138cc3cbb1b4966962b93a" - [[projects]] branch = "master" digest = "1:72347a6143ccb58245c6f8055662ae6cb2d5dd655699f0fc479c25cc610fc582" @@ -523,7 +504,6 @@ packages = [ "expfmt", "internal/bitbucket.org/ww/goautoneg", - "log", "model", ] pruneopts = "" @@ -714,8 +694,6 @@ "cpu", "unix", "windows", - "windows/registry", - "windows/svc/eventlog", ] pruneopts = "" revision = "904bdc257025c7b3f43c19360ad3ab85783fad78" @@ -784,14 +762,6 @@ revision = "b1f26356af11148e710935ed1ac8a7f5702c7612" version = "v1.1.0" -[[projects]] - digest = "1:15d017551627c8bb091bde628215b2861bed128855343fdd570c62d08871f6e1" - name = "gopkg.in/alecthomas/kingpin.v2" - packages = ["."] - pruneopts = "" - revision = "947dcec5ba9c011838740e680966fd7087a71d0d" - version = "v2.2.6" - [[projects]] digest = "1:75fb3fcfc73a8c723efde7777b40e8e8ff9babf30d8c56160d01beffea8a95a6" name = "gopkg.in/inf.v0" @@ -864,6 +834,19 @@ revision = "4480c480c9cd343b54b0acb5b62261cbd33d7adf" version = "v0.0.2" +[[projects]] + digest = "1:40cf02345bfa29fb217cfe0767a9416d99569d4ff21dbb1fd3378ef10682549c" + name = "gopkg.in/square/go-jose.v2" + packages = [ + ".", + "cipher", + "json", + "jwt", + ] + pruneopts = "" + revision = "730df5f748271903322feb182be83b43ebbbe27d" + version = "v2.3.1" + [[projects]] digest = "1:6715e0bec216255ab784fe04aa4d5a0a626ae07a3a209080182e469bc142761a" name = "gopkg.in/src-d/go-billy.v4" @@ -988,12 +971,15 @@ digest = "1:5899da40e41bcc8c1df101b72954096bba9d85b763bc17efc846062ccc111c7b" name = "k8s.io/apimachinery" packages = [ + "pkg/api/equality", "pkg/api/errors", "pkg/api/meta", "pkg/api/resource", + "pkg/api/validation", "pkg/apis/meta/internalversion", "pkg/apis/meta/v1", "pkg/apis/meta/v1/unstructured", + "pkg/apis/meta/v1/validation", "pkg/apis/meta/v1beta1", "pkg/conversion", "pkg/conversion/queryparams", @@ -1038,6 +1024,18 @@ pruneopts = "" revision = "f71dbbc36e126f5a371b85f6cca96bc8c57db2b6" +[[projects]] + branch = "master" + digest = "1:d34d016e76b0df04f7e3063d34accb81d658f56962d1bd5281b318fb0a9aac96" + name = "k8s.io/apiserver" + packages = [ + "pkg/authentication/authenticator", + "pkg/authentication/serviceaccount", + "pkg/authentication/user", + ] + pruneopts = "" + revision = "f4eec59356e4ef91302f773972660a8623a30623" + [[projects]] branch = "release-9.0" digest = "1:77bf3d9f18ec82e08ac6c4c7e2d9d1a2ef8d16b25d3ff72fcefcf9256d751573" @@ -1193,6 +1191,14 @@ pruneopts = "" revision = "c42f3cdacc394f43077ff17e327d1b351c0304e4" +[[projects]] + digest = "1:9eaf86f4f6fb4a8f177220d488ef1e3255d06a691cca95f14ef085d4cd1cef3c" + name = "k8s.io/klog" + packages = ["."] + pruneopts = "" + revision = "d98d8acdac006fb39831f1b25640813fef9c314f" + version = "v0.3.3" + [[projects]] branch = "master" digest = "1:951bc2047eea6d316a17850244274554f26fd59189360e45f4056b424dadf2c1" @@ -1204,6 +1210,17 @@ pruneopts = "" revision = "e3762e86a74c878ffed47484592986685639c2cd" +[[projects]] + digest = "1:63b20edbeadb800a49f7aceeb92dcb4815ef4ecfd825f468514ead1a92ae4822" + name = "k8s.io/kubernetes" + packages = [ + "pkg/apis/core", + "pkg/serviceaccount", + ] + pruneopts = "" + revision = "f6278300bebbb750328ac16ee6dd3aa7d3549568" + version = "v1.15.2" + [[projects]] branch = "master" digest = "1:f6c19347011ba9a072aa55f5c7fa630c0b88303ac4ca83008454aef95b0c2078" @@ -1258,7 +1275,6 @@ "github.com/pkg/errors", "github.com/prometheus/client_golang/prometheus", "github.com/prometheus/client_golang/prometheus/promhttp", - "github.com/prometheus/common/log", "github.com/sirupsen/logrus", "github.com/spf13/cobra", "github.com/stretchr/testify/assert", @@ -1272,6 +1288,7 @@ "gopkg.in/jcmturner/gokrb5.v5/credentials", "gopkg.in/jcmturner/gokrb5.v5/keytab", "gopkg.in/src-d/go-git.v4", + "gopkg.in/src-d/go-git.v4/config", "gopkg.in/src-d/go-git.v4/plumbing/transport", "gopkg.in/src-d/go-git.v4/plumbing/transport/http", "gopkg.in/src-d/go-git.v4/plumbing/transport/ssh", @@ -1291,6 +1308,7 @@ "k8s.io/apimachinery/pkg/util/runtime", "k8s.io/apimachinery/pkg/util/validation", "k8s.io/apimachinery/pkg/util/wait", + "k8s.io/apimachinery/pkg/version", "k8s.io/apimachinery/pkg/watch", "k8s.io/client-go/discovery", "k8s.io/client-go/discovery/fake", @@ -1314,6 +1332,7 @@ "k8s.io/code-generator/cmd/informer-gen", "k8s.io/code-generator/cmd/lister-gen", "k8s.io/kube-openapi/pkg/common", + "k8s.io/kubernetes/pkg/serviceaccount", "k8s.io/utils/pointer", "upper.io/db.v3/lib/sqlbuilder", "upper.io/db.v3/mysql", diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 8ada0f46299f..6c8ffa0414b6 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -848,7 +848,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.Arguments" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountTokenName must be specified if this value is false.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", "type": "boolean" }, "container": { @@ -863,8 +863,8 @@ "description": "DAG template subtype which runs a DAG", "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.DAGTemplate" }, - "executorServiceAccountTokenName": { - "description": "ExecutorServiceAccountTokenName specifies a service account token name for executor containers.", + "executorServiceAccountName": { + "description": "ExecutorServiceAccountName specifies a service account token name for executor containers.", "type": "string" }, "hostAliases": { @@ -1228,7 +1228,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ArtifactRepositoryRef" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountTokenName must be specified if this value is false.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", "type": "boolean" }, "dnsConfig": { @@ -1243,8 +1243,8 @@ "description": "Entrypoint is a template reference to the starting point of the workflow", "type": "string" }, - "executorServiceAccountTokenName": { - "description": "ExecutorServiceAccountTokenName specifies a service account token name for executor containers.", + "executorServiceAccountName": { + "description": "ExecutorServiceAccountName specifies a service account token name for executor containers.", "type": "string" }, "hostAliases": { diff --git a/manifests/base/kustomization.yaml b/manifests/base/kustomization.yaml index da041e0a0b20..dbc178cb7bfc 100644 --- a/manifests/base/kustomization.yaml +++ b/manifests/base/kustomization.yaml @@ -1,10 +1,6 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization -bases: -- crds -- workflow-controller -- argo-ui images: - name: argoproj/argoui @@ -13,3 +9,7 @@ images: - name: argoproj/workflow-controller newName: argoproj/workflow-controller newTag: latest +resources: +- crds +- workflow-controller +- argo-ui diff --git a/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml b/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml index 65cd1b3560eb..8da2b401ec35 100644 --- a/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml +++ b/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml @@ -52,3 +52,13 @@ rules: - get - list - watch +- apiGroups: + - "" + resources: + - serviceaccounts + - serviceaccounts/finalizers + - secrets + - secrets/finalizers + verbs: + - get + - list diff --git a/manifests/install.yaml b/manifests/install.yaml index 8cc28fc47959..d0db29feae77 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -30,12 +30,12 @@ spec: apiVersion: v1 kind: ServiceAccount metadata: - name: argo-ui + name: argo --- apiVersion: v1 kind: ServiceAccount metadata: - name: argo + name: argo-ui --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -142,8 +142,6 @@ rules: resources: - workflows - workflows/finalizers - - workflowtemplates - - workflowtemplates/finalizers verbs: - get - list @@ -151,6 +149,25 @@ rules: - update - patch - delete +- apiGroups: + - argoproj.io + resources: + - workflowtemplates + - workflowtemplates/finalizers + verbs: + - get + - list + - watch +- apiGroups: + - "" + resources: + - serviceaccounts + - serviceaccounts/finalizers + - secrets + - secrets/finalizers + verbs: + - get + - list --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index cd18354fd80f..b1facc4a1eef 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -30,12 +30,12 @@ spec: apiVersion: v1 kind: ServiceAccount metadata: - name: argo-ui + name: argo --- apiVersion: v1 kind: ServiceAccount metadata: - name: argo + name: argo-ui --- apiVersion: rbac.authorization.k8s.io/v1 kind: Role @@ -82,6 +82,16 @@ rules: - update - patch - delete +- apiGroups: + - "" + resources: + - serviceaccounts + - serviceaccounts/finalizers + - secrets + - secrets/finalizers + verbs: + - get + - list --- apiVersion: rbac.authorization.k8s.io/v1 kind: Role diff --git a/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml b/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml index f74a581ed3bb..c465550436ba 100644 --- a/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml +++ b/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml @@ -43,3 +43,13 @@ rules: - update - patch - delete +- apiGroups: + - "" + resources: + - serviceaccounts + - serviceaccounts/finalizers + - secrets + - secrets/finalizers + verbs: + - get + - list diff --git a/pkg/apis/workflow/v1alpha1/openapi_generated.go b/pkg/apis/workflow/v1alpha1/openapi_generated.go index 302068ac3f64..e4c6bcae5a3c 100644 --- a/pkg/apis/workflow/v1alpha1/openapi_generated.go +++ b/pkg/apis/workflow/v1alpha1/openapi_generated.go @@ -1787,14 +1787,14 @@ func schema_pkg_apis_workflow_v1alpha1_Template(ref common.ReferenceCallback) co }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountTokenName must be specified if this value is false.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", Type: []string{"boolean"}, Format: "", }, }, - "executorServiceAccountTokenName": { + "executorServiceAccountName": { SchemaProps: spec.SchemaProps{ - Description: "ExecutorServiceAccountTokenName specifies a service account token name for executor containers.", + Description: "ExecutorServiceAccountName specifies a service account token name for executor containers.", Type: []string{"string"}, Format: "", }, @@ -2265,14 +2265,14 @@ func schema_pkg_apis_workflow_v1alpha1_WorkflowSpec(ref common.ReferenceCallback }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountTokenName must be specified if this value is false.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", Type: []string{"boolean"}, Format: "", }, }, - "executorServiceAccountTokenName": { + "executorServiceAccountName": { SchemaProps: spec.SchemaProps{ - Description: "ExecutorServiceAccountTokenName specifies a service account token name for executor containers.", + Description: "ExecutorServiceAccountName specifies a service account token name for executor containers.", Type: []string{"string"}, Format: "", }, diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go index b6a6e7e1b18f..b9bf9a380635 100644 --- a/pkg/apis/workflow/v1alpha1/workflow_types.go +++ b/pkg/apis/workflow/v1alpha1/workflow_types.go @@ -103,11 +103,11 @@ type WorkflowSpec struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. - // ExecutorServiceAccountTokenName must be specified if this value is false. + // ExecutorServiceAccountName must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` - // ExecutorServiceAccountTokenName specifies a service account token name for executor containers. - ExecutorServiceAccountTokenName string `json:"executorServiceAccountTokenName,omitempty"` + // ExecutorServiceAccountName specifies a service account token name for executor containers. + ExecutorServiceAccountName string `json:"executorServiceAccountName,omitempty"` // Volumes is a list of volumes that can be mounted by containers in a workflow. Volumes []apiv1.Volume `json:"volumes,omitempty"` @@ -300,11 +300,11 @@ type Template struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. - // ExecutorServiceAccountTokenName must be specified if this value is false. + // ExecutorServiceAccountName must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` - // ExecutorServiceAccountTokenName specifies a service account token name for executor containers. - ExecutorServiceAccountTokenName string `json:"executorServiceAccountTokenName,omitempty"` + // ExecutorServiceAccountName specifies a service account token name for executor containers. + ExecutorServiceAccountName string `json:"executorServiceAccountName,omitempty"` // HostAliases is an optional list of hosts and IPs that will be injected into the pod spec HostAliases []apiv1.HostAlias `json:"hostAliases,omitempty"` diff --git a/test/util/serviceaccount.go b/test/util/serviceaccount.go new file mode 100644 index 000000000000..3019d17e0b6a --- /dev/null +++ b/test/util/serviceaccount.go @@ -0,0 +1,28 @@ +package util + +import ( + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// CreateServiceAccountWithToken creates a service account with a given name with a service account token. +// Need to use this function to simulate the actual behavior of Kubernetes API server with the fake client. +func CreateServiceAccountWithToken(clientset kubernetes.Interface, namespace, name, tokenName string) (*corev1.ServiceAccount, error) { + sa, err := clientset.CoreV1().ServiceAccounts(namespace).Create(&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: name}}) + if err != nil { + return nil, err + } + token, err := clientset.CoreV1().Secrets(namespace).Create(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: tokenName, + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: sa.Name, + corev1.ServiceAccountUIDKey: string(sa.UID), + }}, Type: corev1.SecretTypeServiceAccountToken}) + if err != nil { + return nil, err + } + sa.Secrets = []corev1.ObjectReference{{Name: token.Name}} + return clientset.CoreV1().ServiceAccounts(namespace).Update(sa) +} diff --git a/workflow/common/serviceaccount.go b/workflow/common/serviceaccount.go new file mode 100644 index 000000000000..e3c8f1e183a0 --- /dev/null +++ b/workflow/common/serviceaccount.go @@ -0,0 +1,72 @@ +package common + +import ( + "github.com/argoproj/argo/errors" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/kubernetes/pkg/serviceaccount" +) + +// GetServiceAccountTokenByAccountName returns the name of the first referenced secret which is a ServiceAccountToken for the service account +func GetServiceAccountTokenByAccountName(clientset kubernetes.Interface, namespace, name string) (*corev1.Secret, error) { + serviceAccount, err := clientset.CoreV1().ServiceAccounts(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + return nil, err + } + token, err := GetReferencedServiceAccountToken(clientset, serviceAccount) + if err != nil { + return nil, err + } + if token == nil { + return nil, errors.Errorf("Error looking up service account token for %s/%s", serviceAccount.Namespace, serviceAccount.Name) + } + return token, nil +} + +// GetReferencedServiceAccountToken returns the name of the first referenced secret which is a ServiceAccountToken for the service account +func GetReferencedServiceAccountToken(clientset kubernetes.Interface, serviceAccount *corev1.ServiceAccount) (*corev1.Secret, error) { + if len(serviceAccount.Secrets) == 0 { + return nil, nil + } + + tokens, err := GetServiceAccountTokens(clientset, serviceAccount) + if err != nil { + return nil, err + } + + accountTokens := map[string]*corev1.Secret{} + for _, token := range tokens { + accountTokens[token.Name] = token + } + // Prefer secrets in the order they're referenced. + for _, obj := range serviceAccount.Secrets { + token, ok := accountTokens[obj.Name] + if ok { + return token, nil + } + } + + return nil, nil +} + +// GetServiceAccountTokens returns all ServiceAccountToken secrets for the given ServiceAccount +func GetServiceAccountTokens(clientset kubernetes.Interface, serviceAccount *corev1.ServiceAccount) ([]*corev1.Secret, error) { + secrets, err := clientset.CoreV1().Secrets(serviceAccount.Namespace).List(metav1.ListOptions{}) + if err != nil { + return nil, err + } + + tokens := []*corev1.Secret{} + for _, secret := range secrets.Items { + if secret.Type != corev1.SecretTypeServiceAccountToken { + continue + } + + if serviceaccount.IsServiceAccountToken(&secret, serviceAccount) { + // The variable `secret` is overwritten during the loop, so need to deep copy it. + tokens = append(tokens, secret.DeepCopy()) + } + } + return tokens, nil +} diff --git a/workflow/common/serviceaccount_test.go b/workflow/common/serviceaccount_test.go new file mode 100644 index 000000000000..9bd3da136bc2 --- /dev/null +++ b/workflow/common/serviceaccount_test.go @@ -0,0 +1,42 @@ +package common + +import ( + "testing" + + "github.com/argoproj/argo/test/util" + "github.com/stretchr/testify/assert" + "k8s.io/client-go/kubernetes/fake" +) + +// TestGetServiceAccountTokenByAccountName verifies service account token retrieved by service account name. +func TestGetServiceAccountTokenByAccountName(t *testing.T) { + clientset := fake.NewSimpleClientset() + _, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") + assert.NoError(t, err) + token, err := GetServiceAccountTokenByAccountName(clientset, "", "test") + assert.NoError(t, err) + assert.NotNil(t, token) + assert.Equal(t, "test-token", token.Name) +} + +// TestGetReferencedServiceAccountToken verifies service account token retrieved by service account name. +func TestGetReferencedServiceAccountToken(t *testing.T) { + clientset := fake.NewSimpleClientset() + sa, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") + assert.NoError(t, err) + token, err := GetReferencedServiceAccountToken(clientset, sa) + assert.NoError(t, err) + assert.NotNil(t, token) + assert.Equal(t, "test-token", token.Name) +} + +// TestGetReferencedServiceAccountToken verifies service account token retrieved by service account name. +func TestGetServiceAccountTokens(t *testing.T) { + clientset := fake.NewSimpleClientset() + sa, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") + assert.NoError(t, err) + tokens, err := GetServiceAccountTokens(clientset, sa) + assert.NoError(t, err) + assert.Equal(t, 1, len(tokens)) + assert.Equal(t, "test-token", tokens[0].Name) +} diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 9b5ffe9fa076..d9748dc3ade0 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -437,13 +437,13 @@ func (woc *wfOperationCtx) newExecContainer(name string, tmpl *wfv1.Template) *a }) exec.Args = append(exec.Args, "--kubeconfig="+path) } - executorServiceAccountTokenName := "" - if tmpl.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName - } else if woc.wf.Spec.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = woc.wf.Spec.ExecutorServiceAccountTokenName + executorServiceAccountName := "" + if tmpl.ExecutorServiceAccountName != "" { + executorServiceAccountName = tmpl.ExecutorServiceAccountName + } else if woc.wf.Spec.ExecutorServiceAccountName != "" { + executorServiceAccountName = woc.wf.Spec.ExecutorServiceAccountName } - if executorServiceAccountTokenName != "" { + if executorServiceAccountName != "" { exec.VolumeMounts = append(exec.VolumeMounts, apiv1.VolumeMount{ Name: common.ServiceAccountTokenVolumeName, MountPath: common.ServiceAccountTokenMountPath, @@ -853,23 +853,27 @@ func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Templa pod.Spec.AutomountServiceAccountToken = automountServiceAccountToken } - executorServiceAccountTokenName := "" - if tmpl.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName - } else if woc.wf.Spec.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = woc.wf.Spec.ExecutorServiceAccountTokenName + executorServiceAccountName := "" + if tmpl.ExecutorServiceAccountName != "" { + executorServiceAccountName = tmpl.ExecutorServiceAccountName + } else if woc.wf.Spec.ExecutorServiceAccountName != "" { + executorServiceAccountName = woc.wf.Spec.ExecutorServiceAccountName } - if executorServiceAccountTokenName != "" { + if executorServiceAccountName != "" { + token, err := common.GetServiceAccountTokenByAccountName(woc.controller.kubeclientset, pod.Namespace, executorServiceAccountName) + if err != nil { + return err + } pod.Spec.Volumes = append(pod.Spec.Volumes, apiv1.Volume{ Name: common.ServiceAccountTokenVolumeName, VolumeSource: apiv1.VolumeSource{ Secret: &apiv1.SecretVolumeSource{ - SecretName: executorServiceAccountTokenName, + SecretName: token.Name, }, }, }) } else if automountServiceAccountToken != nil && !*automountServiceAccountToken { - return errors.Errorf(errors.CodeBadRequest, "executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + return errors.Errorf(errors.CodeBadRequest, "executorServiceAccountName must not be empty if automountServiceAccountToken is false") } return nil } diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index 79e4641291b7..fd794fcfdaa8 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/argoproj/argo/test/util" "github.com/argoproj/argo/workflow/config" wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" @@ -103,9 +104,12 @@ func TestTmplServiceAccount(t *testing.T) { // TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. func TestWFLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() + _, err := util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "foo", "foo-token") + assert.NoError(t, err) + falseValue := false woc.wf.Spec.AutomountServiceAccountToken = &falseValue - woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + woc.wf.Spec.ExecutorServiceAccountName = "foo" woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) @@ -116,10 +120,13 @@ func TestWFLevelAutomountServiceAccountToken(t *testing.T) { // TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { woc := newWoc() + _, err := util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "foo", "foo-token") + assert.NoError(t, err) + trueValue := true falseValue := false woc.wf.Spec.AutomountServiceAccountToken = &trueValue - woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + woc.wf.Spec.ExecutorServiceAccountName = "foo" woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &falseValue woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) @@ -138,36 +145,42 @@ func verifyServiceAccountTokenVolumeMount(t *testing.T, ctr apiv1.Container, vol t.Fatalf("%v does not have serviceAccountToken mounted properly (name: %s, mountPath: %s)", ctr, volName, mountPath) } -// TestWFLevelAutomountServiceAccountToken verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. -func TestWFLevelExecutorServiceAccountTokenName(t *testing.T) { +// TestWFLevelExecutorServiceAccountName verifies the ability to carry forward workflow level AutomountServiceAccountToken to Podspec. +func TestWFLevelExecutorServiceAccountName(t *testing.T) { woc := newWoc() - woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" + _, err := util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "foo", "foo-token") + assert.NoError(t, err) + woc.wf.Spec.ExecutorServiceAccountName = "foo" woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[2].Name) - assert.Equal(t, "foo", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) + assert.Equal(t, "foo-token", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) waitCtr := pod.Spec.Containers[0] verifyServiceAccountTokenVolumeMount(t, waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") } -// TestTmplLevelAutomountServiceAccountToken verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. -func TestTmplLevelExecutorServiceAccountTokenName(t *testing.T) { +// TestTmplLevelExecutorServiceAccountName verifies the ability to carry forward template level AutomountServiceAccountToken to Podspec. +func TestTmplLevelExecutorServiceAccountName(t *testing.T) { woc := newWoc() - woc.wf.Spec.ExecutorServiceAccountTokenName = "foo" - woc.wf.Spec.Templates[0].ExecutorServiceAccountTokenName = "tmpl" + _, err := util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "foo", "foo-token") + assert.NoError(t, err) + _, err = util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "tmpl", "tmpl-token") + assert.NoError(t, err) + woc.wf.Spec.ExecutorServiceAccountName = "foo" + woc.wf.Spec.Templates[0].ExecutorServiceAccountName = "tmpl" woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, "exec-sa-token", pod.Spec.Volumes[2].Name) - assert.Equal(t, "tmpl", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) + assert.Equal(t, "tmpl-token", pod.Spec.Volumes[2].VolumeSource.Secret.SecretName) waitCtr := pod.Spec.Containers[0] verifyServiceAccountTokenVolumeMount(t, waitCtr, "exec-sa-token", "/var/run/secrets/kubernetes.io/serviceaccount") diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index 54cbd3712e86..68270d820935 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -469,14 +469,14 @@ func (ctx *templateValidationCtx) validateLeaf(scope map[string]interface{}, tmp } else if ctx.wf != nil && ctx.wf.Spec.AutomountServiceAccountToken != nil { automountServiceAccountToken = ctx.wf.Spec.AutomountServiceAccountToken } - executorServiceAccountTokenName := "" - if tmpl.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = tmpl.ExecutorServiceAccountTokenName - } else if ctx.wf != nil && ctx.wf.Spec.ExecutorServiceAccountTokenName != "" { - executorServiceAccountTokenName = ctx.wf.Spec.ExecutorServiceAccountTokenName - } - if automountServiceAccountToken != nil && !*automountServiceAccountToken && executorServiceAccountTokenName == "" { - return errors.Errorf(errors.CodeBadRequest, "templates.%s.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false", tmpl.Name) + executorServiceAccountName := "" + if tmpl.ExecutorServiceAccountName != "" { + executorServiceAccountName = tmpl.ExecutorServiceAccountName + } else if ctx.wf != nil && ctx.wf.Spec.ExecutorServiceAccountName != "" { + executorServiceAccountName = ctx.wf.Spec.ExecutorServiceAccountName + } + if automountServiceAccountToken != nil && !*automountServiceAccountToken && executorServiceAccountName == "" { + return errors.Errorf(errors.CodeBadRequest, "templates.%s.executorServiceAccountName must not be empty if automountServiceAccountToken is false", tmpl.Name) } return nil } diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go index 72fadacfa759..23359004ef05 100644 --- a/workflow/validate/validate_test.go +++ b/workflow/validate/validate_test.go @@ -1702,9 +1702,9 @@ spec: container: image: alpine:latest automountServiceAccountToken: true - executorServiceAccountTokenName: "" + executorServiceAccountName: "" automountServiceAccountToken: false - executorServiceAccountTokenName: foo + executorServiceAccountName: foo ` var validAutomountServiceAccountTokenUseTmplLevel = ` @@ -1718,7 +1718,7 @@ spec: - name: whalesay container: image: alpine:latest - executorServiceAccountTokenName: foo + executorServiceAccountName: foo automountServiceAccountToken: false ` @@ -1766,11 +1766,11 @@ func TestAutomountServiceAccountTokenUse(t *testing.T) { { wf := unmarshalWf(invalidAutomountServiceAccountTokenUseWfLevel) err := ValidateWorkflow(wfClientset, namespace, wf, ValidateOpts{}) - assert.EqualError(t, err, "spec.entrypoint templates.whalesay.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + assert.EqualError(t, err, "spec.entrypoint templates.whalesay.executorServiceAccountName must not be empty if automountServiceAccountToken is false") } { wf := unmarshalWf(invalidAutomountServiceAccountTokenUseTmplLevel) err := ValidateWorkflow(wfClientset, namespace, wf, ValidateOpts{}) - assert.EqualError(t, err, "spec.entrypoint templates.whalesay.executorServiceAccountTokenName must not be empty if automountServiceAccountToken is false") + assert.EqualError(t, err, "spec.entrypoint templates.whalesay.executorServiceAccountName must not be empty if automountServiceAccountToken is false") } } From fce23160299d8f9a979a53fa0349c1435cae6946 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 13 Aug 2019 12:03:13 +0900 Subject: [PATCH 11/13] Structure executor config --- api/openapi-spec/swagger.json | 26 +++++++---- .../workflow/v1alpha1/openapi_generated.go | 43 +++++++++++++------ pkg/apis/workflow/v1alpha1/workflow_types.go | 18 +++++--- .../v1alpha1/zz_generated.deepcopy.go | 26 +++++++++++ workflow/controller/workflowpod.go | 18 ++++---- workflow/controller/workflowpod_test.go | 10 ++--- workflow/validate/validate.go | 10 ++--- workflow/validate/validate_test.go | 13 +++--- 8 files changed, 114 insertions(+), 50 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 2dae155fb098..cb1c3a907f91 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -276,6 +276,16 @@ } } }, + "io.argoproj.workflow.v1alpha1.ExecutorConfig": { + "description": "ExecutorConfig holds configurations of an executor container.", + "type": "object", + "properties": { + "serviceAccountName": { + "description": "ServiceAccountName specifies the service account name of the executor container.", + "type": "string" + } + } + }, "io.argoproj.workflow.v1alpha1.GitArtifact": { "description": "GitArtifact is the location of an git artifact", "type": "object", @@ -892,7 +902,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.Arguments" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ServiceAccountName of ExecutorConfig must be specified if this value is false.", "type": "boolean" }, "container": { @@ -907,9 +917,9 @@ "description": "DAG template subtype which runs a DAG", "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.DAGTemplate" }, - "executorServiceAccountName": { - "description": "ExecutorServiceAccountName specifies a service account token name for executor containers.", - "type": "string" + "executor": { + "description": "Executor holds configurations of the executor container.", + "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ExecutorConfig" }, "hostAliases": { "description": "HostAliases is an optional list of hosts and IPs that will be injected into the pod spec", @@ -1283,7 +1293,7 @@ "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ArtifactRepositoryRef" }, "automountServiceAccountToken": { - "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", + "description": "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ServiceAccountName of ExecutorConfig must be specified if this value is false.", "type": "boolean" }, "dnsConfig": { @@ -1298,9 +1308,9 @@ "description": "Entrypoint is a template reference to the starting point of the workflow", "type": "string" }, - "executorServiceAccountName": { - "description": "ExecutorServiceAccountName specifies a service account token name for executor containers.", - "type": "string" + "executor": { + "description": "Executor holds configurations of executor containers of the workflow.", + "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.ExecutorConfig" }, "hostAliases": { "description": "HostAliases is an optional list of hosts and IPs that will be injected into the pod spec", diff --git a/pkg/apis/workflow/v1alpha1/openapi_generated.go b/pkg/apis/workflow/v1alpha1/openapi_generated.go index 3d9ab19c72ed..7d1b218b919b 100644 --- a/pkg/apis/workflow/v1alpha1/openapi_generated.go +++ b/pkg/apis/workflow/v1alpha1/openapi_generated.go @@ -23,6 +23,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ContinueOn": schema_pkg_apis_workflow_v1alpha1_ContinueOn(ref), "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.DAGTask": schema_pkg_apis_workflow_v1alpha1_DAGTask(ref), "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.DAGTemplate": schema_pkg_apis_workflow_v1alpha1_DAGTemplate(ref), + "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ExecutorConfig": schema_pkg_apis_workflow_v1alpha1_ExecutorConfig(ref), "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.GitArtifact": schema_pkg_apis_workflow_v1alpha1_GitArtifact(ref), "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.HDFSArtifact": schema_pkg_apis_workflow_v1alpha1_HDFSArtifact(ref), "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.HDFSConfig": schema_pkg_apis_workflow_v1alpha1_HDFSConfig(ref), @@ -538,6 +539,26 @@ func schema_pkg_apis_workflow_v1alpha1_DAGTemplate(ref common.ReferenceCallback) } } +func schema_pkg_apis_workflow_v1alpha1_ExecutorConfig(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "ExecutorConfig holds configurations of an executor container.", + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "serviceAccountName": { + SchemaProps: spec.SchemaProps{ + Description: "ServiceAccountName specifies the service account name of the executor container.", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + } +} + func schema_pkg_apis_workflow_v1alpha1_GitArtifact(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ @@ -1831,16 +1852,15 @@ func schema_pkg_apis_workflow_v1alpha1_Template(ref common.ReferenceCallback) co }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ServiceAccountName of ExecutorConfig must be specified if this value is false.", Type: []string{"boolean"}, Format: "", }, }, - "executorServiceAccountName": { + "executor": { SchemaProps: spec.SchemaProps{ - Description: "ExecutorServiceAccountName specifies a service account token name for executor containers.", - Type: []string{"string"}, - Format: "", + Description: "Executor holds configurations of the executor container.", + Ref: ref("github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ExecutorConfig"), }, }, "hostAliases": { @@ -1867,7 +1887,7 @@ func schema_pkg_apis_workflow_v1alpha1_Template(ref common.ReferenceCallback) co }, }, Dependencies: []string{ - "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Arguments", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ArtifactLocation", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.DAGTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Inputs", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Metadata", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Outputs", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ResourceTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.RetryStrategy", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ScriptTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.SuspendTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.TemplateRef", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.UserContainer", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.WorkflowStep", "k8s.io/api/core/v1.Affinity", "k8s.io/api/core/v1.Container", "k8s.io/api/core/v1.HostAlias", "k8s.io/api/core/v1.PodSecurityContext", "k8s.io/api/core/v1.Toleration", "k8s.io/api/core/v1.Volume"}, + "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Arguments", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ArtifactLocation", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.DAGTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ExecutorConfig", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Inputs", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Metadata", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Outputs", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ResourceTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.RetryStrategy", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ScriptTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.SuspendTemplate", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.TemplateRef", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.UserContainer", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.WorkflowStep", "k8s.io/api/core/v1.Affinity", "k8s.io/api/core/v1.Container", "k8s.io/api/core/v1.HostAlias", "k8s.io/api/core/v1.PodSecurityContext", "k8s.io/api/core/v1.Toleration", "k8s.io/api/core/v1.Volume"}, } } @@ -2318,16 +2338,15 @@ func schema_pkg_apis_workflow_v1alpha1_WorkflowSpec(ref common.ReferenceCallback }, "automountServiceAccountToken": { SchemaProps: spec.SchemaProps{ - Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ExecutorServiceAccountName must be specified if this value is false.", + Description: "AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. ServiceAccountName of ExecutorConfig must be specified if this value is false.", Type: []string{"boolean"}, Format: "", }, }, - "executorServiceAccountName": { + "executor": { SchemaProps: spec.SchemaProps{ - Description: "ExecutorServiceAccountName specifies a service account token name for executor containers.", - Type: []string{"string"}, - Format: "", + Description: "Executor holds configurations of executor containers of the workflow.", + Ref: ref("github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ExecutorConfig"), }, }, "volumes": { @@ -2522,7 +2541,7 @@ func schema_pkg_apis_workflow_v1alpha1_WorkflowSpec(ref common.ReferenceCallback }, }, Dependencies: []string{ - "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Arguments", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ArtifactRepositoryRef", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.PodGC", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Template", "k8s.io/api/core/v1.Affinity", "k8s.io/api/core/v1.HostAlias", "k8s.io/api/core/v1.LocalObjectReference", "k8s.io/api/core/v1.PersistentVolumeClaim", "k8s.io/api/core/v1.PodDNSConfig", "k8s.io/api/core/v1.PodSecurityContext", "k8s.io/api/core/v1.Toleration", "k8s.io/api/core/v1.Volume"}, + "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Arguments", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ArtifactRepositoryRef", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.ExecutorConfig", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.PodGC", "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1.Template", "k8s.io/api/core/v1.Affinity", "k8s.io/api/core/v1.HostAlias", "k8s.io/api/core/v1.LocalObjectReference", "k8s.io/api/core/v1.PersistentVolumeClaim", "k8s.io/api/core/v1.PodDNSConfig", "k8s.io/api/core/v1.PodSecurityContext", "k8s.io/api/core/v1.Toleration", "k8s.io/api/core/v1.Volume"}, } } diff --git a/pkg/apis/workflow/v1alpha1/workflow_types.go b/pkg/apis/workflow/v1alpha1/workflow_types.go index 4648ca66e764..9a61c078f2cf 100644 --- a/pkg/apis/workflow/v1alpha1/workflow_types.go +++ b/pkg/apis/workflow/v1alpha1/workflow_types.go @@ -114,11 +114,11 @@ type WorkflowSpec struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. - // ExecutorServiceAccountName must be specified if this value is false. + // ServiceAccountName of ExecutorConfig must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` - // ExecutorServiceAccountName specifies a service account token name for executor containers. - ExecutorServiceAccountName string `json:"executorServiceAccountName,omitempty"` + // Executor holds configurations of executor containers of the workflow. + Executor *ExecutorConfig `json:"executor,omitempty"` // Volumes is a list of volumes that can be mounted by containers in a workflow. Volumes []apiv1.Volume `json:"volumes,omitempty"` @@ -314,11 +314,11 @@ type Template struct { ServiceAccountName string `json:"serviceAccountName,omitempty"` // AutomountServiceAccountToken indicates whether a service account token should be automatically mounted in pods. - // ExecutorServiceAccountName must be specified if this value is false. + // ServiceAccountName of ExecutorConfig must be specified if this value is false. AutomountServiceAccountToken *bool `json:"automountServiceAccountToken,omitempty"` - // ExecutorServiceAccountName specifies a service account token name for executor containers. - ExecutorServiceAccountName string `json:"executorServiceAccountName,omitempty"` + // Executor holds configurations of the executor container. + Executor *ExecutorConfig `json:"executor,omitempty"` // HostAliases is an optional list of hosts and IPs that will be injected into the pod spec HostAliases []apiv1.HostAlias `json:"hostAliases,omitempty"` @@ -961,6 +961,12 @@ func (h *HTTPArtifact) HasLocation() bool { return h != nil && h.URL != "" } +// ExecutorConfig holds configurations of an executor container. +type ExecutorConfig struct { + // ServiceAccountName specifies the service account name of the executor container. + ServiceAccountName string `json:"serviceAccountName,omitempty"` +} + // ScriptTemplate is a template subtype to enable scripting through code steps type ScriptTemplate struct { apiv1.Container `json:",inline"` diff --git a/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go index f3efa3379364..e299ea7e7c00 100644 --- a/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go @@ -290,6 +290,22 @@ func (in *DAGTemplate) DeepCopy() *DAGTemplate { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ExecutorConfig) DeepCopyInto(out *ExecutorConfig) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ExecutorConfig. +func (in *ExecutorConfig) DeepCopy() *ExecutorConfig { + if in == nil { + return nil + } + out := new(ExecutorConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitArtifact) DeepCopyInto(out *GitArtifact) { *out = *in @@ -916,6 +932,11 @@ func (in *Template) DeepCopyInto(out *Template) { *out = new(bool) **out = **in } + if in.Executor != nil { + in, out := &in.Executor, &out.Executor + *out = new(ExecutorConfig) + **out = **in + } if in.HostAliases != nil { in, out := &in.HostAliases, &out.HostAliases *out = make([]v1.HostAlias, len(*in)) @@ -1072,6 +1093,11 @@ func (in *WorkflowSpec) DeepCopyInto(out *WorkflowSpec) { *out = new(bool) **out = **in } + if in.Executor != nil { + in, out := &in.Executor, &out.Executor + *out = new(ExecutorConfig) + **out = **in + } if in.Volumes != nil { in, out := &in.Volumes, &out.Volumes *out = make([]v1.Volume, len(*in)) diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index d9748dc3ade0..31899b2b2eae 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -438,10 +438,10 @@ func (woc *wfOperationCtx) newExecContainer(name string, tmpl *wfv1.Template) *a exec.Args = append(exec.Args, "--kubeconfig="+path) } executorServiceAccountName := "" - if tmpl.ExecutorServiceAccountName != "" { - executorServiceAccountName = tmpl.ExecutorServiceAccountName - } else if woc.wf.Spec.ExecutorServiceAccountName != "" { - executorServiceAccountName = woc.wf.Spec.ExecutorServiceAccountName + if tmpl.Executor != nil && tmpl.Executor.ServiceAccountName != "" { + executorServiceAccountName = tmpl.Executor.ServiceAccountName + } else if woc.wf.Spec.Executor != nil && woc.wf.Spec.Executor.ServiceAccountName != "" { + executorServiceAccountName = woc.wf.Spec.Executor.ServiceAccountName } if executorServiceAccountName != "" { exec.VolumeMounts = append(exec.VolumeMounts, apiv1.VolumeMount{ @@ -854,10 +854,10 @@ func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Templa } executorServiceAccountName := "" - if tmpl.ExecutorServiceAccountName != "" { - executorServiceAccountName = tmpl.ExecutorServiceAccountName - } else if woc.wf.Spec.ExecutorServiceAccountName != "" { - executorServiceAccountName = woc.wf.Spec.ExecutorServiceAccountName + if tmpl.Executor != nil && tmpl.Executor.ServiceAccountName != "" { + executorServiceAccountName = tmpl.Executor.ServiceAccountName + } else if woc.wf.Spec.Executor != nil && woc.wf.Spec.Executor.ServiceAccountName != "" { + executorServiceAccountName = woc.wf.Spec.Executor.ServiceAccountName } if executorServiceAccountName != "" { token, err := common.GetServiceAccountTokenByAccountName(woc.controller.kubeclientset, pod.Namespace, executorServiceAccountName) @@ -873,7 +873,7 @@ func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Templa }, }) } else if automountServiceAccountToken != nil && !*automountServiceAccountToken { - return errors.Errorf(errors.CodeBadRequest, "executorServiceAccountName must not be empty if automountServiceAccountToken is false") + return errors.Errorf(errors.CodeBadRequest, "executor.serviceAccountName must not be empty if automountServiceAccountToken is false") } return nil } diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go index fd794fcfdaa8..ec80a42397f5 100644 --- a/workflow/controller/workflowpod_test.go +++ b/workflow/controller/workflowpod_test.go @@ -109,7 +109,7 @@ func TestWFLevelAutomountServiceAccountToken(t *testing.T) { falseValue := false woc.wf.Spec.AutomountServiceAccountToken = &falseValue - woc.wf.Spec.ExecutorServiceAccountName = "foo" + woc.wf.Spec.Executor = &wfv1.ExecutorConfig{ServiceAccountName: "foo"} woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) @@ -126,7 +126,7 @@ func TestTmplLevelAutomountServiceAccountToken(t *testing.T) { trueValue := true falseValue := false woc.wf.Spec.AutomountServiceAccountToken = &trueValue - woc.wf.Spec.ExecutorServiceAccountName = "foo" + woc.wf.Spec.Executor = &wfv1.ExecutorConfig{ServiceAccountName: "foo"} woc.wf.Spec.Templates[0].AutomountServiceAccountToken = &falseValue woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) @@ -151,7 +151,7 @@ func TestWFLevelExecutorServiceAccountName(t *testing.T) { _, err := util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "foo", "foo-token") assert.NoError(t, err) - woc.wf.Spec.ExecutorServiceAccountName = "foo" + woc.wf.Spec.Executor = &wfv1.ExecutorConfig{ServiceAccountName: "foo"} woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) @@ -172,8 +172,8 @@ func TestTmplLevelExecutorServiceAccountName(t *testing.T) { _, err = util.CreateServiceAccountWithToken(woc.controller.kubeclientset, "", "tmpl", "tmpl-token") assert.NoError(t, err) - woc.wf.Spec.ExecutorServiceAccountName = "foo" - woc.wf.Spec.Templates[0].ExecutorServiceAccountName = "tmpl" + woc.wf.Spec.Executor = &wfv1.ExecutorConfig{ServiceAccountName: "foo"} + woc.wf.Spec.Templates[0].Executor = &wfv1.ExecutorConfig{ServiceAccountName: "tmpl"} woc.executeContainer(woc.wf.Spec.Entrypoint, &woc.wf.Spec.Templates[0], &wfv1.Template{}, "") podName := getPodName(woc.wf) pod, err := woc.controller.kubeclientset.CoreV1().Pods("").Get(podName, metav1.GetOptions{}) diff --git a/workflow/validate/validate.go b/workflow/validate/validate.go index 7b739e0941e0..4f17f187b99d 100644 --- a/workflow/validate/validate.go +++ b/workflow/validate/validate.go @@ -480,13 +480,13 @@ func (ctx *templateValidationCtx) validateLeaf(scope map[string]interface{}, tmp automountServiceAccountToken = ctx.wf.Spec.AutomountServiceAccountToken } executorServiceAccountName := "" - if tmpl.ExecutorServiceAccountName != "" { - executorServiceAccountName = tmpl.ExecutorServiceAccountName - } else if ctx.wf != nil && ctx.wf.Spec.ExecutorServiceAccountName != "" { - executorServiceAccountName = ctx.wf.Spec.ExecutorServiceAccountName + if tmpl.Executor != nil && tmpl.Executor.ServiceAccountName != "" { + executorServiceAccountName = tmpl.Executor.ServiceAccountName + } else if ctx.wf != nil && ctx.wf.Spec.Executor != nil && ctx.wf.Spec.Executor.ServiceAccountName != "" { + executorServiceAccountName = ctx.wf.Spec.Executor.ServiceAccountName } if automountServiceAccountToken != nil && !*automountServiceAccountToken && executorServiceAccountName == "" { - return errors.Errorf(errors.CodeBadRequest, "templates.%s.executorServiceAccountName must not be empty if automountServiceAccountToken is false", tmpl.Name) + return errors.Errorf(errors.CodeBadRequest, "templates.%s.executor.serviceAccountName must not be empty if automountServiceAccountToken is false", tmpl.Name) } return nil } diff --git a/workflow/validate/validate_test.go b/workflow/validate/validate_test.go index 21618caa0c5b..44355cc9c309 100644 --- a/workflow/validate/validate_test.go +++ b/workflow/validate/validate_test.go @@ -1733,9 +1733,11 @@ spec: container: image: alpine:latest automountServiceAccountToken: true - executorServiceAccountName: "" + executor: + ServiceAccountName: "" automountServiceAccountToken: false - executorServiceAccountName: foo + executor: + ServiceAccountName: foo ` var validAutomountServiceAccountTokenUseTmplLevel = ` @@ -1749,7 +1751,8 @@ spec: - name: whalesay container: image: alpine:latest - executorServiceAccountName: foo + executor: + ServiceAccountName: foo automountServiceAccountToken: false ` @@ -1797,11 +1800,11 @@ func TestAutomountServiceAccountTokenUse(t *testing.T) { { wf := unmarshalWf(invalidAutomountServiceAccountTokenUseWfLevel) err := ValidateWorkflow(wfClientset, namespace, wf, ValidateOpts{}) - assert.EqualError(t, err, "templates.whalesay.executorServiceAccountName must not be empty if automountServiceAccountToken is false") + assert.EqualError(t, err, "templates.whalesay.executor.serviceAccountName must not be empty if automountServiceAccountToken is false") } { wf := unmarshalWf(invalidAutomountServiceAccountTokenUseTmplLevel) err := ValidateWorkflow(wfClientset, namespace, wf, ValidateOpts{}) - assert.EqualError(t, err, "templates.whalesay.executorServiceAccountName must not be empty if automountServiceAccountToken is false") + assert.EqualError(t, err, "templates.whalesay.executor.serviceAccountName must not be empty if automountServiceAccountToken is false") } } From 75e5f0b4a58926f9e8d3c6b614b87081f4c59307 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Tue, 13 Aug 2019 13:22:31 +0900 Subject: [PATCH 12/13] Fix lint failure --- workflow/common/serviceaccount.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workflow/common/serviceaccount.go b/workflow/common/serviceaccount.go index e3c8f1e183a0..4c2833f30c66 100644 --- a/workflow/common/serviceaccount.go +++ b/workflow/common/serviceaccount.go @@ -64,7 +64,7 @@ func GetServiceAccountTokens(clientset kubernetes.Interface, serviceAccount *cor } if serviceaccount.IsServiceAccountToken(&secret, serviceAccount) { - // The variable `secret` is overwritten during the loop, so need to deep copy it. + // The variable `secret` is overwritten during the loop, so need to deep copy it. tokens = append(tokens, secret.DeepCopy()) } } From 2e734a66286a9846b162e4c2ceb6b611bcc91525 Mon Sep 17 00:00:00 2001 From: Daisuke Taniwaki Date: Wed, 21 Aug 2019 13:23:16 +0900 Subject: [PATCH 13/13] Stop getting actual secrets to find a service account token name --- .../workflow-controller-clusterrole.yaml | 3 - manifests/install.yaml | 3 - manifests/namespace-install.yaml | 3 - .../workflow-controller-role.yaml | 3 - workflow/common/serviceaccount.go | 63 ++----------------- workflow/common/serviceaccount_test.go | 31 ++------- workflow/controller/workflowpod.go | 4 +- 7 files changed, 11 insertions(+), 99 deletions(-) diff --git a/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml b/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml index 8da2b401ec35..b7a642a34978 100644 --- a/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml +++ b/manifests/cluster-install/workflow-controller-rbac/workflow-controller-clusterrole.yaml @@ -56,9 +56,6 @@ rules: - "" resources: - serviceaccounts - - serviceaccounts/finalizers - - secrets - - secrets/finalizers verbs: - get - list diff --git a/manifests/install.yaml b/manifests/install.yaml index d0db29feae77..6f832ea83c39 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -162,9 +162,6 @@ rules: - "" resources: - serviceaccounts - - serviceaccounts/finalizers - - secrets - - secrets/finalizers verbs: - get - list diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index b1facc4a1eef..0d16a7dfc245 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -86,9 +86,6 @@ rules: - "" resources: - serviceaccounts - - serviceaccounts/finalizers - - secrets - - secrets/finalizers verbs: - get - list diff --git a/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml b/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml index c465550436ba..016cba426a0d 100644 --- a/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml +++ b/manifests/namespace-install/workflow-controller-rbac/workflow-controller-role.yaml @@ -47,9 +47,6 @@ rules: - "" resources: - serviceaccounts - - serviceaccounts/finalizers - - secrets - - secrets/finalizers verbs: - get - list diff --git a/workflow/common/serviceaccount.go b/workflow/common/serviceaccount.go index 4c2833f30c66..6865cbe4df07 100644 --- a/workflow/common/serviceaccount.go +++ b/workflow/common/serviceaccount.go @@ -2,71 +2,18 @@ package common import ( "github.com/argoproj/argo/errors" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" - "k8s.io/kubernetes/pkg/serviceaccount" ) -// GetServiceAccountTokenByAccountName returns the name of the first referenced secret which is a ServiceAccountToken for the service account -func GetServiceAccountTokenByAccountName(clientset kubernetes.Interface, namespace, name string) (*corev1.Secret, error) { +// GetServiceAccountTokenName returns the name of the first referenced ServiceAccountToken secret of the service account. +func GetServiceAccountTokenName(clientset kubernetes.Interface, namespace, name string) (string, error) { serviceAccount, err := clientset.CoreV1().ServiceAccounts(namespace).Get(name, metav1.GetOptions{}) if err != nil { - return nil, err + return "", err } - token, err := GetReferencedServiceAccountToken(clientset, serviceAccount) - if err != nil { - return nil, err - } - if token == nil { - return nil, errors.Errorf("Error looking up service account token for %s/%s", serviceAccount.Namespace, serviceAccount.Name) - } - return token, nil -} - -// GetReferencedServiceAccountToken returns the name of the first referenced secret which is a ServiceAccountToken for the service account -func GetReferencedServiceAccountToken(clientset kubernetes.Interface, serviceAccount *corev1.ServiceAccount) (*corev1.Secret, error) { if len(serviceAccount.Secrets) == 0 { - return nil, nil - } - - tokens, err := GetServiceAccountTokens(clientset, serviceAccount) - if err != nil { - return nil, err - } - - accountTokens := map[string]*corev1.Secret{} - for _, token := range tokens { - accountTokens[token.Name] = token - } - // Prefer secrets in the order they're referenced. - for _, obj := range serviceAccount.Secrets { - token, ok := accountTokens[obj.Name] - if ok { - return token, nil - } - } - - return nil, nil -} - -// GetServiceAccountTokens returns all ServiceAccountToken secrets for the given ServiceAccount -func GetServiceAccountTokens(clientset kubernetes.Interface, serviceAccount *corev1.ServiceAccount) ([]*corev1.Secret, error) { - secrets, err := clientset.CoreV1().Secrets(serviceAccount.Namespace).List(metav1.ListOptions{}) - if err != nil { - return nil, err - } - - tokens := []*corev1.Secret{} - for _, secret := range secrets.Items { - if secret.Type != corev1.SecretTypeServiceAccountToken { - continue - } - - if serviceaccount.IsServiceAccountToken(&secret, serviceAccount) { - // The variable `secret` is overwritten during the loop, so need to deep copy it. - tokens = append(tokens, secret.DeepCopy()) - } + return "", errors.Errorf("Service account %s/%s does not have any token", serviceAccount.Namespace, serviceAccount.Name) } - return tokens, nil + return serviceAccount.Secrets[0].Name, nil } diff --git a/workflow/common/serviceaccount_test.go b/workflow/common/serviceaccount_test.go index 9bd3da136bc2..b3c4a73ae852 100644 --- a/workflow/common/serviceaccount_test.go +++ b/workflow/common/serviceaccount_test.go @@ -8,35 +8,12 @@ import ( "k8s.io/client-go/kubernetes/fake" ) -// TestGetServiceAccountTokenByAccountName verifies service account token retrieved by service account name. -func TestGetServiceAccountTokenByAccountName(t *testing.T) { +// TestGetServiceAccountTokenName verifies service account token retrieved by service account name. +func TestGetServiceAccountTokenName(t *testing.T) { clientset := fake.NewSimpleClientset() _, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") assert.NoError(t, err) - token, err := GetServiceAccountTokenByAccountName(clientset, "", "test") + tokenName, err := GetServiceAccountTokenName(clientset, "", "test") assert.NoError(t, err) - assert.NotNil(t, token) - assert.Equal(t, "test-token", token.Name) -} - -// TestGetReferencedServiceAccountToken verifies service account token retrieved by service account name. -func TestGetReferencedServiceAccountToken(t *testing.T) { - clientset := fake.NewSimpleClientset() - sa, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") - assert.NoError(t, err) - token, err := GetReferencedServiceAccountToken(clientset, sa) - assert.NoError(t, err) - assert.NotNil(t, token) - assert.Equal(t, "test-token", token.Name) -} - -// TestGetReferencedServiceAccountToken verifies service account token retrieved by service account name. -func TestGetServiceAccountTokens(t *testing.T) { - clientset := fake.NewSimpleClientset() - sa, err := util.CreateServiceAccountWithToken(clientset, "", "test", "test-token") - assert.NoError(t, err) - tokens, err := GetServiceAccountTokens(clientset, sa) - assert.NoError(t, err) - assert.Equal(t, 1, len(tokens)) - assert.Equal(t, "test-token", tokens[0].Name) + assert.Equal(t, "test-token", tokenName) } diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go index 31899b2b2eae..8cd706c23b9e 100644 --- a/workflow/controller/workflowpod.go +++ b/workflow/controller/workflowpod.go @@ -860,7 +860,7 @@ func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Templa executorServiceAccountName = woc.wf.Spec.Executor.ServiceAccountName } if executorServiceAccountName != "" { - token, err := common.GetServiceAccountTokenByAccountName(woc.controller.kubeclientset, pod.Namespace, executorServiceAccountName) + tokenName, err := common.GetServiceAccountTokenName(woc.controller.kubeclientset, pod.Namespace, executorServiceAccountName) if err != nil { return err } @@ -868,7 +868,7 @@ func (woc *wfOperationCtx) setupServiceAccount(pod *apiv1.Pod, tmpl *wfv1.Templa Name: common.ServiceAccountTokenVolumeName, VolumeSource: apiv1.VolumeSource{ Secret: &apiv1.SecretVolumeSource{ - SecretName: token.Name, + SecretName: tokenName, }, }, })