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

ghost · 2020-11-13T15:31:20Z

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.

ghost · 2020-11-16T17:07:08Z

/kind tep

afrittoli · 2020-11-23T10:54:46Z

/test pull-community-teps-lint

ghost · 2020-11-30T13:44:40Z

/test pull-community-teps-lint

bobcatfish

I really like it! i brainstormed a few more drawbacks but 👍 from me to the overall proposal

/approve

I think at this point it might make sense to include the change to status: implementable in this PR also?

bobcatfish · 2020-11-30T15:19:58Z

teps/0029-step-workspaces.md

+## 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.


good point! and i think we'd run into that for params (and maybe results??) too

bobcatfish · 2020-11-30T15:21:03Z

teps/0029-step-workspaces.md

+- 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.
+- 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`.


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)

bobcatfish · 2020-11-30T15:23:41Z

teps/0029-step-workspaces.md

+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).


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

tekton-robot · 2020-11-30T15:26:46Z

[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

ghost · 2020-11-30T17:17:18Z

Updated status to implementable and updated the TOC.

gabemontero

a late coming comment and possible ask for clarification @sbwsg

please apply non-SME type of disclaimers to my question :-)

gabemontero · 2021-01-11T17:31:36Z

teps/0029-step-workspaces.md

+      workspaces:
+      - name: my-sensitive-workspace
+    ```
+3. When a `workspace` is listed in a Step, it is no longer automatically mounted - either to


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

@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: # ...

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

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

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

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.

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

gabemontero · 2021-01-11T17:33:08Z

teps/0029-step-workspaces.md

+      - name: my-workspace
+    ```
+
+4. When a `workspace` is listed in a Sidecar, it is no longer automatically mounted - either to


same note as above on the no longer automatically mounted text

vdemeester

One question on mountPath in steps, otherwise looks good to me 👍🏼

teps/0029-step-workspaces.md

vdemeester

/lgtm

teps/0029-step-workspaces.md

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.

vdemeester

/lgtm

tekton-robot requested review from iancoffey and mnuttall November 13, 2020 15:31

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 13, 2020

ghost changed the title ~~TEP-0029 Propose technical design for Step & Sidecar Workspaces~~ TEP-0029 Technical design for Step & Sidecar Workspaces Nov 13, 2020

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Nov 16, 2020

bobcatfish reviewed Nov 30, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2020

ghost mentioned this pull request Dec 4, 2020

TEP 0030 Workspace Paths Technical Proposal #285

Closed

gabemontero reviewed Jan 11, 2021

View reviewed changes

vdemeester reviewed Jan 12, 2021

View reviewed changes

teps/0029-step-workspaces.md Show resolved Hide resolved

teps/0029-step-workspaces.md Show resolved Hide resolved

vdemeester reviewed Jan 12, 2021

View reviewed changes

tekton-robot assigned vdemeester Jan 12, 2021

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 12, 2021

vdemeester reviewed Jan 12, 2021

View reviewed changes

teps/0029-step-workspaces.md Show resolved Hide resolved

vdemeester reviewed Jan 12, 2021

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2021

tekton-robot merged commit d869831 into tektoncd:master Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Nov 13, 2020

ghost commented Nov 16, 2020

afrittoli commented Nov 23, 2020

ghost commented Nov 30, 2020

bobcatfish left a comment

bobcatfish Nov 30, 2020

bobcatfish Nov 30, 2020

bobcatfish Nov 30, 2020

tekton-robot commented Nov 30, 2020

ghost commented Nov 30, 2020

gabemontero left a comment

gabemontero Jan 11, 2021

ghost Jan 11, 2021 •

edited by ghost

Loading

ghost Jan 11, 2021

gabemontero Jan 11, 2021

ghost Jan 11, 2021

gabemontero Jan 11, 2021

gabemontero Jan 12, 2021

gabemontero Jan 11, 2021

vdemeester left a comment

vdemeester left a comment

vdemeester left a comment

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

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

Conversation

ghost commented Nov 13, 2020

ghost commented Nov 16, 2020

afrittoli commented Nov 23, 2020

ghost commented Nov 30, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Nov 30, 2020

ghost commented Nov 30, 2020

gabemontero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 11, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

ghost Jan 11, 2021 •

edited by ghost

Loading