TEP-0082 Workspace Hinting #510

ghost · 2021-09-03T16:42:57Z

Workspaces are a general-purpose way of declaring portions of a Task's
filesystem to be supplied at runtime. Their general-purpose nature means
that automated systems aren't able to attach meaning to them. A Workspace
is "just a Workspace" to a machine whereas humans can use the "description" field
of a Workspace to figure out if the intented use is a credential, a shared
data volume, etc.

This TEP proposes adding hints to workspace declarations in both Tasks and
Pipelines that allow a system to assign basic meaning (and therefore some
sane default bindings) to workspaces.

vdemeester · 2021-09-03T18:08:00Z

/assign

vdemeester · 2021-09-03T18:29:00Z

/kind tep

bobcatfish · 2021-09-03T19:56:54Z

/assign

ScrapCodes · 2021-09-07T05:52:55Z

teps/0082-workspace-hinting.md

+- name: deploy-key
+  readOnly: true
+  optional: true
+  profile: credential


Does readOnly overrides the profile defaults provided by profile credential ?

readOnly: true is added to the volumeMount in the step's container (this is how it works today with workspaces). I think that would remain the same after this feature is implemented too.

There aren't any defaults attached to this "credential" profile tho - it's just a string hint to the system creating the PipelineRuns / TaskRuns.

Thanks Scott, I was confused by :

Each of these profiles can have some fairly reasonable assumptions attached to them. A "credential" Workspace likely should be populated from a Secret or Secret-like volume, for example. A "cache" Workspace might most often be supplied with a long-lived read-only Persistent Volume. A "data" Workspace might be safely assumed to require an ephemeral Persistent Volume that lives only as long as the PipelineRun.

Could this be phrased differently or edited to make it clearer? I do mention directly after this that Most importantly: these specific decisions should be left up to the platform but maybe there's a better way to communicate the point?

It might be due to my own limitation. I was also wondering to myself, is this hinting mechanism sufficient to address the said use case i.e.

The Tekton Workflows project is currently exploring ways to pass Secrets from a high-level Workflow description into a PipelineRun. This is made considerably more difficult because Pipelines can't indicate which of their Workspaces might be the right one to bind those Secrets to.

e.g. a Pipeline may need multiple secrets from different workspaces, and indicating all of them are ~~secret~~ credential profile - will it be sufficient information to bind?

Forgive me, if this question is as rookie as the previous.

You nailed it: that's the biggest drawback I think. I described it like this in the "Cons" section below:

Given a Task that optionally accepts both an "ssh-key" and "basic-auth" Workspace marked with the "credential" profile, would an automated system still be able to infer a correct mapping?

I'm going to rewrite this section to clear things up, I reckon it would read better with some edits.

Rewritten it, hopefully a bit clearer. Lemme know what you think.

Looks good !! 😄

afrittoli · 2021-09-13T16:15:54Z

/assign @afrittoli

bobcatfish

I've included a few question but generally makes lots of sense to me and nice coverage of alternatives 👍 👍 👍

I feel the explicit hinting meets the user requirements a bit better than the defaulting option; however I like that the default option gives us like 80% of what we need, plus other features, and doesn't preclude adding the explicit hinting (tho i have a feeling if we go that route we'll eventually add both)

/approve

bobcatfish · 2021-09-15T17:44:34Z

teps/0082-workspace-hinting.md

+
+### Non-Goals
+
+- Adding constraint-checking or any other logic to Pipelines to validate bound Workspaces based on workspace hints.


I'm curious why not - even if it wasn't in the initial implementation maybe this would be a reasonable follow on? curious if you excluded this just to keep the scope down or if there's another motivation that it might be interesting to elaborate on

maybe it's something like: even if the pipeline author hints that a volume claim could be used, the pipeline might still function correctly with other types and we don't want to arbitrarily prevent that?

Yeah you've hit the nail on the head with both your comments here.

If we expand scope to include validation then we'll end up burning a lot of cycles designing the shape of that, how it works, and how configurable it is. I allude to this in the "Questions around future extensions stemming from this change are quite nuanced" bit later on but I'll bring some of that same sentiment up here to support this Non-Goal. I'll also add this as possible future work as well.

bobcatfish · 2021-09-15T17:45:05Z

teps/0082-workspace-hinting.md

+
+### Use Cases (optional)
+
+The Tekton Workflows project is currently exploring ways to pass Secrets from a high-level Workflow description into a PipelineRun. This is made considerably more difficult because Pipelines can't indicate which of their Workspaces might be the right one to bind those Secrets to.


is there anything we can link to around this? even a discussion the working group notes maybe

I'll see if I can find something in our WG notes and link it in here.

bobcatfish · 2021-09-15T17:45:43Z

teps/0082-workspace-hinting.md

+
+## Requirements
+
+- Hinting must be optional: we don't want to suddenly invalidate every Task or Pipeline that currently includes a Workspace.


kind of a tangent, but maybe this is something we'd eventually require in catalog workspaces 🤔 + @jerop

bobcatfish · 2021-09-15T17:46:42Z

teps/0082-workspace-hinting.md

+    default:
+      secret:
+        secretName: my-docker-json
+```


hmm interesting option - my default thought is I don't like encoding this runtime info here, but if it's just a default, maybe that's a nice balance?

this also feels like it might be solving a slightly different use case/requirements 🤔 🤔 🤔 but maybe this is a two birds with one stone situation? it sounds like the implication is that this default provides the "hinting" needed for an outside tool to know what kind of workspace the author intends to be used here 🤔

Exactly right on the hinting: it's an unspoken assumption here that the outside tool can use defaults to inform how it builds a PipelineRun / TaskRun. Two birds with one stone is spot on.

Another con here is that it embeds the k8s type directly in the workspace declaration (and therefore this bit of our API spec) but 🤷 I think the horse might have left the stable on that issue. We embed the volume types in workspace bindings already.

Another con here is that it embeds the k8s type directly in the workspace declaration (and therefore this bit of our API spec) but 🤷 I think the horse might have left the stable on that issue. We embed the volume types in workspace bindings already.

My hope here is that we could define our conformance surface such that the actual workspace binding values are not included - in the current spec, only emptyDir support is required and the result are optional: https://github.com/tektoncd/pipeline/blob/main/docs/api-spec.md#workspacebinding

I think it's ok to hint that a workspace that is meant to provide credentials, the source of this credentials is up to the consumer of the task - it could be a secret or it could be a PVC provisioned with creds extracted from a vault of some kind.

This is already mentioned later in the TEP :)

bobcatfish · 2021-09-15T17:50:34Z

teps/0082-workspace-hinting.md

+
+#### Pros:
+- Very explicit about the Task's expectations for the content provided.
+- Pipelines can override a Tasks' expected types - so, for example, a Task might expect a ConfigMap but a Pipeline might override with a PV instead.


maybe another pro: adding defaults wouldn't preclude us from adding more explicit hinting later - i.e. there are benefits to providing this functionality in general (e.g. a pipeline that just works out of the box without having to deal with workspaces), AND if this isn't enough we could still add explicit hints (and/or even enforcement) later without needing to undo any of this?

I'll add this, thanks.

bobcatfish · 2021-09-15T17:51:10Z

teps/0082-workspace-hinting.md

+- Not just an API change - there will be some logic involved here on the Pipelines controller
+  side to apply the default workspace config to runs.
+- Questions around future extensions stemming from this change are quite nuanced:
+  - What if an author wants the "default" to actually be a requirement and for the TaskRun to fail if it's missing? For example a deploy Task requiring a Secret with name "cluster-key" to exist in the TaskRun's namespace.


ooo good point

bobcatfish · 2021-09-15T17:52:52Z

teps/0082-workspace-hinting.md

+
+#### Cons
+
+- Syntactically different from the "profiles" alternative but otherwise not functionally all that different.


also i feel like this sets an interesting (imo bad?) precedent for embedding information in descriptions - i guess we kind of do something like this with labels but i feel like at least that's the purpose of labels

Good point, I'll include this too.

ScrapCodes · 2021-09-17T05:46:22Z

teps/0082-workspace-hinting.md

+to the external / platform. Our own Workflows project may be able to
+utilize these profiles, for example, to make informed choices when
+creating a `PipelineRun` based solely on `Pipeline` YAML, supplied list
+of volumes and set of Secret references.


bobcatfish · 2021-10-07T17:04:31Z

looks like this is nearly ready to go but I think we need a review from @afrittoli 🙏

afrittoli

Thanks for this!
We do already have API fields that are not directly used by Tekton (see "description") so it may be ok to add more?

Another couple of alternatives:

the external system could use loosely coupled metadata - an external json file or annotations added to the task that would add the required extra workspace metadata
syntactic alternatives to workspaces which come with special defaults - e.g. credentials are always readonly:

workspaces: 
- name: data
credentials:
- name: git
- name: shortlivedtoken
  readOnly: false

The example above could be used in a task clones a private git repo and that generates a short lived token and stores it for the next task to use.

This would be functionally similar to profiles, but it would require API changes to add new profiles.

/approve

afrittoli · 2021-10-18T12:45:21Z

teps/0082-workspace-hinting.md

+
+## Requirements
+
+- Hinting must be optional: we don't want to suddenly invalidate every


afrittoli · 2021-10-18T12:48:21Z

teps/0082-workspace-hinting.md

+
+- Adding constraint-checking or any other logic to Pipelines to validate
+  bound Workspaces based on workspace hints. The potential scope related
+  to a feature like this would be subtly massive. This TEP is trying to


We might decide in future that we want to have higher level abstractions in the Tekton API directly - if so, the new API could easily work on top of the hinting mechanism.

Thanks, added this extra detail!

afrittoli · 2021-10-18T12:55:35Z

teps/0082-workspace-hinting.md

+- If a Task has multiple "credential" Workspaces, a system still needs
+  to figure out how to bind them. This might be automatic (an internal
+  rule like "names must match") or it could require some UI to be
+  exposed asking the user what the correct mapping is.


Is this specific to this alternative? It seems to be a common issue or limitation.

Yeah I think you're right. Removed.

afrittoli · 2021-10-18T13:09:30Z

teps/0082-workspace-hinting.md

+- It's a bit unclear what the incentive for including `profiles` would
+  be for Catalog Task authors. How would they "figure out" the purpose
+  and correct values to put in here?


I think this is the main issue.
Since the consumer is external, the list valid values would be unknown.
If Tekton defines a list of valid values, it may not be useful to external systems.
It would be easier if those values had some meaning for Tekton pipelines too - e.g. we could consider "credentials" workspaces to be read-only by default. We could have checks to prevent the content of a "credentials" workspace ending up on the taskrun log.

tekton-robot · 2021-10-18T13:34:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, vdemeester

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~~ [afrittoli,bobcatfish,vdemeester]

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

vdemeester · 2021-10-25T15:07:47Z

Note this has been approved by 3 owners from different companies, so @sbwsg we can merge as is and iterate over it or wait for some update here, as you prefer 😉

bobcatfish · 2021-10-25T16:12:39Z

/lgtm

up to you if you want to merge as-is or make any last updates first @sbwsg :

/hold

Workspaces are a general-purpose way of declaring portions of a Task's filesystem to be supplied at runtime. Their general-purpose nature means that automated systems aren't able to attach meaning to them. A Workspace is "just a Workspace" to a machine whereas humans can use the "description" field of a Workspace to figure out if the intented use is a credential, a shared data volume, etc. This TEP proposes adding hints to workspace declarations in both Tasks and Pipelines that allow a system to assign basic meaning (and therefore some sane default bindings) to workspaces.

ghost · 2021-10-26T19:56:35Z

Thanks for the reviews, I've added the new alternatives @afrittoli proposed. Will need one more lgtm with the pushed changes.

/hold cancel

bobcatfish · 2021-10-27T18:02:25Z

Thanks @sbwsg !

/lgtm

tekton-robot requested review from ncskier and sm43 September 3, 2021 16:42

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 3, 2021

tekton-robot assigned vdemeester Sep 3, 2021

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 3, 2021

tekton-robot assigned bobcatfish Sep 3, 2021

ScrapCodes reviewed Sep 7, 2021

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2021

tekton-robot assigned afrittoli Sep 13, 2021

bobcatfish mentioned this pull request Sep 15, 2021

Add support for optional volumes in Tasks tektoncd/pipeline#4083

Closed

bobcatfish reviewed Sep 15, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2021

ScrapCodes reviewed Sep 17, 2021

View reviewed changes

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021

vdemeester approved these changes Sep 21, 2021

View reviewed changes

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2021

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 6, 2021

bobcatfish mentioned this pull request Oct 12, 2021

Default workspace types for PipelineRuns and TaskRuns tektoncd/pipeline#2595

Open

afrittoli reviewed Oct 18, 2021

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2021

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

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2021

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

tekton-robot merged commit 16b0c42 into tektoncd:main Oct 27, 2021


		### Non-Goals

		- Adding constraint-checking or any other logic to Pipelines to validate bound Workspaces based on workspace hints.


		### Use Cases (optional)

		The Tekton Workflows project is currently exploring ways to pass Secrets from a high-level Workflow description into a PipelineRun. This is made considerably more difficult because Pipelines can't indicate which of their Workspaces might be the right one to bind those Secrets to.


		## Requirements

		- Hinting must be optional: we don't want to suddenly invalidate every Task or Pipeline that currently includes a Workspace.


		#### Cons

		- Syntactically different from the "profiles" alternative but otherwise not functionally all that different.

TEP-0082 Workspace Hinting #510

TEP-0082 Workspace Hinting #510

Conversation

ghost commented Sep 3, 2021

vdemeester commented Sep 3, 2021

vdemeester commented Sep 3, 2021

bobcatfish commented Sep 3, 2021

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

ScrapCodes Sep 14, 2021 • edited 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

afrittoli commented Sep 13, 2021

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

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

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

bobcatfish commented Oct 7, 2021

afrittoli 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Oct 18, 2021

vdemeester commented Oct 25, 2021

bobcatfish commented Oct 25, 2021

ghost commented Oct 26, 2021

bobcatfish commented Oct 27, 2021

ScrapCodes Sep 14, 2021 •

edited

Loading