Add SecurityContext to TaskSpec

[dicarlo2]

Changes

Add securityContext to TaskSpec. Alternatives considered: add to PipelineRun and TaskRun - rejected because the security context a task runs in should not change from run to run.

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

Add `securityContext` to TaskSpec.

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dicarlo2
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: imjasonh

If they are not already assigned, you can assign the PR to them by writing /assign @imjasonh in a comment when ready.

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

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

[shashwathi]

@dicarlo2 :

If security context is added at Task level then all users of Task are expected to run this with context. Is this run time configuration for TaskRun instead of Task?

cc @bobcatfish @vdemeester @pivotal-nader-ziada

[nader-ziada]

agree with @shashwathi, makes more sense in the taskRun

[dicarlo2]

@shashwathi Sure, I can change that.

[vdemeester]

/ok-to-test
@dicarlo2 we may want some tests though (really simple unit test, on e2e we may need to think how we would do that — aka the security context we test has to be "available")

[dicarlo2]

@vdemeester will do - I might be a bit slow to respond in the next week or so, but I'll be back at it asap.

[bobcatfish]

I'm noticing a trend where it feels like we are slowly adding more and more pod attributes to both our TaskRuns and PipelineRuns.

Should we:

1. Go ahead and add all of them to both? (maybe even a pod spec itself as a template)
2. Consider having some other type that holds "templating" information about how a pod should be created?

I added this to the working group agenda for next week in case we want to discuss more out loud. If you folks agree I'm happy to create an issue to deal with allowing a user to specify all the pod attributes they could possibly want.

((Also im not very familiar with these pod attributes so I think I'm not totally clear on when they come into play so probably someone more informed than me will have better opinions here!))

[chmouel]

@bobcatfish I think your 2nd idea would be great to have but perhaps the two implementations may be needed.

Correct me if I'm wrong but my interpretation of "pod template" would be similar of this kubernetes feature :

https://github.com/kubernetes/community/blob/master/contributors/design-proposals/service-catalog/pod-preset.md

this gives a global preset way of how we are going to have each pods created as defined by the adminstrator.

But perhaps for some specifics use case user would want to override this and that's where feature 1 would be able to help us.

For example an administrator may want to set a default securityContext to have all pods running as user and then use a pod preset/template that by default does this.

But in some case user would need to run with a different securityContext (i.e: with privliged mode) for example when running kaniko or another container image building tools and then may wants to override it in her template to allows it.

Just my 2c and food for thoughts before next week meeting.

[dicarlo2]

Checking in here - was there a conclusion reached on how to move forward with this PR and/or the more generic templating solution? @bobcatfish @chmouel @vdemeester

[vdemeester]

@tekton-robot yes, if you rebase, now that #767 is merged, you should be able to move the SecurityContext to the template section 👼

[chmouel]

@vdemeester If I read properly #767 those templates apply to all the containers specified in the tasks, If i have a use case like this :

• My build container (i.e: docker) needs a priviliged: true
• My other containers don't need priviliged: true

I think Alex's PR allows to override the "global template', which would be usefull to address that use case.

We could maybe improve on this in the future and allow instead a task to specify a label that match another template, for example you could say from the task I was to apply the secure template instead of the default template.

[dicarlo2]

Hmm, looking at #767 it only allows specifying the container security context right? I'll have to do some testing, but I think we might need some of the PodSecurityContext only fields (e.g. fsGroup). In that case, should we add a "pod template" field - basically a v1.Pod that is merged into the final pod in a similar manner as the container spec in #767?

[dicarlo2]

For reference:

• PodSecurityContext 1.12-1.14
• SecurityContext 1.12-1.14

[bobcatfish]

@dicarlo2 we discussed this (pretty briefly!) in the most recent working group meeting, and this is where we landed:

• We want to add some kind of "pod template" that can be provided to TaskRuns and PipelineRuns (possibly in a separate CRD - since we want to decouple kube specific stuff from our types when possible; eventually Tasks may sometimes execute on completely different systems) (I volunteered to try to put together a proposal for this but if anyone else is interested in doing it instead that's great!)
• Re. @chmouel 's request to apply different templates to different steps in a Task, maybe the key there is to use multiple Tasks, each of which can have its own template? (i.e. use Tasks for the template grouping)

[dlorenc]

• Re. @chmouel 's request to apply different templates to different steps in a Task, maybe the key there is to use multiple Tasks, each of which can have its own template? (i.e. use Tasks for the template grouping)

Yeah I think we need to make a decision on this - since steps run inside a Pod today, it doesn't really make sense to allow different sets of permissions per step. Maybe other fields could be done, though.

[dicarlo2]

SGTM re: pod templates. Should we go ahead and close this PR then? FWIW - we still need this and are using it on a fork.

[bobcatfish]

@dicarlo2 since it's taking me ages to get around to making the proposal, and you are needing this now, I'm inclined to suggest we merge this the way it is now (since we've already got other pod/k8s specific attributes in the Runs, so we're not making it any worse)

Would love a second opinion @vdemeester @dlorenc @imjasonh

[AkihiroSuda]

Can we also have field for Pod metadata so that we can specify seccomp and AppArmor annotations?

[dicarlo2]

I'm inclined to suggest we merge this the way it is now

@bobcatfish totally up to you, I honestly don't mind maintaining a fork with these changes in the meantime.

[dicarlo2]

@AkihiroSuda #894 should satisfy your use-case, you can set annotations on the Task/Pipeline, etc. and they will propagate.

[bobcatfish]

I honestly don't mind maintaining a fork with these changes in the meantime.

Okay whichever you prefer @dicarlo2 ! In the meantime @vdemeester said he was going to take on proposing the pod template which I have continued to not get around to proposing 😇

[bobcatfish]

@vdemeester is adding securityContext in #1004 so I'll close this! 🎉 Thanks @dicarlo2 :D