TEP-0101 PodTemplate should support environment variables. #5699

ScrapCodes · 2022-10-31T15:01:03Z

Changes

Envs i.e. environment variables specified in podTemplate for PipelineRun and TaskRun, will take precdence over envs defined in step and StepTemplate.
config-defaults' default-pod-template can contain global default envs applicable for all PipelineRun and taskRun. These are overriden by user defined podTemplate.
Support forbidden envs.
a. A new config-default configuration: forbidden-envs
b. Envs added in the forbidden-envs are filtered while updating from a podTemplate.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

PodTemplate can be used to update environment variables globally as well as PipelineRun and TaskRun level.
Added new configuration option default-forbidden-env to disallow specified environment variables from being updated via podTemplate.

/kind tep

tekton-robot · 2022-10-31T15:07:28Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

tekton-robot · 2022-10-31T16:29:55Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

tekton-robot · 2022-11-01T08:33:48Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

tekton-robot · 2022-11-01T09:54:32Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

ScrapCodes · 2022-11-01T16:33:00Z

/retest

tekton-robot · 2022-11-02T10:05:17Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

docs/pipeline-api.md

vdemeester

/cc @piyush-garg

Tomcli · 2022-11-10T17:22:54Z

/cc @afrittoli

tekton-robot · 2022-11-15T10:19: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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

ScrapCodes · 2022-11-15T10:24:23Z

Hi @JeromeJu, @vdemeester and @afrittoli Running codegen after updating to latest main branch adds a whole bunch of stuff not related to this commit.

tekton-robot · 2022-11-15T10:27:29Z

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/config/default.go	88.6%	89.4%	0.7
pkg/pod/pod.go	89.5%	90.6%	1.1

pritidesai · 2022-11-15T22:22:02Z

/kind feature

ScrapCodes · 2022-11-16T11:58:46Z

opened #5764 to fix codegen out of sync issue.

XinruZhang

Mostly LGTM, just some minor nits. Thanks for the PR ❤️

pkg/pod/pod.go

pkg/apis/config/default.go

ScrapCodes · 2022-11-22T11:50:16Z

Thank you @XinruZhang ! for taking a look, please take a look again

chitrangpatel · 2022-12-07T17:00:08Z

docs/install.md

+          value: "false"
+```
+
+_In the above example the environment variable `TEST_TEKTON` will not be overriden by value specified in podTemplate, because the `config-default` option `default-forbidden-env` is configured with value `TEST_TEKTON`._


Should a webhook validation error be thrown if a user is trying to update a forbidden env? Might be better user experience this way instead of allowing a user to provide it and silently rejecting it?

chitrangpatel

Apart from the behaviour of how to deal with podTemplate that try to update forbidden-env, it looks good!

jerop · 2022-12-19T19:12:28Z

@ScrapCodes - it seems there's an unaddressed feedback from @afrittoli and @chitrangpatel, do you think they'd be resolved soon? wondering if I should move this PR to the next milestone - hoping to cut v0.43 tomorrow

afrittoli · 2022-12-20T10:08:51Z

It looks like the TEP for this feature is not implementable

Good catch @lbernick , thank you. It is implementable now

afrittoli · 2022-12-20T10:10:12Z

/hold cancel

ScrapCodes · 2022-12-20T10:13:35Z

Hi @chitrangpatel, do you think - we can add the webhook validations in a separate pull request?

chitrangpatel · 2022-12-20T11:58:03Z

Hi @chitrangpatel, do you think - we can add the webhook validations in a separate pull request?

Yes, that's completely fine.

ScrapCodes · 2022-12-20T12:32:11Z

Thank you @chitrangpatel !

tekton-robot · 2022-12-20T14:25:32Z

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/config/default.go	88.6%	90.0%	1.4
pkg/apis/pipeline/v1/pipelinerun_validation.go	88.1%	88.3%	0.2
pkg/apis/pipeline/v1/taskrun_validation.go	98.4%	97.7%	-0.6
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	97.4%	96.8%	-0.6
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.4%	97.8%	-0.6
pkg/pod/pod.go	91.3%	91.8%	0.5

chitrangpatel · 2022-12-20T14:28:47Z

pkg/apis/pipeline/v1beta1/pipelinerun_validation.go

@@ -334,5 +337,8 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap
 		errs = errs.Also(version.ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources"))
 		errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepOverrides))
 	}
+	if trs.TaskPodTemplate != nil {
+		errs = errs.Also(validatePodTemplateEnv(ctx, *trs.TaskPodTemplate))
+	}


shouldn't there be a similar update to v1/pipelinerun_validation.go?

pkg/apis/pipeline/v1/pipelinerun_validation.go

tekton-robot · 2022-12-20T17:21: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/config/default.go	88.6%	90.0%	1.4
pkg/apis/pipeline/v1/pipelinerun_validation.go	88.1%	88.3%	0.2
pkg/apis/pipeline/v1/taskrun_validation.go	98.4%	97.7%	-0.6
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	97.4%	96.8%	-0.6
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.4%	97.8%	-0.6
pkg/pod/pod.go	91.3%	91.8%	0.5

tekton-robot · 2022-12-20T17:21:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.6%	90.0%	1.4
pkg/apis/pipeline/v1/pipelinerun_validation.go	88.1%	88.3%	0.2
pkg/apis/pipeline/v1/taskrun_validation.go	98.4%	97.7%	-0.6
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	97.4%	96.8%	-0.6
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.4%	97.8%	-0.6
pkg/pod/pod.go	91.3%	91.8%	0.5

1. Envs i.e. environment variables specified in podTemplate for PipelineRun and TaskRun, will take precdence over envs defined in step and StepTemplate. 2. config-defaults' default-pod-template can contain global default envs applicable for all PipelineRun and taskRun. These are overriden by user defined podTemplate. TEP-101 Support forbidden envs. 1. A new config-default configuration: default-forbidden-envs 2. Envs added in the default-forbidden-envs are filtered while updating from a podTemplate.

tekton-robot · 2022-12-21T00:39: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/apis/config/default.go	88.6%	90.0%	1.4
pkg/apis/pipeline/v1/pipelinerun_validation.go	88.1%	88.3%	0.2
pkg/apis/pipeline/v1/taskrun_validation.go	98.4%	97.7%	-0.6
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	97.4%	96.8%	-0.6
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.4%	97.8%	-0.6
pkg/pod/pod.go	91.3%	91.8%	0.5

tekton-robot · 2022-12-21T00:41:12Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	88.6%	90.0%	1.4
pkg/apis/pipeline/v1/pipelinerun_validation.go	88.1%	88.3%	0.2
pkg/apis/pipeline/v1/taskrun_validation.go	98.4%	97.7%	-0.6
pkg/apis/pipeline/v1beta1/pipelinerun_validation.go	97.4%	96.8%	-0.6
pkg/apis/pipeline/v1beta1/taskrun_validation.go	98.4%	97.8%	-0.6
pkg/pod/pod.go	91.3%	91.8%	0.5

afrittoli

Thank you for the update @ScrapCodes !
/approve

tekton-robot · 2022-12-21T11:08:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

afrittoli · 2022-12-21T11:09:20Z

@chitrangpatel ok to merge this? @ScrapCodes implemented both v1beta1 and v1 validation.

afrittoli · 2022-12-21T11:09:41Z

@jerop this should be ready to merge and be included in v0.43

chitrangpatel · 2022-12-21T14:05:38Z

@chitrangpatel ok to merge this? @ScrapCodes implemented both v1beta1 and v1 validation.

Yes! All good!

afrittoli · 2022-12-21T14:18:59Z

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2022

tekton-robot requested review from afrittoli and imjasonh October 31, 2022 15:01

ScrapCodes force-pushed the tep-101 branch from 68dc6ba to 3f6171b Compare November 1, 2022 08:28

ScrapCodes force-pushed the tep-101 branch 2 times, most recently from eb678f2 to b28f802 Compare November 1, 2022 09:49

ScrapCodes force-pushed the tep-101 branch from b28f802 to 58634fe Compare November 2, 2022 09:59

ScrapCodes commented Nov 3, 2022

View reviewed changes

docs/pipeline-api.md Outdated Show resolved Hide resolved

vdemeester approved these changes Nov 3, 2022

View reviewed changes

tekton-robot requested a review from piyush-garg November 3, 2022 16:09

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

ScrapCodes force-pushed the tep-101 branch from 58634fe to 602a8d2 Compare November 15, 2022 10:15

ScrapCodes force-pushed the tep-101 branch from 602a8d2 to 6903652 Compare November 15, 2022 10:23

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 15, 2022

XinruZhang reviewed Nov 21, 2022

View reviewed changes

pkg/pod/pod.go Outdated Show resolved Hide resolved

pkg/apis/config/default.go Outdated Show resolved Hide resolved

ScrapCodes force-pushed the tep-101 branch from 6903652 to 5388a87 Compare November 22, 2022 11:49

imjasonh removed their request for review December 3, 2022 19:49

chitrangpatel reviewed Dec 7, 2022

View reviewed changes

chitrangpatel approved these changes Dec 7, 2022

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 Dec 20, 2022

chitrangpatel reviewed Dec 20, 2022

View reviewed changes

ScrapCodes commented Dec 20, 2022

View reviewed changes

pkg/apis/pipeline/v1/pipelinerun_validation.go Show resolved Hide resolved

ScrapCodes force-pushed the tep-101 branch from ed194d7 to 06efeae Compare December 20, 2022 17:15

ScrapCodes added 2 commits December 21, 2022 06:02

Added validation for forbidden env.

3160f18

ScrapCodes force-pushed the tep-101 branch from 06efeae to 3160f18 Compare December 21, 2022 00:33

afrittoli reviewed Dec 21, 2022

View reviewed changes

afrittoli self-assigned this Dec 21, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2022

tekton-robot merged commit 49fdbfb into tektoncd:main Dec 21, 2022

TEP-0101 PodTemplate should support environment variables. #5699

TEP-0101 PodTemplate should support environment variables. #5699

Conversation

ScrapCodes commented Oct 31, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Oct 31, 2022

tekton-robot commented Oct 31, 2022

tekton-robot commented Nov 1, 2022

tekton-robot commented Nov 1, 2022

ScrapCodes commented Nov 1, 2022

tekton-robot commented Nov 2, 2022

vdemeester left a comment

Choose a reason for hiding this comment

Tomcli commented Nov 10, 2022

tekton-robot commented Nov 15, 2022

ScrapCodes commented Nov 15, 2022

tekton-robot commented Nov 15, 2022

pritidesai commented Nov 15, 2022

ScrapCodes commented Nov 16, 2022

XinruZhang left a comment

Choose a reason for hiding this comment

ScrapCodes commented Nov 22, 2022

chitrangpatel Dec 7, 2022

Choose a reason for hiding this comment

chitrangpatel left a comment

Choose a reason for hiding this comment

jerop commented Dec 19, 2022

afrittoli commented Dec 20, 2022

afrittoli commented Dec 20, 2022

ScrapCodes commented Dec 20, 2022

chitrangpatel commented Dec 20, 2022

ScrapCodes commented Dec 20, 2022

tekton-robot commented Dec 20, 2022

chitrangpatel Dec 20, 2022

Choose a reason for hiding this comment

tekton-robot commented Dec 20, 2022

tekton-robot commented Dec 20, 2022

tekton-robot commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 21, 2022

afrittoli commented Dec 21, 2022

afrittoli commented Dec 21, 2022

chitrangpatel commented Dec 21, 2022

afrittoli commented Dec 21, 2022

ScrapCodes commented Oct 31, 2022 •

edited

Loading