TEP: Step and Sidecar Workspaces

[ghost]

Prior to this TEP access to Workspaces has been defined through Tasks, TaskRuns, Pipelines and PipelineRuns. There are known limitations with this:

• Sensitive data is exposed to every single Step in a Task instead of only those Steps that actually make use of the data. This is a similar problem to one we face with our credentials system. Ideally, access to sensitive data should be isolated to only the code that uses it.

• Sidecars do not have a "blessed" path to accessing a Workspace. Instead they must rely on a hack by adding the $(workspaces.<name>.volume) variable to the name field of a volumeMount.

The TEP in this commit proposes adding a workspaces field to Steps and Sidecars. The meaning of this field would be to provide access to that Workspace in either the Step or Sidecar. Explicitly using a Workspace in a Step would have an additional effect on other Steps: they would be prevented from accessing the Workspace unless they too were "opted-in" to the Workspace by adding it to their list of Workspaces.

An example use case for this in Steps is mounting some SSH credentials to perform a clone and then run tests against that code. If the clone happens in a separate Step from the testing then the SSH credentials only need to be exposed to the clone. The "untrusted" code in the unit tests does not need to run with access to the credentials workspace.

An example use case for this in Sidecars is using a Workspace to supply configuration for a mock API server that runs alongside Steps in a Sidecar and responds with preconfigured responses during integration tests.

[vdemeester]

Do we want to be able to have different mountPath per steps ? Imagine we have a workspace in a Task that we use in 2 steps (out of 6), but want/need this workspace to be mounted at different path ?

[ghost]

I could see utility in overriding the mountPath on a per-Step basis, yeah. I'll spend some time thinking of the use-cases it might be desirable for.

Do you think mountPath should also be a part of this TEP or tackled separately? If we decide to move ahead with #235 in some form then there may be other fields that are desirable per-Step as well.

[bobcatfish]

I think this looks great and generally am in favor!

Explicitly using a Workspace in a Step would have an additional effect on other Steps: they would be prevented from accessing the Workspace unless they too were "opted-in" to the Workspace by adding it to their list of Workspaces.

There's one thing about this that feels a little odd:

1. Including a workspace in a step excludes it from other steps
   BUT
2. Including a workspace in a sidecar... does not?

It feels like there is a mismatch between how workspaces are treated for steps vs sidecars. I think this comes down to the "automagic" that if you specify a workspace, it's available to all steps, but not to sidecars.

I added a couple of tweaks on the proposal that might address this, but im not sure either is better than the original proposal - maybe we just live with the automagic mismatch :)

[bobcatfish]

apologies for this TOTALY nitpick, but is it possible to call "WorkspaceBinding" something else here, or define it? just cuz you'd have to be familiar with the code to know what this is, I assume this is:

https://github.com/tektoncd/pipeline/blob/ae69af262f4601011481fe2e6cb28f08a4f16003/pkg/apis/pipeline/v1beta1/workspace_types.go#L50-L77

i.e. where you specify which workspace to actually use at runtime (https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#specifying-workspaces)?

I'm a bit confused about why these are non-goals - maybe it's because it seems like this proposal doesn't involve changing anything about the runtime configuration, if I understand correctly? (cuz at runtime you still specify workspaces in the same way you did before?)

[ghost]

I'm a bit confused about why these are non-goals - maybe it's because it seems like this proposal doesn't involve changing anything about the runtime configuration, if I understand correctly? (cuz at runtime you still specify workspaces in the same way you did before?)

Re: the first two non-goals, there was something about specifying Workspaces at the granularity of a Step/Sidecar that I guessed would make people assume it validates the workspace in some way. I don't quite know why my head went there but you're right that these don't totally fit with the rest of the proposal. I'll remove those two. For "Overriding configuration of a WorkspaceBinding" we are in a sense allowing the user to override the automagic configuration of a Workspace so I think I wanted to make sure it was clear that the only thing which is changing here is the isolation of the volume mount to specific Steps/Sidecars.

apologies for this TOTALY nitpick, but is it possible to call "WorkspaceBinding" something else here, or define it? just cuz you'd have to be familiar with the code to know what this is, I assume this is:

https://github.com/tektoncd/pipeline/blob/ae69af262f4601011481fe2e6cb28f08a4f16003/pkg/apis/pipeline/v1beta1/workspace_types.go#L50-L77

i.e. where you specify which workspace to actually use at runtime (https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#specifying-workspaces)?

For what it's worth we talk about Workspace Bindings a lot in the workspaces.md doc in the pipelines repo. It's useful to disambiguate between Workspace Declarations (in Tasks) and Workspace Bindings (TaskRuns / PipelineRuns). But I hear you on the use of WorkspaceBinding in its one-word struct-name format. I've made edits to clarify, lmk if that reads better.

[bobcatfish]

in the case where a specific step has opted into using a workspace, do we prevent other steps from "hacking" in access to the workspace via something like workspaces.<workspaceName>.volume?

(I'm guessing "no", just trying to think of other stuff we might want to cover here)

[ghost]

Good call, added Task author can still use the volume "hack" to attach Workspaces to Sidecars in combination with the feature proposed here as another Requirement.

[bobcatfish]

Another alternative which is just a syntax tweak is instead of specifying a top level workspace AND referencing it in the step like:

    workspaces:
    - name: my-sensitive-workspace
    steps:
    - name: foo
      workspaces:
      - name: my-sensitive-workspace

In the case where you want to limit access to the workspace to just a step, you could have the step contain all the info, i.e. the following would imply that the task needs a workspace my-sensitive-workspace

    steps:
    - name: foo
      workspaces:
      - name: my-sensitive-workspace

Downsides:

1. If you want multiple steps to use the same workspace (and other steps not to), you have to rely on the same volume being provided at runtime - or you allow this to be indicated by using the same workspace name, e.g.:

    steps:
    - name: foo
      workspaces:
      - name: my-sensitive-workspace
    - name: step-that-also-needs-sensitive-workspace
      workspaces:
      - name: my-sensitive-workspace

2. This is bit more confusing for the TaskRun / PipelineTask author in that in the above proposal, you know what workspaces you need to provide by looking at the list of workspaces; in this version you'd also need to look at the individual steps as well

Upsides:

1. Might help with @vdemeester 's point about different mount paths? i.e. if we moved the entire workspace definition into the step

[bobcatfish]

Possible way to work around downside (2) would be to introduce a "steps" stanza into PipelineTasks and TaskRuns, e.g. instead of:

    workspaces:
    - name: my-workspace
    sidecars:
    - name: watch-workspace
      workspaces:
      - name: my-workspace

have something like:

    workspaces:
    - name: my-workspace
    sidecars:
    - name: watch-workspace
      workspaces:
      - name: my-workspace
    steps:
    - name: sensitive-workspace
      workspaces:
      - name: some-sensitive-workspace

[ghost]

I've added this as another alternative. I think naming collisions introduce a source of ambiguity here. First, the one you documented wrt Steps declaring the same Workspace name. It does make intuitive sense to me that the Task would only expose a single Workspace declaration for TaskRuns / PipelineRuns to bind to, and that binding would be shared across the Steps that include it, so this isn't so much of a problem to my mind.

What about the following example:

workspaces:
- name: my-sensitive-workspace
steps:
- name: foo
  workspaces:
  - name: my-sensitive-workspace
- name: bar
  workspaces:
  - name: my-sensitive-workspace

Here ^ we've declared a workspace at both the Task level and individual Steps, all with the same name. Is this a validation error or does it follow the behavior of this proposal? Again, not a big deal, just another rule to decide on and code in.

Finally, another source of potential validation error, consider the following:

steps:
- name: foo
  workspaces:
  - name: my-sensitive-workspace
- name: bar
  workspaces:
  - name: my-senistive-workspace

Here, the validator won't be able to catch the mis-spelling of my-senistive-workspace. If they're pre-declared at the top of the Task spec we can catch that and reject on submission, tightening the author's write-apply-test loop. Again, not a major concern I don't think but worth bearing in mind.

Possible way to work around downside (2) would be to introduce a "steps" stanza into PipelineTasks and TaskRuns, e.g. instead of:
yaml workspaces: - name: my-workspace sidecars: - name: watch-workspace workspaces: - name: my-workspace

have something like:

```yaml
workspaces:
- name: my-workspace
sidecars:
- name: watch-workspace
  workspaces:
  - name: my-workspace
steps:
- name: sensitive-workspace
  workspaces:
  - name: some-sensitive-workspace
```

At the moment we don't expose either sidecars or steps for configuration in Runs and it feels to me like a change that doesn't serve a broad-enough purpose to warrant it. However, if there are good reasons to broaden the type changes this far I'm open to it.

[bobcatfish]

Here ^ we've declared a workspace at both the Task level and individual Steps, all with the same name. Is this a validation error or does it follow the behavior of this proposal? Again, not a big deal, just another rule to decide on and code in.

workspaces:
- name: my-sensitive-workspace
steps:
- name: foo
  workspaces:
  - name: my-sensitive-workspace
- name: bar
  workspaces:
  - name: my-sensitive-workspace

yeah that seems icky! I think this is a good point against this approach.

looking at that example (and the misspelling example) I like the version better where we declare the workspace in the workspaces list and reference it

[bobcatfish]

do we have a specific use case where someone wants to do this?

[ghost]

Ah, good catch! Reading back it looks like Stories 2, 3 and 4 are all very woolly 🐑 so I've rewritten to make them concrete 🏙️ .

[bobcatfish]

another possible alternative, to tackle the sidecar issue specifically, would be to also make workspaces available to sidecars by default

[ghost]

I mentioned this in another comment elsewhere but agreed that this could be a possible way forward but I'd prefer to have that convo in a new TEP rather than expand this one. Happy to do that though if preferable. For now I've added this as an explicit Non-Goal.

[bobcatfish]

Hmmm related question: why tackle these 2 features in the same TEP? Making workspaces available to sidecars and making steps be able to access them exclusively feel like different features to me - and maybe part of why I'm getting hung up about what feels to me like a mismatch in their functionality.

If we tackled them separately we could:

1. Have a TEP to add the ability to limit workspaces to only the steps that declare them
2. Then tackle making workspaces available to sidecars: we could decide if we want to mirror the step functionality, i.e. make workspaces automatically available to all sidecars, make it so that adding them explicitly to a sidecar excludes them from the rest; or not

Oh oh oh here is a question that I think might help explain my confusion about the "mismatch":

What if you want to add a workspace ONLY to a sidecar? e.g. the workspace contains something sensitive the sidecar needs, but you don't want to make it automatically available to all steps?

[ghost]

What if you want to add a workspace ONLY to a sidecar?

Great point and not something I'd considered at all.

OK, after our conversations here and online, a summary of where we're at:

1. Consider Steps and Sidecars as a single pool of containers
2. Sidecars automatically receive Workspaces, just as Steps do today
3. Adding a Workspace to a Step or Sidecar isolates it from all other containers in the pool
   • If you add workspace to a Step you disable automatic mounting to other Steps & Sidecars
   • If you add workspace to a Sidecar you prevent automatic mounting to other Sidecars & Steps

[ghost]

There's one thing about this that feels a little odd:

Including a workspace in a step excludes it from other steps
BUT
Including a workspace in a sidecar... does not?

Hm, I'm not sure I totally agree with this framing. Re-wording it a bit: Workspaces are currently excluded from all Sidecars. After this TEP, adding a Workspace to a Sidecar continues to exclude it from other Sidecars.

I think this comes down to the "automagic" that if you specify a workspace, it's available to all steps, but not to sidecars.

Totally agree. The key differentiator here is the automagical behaviour of Workspaces being mounted to all Steps. I think this TEP moves us in the right direction in terms of evening out that behaviour across Steps and Sidecars while also providing the credentials-specific benefit of isolating sensitive Workspaces to certain Steps when necessary.

The question of whether we one day remove the automagical behaviour in Steps or add it to Sidecars is a subject for another TEP (imo). The first (remove from Steps) would be backwards incompatible. The second (add to Sidecars) would be backwards compatible. But I lean heavily towards explicitness / opt-in behaviour of adding Workspaces to Steps and Sidecars rather than automatically wiring them in. I look at the automagic as a concession to UX that muddies the "API-ness" of the API.

[ghost]

Thanks for the feedback @bobcatfish ! Updated with new Goals, Requirements and proposal details.

--- a/teps/XXXX-step-workspaces.md
+++ b/teps/XXXX-step-workspaces.md
@@ -53,11 +53,11 @@ tags, and then generate with `hack/update-toc.sh`.
 - Provide a mechanism to limit the exposure of sensitive workspaces to only those Steps in a Task
 that actually require access to them.
 - Provide explicit access to Task workspaces from sidecars without using `volumeMounts`.
+- Normalize behaviour of Workspaces across Steps and Sidecars.

 ### Non-Goals

 - Overriding configuration of a Workspace Binding provided by a TaskRun/PipelineRun.
-- Adding `Workspaces` to `Sidecars` automatically, in the same way that `Steps` receive them today.

 ## Requirements

@@ -83,8 +83,8 @@ combination with the feature proposed here.
       workspaces:
       - name: my-sensitive-workspace
     ```
-3. A `workspace` named in a `Step` is not given to other `Steps` unless they too have the
-`workspace` in their own `workspaces` list.
+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:

@@ -114,8 +114,9 @@ the code for the unit tests does not also compromise the SSH credentials.

 ### Add `workspaces` to `Sidecars`

-1. Add a `workspaces` list to `Sidecars`.
-2. Allow `workspaces` from the `Task` to be explicitly named in that list like this:
+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:
@@ -126,7 +127,8 @@ the code for the unit tests does not also compromise the SSH credentials.
       - name: my-workspace
     ```

-The Sidecar `watch-workspace` will now have access to the `my-workspace` 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.

[ghost]

In response to yesterday's API Working Group call I've stripped back this proposal to just the problem statement. I've removed any detail of the proposed solution but kept the motivation, goals, non-goals, and desirable user stories.

[bobcatfish]

Looks good to me! I'm also ready put my rubber stamp on the design as well :D

/approve

@afrittoli @vdemeester @pritidesai whaddaya think?

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

• teps/OWNERS [bobcatfish]

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

[bobcatfish]

(once we get back to the design @sbwsg one thought: i think we might want to add a "risk" that sidecars previously did not have these workspaces mounted and now suddenly will - its possible (but i think unlikely?) that this could cause a problem for someone (e.g. if they were already using a path that the workspace is mounted at for something - seems pretty unlikely tho!)

[vdemeester]

/lgtm