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.
tektoncd · Jan 12, 2021 · d869831 · d869831
1 parent 0c55782
commit d869831
Show file tree

Hide file tree

Showing 2 changed files with 337 additions and 9 deletions.
diff --git 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 <code>workspaces</code> to <code>Steps</code>](#add--to-)
+  - [Add <code>workspaces</code> to <code>Sidecars</code>](#add--to--1)
+  - [Allow <code>workspaces</code> 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 &amp; Migration Strategy (optional)](#upgrade--migration-strategy-optional)
+- [References (optional)](#references-optional)
 <!-- /toc -->
 
 ## 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
@@ -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 |