Add Step and Sidecar Workspaces feature #3700

ghost · 2021-01-20T15:21:47Z

Changes

TEP: https://github.com/tektoncd/community/blob/master/teps/0029-step-workspaces.md
Fixes #2343

Access to Workspaces can now be limited to specific Steps and Sidecars in a Task.
This allows Task authors to isolate sensitive data to specific images, reducing
exposure of assets like credentials.

As a side-effect of this change Workspaces are now mounted to Sidecars by
default, just as they are to Steps.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

- New alpha feature added: "Step and Sidecar Workspaces"
    - Requires "enable-api-fields: alpha" feature gate.
    - Sidecars in a Task now receive Workspaces, just like Steps do.
    - Steps and Sidecars can now specify which Workspaces they want access to. This isolates the workspace to only those Steps or Sidecars that need them, allowing Task authors to limit exposure of sensitive data like credentials.
    - The default behaviour for workspaces in Steps remains the same - all Steps get access to them.

tekton-robot · 2021-01-20T15:24:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	91.3%	-3.3

tekton-robot · 2021-01-20T15:42:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

ghost · 2021-01-20T16:03:43Z

Build tests are failing with following message:

/home/prow/go/src/github.com/tektoncd/pipeline is out of date. Please run hack/update-codegen.sh

But update-codegen.sh doesn't create any new code when I run it. This error is actually appearing because update-openapigen.sh needs to be run. I'm going to update that error message as part of this PR.

tekton-robot · 2021-01-20T16:06:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

bobcatfish

Started with some initial comments on docs and examples, all super minor so far! Will return to review more ASAP

docs/workspaces.md

bobcatfish · 2021-02-03T17:53:18Z

docs/workspaces.md

-    # Note: must explicitly include volumeMount for the workspace to be accessible in the Sidecar
-    volumeMounts:
-    - name: $(workspaces.signals.volume)
-      mountPath: $(workspaces.signals.path)


ahhh its so much better now :D

docs/workspaces.md

bobcatfish · 2021-02-03T17:54:25Z

examples/v1beta1/taskruns/workspace-in-sidecar.yaml

-      # Note: must explicitly add volumeMount to gain access to Workspace in sidecar
-      volumeMounts:
-      - name: $(workspaces.signals.volume)
-        mountPath: $(workspaces.signals.path)


bobcatfish · 2021-02-03T17:54:51Z

examples/v1beta1/taskruns/workspace-isolation.yaml

+      script: |
+        #!/usr/bin/env ash
+        if [ -f "$(workspaces.signals.path)"/start ] ; then
+          echo "This step should not have access to the signals workspace."


👍 👍 👍

bobcatfish · 2021-02-03T17:56:04Z

examples/v1beta1/taskruns/workspace-isolation.yaml

+metadata:
+  generateName: workspace-isolation-
+spec:
+  timeout: 2m0s


oooo is this here b/c of the loop in the sidecar? might be worth adding a couple comments (e.g. in the sidecar loop), i had to work through that myself and was about to say "we should add a timeout"

Updated with comments in both loops mentioning the use of timeout to prevent looping forever.

hack/verify-codegen.sh

tekton-robot · 2021-02-03T18:38:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

tekton-robot · 2021-02-03T19:01:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

tekton-robot · 2021-02-04T14:57:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

tekton-robot · 2021-02-04T19:51:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	97.1%	2.5

bobcatfish

I think pretty much all my comments are just idle thoughts or unimportant nitpicks! The only at all "significant" comment I have is a think there might be a bit of missing test coverage on the mount path overriding.

Looks great!

/approve

i wonder if we want to explicitly do anything about how this impacts pipelineresources? since this was implemented using steptemplates, removing the volumes from the step template would also mean removing it from pipelineresources, but right now we don't cover it with any tests or mention it in docs - i think its probably better this way; its hard to imagine any pipelineresources that would be impacted, probably no one even knew that was happening, and lastly we probably want to discourage using pipelineresources anyway, but i wanted to bring it up and see what you think

bobcatfish · 2021-02-04T19:50:26Z

DEVELOPMENT.md

@@ -217,13 +217,14 @@ While iterating on the project, you may need to:
 1.  Verify it's working by [looking at the logs](#accessing-logs)
 1.  Update your (external) dependencies with: `./hack/update-deps.sh`.
 1.  Update your type definitions with: `./hack/update-codegen.sh`.
+1.  Update your OpenAPI specs with: `./hack/update-openapigen.sh`.


thanks @sbwsg 🙏

pkg/apis/pipeline/v1beta1/workspace_types.go

docs/workspaces.md

pkg/workspace/apply_test.go

bobcatfish · 2021-02-04T20:04:30Z

pkg/workspace/apply_test.go

+			Workspaces: []v1beta1.WorkspaceDeclaration{{
+				Name: "source",
+			}},
+		},


nice, clear test cases!

pkg/workspace/apply.go

pkg/reconciler/taskrun/resources/apply_test.go

pkg/reconciler/taskrun/resources/apply.go

tekton-robot · 2021-02-04T20:55:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	100.0%	5.4

bobcatfish · 2021-02-04T21:00:37Z

yaaaasssss

ghost · 2021-02-04T21:27:18Z

Thanks for reviewing @bobcatfish !

tekton-robot · 2021-02-04T21:31:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	94.6%	100.0%	5.4

docs/workspaces.md

pritidesai · 2021-02-08T23:10:46Z

examples/v1beta1/taskruns/workspace-isolation.yaml

+    workspaces:
+    - name: signals
+    steps:
+    - image: alpine:3.12.0


NIT: step names are missing

afrittoli · 2021-02-09T08:44:01Z

Thank you for this, it's a great feature to add. I skimmed through the code and it looks good (thanks for nice docs 🙏 ) - the only concern I have is the lack of a feature flag. Since this adds to a beta API, adding it without a flag means that the new API field becomes beta the moment it's released.

I'm aware that we don't have a clear policy right now, I'll try to progress on https://github.com/tektoncd/community/blob/main/teps/0033-tekton-feature-gates.md so that we may have better guidance for the future.

I agree with @bobcatfish proposal to have a single API flag for (most) API changes would make sense, and I know @vdemeester expressed positive feedback to that idea, so perhaps we should add the flag along with documentation so that people may start using it.

@sbwsg @bobcatfish @vdemeester we're almost at release time, I see three alternatives:

complete the work on the TEP, implement the changes associated and then merge this PR, hopefully in the next release
implement an API feature flag now, rebase this one on top and they might both be merged in time for this release
decide we're confident enough with this change that we want to go straight to beta

In terms of the change in behaviour (volumes being available to steps right away), I think that should be fine, I cannot imaging that change breaking anyones workflow today - is that correct?

afrittoli · 2021-02-09T08:46:50Z

/hold until the need for a feature flag is discussed / clarified

vdemeester · 2021-02-09T09:03:43Z

/hold

tekton-robot · 2021-04-30T18:41:30Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

ghost · 2021-04-30T19:03:50Z

now that the feature gates PR is merged, this will need a rebase 🙏

Rebased on top of new feature gates code. I think this is ready for another look.

ghost · 2021-05-04T12:43:15Z

I'm going to cancel the hold. I can't find the comment where it was added and everythoing's up to date now with the feature gate work.

/hold cancel

pritidesai · 2021-05-04T18:59:40Z

examples/v1beta1/taskruns/workspace-in-sidecar.yaml

@@ -19,6 +19,10 @@ spec:
    - name: signals
    steps:
    - image: alpine:3.12.0
+      resources:


@sbwsg I am wondering why these changes are showing up here 🤔 These were already a part of previous PR you created.

and which was merged ...

pritidesai · 2021-05-04T19:03:22Z

pkg/apis/config/feature_flags.go

@@ -118,7 +118,7 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) {
 }

 // setEnabledAPIFields sets the "enable-api-fields" flag based on the content of a given map.
-// If the feature gate is invalid or missing then an error is returned.
+// If the feature gate is invalid or missing then the flag is set to its default.


NIT: this is not true, it does return error if the value is anything outside of stable and alpha, some git weirdness?

Ah 😅 result of a bad merge. Good catch, thank you!

pritidesai · 2021-05-04T19:24:25Z

docs/workspaces.md

@@ -135,6 +158,9 @@ spec:

 #### Sharing `Workspaces` with `Sidecars`


NIT: reading for the first time looks like, the two notes in the same section is contradicting each other. The first note is saying with alpha API, the workspaces are automatically available to sidecars and second note is about stable API. With stable API, sidecars have to opt-in. This is fine for now, no need to change anything here. But we might have to repeat ourselves and be explicit about the behavior with stable compared to alpha 😞 May be keep the stable API behavior as is and add changes being introduced with alpha in the end of the section 🤔

Yeah you're right it was confusing in this context. I removed mention of the alpha api from here.

tekton-robot · 2021-05-04T20:09:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

ghost

I rebased off of latest main; the weird artifacts are gone now.

ghost · 2021-05-04T20:30:52Z

pkg/apis/config/feature_flags.go

@@ -118,7 +118,7 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) {
 }

 // setEnabledAPIFields sets the "enable-api-fields" flag based on the content of a given map.
-// If the feature gate is invalid or missing then an error is returned.
+// If the feature gate is invalid or missing then the flag is set to its default.


Ah 😅 result of a bad merge. Good catch, thank you!

tekton-robot · 2021-05-04T20:32:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

tekton-robot · 2021-05-04T20:36:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

pritidesai · 2021-05-05T00:14:26Z

pkg/apis/pipeline/v1beta1/task_validation.go

+	for stepIdx, step := range steps {
+		if len(step.Workspaces) != 0 {
+			errs = errs.Also(ValidateEnabledAPIFields(ctx, "step workspaces", config.AlphaAPIFields).ViaIndex(stepIdx).ViaField("steps"))
+		}


Should we return an error here or continue validating workspaces even when a feature gate is not set to alpha? 🤔 Generally we accumulate all the validation errors before exiting.

Yeah, I've decided to leave them accumulating. We can always return early in future if we see reports that it's confusing this way.

pritidesai · 2021-05-05T06:12:42Z

Is this feature supported for a task within a pipeline? if so, will be great to have an example for the same 🙏

tekton-robot · 2021-05-05T10:36:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

ghost · 2021-05-05T10:37:13Z

Is this feature supported for a task within a pipeline? if so, will be great to have an example for the same 🙏

Added a pipelinerun example.

pritidesai · 2021-05-05T18:11:09Z

Added a pipelinerun example.

Thanks a bunch @sbwsg, one more question (sorry for nagging on this, trying to make sure I understand it since this is the first alpha feature 😜 ). I am not seeing the examples under v1beta1/taskruns/alpha and v1beta1/pipelineruns/alpha ran as part of the integration tests. Is that expected? The examples are more of a reference rather than validating the functionality?

afrittoli · 2021-05-05T18:56:18Z

Release Notes

- Sidecars in a Task now receive Workspaces, just like Steps do.
- Steps and Sidecars can now specify which Workspaces they want access to. This isolates the workspace to only those Steps or Sidecars that need them, allowing Task authors to limit exposure of sensitive data like credentials.
- The default behaviour for workspaces in Steps remains the same - all Steps get access to them.

Could you mention in the release notes that this change in behaviour is alpha / gated by a feature flag - so there will be no change in default behaviour when upgrading to v0.24? 🙏

ghost · 2021-05-05T18:58:27Z

Added a pipelinerun example.

Thanks a bunch @sbwsg, one more question (sorry for nagging on this, trying to make sure I understand it since this is the first alpha feature 😜 ). I am not seeing the examples under v1beta1/taskruns/alpha and v1beta1/pipelineruns/alpha ran as part of the integration tests. Is that expected? The examples are more of a reference rather than validating the functionality?

I created #3896 to tackle it and will pick it up asap.

Could you mention in the release notes that this change in behaviour is alpha / gated by a feature flag - so there will be no change in default behaviour when upgrading to v0.24? 🙏

Sure thing! Done.

pritidesai · 2021-05-05T19:09:57Z

pkg/workspace/apply.go

@@ -98,6 +100,23 @@ func Apply(ts v1beta1.TaskSpec, wb []v1beta1.WorkspaceBinding, v map[string]core
 		ts.StepTemplate = &corev1.Container{}
 	}

+	isolatedWorkspaces := sets.NewString()
+
+	alphaAPIEnabled := config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha"


NIT: can use config.AlphaAPIFields instead of string literal "alpha"

afrittoli

Review in progress 🙏 I will finish tomorrow if no-one gets to it first

examples/v1beta1/taskruns/alpha/workspace-isolation.yaml

Access to Workspaces can now be limited to specific Steps and Sidecars in a Task. This allows Task authors to isolate sensitive data to specific images, reducing exposure of assets like credentials. As a side-effect of this change Workspaces are now mounted to Sidecars by default, just as they are to Steps.

tekton-robot · 2021-05-05T20:20:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/task_validation.go	95.8%	96.3%	0.5
pkg/workspace/apply.go	94.6%	100.0%	5.4

pritidesai · 2021-05-05T20:21:35Z

pkg/workspace/apply.go

+			if isolatedWorkspaces.Has(w.Name) {
+				mountAsIsolatedWorkspace(ts, w.Name, volumeMount)
+			} else {
+				mountAsSharedWorkspace(ts, volumeMount)


I am guessing this will go away once this feature is promoted to stable 🤔

or rather the else ⬇️

pritidesai · 2021-05-05T20:37:31Z

pkg/workspace/apply_test.go

@@ -522,3 +524,310 @@ func TestApply(t *testing.T) {
 		})
 	}
 }
+
+func TestApply_IsolatedWorkspaces(t *testing.T) {


NIT: The tests included here are pretty comprehensive (go coverage is very obvious) 😍 but will be great to have a test with two different workspaces out of which one is isolated to a step and one is isolated to a sidecar.

pritidesai · 2021-05-05T20:53:03Z

thanks a bunch @sbwsg for your patience on this 🙏

It looks great. I am a little nervous without having alpha examples running as part of the CI process. But not a blocker here. I have validated all the examples on my local cluster and works smooth. We can wait until #3896 is implemented (hopefully soon 🤞). I have also validated workspaces.yaml pipelineRun with alpha API and works as expected without any issues.

/lgtm

ghost added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 20, 2021

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 20, 2021

tekton-robot requested review from dlorenc and vdemeester January 20, 2021 15:21

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

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2021

ghost added this to the Pipelines 0.21 milestone Jan 25, 2021

ghost mentioned this pull request Jan 29, 2021

Add non-root user support and workspace credentials to git-clone tektoncd/catalog#637

Merged

9 tasks

bobcatfish self-assigned this Feb 2, 2021

bobcatfish reviewed Feb 3, 2021

View reviewed changes

bobcatfish reviewed Feb 4, 2021

View reviewed changes

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

pritidesai reviewed Feb 8, 2021

View reviewed changes

docs/workspaces.md Show resolved Hide resolved

pritidesai reviewed Feb 8, 2021

View reviewed changes

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

pritidesai reviewed May 4, 2021

View reviewed changes

ghost commented May 4, 2021

View reviewed changes

pritidesai reviewed May 5, 2021

View reviewed changes

afrittoli reviewed May 5, 2021

View reviewed changes

examples/v1beta1/taskruns/alpha/workspace-isolation.yaml Outdated Show resolved Hide resolved

pritidesai reviewed May 5, 2021

View reviewed changes

tekton-robot assigned pritidesai May 5, 2021

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

tekton-robot merged commit 98aef3b into tektoncd:main May 5, 2021

pritidesai mentioned this pull request Feb 3, 2023

Promote TEP-0029 - Isolated Step and Sidecar Workspaces #6109

Closed

dibyom mentioned this pull request Nov 20, 2023

Promote to GA: TEP-0029 - Isolated Step and Sidecar Workspaces #7398

Open

		@@ -135,6 +158,9 @@ spec:

		#### Sharing `Workspaces` with `Sidecars`

Add Step and Sidecar Workspaces feature #3700

Add Step and Sidecar Workspaces feature #3700

Conversation

ghost commented Jan 20, 2021 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jan 20, 2021

tekton-robot commented Jan 20, 2021

ghost commented Jan 20, 2021

tekton-robot commented Jan 20, 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

tekton-robot commented Feb 3, 2021

tekton-robot commented Feb 3, 2021

tekton-robot commented Feb 4, 2021

tekton-robot commented Feb 4, 2021

bobcatfish left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 4, 2021

bobcatfish commented Feb 4, 2021

ghost commented Feb 4, 2021

tekton-robot commented Feb 4, 2021

Choose a reason for hiding this comment

afrittoli commented Feb 9, 2021 • edited Loading

afrittoli commented Feb 9, 2021

vdemeester commented Feb 9, 2021

tekton-robot commented Apr 30, 2021

ghost commented Apr 30, 2021

ghost commented May 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 4, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 4, 2021

tekton-robot commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented May 5, 2021

tekton-robot commented May 5, 2021

ghost commented May 5, 2021

pritidesai commented May 5, 2021

afrittoli commented May 5, 2021

Release Notes

ghost commented May 5, 2021

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai commented May 5, 2021

ghost commented Jan 20, 2021 •

edited by ghost

Loading

bobcatfish left a comment •

edited

Loading

afrittoli commented Feb 9, 2021 •

edited

Loading