Adding possibility to configure multiple service accounts for different tasks #902

cezkuj · 2019-05-23T17:25:01Z

Changes

The pipeline specification allows multiple different service accounts for tasks so that different task can use the right service account.
E.g. build task may need different privileges than deploy task

Fixes

Pipeline doesn't allow different service accounts for different tasks #777

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Adding possibility to configure multiple service accounts for different tasks

dibyom · 2019-05-23T17:27:08Z

/ok-to-test

dibyom · 2019-05-23T20:07:30Z

Gopkg.lock

@@ -22,7 +22,7 @@
  version = "v0.2.0"

 [[projects]]
-  digest = "1:355d491068cd888a3725d68cd09681a49251d6d8e3cb49120bf6b83a86c7ee0f"
+  digest = "1:c7ec3a8daf56c9d7c4bb36f498f6db3f8c339b1142300dcd4328950574f91c6f"


Was updating this file intentional?

This file was updated by hack/update-deps.sh (dep ensure). First version of PR didn't have that change and failed on integration test(please see Prow history).

cezkuj · 2019-05-25T10:29:07Z

/assign @dlorenc

dlorenc · 2019-05-25T14:28:10Z

In general this makes sense to me, but given it's an API change we should probably have another reviewer look.

My reasoning for why this would make sense:

Tasks within a pipeline may require access to different sets of resources, requiring different credentials. The principle of least-privilege would ask that Tasks get the minimum access they need, rather than the "highest-common-denominator".
Some Tasks may be authored in-house while others are reused from something like our catalog. Users may feel comfortable offering higher levels of access to tasks that they authored.

dlorenc · 2019-05-25T14:28:28Z

cc @bobcatfish @imjasonh to give the second sign off for an API review

vdemeester · 2019-05-27T06:58:18Z

Same as @dlorenc, make sense to me ; mainly though of the first point, aka least-privilege 👼

bobcatfish · 2019-05-28T17:18:56Z

Thanks for including the docs in the PR @cezkuj and welcome to the project!!! ❤️

In general this makes sense to me, but given it's an API change we should probably have another reviewer look.

I agree in general with being able to specify service accounts at the level of the Tasks within a Pipeline (and possibly also at the level of the PipelineResources, but that's another story :D).

However I don't think we should put this in the Pipeline though: right now service accounts are considered runtime information and are included in the PipelineRun and TaskRun specs - I think if we want to add task specific service accounts, we should do that in the PipelineRun. A couple ideas how we could do that:

We could have a section for config specific to each task:

kind: PipelineRun
...
spec:
  pipelineRef:
    name: demo-pipeline
  serviceAccount: 'default'
  tasks:
  ...
  - name: source-repo
    serviceAccount: my-special-service-account

Or we could have a serviceAccount section:

kind: PipelineRun
...
spec:
  pipelineRef:
    name: demo-pipeline
  serviceAccount: 'default'
  serviceAccounts:
  ...
  - taskName: source-repo
    serviceAccount: my-special-service-account

What do you think @vdemeester @dlorenc @imjasonh ?

bobcatfish · 2019-05-28T17:20:03Z

/hold

bobcatfish · 2019-05-28T17:22:16Z

(Adding this to the WG agenda tomorrow in case folks want to discuss in person)

vdemeester · 2019-05-28T17:23:02Z

oh yeah, definitely on PipelineRun 👍
@bobcatfish one question on your 2nd examples : having serviceAccounts would allow us to have multiple service accounts on each tasks ?

bobcatfish · 2019-05-28T18:15:00Z

@bobcatfish one question on your 2nd examples : having serviceAccounts would allow us to have multiple service accounts on each tasks ?

Hm i don't think so - I'm not sure if that would work with our current design 🤔

The only difference b/w the two examples is whether the top level thing is called tasks and each section is about configuration for tasks (which would assume we would want to put other task level info here later? maybe planning too far ahead, not sure what that would be ... 😅 ) and the other example has the top level thing called serviceAccounts instead and is just about serviceAccounts <-- probably simpler

cezkuj · 2019-05-28T19:43:09Z

Hmm, I see some reasoning behind moving it to PipelineRun. I would definitely go with serviceAccounts way as it seems more obvious for me. I will try to join WG tomorrow in case of further discussion.

bobcatfish · 2019-05-29T22:05:51Z

Didn't end up having time to discuss in the WG - I think go ahead with the serviceAccounts design or something similar @cezkuj ! If you want more discussion you could create a design doc, but I think it's a small enough change that we could just continue to discuss in the PR.

cezkuj · 2019-05-30T08:51:26Z

Updated PR with serviceAccounts implementation. Wasn't sure if taskName in serviceAccounts should refer to Task or PipelineTask - I've chosen Task. As a side effect of update-deps.sh LICENSE got also updated - not sure if this is wanted.

bobcatfish

Looking great @cezkuj !! Thanks for your patience getting this reviewed 🙏

Since this changes the interface (#906), @vdemeester @dlorenc @imjasonh what do you think of this interface in PipelineRuns for specifying service accounts per task:

spec:
  serviceAccount: sa-1
  serviceAccounts:
    - taskName: build-task
      serviceAccount: sa-for-build

bobcatfish · 2019-06-13T23:28:46Z

docs/pipelineruns.md

+  serviceAccounts:
+    - taskName: build-task
+      serviceAccount: sa-for-build
+```


this interface looks great! 👍

bobcatfish · 2019-06-13T23:34:29Z

docs/pipelineruns.md

@@ -94,6 +94,15 @@ that is in the
 [namespace](https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/)
 of the `TaskRun` resource object.

+There is also possibility to configure service account for concrete `Task`, overriding `ServiceAccount` set in `PipelineRun`, for example:


i think it would be good to be clear that the taskName below is the name of the task within the Pipeline, vs. the Task crd.

maybe you could just add a bit more to the example, e.g. something like:

For example with this serviceAccount configuration:

spec: serviceAccount: sa-1 serviceAccounts: - taskName: build-task serviceAccount: sa-for-build

If used with this Pipeline, test-task will use the ServiceAccount sa-1, while build-task will use sa-for-build.

kind: Pipeline spec: tasks: - name: build-task taskRef: name: build-push tasks: - name: test-task taskRef: name: test

Hmm, TBH, as far as I see I've implemented it to refer to Task crd. Now it seems less obvious to me why I'd go this way (plus it seems obvious for you to go with referring PipelineTask ). I'll change that and update docs as you've mentioned. Thanks for review!:)

Changed to PipelineTask way and extended docs. Please take a look once again. :)

test/builder/pipeline.go

vdemeester

@bobcatfish SGTM 👼

tekton-robot · 2019-06-14T11:02:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezkuj, dibyom, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…nt tasks The pipeline specification allows multiple different service accounts for tasks so that different task can use the right service account. E.g. build task may need different privileges than deploy task Fixes - #777

cezkuj · 2019-06-15T13:22:07Z

/retest

bobcatfish

Looks great, thanks @cezkuj !!! 🙏

/hold cancel
/lgtm
/meow space

bobcatfish · 2019-06-17T23:01:42Z

docs/pipelineruns.md

+  tasks:
+    - name: test-task
+      taskRef:
+        name: test


👍 thanks for adding this!

tekton-robot · 2019-06-17T23:02:12Z

@bobcatfish:

In response to this:

Looks great, thanks @cezkuj !!! 🙏

/hold cancel
/lgtm
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot requested review from imjasonh and vdemeester May 23, 2019 17:25

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 23, 2019

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 23, 2019

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 23, 2019

dibyom approved these changes May 23, 2019

View reviewed changes

tekton-robot assigned dlorenc May 25, 2019

dlorenc mentioned this pull request May 25, 2019

Formalize review requirements for API changes #906

Closed

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2019

bobcatfish reviewed Jun 13, 2019

View reviewed changes

vdemeester approved these changes Jun 14, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2019

bobcatfish reviewed Jun 17, 2019

View reviewed changes

docs/pipelineruns.md

tasks:

- name: test-task

taskRef:

name: test

Copy link

Collaborator

bobcatfish Jun 17, 2019

Choose a reason for hiding this comment

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

👍 thanks for adding this!

tekton-robot assigned bobcatfish Jun 17, 2019

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 17, 2019

tekton-robot merged commit b6b2b6a into tektoncd:master Jun 17, 2019

bobcatfish mentioned this pull request Jun 20, 2019

Pipeline doesn't allow different service accounts for different tasks #777

Closed

hrishin mentioned this pull request Jun 21, 2019

Add Start pipeline command tektoncd/cli#98

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding possibility to configure multiple service accounts for different tasks #902

Adding possibility to configure multiple service accounts for different tasks #902

cezkuj commented May 23, 2019 •

edited

Loading

dibyom commented May 23, 2019

dibyom May 23, 2019

cezkuj May 24, 2019

cezkuj commented May 25, 2019

dlorenc commented May 25, 2019

dlorenc commented May 25, 2019

vdemeester commented May 27, 2019

bobcatfish commented May 28, 2019

bobcatfish commented May 28, 2019

bobcatfish commented May 28, 2019 •

edited

Loading

vdemeester commented May 28, 2019

bobcatfish commented May 28, 2019

cezkuj commented May 28, 2019

bobcatfish commented May 29, 2019

cezkuj commented May 30, 2019

bobcatfish left a comment

bobcatfish Jun 13, 2019

bobcatfish Jun 13, 2019

cezkuj Jun 14, 2019

cezkuj Jun 15, 2019

vdemeester left a comment

tekton-robot commented Jun 14, 2019

cezkuj commented Jun 15, 2019

bobcatfish left a comment

bobcatfish Jun 17, 2019

tekton-robot commented Jun 17, 2019

Adding possibility to configure multiple service accounts for different tasks #902

Adding possibility to configure multiple service accounts for different tasks #902

Conversation

cezkuj commented May 23, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

dibyom commented May 23, 2019

dibyom May 23, 2019

Choose a reason for hiding this comment

cezkuj May 24, 2019

Choose a reason for hiding this comment

cezkuj commented May 25, 2019

dlorenc commented May 25, 2019

dlorenc commented May 25, 2019

vdemeester commented May 27, 2019

bobcatfish commented May 28, 2019

bobcatfish commented May 28, 2019

bobcatfish commented May 28, 2019 • edited Loading

vdemeester commented May 28, 2019

bobcatfish commented May 28, 2019

cezkuj commented May 28, 2019

bobcatfish commented May 29, 2019

cezkuj commented May 30, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Jun 13, 2019

Choose a reason for hiding this comment

bobcatfish Jun 13, 2019

Choose a reason for hiding this comment

cezkuj Jun 14, 2019

Choose a reason for hiding this comment

cezkuj Jun 15, 2019

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 14, 2019

cezkuj commented Jun 15, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Jun 17, 2019

Choose a reason for hiding this comment

tekton-robot commented Jun 17, 2019

cezkuj commented May 23, 2019 •

edited

Loading

bobcatfish commented May 28, 2019 •

edited

Loading