Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TEP-0029 Technical design for Step & Sidecar Workspaces #260

Merged
merged 1 commit into from Jan 12, 2021
Merged

TEP-0029 Technical design for Step & Sidecar Workspaces #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
344 changes: 336 additions & 8 deletions teps/0029-step-workspaces.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
vdemeester marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

apologies in advance for coming late to this, and possibly re-introducing prior discussion, as well as coming in without hands on experience with sidecars

but the is no longer automatically mounted text implies a change in behaviour and "breaking API change" of sorts

is my non-SME interpretation correct? if so, should this TEP be explicit in stating "a breaking API change is being suggested to facilitate X/Y/Z" ?

some of the referenced design docs may claim that more decisively (apologies, I did not search them for such), but I would contend that detail of such ilk should be surfaced in the TEP.

conversely, if this is not a breaking API sort of change, perhaps some clarification somewhere in the TEP as to why that is the case would help those coming in later on to the discussion

WDYT @sbwsg ?

thanks

Copy link
Author

@ghost ghost Jan 11, 2021 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

@gabemontero thanks for reviewing!

"is no longer automatically mounted" -> Sorry for the confusion. It's definitely not a breaking API change because we haven't had "Step-level" workspaces before this. So it's a new behaviour that the user opts in to by adding the workspace to a Step. They couldn't do this before - add a workspace to a Step - and by doing so they're effectively saying "I only want this Step to have access to this workspace."

Here's what a Task with workspaces currently looks like:

spec:
  workspaces:
  - name: foo # All Steps get this workspace (they're automatically mounted to all Steps)
  steps:
  - image: ubuntu
    script: # ...

and here's the new thing we're adding:

spec:
  workspaces:
  - name: foo
  steps:
  - image: ubuntu
    workspaces:
    - name: foo # Only this Step gets this workspace (they're no longer automatically mounted to all Steps)
    script: # ...

Copy link
Author

Choose a reason for hiding this comment

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

I'll take another breeze through this doc and try to clarify that language.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah OK good to know @sbwsg

the before / after yaml's together like that are concise and make the net change clear ... adding the before yaml right before the existing after yaml and changing that text would benefit me at least

thanks

Copy link
Author

Choose a reason for hiding this comment

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

Added an explicit before/after example at the top of the Proposal section. Cheers!

Copy link
Contributor

Choose a reason for hiding this comment

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

OK the new adding new API / behavior but preserving existing API / behavior verbiage makes it very clear now @sbwsg thanks!

LGTM

I'll circle back tomorrow to see if others have chimed in or our planning to chime in soon for the "multi-company" sign off, otherwise I can do it if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vdemeester says he is going to look at this today @sbwsg .... I'll defer to him on LGTM-ing

`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
vdemeester marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

same note as above on the no longer automatically mounted text

`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

Expand Down Expand Up @@ -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"`
vdemeester marked this conversation as resolved.
Show resolved Hide resolved
// 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

one more potential drawback/risk: automounting workspaces into sidecars could have some kinda unexpected side effect. seems pretty unlikely, but it's possible (e.g. if someone was using a sidecar that mounts something at the same path as a workspace?? pretty far fetched tho)

- 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

it also removes some of the abstraction that workspaces add - i like that (in my mind anyway) workspaces could be fulfilled by anything the underlying system chooses but by using volume mounts we're saying explicitly it's a mounted volume - and i think 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


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.
2 changes: 1 addition & 1 deletion teps/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down