Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AutomountServiceAccountToken #1480

Merged
merged 15 commits into from
Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,10 @@
"description": "Arguments hold arguments to the template.",
"$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.",
"type": "boolean"
},
"container": {
"description": "Container is the main container image to run in the pod",
"$ref": "#/definitions/io.k8s.api.core.v1.Container"
Expand All @@ -859,6 +863,10 @@
"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.",
"type": "string"
},
"hostAliases": {
"description": "HostAliases is an optional list of hosts and IPs that will be injected into the pod spec",
"type": "array",
Expand Down Expand Up @@ -1219,6 +1227,10 @@
"description": "ArtifactRepositoryRef specifies the configMap name and key containing the artifact repository config.",
"$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.",
"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"
Expand All @@ -1231,6 +1243,10 @@
"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.",
"type": "string"
},
"hostAliases": {
"description": "HostAliases is an optional list of hosts and IPs that will be injected into the pod spec",
"type": "array",
Expand Down
28 changes: 28 additions & 0 deletions pkg/apis/workflow/v1alpha1/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions pkg/apis/workflow/v1alpha1/workflow_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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 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"`
Copy link
Member

@jessesuen jessesuen Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtaniwaki - i was thinking, instead of executorServiceAccountTokenName, should this reference a service account name instead (i.e. executorServiceAccountName) and we would have the controller look up the the secret name from the service account for the volume mount? (similar to how serviceAccountName works)

This seems like it would be more user-friendly since users do not generally create tokens, they create service accounts. Note that this would require expanding the privileges of the controller to have read permissions on service accounts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I will change it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use ExecutorServiceAccountName instead of ExecutorServiceAccountTokenName, do you think AutomountServiceAccountToken still make sense to only disable automount service account token on the main container?

Copy link
Member Author

@dtaniwaki dtaniwaki Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use ExecutorServiceAccountName instead of ExecutorServiceAccountTokenName, do you think AutomountServiceAccountToken still make sense to only disable automount service account token on the main container?

Yes. I think the following should disable mounting of service accounts entirely (in both main and wait):

spec:
  automountServiceAccountToken: false

and the following will disable it on main, but mount a different service account for the executor:

spec:
  automountServiceAccountToken: false
  executorServiceAccountName: foo

And the following would use separate service accounts for main and wait

spec:
  serviceAccountName: bar
  executorServiceAccountName: foo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.
I realized we need to add read permission for service accounts to the cluster role.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the following case:

spec:
  automountServiceAccountToken: false

We can't run a workflow because the current executor needs a service account token during the initialization, so I made it cause an error during validation.


// Volumes is a list of volumes that can be mounted by containers in a workflow.
Volumes []apiv1.Volume `json:"volumes,omitempty"`

Expand Down Expand Up @@ -292,6 +299,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 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"`

Expand Down
10 changes: 10 additions & 0 deletions pkg/apis/workflow/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions workflow/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,11 @@ const (
// LocalVarPodName is a step level variable that references the name of the pod
LocalVarPodName = "pod.name"

KubeConfigDefaultMountPath = "/kube/config"
KubeConfigDefaultVolumeName = "kubeconfig"
SecretVolMountPath = "/argo/secret"
KubeConfigDefaultMountPath = "/kube/config"
KubeConfigDefaultVolumeName = "kubeconfig"
ServiceAccountTokenMountPath = "/var/run/secrets/kubernetes.io/serviceaccount"
ServiceAccountTokenVolumeName = "exec-sa-token"
SecretVolMountPath = "/argo/secret"
)

// GlobalVarWorkflowRootTags is a list of root tags in workflow which could be used for variable reference
Expand Down
5 changes: 1 addition & 4 deletions workflow/controller/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -1695,11 +1695,8 @@ 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,
}
_, err = woc.createWorkflowPod(nodeName, *mainCtr, tmpl, false)
if err != nil {
return woc.initializeNode(nodeName, wfv1.NodeTypePod, orgTmpl, boundaryID, wfv1.NodeError, err.Error())
Expand Down
73 changes: 62 additions & 11 deletions workflow/controller/workflowpod.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ 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
}

mainCtr.Name = common.MainContainerName
pod := &apiv1.Pod{
Expand All @@ -115,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,
},
}
Expand Down Expand Up @@ -144,6 +139,11 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont
return nil, err
}

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
Expand Down Expand Up @@ -271,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:
Expand Down Expand Up @@ -402,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(),
Expand All @@ -429,15 +429,27 @@ 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)
}
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,
ReadOnly: true,
})
}
return &exec
}

Expand Down Expand Up @@ -823,6 +835,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) {
Expand Down
77 changes: 75 additions & 2 deletions workflow/controller/workflowpod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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], &wfv1.Template{}, "")
Expand All @@ -100,6 +100,79 @@ 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()
falseValue := false
woc.wf.Spec.AutomountServiceAccountToken = &falseValue
woc.wf.Spec.ExecutorServiceAccountTokenName = "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.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()
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], &wfv1.Template{}, "")
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)
}

// 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], &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)

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], &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)

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()
Expand Down
Loading