From d869831bfafa23e0c9ef3b782344473f86a5576f Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 13 Nov 2020 10:21:09 -0500 Subject: [PATCH] TEP-0029 Propose technical design for Step & Sidecar Workspaces In f22828 we introduced a new TEP describing two problems: first, Sidecars don't have access to Workspaces. Second: Workspaces are globally available to all Steps all of the time. For sensitive data this is less than ideal - preferably a Task should be able to declare precisely which Steps or Sidecars are allowed to access a given Workspace. This commit adds a technical design to TEP-0029 that shows how we can add both Workspace support to Sidecars as well as a mechanism for isolating Workspaces to specific Steps/Sidecars in a Task. --- teps/0029-step-workspaces.md | 344 ++++++++++++++++++++++++++++++++++- teps/README.md | 2 +- 2 files changed, 337 insertions(+), 9 deletions(-) diff --git a/teps/0029-step-workspaces.md b/teps/0029-step-workspaces.md index 5b7204dc6..75bb3b8b1 100644 --- a/teps/0029-step-workspaces.md +++ b/teps/0029-step-workspaces.md @@ -4,7 +4,7 @@ authors: - "@sbwsg" creation-date: 2020-10-02 last-updated: 2020-10-02 -status: proposed +status: implementable --- # TEP-0029: Step and Sidecar Workspaces @@ -20,25 +20,38 @@ tags, and then generate with `hack/update-toc.sh`. - [Motivation](#motivation) - [Goals](#goals) - [Requirements](#requirements) +- [Proposal](#proposal) + - [Add workspaces to Steps](#add--to-) + - [Add workspaces to Sidecars](#add--to--1) + - [Allow workspaces in `Steps` and `Sidecars` to have their own `mountPath`](#allow-workspaces-in-steps-and-sidecars-to-have-their-own-mountpath) - [User Stories](#user-stories) - [Story 1](#story-1) - [Story 2](#story-2) - [Story 3](#story-3) - [Story 4](#story-4) - [Story 5](#story-5) +- [Design Details](#design-details) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) + - [Use an explicit volumeMount instead](#use-an-explicit-volumemount-instead) + - [Advantages](#advantages) + - [Drawbacks](#drawbacks-1) + - [Specify complete Workspace declarations in Steps](#specify-complete-workspace-declarations-in-steps) + - [Advantages](#advantages-1) + - [Disadvantages](#disadvantages) +- [Upgrade & Migration Strategy (optional)](#upgrade--migration-strategy-optional) +- [References (optional)](#references-optional) ## Summary ## Motivation -This TEP is motivated by 3 major goals: First, to allow a Task to explicitly declare the -Steps and Sidecars that have access to workspaces so that sensitive data can be isolated -to only the containers that need it. Second, to add blessed support for Sidecars to access -Workspaces without using a "hack" requiring the Task author to wire a Workspace's `volume` -name to a `volumeMount` in the pod spec. Third, to make the behaviour of Workspaces uniform -across Steps and Sidecars so that understanding the behaviour of one eases understanding -the behaviour of the other. +This TEP is motivated by 3 major goals: + +1. Limit access to sensitive credentials. +2. Add blessed support for Sidecars to access workspaces. +3. Make Workspace behaviour uniform across Steps and Sidecars. ### Goals @@ -56,6 +69,127 @@ that has access to those contents as well. - A Task author can use a Workspace from a `Sidecar`. - A Task author can still use the volume "hack" to attach `Workspaces` to `Sidecars` in combination with the feature proposed here. +- A Task author can choose different `mountPaths` for each Step that receives the +`Workspace`. + +## Proposal + +Add `workspaces` lists to Steps and Sidecars. Here's the existing YAML and behaviour: + +```yaml +spec: + workspaces: + - name: foo + steps: + - image: ubuntu + script: # ... This Step automatically mounts "foo" workspace. + - image: ubuntu + script: # ... And so does this Step. +``` + +And here's a Task using the new Step Workspaces feature: + +```yaml +spec: + workspaces: + - name: foo + steps: + - image: ubuntu + workspaces: + - name: foo + script: # ... This Step mounts "foo" workspace. + - image: ubuntu + script: # ... But this Step does not. +``` + +The existing YAML and behaviour will continue to be supported. + +### Add `workspaces` to `Steps` + +1. Add a `workspaces` list to `Steps`. +2. Allow `workspaces` from the `Task` to be explicitly named in that list like this: + + ```yaml + workspaces: + - name: my-sensitive-workspace + steps: + - name: foo + workspaces: + - name: my-sensitive-workspace + ``` +3. When a `workspace` is listed in a Step, it is no longer automatically mounted - either to +`Steps` or `Sidecars` - unless they also have the `workspace` in their own `workspaces` list. + +Example YAML: + +```yaml +# task spec +spec: + workspaces: + - name: git-ssh-credentials + mountPath: /root/.ssh + steps: + - name: clone-repo + image: alpine/git:v2.26.2 + workspaces: + - name: git-ssh-credentials + script: | + git clone $(params.repo-url) /workspace/source + - name: run-unit-tests + script: | + cd /workspace/source + go test ./... +``` + +In the above example only the `clone-repo` `Step` will receive access to the `git-ssh-credentials` +`Workspace`. The `run-unit-tests` `Step` will not receive access to the Workspace volume. Importantly +this also means that the user-supplied code does not have access to the credential files. Compromising +the code for the unit tests does not also compromise the SSH credentials. + +### Add `workspaces` to `Sidecars` + +1. Automatically mount `workspaces` to `Sidecars` just as they're automatically mounted to `Steps` today. +2. Add a `workspaces` list to `Sidecars`. +3. Allow `workspaces` from the `Task` to be explicitly named in that list like this: + + ```yaml + workspaces: + - name: my-workspace + sidecars: + - name: watch-workspace + workspaces: + - name: my-workspace + ``` + +4. When a `workspace` is listed in a Sidecar, it is no longer automatically mounted - either to +`Steps` or `Sidecars` - unless they also have the `workspace` in their own `workspaces` list. + +### Allow `workspaces` in `Steps` and `Sidecars` to have their own `mountPath` + +When declaring the Workspace in the Step or Sidecar a custom mountPath can also be specified. +This allows for situations where different images may have different expectations for the location +of files from a workspace. This mountPath overrides whatever mountPath is set on the Task Spec's +`workspaces` entry. + +```yaml +# Task Spec +workspaces: +- name: ws + mountPath: /workspaces/ws +steps: +- name: edit-files-1 + workspaces: + - name: foo + mountPath: /foo # overrides mountPath +- name: edit-files-2 + workspaces: + - name: foo # no mountPath specified so will use /workspaces/ws +sidecars: +- name: watch-files-on-workspace + workspaces: + - name: foo + mountPath: /files # overrides mountPath +``` ### User Stories @@ -101,3 +235,197 @@ to other `Steps` in the same Task performing ancillary work. As a Pipeline author I want to be able to quickly audit that the `git-fetch` Task I am using in my Pipeline is only exposing the git SSH key for my team's source repo in the single `Step` that performs `git clone`, and not to any `Steps` in the same Task performing ancillary work. + +## Design Details + +Add a new `WorkspaceUsage` struct to the `task_types.go` file: + +```go +// WorkspaceUsage declares that a Step or Sidecar utilizes a Task's Workspace. +type WorkspaceUsage struct { + // Name is the name of the Task's WorkspaceDeclaration that this Step or Sidecar is utilizing. It is required. + Name string `json:"name"` + // MountPath is an optional path that overrides where the Workspace will be mounted in the Task's filesystem. + MountPath string `json:"mountPath"` +} +``` + +Update the `Step` struct to include a slice of `WorkspaceUsage`: + +```go +type Step struct { + corev1.Container `json:",inline"` + Script string `json:"script,omitempty"` + + // Workspaces is a list of workspaces that this Step declares it will use in some way. The presence + // of a Workspace in this list prevents other Steps from automatically receiving the Workspace - + // they must also explicitly opt-in to receiving it as well in their own workspaces list. + Workspaces []WorkspaceUsage `json:"workspaces,omitempty"` +} +``` + +Update the `Sidecar` struct to include a slice of `WorkspaceUsage`: + +```go +// Sidecar embeds the Container type, which allows it to include fields not +// provided by Container. +type Sidecar struct { + corev1.Container `json:",inline"` + Script string `json:"script,omitempty"` + + // Workspaces is a list of workspaces that this Sidecar declares it will use in some way. + Workspaces []WorkspaceUsage `json:"workspaces,omitempty"` +} +``` + +## Drawbacks + +- If we were to pursue the idea of a shareable `Step` type then we would need to find some way to map from +the workspaces a Task declares into those that a referenced `Step` declares. We'd similarly need to do this +for params and results. +- It takes the `Step` and `Sidecar` types further from being "pure" k8s Container. However we've already +made moves in this direction by introducing our own `Script` field to `Steps` and `Sidecars`. +- Automounting workspaces into sidecars could have unexpected side effects. If a user is already mounting +a workspace to a specific location for their `Steps` but are relying on the fact that the sidecar does _not_ +mount to the same spot then they could potentially be broken by this change. + +## Alternatives + +### Use an explicit volumeMount instead + +Instead of adding a `workspaces` list to `Steps`, we could instead lean on the existing +`volumeMounts` list like this: + +```yaml +# task spec +spec: + workspaces: + - name: git-ssh-credentials + steps: + - name: clone-repo + image: alpine/git:v2.26.2 + volumeMounts: + - name: $(workspaces.git-ssh-credentials.volume) + script: | + git clone $(params.repo-url) /workspace/source + - name: run-unit-tests + script: | + cd /workspace/source + go test ./... +``` + +And then add a rule that says "if you use a volumeMount to mount a workspace to your Step +then Tekton will not automatically add volumeMounts to any other Steps." We would also need +to document this rule. + +#### Advantages + +Uses an existing Kubernetes container field that the user may already be familiar with. + +#### Drawbacks + +It's surprising. This approach overloads the meaning of `volumeMounts` with +additional Tekton-specific context and behaviour (i.e. the additional constraint +that adding a workspace volume to a `volumeMount` entry will prevent other Steps +from receiving that `workspace` unless they too include it in their +`volumeMounts` list). + +A further drawback is that `workspaces` provide a useful abstraction for API implementers. +Leaning on `volumeMounts` would move a Kubernetes-specific field into Tekton's API. We'd have +to decide which of the fields at https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#volumemount-v1-core +to support. + +### Specify complete Workspace declarations in Steps + +Allow `Steps` to fully-specify Workspace declarations so that they're not also required +at the top-level of the Task spec. Here's how that might look: + +```yaml +steps: +- name: foo + workspaces: + - name: my-sensitive-workspace +``` + +In contrast with the existing proposal: + +```yaml +workspaces: +- name: my-sensitive-workspace +steps: +- name: foo + workspaces: + - name: my-sensitive-workspace +``` + +#### Advantages + +- Shorter syntax for isolating `Workspaces` to single `Steps`. +- Allows for immediate use of fields like `mountPath` as part of the `Workspace` entry in the `Step`. + +#### Disadvantages + +- What's the behaviour when a `Task` declares a `Workspace` and `Steps` declare a `Workspace` with the +same name? Possibly the same behaviour as is being proposed by this document? Or a validation error? + +- Open question whether a Task author would be able to share a `Workspace` this way across +multiple `Steps` if they share a common `Workspace` name: + + ```yaml + steps: + - name: foo + workspaces: + - name: my-sensitive-workspace + - name: bar + workspaces: + - name: my-sensitive-workspace + ``` + + Here the `Task` would presumably only expose a single `Workspace` named "my-sensitive-workspace" + to be populated by a `TaskRun` / `PipelineRun`? + + Validation might be made more difficult here - if two `Workspaces` in two `Steps` are named + almost the same thing the reconciler won't be able to tell if they're "supposed" to be the same or not. + Consider the following example: + + ```yaml + steps: + - name: foo + workspaces: + - name: my-sensitive-workspace + - name: bar + workspaces: + - name: my-senistive-workspace + ``` + + Was this a mis-spelling by the `Task` author or intentionally separate `Workspace` declarations? + + One further extension to this idea would be to allow `TaskRuns` or `PipelineRuns` to bind `Workspaces` + to specific `Steps` and `Sidecars`: + + ```yaml + workspaces: + - name: my-workspace + emptyDir: {} + - name: sensitive-workspace + secretRef: + name: foo-secret + sidecars: + - name: do-something-not-secret + workspaces: + - name: my-workspace + steps: + - name: do-something-secret + workspaces: + - name: some-sensitive-workspace + ``` + +## Upgrade & Migration Strategy (optional) + +The feature as proposed is entirely backwards-compatible. Omitting the +`workspaces` field from `Steps` leaves the existing behaviour exactly as +it works today - all Steps will receive all workspaces. + +## References (optional) + +- Original design part of the [Credentials UX](https://github.com/tektoncd/pipeline/issues/2343#issuecomment-611155667) issue. diff --git a/teps/README.md b/teps/README.md index 1027750c4..43d1e713a 100644 --- a/teps/README.md +++ b/teps/README.md @@ -141,7 +141,7 @@ This is the complete list of Tekton teps: |[TEP-0026](0026-interceptor-plugins.md) | interceptor-plugins | implementable | 2020-10-08 | |[TEP-0027](0027-https-connection-to-triggers-eventlistener.md) | HTTPS Connection to Triggers EventListener | implementable | 2020-11-01 | |[TEP-0028](0028-task-execution-status-at-runtime.md) | task-exec-status-at-runtime | implementable | 2020-11-02 | -|[TEP-0029](0029-step-workspaces.md) | step-and-sidecar-workspaces | proposed | 2020-10-02 | +|[TEP-0029](0029-step-workspaces.md) | step-and-sidecar-workspaces | implementable | 2020-10-02 | |[TEP-0030](0030-workspace-paths.md) | workspace-paths | proposed | 2020-10-18 | |[TEP-0031](0031-tekton-bundles-cli.md) | tekton-bundles-cli | proposed | 2020-11-18 | |[TEP-0032](0032-tekton-notifications.md) | Tekton Notifications | proposed | 2020-11-18 |