Add variable expansion for params in Projected Volume fields #2601

jlpettersson · 2020-05-11T21:00:10Z

Changes

A projected volume can mount/project files from Secrets, ConfigMaps and ServiceAccountTokens.

It is good if the end-user can choose the name of Secrets, ConfigMaps and the audience of ServiceAccountTokens. With this commit, the task author can use params for secret.name, configmap.name and serviceaccounttoken.audience in a Projected Volume.

Example usage:

  volumes:
  - name: ssh-auth
    projected:
      defaultMode: 0400
      sources:
      - secret:
          name: $(params.known-hosts-secret) 
      - secret:
          name: $(params.private-key-secret)

See more example use cases in #2597

Fixes #2597
/kind feature

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Add variable expansion for params in Projected Volume fields

tekton-robot · 2020-05-11T21:00:25Z

Hi @jlpettersson. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vincent-pli · 2020-05-12T01:09:31Z

/ok-to-test

vdemeester

/meow
/cc @sbwsg

tekton-robot · 2020-05-12T09:24:46Z

@vdemeester:

In response to this:

/meow
/cc @sbwsg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2020-05-12T09:24:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [vdemeester]

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

ghost · 2020-05-12T11:26:57Z

/lgtm

I had no idea we already supported substitution for other volume types. Thanks @jlpettersson !

ghost · 2020-05-12T11:27:41Z

Oh, we should probably also document this!

jlpettersson · 2020-05-12T11:41:55Z

@sbwsg yes. This is variable expansion for params. This is actually documented already, but we don't document all places where it is possible to use variable expansion for params.

But I think that I instead should document on how to use Projected Volumes since that is usable with Secrets, ConfigMaps and ServiceAccountTokens.

ghost · 2020-05-12T12:27:08Z

Created #2605 to record all variable expansion locations.

jlpettersson · 2020-05-12T13:42:47Z

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2020-05-12T13:43:10Z

Is there a documentation change that would make sense to add with this?

jlpettersson · 2020-05-12T13:44:36Z

@bobcatfish see #2601 (comment)

bobcatfish · 2020-05-12T13:47:47Z

ah kk

but we don't document all places where it is possible to use variable expansion for params.

It does seem like something that would make sense to document, but I understand not wanting to take that on when there is no precedent. Documenting projected volumes as well makes sense, but I think it makes sense to be able to refer to docs that show you where you can use the variable replacement

jlpettersson · 2020-05-12T14:37:22Z

/test tekton-pipeline-unit-tests

jlpettersson · 2020-05-12T15:26:05Z

Fails on this now

<testsuite tests="1" failures="1" time="0.000" name="pkg/reconciler/taskrun/resources">
<properties>
<property name="go.version" value="go1.14.1"/>
</properties>
<testcase classname="resources" name="[build failed]" time="0.000">
<failure message="Failed" type=""/>
</testcase>
</testsuite>

vdemeester · 2020-05-12T15:34:05Z

@jlpettersson can you rebase against master ?

jlpettersson · 2020-05-12T15:55:37Z

Am I hitting this feature gate?

The projection of service account tokens is a feature introduced in Kubernetes 1.11 and promoted to Beta in 1.12. To enable this feature on 1.11, you need to explicitly set the TokenRequestProjection feature gate to True.

level=warning msg="[runner] Can't run linter goanalysis_metalinter: gosec: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal]"
level=error msg="Running error: gosec: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal]"

Is this test running on a cluster with different configuration/version than earlier tests?

jlpettersson · 2020-05-12T19:58:16Z

/test tekton-pipeline-unit-tests
/test pull-tekton-pipeline-build-tests

vdemeester · 2020-05-13T09:40:42Z

Am I hitting this feature gate?

The projection of service account tokens is a feature introduced in Kubernetes 1.11 and promoted to Beta in 1.12. To enable this feature on 1.11, you need to explicitly set the TokenRequestProjection feature gate to True.
level=warning msg="[runner] Can't run linter goanalysis_metalinter: gosec: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal]"
level=error msg="Running error: gosec: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/prow/go/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal]"
Is this test running on a cluster with different configuration/version than earlier tests?

Nah, this is the linter, it shouldn't fail on this (and it doesn't have anything to do with the cluster the build are run into)… This is a weird error…

/retest

vdemeester · 2020-05-13T09:47:10Z

@jlpettersson actually the error is legit

λ make golangci-lint 
🐱 getting golangci-lint v1.25.0
golangci/golangci-lint info checking GitHub for tag 'v1.25.0'
golangci/golangci-lint info found version: 1.25.0 for v1.25.0/linux/amd64
golangci/golangci-lint info installed /home/vincent/src/github.com/tektoncd/pipeline/.bin/golangci-lint
🐱 running golangci-lint…
WARN [runner] Can't run linter goanalysis_metalinter: gocritic: analysis skipped: errors in package: [/home/vincent/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/vincent/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal] 
ERRO Running error: gocritic: analysis skipped: errors in package: [/home/vincent/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field Name in struct literal /home/vincent/src/github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field VolumeSource in struct literal] 
make: *** [Makefile:150: golangci-lint] Error 3

⛄ λ go test ./pkg/reconciler/taskrun/resources/... 
# github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources_test [github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources.test]
pkg/reconciler/taskrun/resources/apply_test.go:132:5: unknown field 'Name' in struct literal of type "k8s.io/api/core/v1".EnvFromSource
pkg/reconciler/taskrun/resources/apply_test.go:133:5: unknown field 'VolumeSource' in struct literal of type "k8s.io/api/core/v1".EnvFromSource
FAIL	github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources [build failed]
ok  	github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/cloudevent	0.013s
FAIL

jlpettersson · 2020-05-13T12:00:10Z

Thank you @vdemeester !

I did not notice this. But a part of the code was actually moved to a similar part. I got the error locally as well now. It must have happened during my git rebase master. This is a good example of how important it is to practice Continuous Integration ;) Tekton is becoming a great product for that :)

Jonas has recently become a regularly contributor. He started with adding a minor [_missing_ `omitempty`](tektoncd/pipeline#2301) and then [proposed some ideas](tektoncd/pipeline#1986 (comment)) around workspaces and PersistentVolumeClaim creation and continued to [elaborate around those ideas](tektoncd/pipeline#1986 (comment)). A sunny day a few days later, he also submitted an [extensive implementation for volumeClaimTemplate](tektoncd/pipeline#2326), corresponding to the idea discussions. A few days later submitted a [small refactoring PR](tektoncd/pipeline#2392), and he also listened to community members that [proposed changes](tektoncd/pipeline#2450) to his implementation about volumeClaimTemplates and did an [implementation for that proposal](tektoncd/pipeline#2453). A rainy day, he also wrote [technical documentation about PVCs](tektoncd/pipeline#2521) including adding an example that caused _flaky_ integration tests for the whole community during multiple days. When he understood his mistake, he submitted a [removal of the example](tektoncd/pipeline#2546) that caused flaky tests. He has also put his toe into Tekton Catalog and [contributed to the buildah task](tektoncd/pipeline#2546). This has followed, mostly with more PRs to the Pipeline project: - tektoncd/pipeline#2460 - tektoncd/pipeline#2491 - tektoncd/pipeline#2502 - tektoncd/pipeline#2506 - tektoncd/pipeline#2632 - tektoncd/pipeline#2633 - tektoncd/pipeline#2634 - tektoncd/pipeline#2636 - tektoncd/pipeline#2601 - tektoncd/pipeline#2630 Jonas is excited about the great community around Tekton and the project! He now would like to join the org.

A [projected volume](https://kubernetes.io/docs/concepts/storage/volumes/#projected) can mount/project files from `Secrets`, `ConfigMaps` and `ServiceAccountTokens`. Is is good if the end user can choose the name of `Secrets`, `ConfigMaps` and the audience of `ServiceAccountTokens`. With this commit, the task author can use `params` for `secret.name`, `configmap.name` and `serviceaccounttoken.audience` in a Projected Volume. See examples of use cases in tektoncd#2597 Fixes tektoncd#2597

afrittoli

Nice, thank you!
/lgtm

tekton-robot requested review from bobcatfish and dibyom May 11, 2020 21:00

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2020

vdemeester approved these changes May 12, 2020

View reviewed changes

tekton-robot requested a review from a user May 12, 2020 09:24

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. labels May 12, 2020

tekton-robot assigned ghost May 12, 2020

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

jlpettersson force-pushed the variable_expansion_in_projected_volumes branch from 53d6395 to df7e19e Compare May 12, 2020 15:44

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 12, 2020

jlpettersson force-pushed the variable_expansion_in_projected_volumes branch from df7e19e to 4d2babc Compare May 13, 2020 11:27

jlpettersson mentioned this pull request May 19, 2020

Add Jonas to the org tektoncd/community#106

Merged

jlpettersson force-pushed the variable_expansion_in_projected_volumes branch from 4d2babc to 4215ec5 Compare May 20, 2020 22:53

afrittoli reviewed May 22, 2020

View reviewed changes

tekton-robot assigned afrittoli May 22, 2020

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

tekton-robot merged commit fc24674 into tektoncd:master May 22, 2020

jlpettersson mentioned this pull request May 23, 2020

Declaring Task specific Volumes vs Workspaces #2680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variable expansion for params in Projected Volume fields #2601

Add variable expansion for params in Projected Volume fields #2601

jlpettersson commented May 11, 2020 •

edited

Loading

tekton-robot commented May 11, 2020

vincent-pli commented May 12, 2020

vdemeester left a comment

tekton-robot commented May 12, 2020

tekton-robot commented May 12, 2020

ghost commented May 12, 2020

ghost commented May 12, 2020

jlpettersson commented May 12, 2020

ghost commented May 12, 2020

jlpettersson commented May 12, 2020

bobcatfish commented May 12, 2020

jlpettersson commented May 12, 2020

bobcatfish commented May 12, 2020

jlpettersson commented May 12, 2020

jlpettersson commented May 12, 2020

vdemeester commented May 12, 2020

jlpettersson commented May 12, 2020

jlpettersson commented May 12, 2020

vdemeester commented May 13, 2020

vdemeester commented May 13, 2020

jlpettersson commented May 13, 2020

afrittoli left a comment

Add variable expansion for params in Projected Volume fields #2601

Add variable expansion for params in Projected Volume fields #2601

Conversation

jlpettersson commented May 11, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented May 11, 2020

vincent-pli commented May 12, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented May 12, 2020

tekton-robot commented May 12, 2020

ghost commented May 12, 2020

ghost commented May 12, 2020

jlpettersson commented May 12, 2020

ghost commented May 12, 2020

jlpettersson commented May 12, 2020

bobcatfish commented May 12, 2020

jlpettersson commented May 12, 2020

bobcatfish commented May 12, 2020

jlpettersson commented May 12, 2020

jlpettersson commented May 12, 2020

vdemeester commented May 12, 2020

jlpettersson commented May 12, 2020

jlpettersson commented May 12, 2020

vdemeester commented May 13, 2020

vdemeester commented May 13, 2020

jlpettersson commented May 13, 2020

afrittoli left a comment

Choose a reason for hiding this comment

jlpettersson commented May 11, 2020 •

edited

Loading