argo-workflows icon indicating copy to clipboard operation
argo-workflows copied to clipboard

service-account defined for individual steps.

Open KowalczykBartek opened this issue 5 years ago • 9 comments

Buenos Dias :)

Our team is making research around argo-workflow project usage, and we found one potential security-issue (because of company guidelines) - we want to address it somehow, potentially contributing PR, but first I wanted to consult an idea.

Summary

Allow to define service-account on per step basis (each pod have different service-account)

Motivation

How we want use argo workflow: We want to define single workflow that will gather some data, compress it, parse and do other stuff around - all work required is defined as steps, lets say

step1: get sensitive data and anonymize that data. step2: add metrics to already received data
step3: send to processing engine

pretty simple - but, step1 and step3 are maintained by two different teams, those steps need to access different secrets (lets say secre1 for step1 and secret3 for step3), and no other team can have access to secret used by other team (team3 cannot access secret1).

Proposal

I did already some research around code-base, and made following change (i was working on code-base commit 0a928e93):

first, Template can have its own service-account-name defined

type Template struct {
   // per template service-account-name
   ServiceAccountName string `json:"serviceAccountName,omitempty"`
}

then, before spawning workflowpod in createWorkflowPod function, I am using it

var desiredServiceAccountName = woc.wf.Spec.ServiceAccountName
if len(tmpl.ServiceAccountName) > 0 {
	fmt.Println("GOOD")
	desiredServiceAccountName = tmpl.ServiceAccountName
}

...


Spec: apiv1.PodSpec{
	RestartPolicy: apiv1.RestartPolicyNever,
	Containers: []apiv1.Container{
		mainCtr,
	},
	Volumes:               woc.createVolumes(),
	ActiveDeadlineSeconds: tmpl.ActiveDeadlineSeconds,
	ServiceAccountName:    desiredServiceAccountName,
	ImagePullSecrets:      woc.wf.Spec.ImagePullSecrets,
},

and how I want to use this in yaml

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: coinflip-
spec:
  entrypoint: coinflip
  templates:
  - name: coinflip
    steps:
    - - name: flip-coin
        template: flip-coin
    - - name: heads
        template: heads
        when: "{{steps.flip-coin.outputs.result}} == heads"
      - name: tails
        template: tails
        when: "{{steps.flip-coin.outputs.result}} == tails"

  - name: flip-coin
    serviceAccountName: zarzamora
    script:
      image: python:alpine3.6
      command: [python]
      source: |
        import random
        import time
        time.sleep(5000000)
        result = "heads" if random.randint(0,1) == 0 else "tails"
        print(result)

  - name: heads
    serviceAccountName: manzana
    container:
      image: alpine:3.6
      command: [sh, -c]
      args: ["echo \"it was heads\""]

  - name: tails
    serviceAccountName: aguacate
    container:
      image: alpine:3.6
      command: [sh, -c]
      args: ["echo \"it was tails\""]

pretty simple - and probably from code-base point of view just wrong, but i wanted only to test the idea.

I don't expect that you will implement it - just give feedback about idea, I am willing to provide PR.

thanks.

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

KowalczykBartek avatar Jan 24 '20 11:01 KowalczykBartek

Reopening since I think this was closed on error

simster7 avatar Jan 26 '20 17:01 simster7

honestly i closed it because now i have doubts if it makes sense xD but we can let this opened, maybe someone will have some idea around this ticket - atm I am trying to add "namespace" parameter to step definition to allow running workflow's steps in different namespace (maybe that is better option)

KowalczykBartek avatar Jan 26 '20 17:01 KowalczykBartek

This actually totally makes sense and a much-needed feature. One of the use cases that we have is different service accounts in the namespaces have different permissions and IAM roles annotated to them. Currently, it is impossible to run steps as part of a single workflow which requires a different set of permissions

azhar22k avatar Nov 03 '20 07:11 azhar22k

Could you do this using podSpecPatch?

alexec avatar Nov 03 '20 15:11 alexec

I can confirm this can be achieved by podSpecPatch. For example:

  templates:
  - name: whalesay
    podSpecPatch: '{"serviceAccountName": "custom-serviceaccount"}'
    container:
      image: docker/whalesay:latest
      command: [cowsay]
      args: ["hello world"]

dinever avatar Apr 26 '21 20:04 dinever

@dinever you saved my day!

This is the way to implement executer ServiceAccounts for WorkflowTemplate too!

fabioaraujopt avatar Apr 22 '22 11:04 fabioaraujopt

This enhancement make a lot of sense to me. podSpecPatch works, but is clunky and not declarative/programmable. It would be great if someone wanted to submit a PR for this. I think it would be a good first issue.

alexec avatar Apr 22 '22 14:04 alexec

@dinever you saved my day!

This is the way to implement executer ServiceAccounts for WorkflowTemplate too!

Being able to define a different service account for the executor (ie for artifact uploads) would help a lot on the operational side of things.

agrvu avatar Jun 23 '22 12:06 agrvu

This enhancement make a lot of sense to me. podSpecPatch works, but is clunky and not declarative/programmable. It would be great if someone wanted to submit a PR for this. I think it would be a good first issue.

@alexec Does the approach mentioned in the issue description, by @KowalczykBartek make sense? Or is there some different approach that should be explored?

I'm taking a look at this issue and am working on a PR.

mbtamuli avatar Aug 27 '22 13:08 mbtamuli

So I was looking to implement this feature, but when I went through the code (and suggestions from OP!), it seemed like this was already possible?

In workflow_types.go, the Template struct already has a ServiceAccountName: https://github.com/argoproj/argo-workflows/blob/256ff72816b4314f59fe39f0a458644af0075ebe/pkg/apis/workflow/v1alpha1/workflow_types.go#L717

(meaning the WorkflowSpec struct can specify an SA and templates within the workflow also can specify SAs)

Further, in workflowpod.go, there is already logic to set the Pod's SA to the template SA: https://github.com/argoproj/argo-workflows/blob/256ff72816b4314f59fe39f0a458644af0075ebe/workflow/controller/workflowpod.go#L999

I haven't tried setting these myself yet though since I stumbled upon this issue beforehand, but maybe this is already possible? This code seems to be older than this issue (per the blame), so that makes me feel like maybe I'm missing something? or there's a bug or something? If anyone knows otherwise, please point in the right direction!

agilgur5 avatar Jun 19 '23 17:06 agilgur5

Yeah thanks for the tip @agilgur5! Removing serviceAccountName and executor.serviceAccountName from my WorkflowSpec and adding executor.serviceAccountNames with different values to my step templates worked fine for me in Argo 3.4.8. I also ensured I didn't have any service accounts configured as workflow defaults via the controller configmap, not sure how much of those extra precautions I really needed to worry about though :)

mbaynton avatar Jul 27 '23 15:07 mbaynton

Yeah thanks for the tip @agilgur5!

Awesome! Thanks for confirming! This can be closed out then 🙂

not sure how much of those extra precautions I really needed to worry about though :)

As is k8s convention, the most specific property should be used; so the step template one should override the Workflow one which should override the default one. Or to put it visually with some pseudo-code:

step > Workflow > default

If something doesn't follow that properly, that would be a bug

agilgur5 avatar Jul 27 '23 15:07 agilgur5

Yeah thanks for the tip @agilgur5! Removing serviceAccountName and executor.serviceAccountName from my WorkflowSpec and adding executor.serviceAccountNames with different values to my step templates worked fine for me in Argo 3.4.8. I also ensured I didn't have any service accounts configured as workflow defaults via the controller configmap, not sure how much of those extra precautions I really needed to worry about though :)

@mbaynton can you post an example workflow yaml for newbies like myself?

spolloni avatar Dec 29 '23 01:12 spolloni

can you post an example workflow yaml for newbies like myself?

Taking an existing example and modifying it slightly, you can do:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: arguments-parameters-from-configmap-
spec:
  entrypoint: whalesay

  templates:
  - name: whalesay
    serviceAccountName: argo
    inputs:
      parameters:
      # Parameters can also be passed via configmap reference.
      - name: message
        valueFrom:
          configMapKeyRef:
            name: simple-parameters
            key: msg
    container:
      image: argoproj/argosay:v2
      args: ["echo", "{{inputs.parameters.message}}"]

agilgur5 avatar Dec 29 '23 04:12 agilgur5