Skip to content

Conversation

danielhelfand
Copy link
Member

Closes #2179

This pull request adds documentation on running step containers as non root as well as a TaskRun example of doing so.

A follow up to this should be running containers as root best practices especially in Kubernetes clusters that enforce not allowing containers to run as root by default (e.g. clusters provisioned by Tanzu Mission Control).

I have intentionally left out doing this for PipelineRuns as there are features coming in around allowing specifying taskRunSpecs for PipelineRuns that could change best approaches, but I feel that having examples of Tasks/TaskRuns should be enough to illustrate the concept. Let me know if adding this info to PipelineRuns would be helpful, and I can do so.

Submitter Checklist

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add documentation and example on running step containers as non root

@tekton-robot tekton-robot requested review from dibyom and a user April 30, 2020 15:26
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 30, 2020
- --destination=$(outputs.resources.builtImage.url)
- --context=$(inputs.params.pathToContext)
- --oci-layout-path=$(inputs.resources.builtImage.path)
securityContext:
Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary as will run as root by default

Copy link
Member

Choose a reason for hiding this comment

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

@danielhelfand this is not entirely true 😝 It depends on the kubernetes configuration. For example, we need this in OpenShift to make sure it knows it wants to run as root.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah. Goes back downstream. My fault. Will add back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added back

podTemplate:
securityContext:
runAsNonRoot: true
runAsUser: 1001
Copy link

Choose a reason for hiding this comment

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

Maybe update the last sentence of the paragraph describing this example from and executes it as a non-root user. to and executes it as user 1001.?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

docs/taskruns.md Outdated
user to run as:

```yaml
---
Copy link

Choose a reason for hiding this comment

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

v minor nit: not sure this is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not. Copy and paste from example gone bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

```

In the example above, the step `show-user-2000` specifies via a `securityContext` that the container
for the step should run as user 2000. A `securityContext` must still be specified via a TaskRun `podTemplate`
Copy link

Choose a reason for hiding this comment

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

I'm wary of redocumenting things that kubernetes does but do you think it would it be worth calling out runAsNonRoot for what it does? It is suuuuper confusing that it looks so similar to runAsUser but only validates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I agree about it being confusing. I can tweak the language to highlight this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2020
@danielhelfand danielhelfand force-pushed the security_context_docs branch from f80fbb9 to 62a2a1e Compare April 30, 2020 18:54

The `runAsNonRoot` property specified via the `podTemplate` above validates that steps part of this TaskRun are
running as non root users and will fail to start any step container that attempts to run as root. Only specifying
`runAsNonRoot: true` will not actually run containers as non root as the property simply validates that steps are not
Copy link

Choose a reason for hiding this comment

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

👍

@ghost
Copy link

ghost commented May 1, 2020

Thanks for documenting this!

/lgtm

@tekton-robot tekton-robot assigned ghost May 1, 2020
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2020
@tekton-robot tekton-robot merged commit 2cb262a into tektoncd:master May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task fails to run when using runAsNonRoot and runAsUser
3 participants