Add script mode support to windows tasks #4128

DrWadsy · 2021-07-30T05:31:05Z

Changes

Steps and sidecars can contain a script field. In Linux tasks these
scripts are copied into files which are made executable and then
steps are added to the task to execute those files. This commit adds
comparable functionality if scripts are used in a task which will run
on a Windows node.

On a Windows node the mechanics are different, due to how Windows
handles executable files. The key difference is that Tekton needs to
know that a script will run on Windows, and how to run the file
(which interpreter to use). This is done through a ‘windows shebang’
line at the start of the script.

The line must begin with #!win. After that the user needs to provide
the interpreter to use, as well as any necessary arguments. The line
must be written such that the name of the file containing the script to
execute can be appended to the end.

For example, to run the script in the file test.ps1 with powershell,
the command would usually be powershell -File test.ps1 and so the
shebang line must be #!win powershell -File.

If no interpreter is provided (i.e. the shebang line is only ‘#!win’) then
the script contents will be stored in a .cmd file and executed.

Finally, since a pod cannot contain a mix of Windows and Linux
containers a windows shell image has been added to the Images
structure, which will be used in the place-scripts step when needed
on a windows node.

Related:
Issue #1826
TEP-0057

/kind feature

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Add script mode support for windows Tasks/Pipelines

tekton-robot · 2021-07-30T05:31:20Z

Hi @DrWadsy. 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.

DrWadsy · 2021-07-30T05:38:06Z

/assign @dibyom

vdemeester · 2021-07-30T10:19:37Z

/ok-to-test

tekton-robot · 2021-07-30T10:54:02Z

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/pod/script.go	100.0%	69.6%	-30.4

imjasonh

First of all, this is great. 🤩

I'd love to have an e2e test covering this, which probably means engaging with the plumbing repo to ensure the test cluster has some Windows nodes. That'd be great to have even without this script support, to ensure we can run even non-script Windows tasks.

I don't think we necessarily need to block this PR on e2e tests, but I want to make sure we have those in place before we promise much more support.

imjasonh · 2021-07-30T12:36:47Z

pkg/pod/script.go

+	for _, step := range steps {
+		cleaned := strings.TrimSpace(step.Script)
+		if strings.HasPrefix(cleaned, "#!win") {
+			requiresWindows = true


Could this just be return true?

Same below with the requiresWindows = true; break which just does return requiresWIndows.

Yes, this should just be return true. The variable is an artifact from before I refactored this into a helper method, I'll change that :)

imjasonh · 2021-07-30T12:39:44Z

pkg/pod/script_test.go

@@ -365,7 +365,7 @@ func TestConvertScripts_WithSidecar(t *testing.T) {
 		MountPath: "/another/one",
 	}}

-	gotInit, gotSteps, gotSidecars := convertScripts(images.ShellImage, []v1beta1.Step{{
+	gotInit, gotSteps, gotSidecars := convertScripts(images.ShellImage, images.ShellImageWin, []v1beta1.Step{{
 		Script: `#!/bin/sh
 script-1`,
 		Container: corev1.Container{Image: "step-1"},


Can we also add unit test coverage for this new behavior in this file?

imjasonh · 2021-07-30T12:40:23Z

pkg/pod/pod.go

@@ -133,9 +133,9 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
 	// Convert any steps with Script to command+args.
 	// If any are found, append an init container to initialize scripts.
 	if alphaAPIEnabled {
-		scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, steps, taskSpec.Sidecars, taskRun.Spec.Debug)
+		scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, b.Images.ShellImageWin, steps, taskSpec.Sidecars, taskRun.Spec.Debug)


Should windows support go into the alpha feature gate, alongside debug support? 🤔

Good point.
I changed this call because I changed the signature of the convertScripts method, but we could pass nil and check for that before we do any windows checks?

Passing nil or an empty string for the windows shell image when alphaAPIEnabled is false would work ok I think. An alternative would be to write an entirely windows-focused convertScripts implementation and only invoke it if alphaAPIEnabled is true and requiresWindows would be true. Might involve a bit more duplication or end up being more lines but it might reduce the number of windows-specific branches? Could also push a change like that back to a refactor pr in future as well though.

linux-foundation-easycla · 2021-08-02T02:52:31Z

The committers are authorized under a signed CLA.

✅ Eugene McArdle (3f523d4, 93e62e9)

tekton-robot · 2021-08-02T02:54:35Z

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/pod/script.go	100.0%	97.8%	-2.2

DrWadsy · 2021-08-09T23:28:38Z

Is there anything else I need to do here to get this moving forward?

imjasonh · 2021-08-11T17:25:47Z

After #4139 is merged it would be great to have some e2e tests defined for this as well. We're still working on adding automation to those tests, though.

DrWadsy · 2021-08-19T00:50:52Z

/retest

DrWadsy · 2021-08-19T02:13:23Z

/retest

imjasonh · 2021-08-19T22:09:34Z

/lgtm

ghost

I left a bunch of comments but it looks like it's almost ready to go to me. Biggest question to my mind is whether we branch in our existing funcs (like convertScripts) or write windows-specific versions of those.

ghost · 2021-09-01T15:29:47Z

docs/tasks.md

@@ -265,6 +265,51 @@ steps:
      #!/usr/bin/env bash
      /bin/my-binary
 ```
+
+**Windows scripts**


Suggest making this a heading and adding it to the Table of Contents at the top. Maybe a sub-heading of "Running scripts within Steps" so:

##### Windows Scripts

ghost · 2021-09-01T15:31:47Z

docs/tasks.md

+
+`#!win <interpreter command> <args>`
+
+Unlike linux, we need to specify how to interpret the script file which is generated by Tekton. The example below shows how to execute a powershell script:


I didn't totally understand this bit on first read, since linux also uses shebang lines to specify how to interpret script files. Is it referring to the use of arguments (-File)? Or just that we have to be completely explicit otherwise tekton will assume it's for linux?

We have to be explicit that this script will run under windows, and we also have to tell tekton how this script is to be run (which is what a linux shebang does for us). Windows needs to know which executable (e.g. powershell.exe, or pwsh.exe) it needs to use in order to run the script, and also specify any args (e.g. both of those .exe's require the -File argument in order to interpret commands in a file).

ghost · 2021-09-01T17:13:36Z

pkg/entrypoint/termination

@@ -0,0 +1 @@
+[{"key":"StartedAt","value":"2021-08-19T11:40:06.469+10:00","type":"InternalTektonResult"},{"key":"ExitCode","value":"2","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.504+10:00","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.519+10:00","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.531+10:00","type":"InternalTektonResult"}]


This is an artifact of a unit test in pkg/entrypoint and should be removed I think. If you rebase on main and re-run go test ./... it shouldn't appear again.

Good catch :) I'm pretty sure I've rebased on main recently, so I'll remove that file...

ghost · 2021-09-01T17:19:11Z

pkg/pod/pod.go

@@ -133,9 +133,9 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
 	// Convert any steps with Script to command+args.
 	// If any are found, append an init container to initialize scripts.
 	if alphaAPIEnabled {
-		scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, steps, taskSpec.Sidecars, taskRun.Spec.Debug)
+		scriptsInit, stepContainers, sidecarContainers = convertScripts(b.Images.ShellImage, b.Images.ShellImageWin, steps, taskSpec.Sidecars, taskRun.Spec.Debug)


Passing nil or an empty string for the windows shell image when alphaAPIEnabled is false would work ok I think. An alternative would be to write an entirely windows-focused convertScripts implementation and only invoke it if alphaAPIEnabled is true and requiresWindows would be true. Might involve a bit more duplication or end up being more lines but it might reduce the number of windows-specific branches? Could also push a change like that back to a refactor pr in future as well though.

ghost · 2021-09-01T17:21:23Z

pkg/pod/script.go

@@ -74,16 +74,28 @@ var (
 // It does this by prepending a container that writes specified Script bodies
 // to executable files in a shared volumeMount, then produces Containers that
 // simply run those executable files.
-func convertScripts(shellImage string, steps []v1beta1.Step, sidecars []v1beta1.Sidecar, debugConfig *v1beta1.TaskRunDebug) (*corev1.Container, []corev1.Container, []corev1.Container) {
+func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta1.Step, sidecars []v1beta1.Sidecar, debugConfig *v1beta1.TaskRunDebug) (*corev1.Container, []corev1.Container, []corev1.Container) {
 	placeScripts := false
 	// Place scripts is an init container used for creating scripts in the


This comment goes with the corev1.Container struct declared on line 94 I think. Suggest moving it down or removing it.

ghost · 2021-09-01T17:27:23Z

pkg/pod/script.go

+		}
+	}
+	// If no step needs windows, then check sidecars to be sure
+	if !requiresWindows {


Given requiresWindows is never changed I don't think it's needed. Simply running the loop over steps and then sidecars should be equivalent I think?

DrWadsy · 2021-09-02T00:09:45Z

/retest

ghost · 2021-09-02T14:04:14Z

Thanks a lot for tackling those changes @DrWadsy . One more suggestion here - to maintain parity with other alpha features I think we should probably reject submitted TaskRuns / Tasks that use a windows script if the enable-api-fields feature flag is not alpha. This gives users immediate feedback when they submit rather than waiting for a runtime error in the TaskRun's description.

Adding validation is super easy - we can do this in pkg/apis/pipeline/v1beta1/task_validation.go; here's existing code that performs a very similar check for the onError field of each Step: https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/task_validation.go#L224-L233

I think the additional validation would look something like:

if s.Script != "" {
  cleaned := strings.TrimSpace(s.Script)
  if strings.HasPrefix(cleaned, "#!win") {
    errs = errs.Also(ValidateEnabledAPIFields(ctx, "windows script support", config.AlphaAPIFields).ViaField("script"))
  }
}

Adding a unit test to confirm this behaviour should then be as simple as adding an entry to the table in https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1beta1/task_validation_test.go#L1200-L1202

tekton-robot · 2021-09-03T00:09: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/apis/pipeline/v1beta1/task_validation.go	97.0%	97.1%	0.1

ghost · 2021-09-03T00:26:05Z

Nice one, cheers!

/approve

tekton-robot · 2021-09-03T00:26:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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~~ [sbwsg]

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

ghost · 2021-09-03T00:42:33Z

Also just noticed - could you squash the commits down too? We typically squash down into 1 before merge to help keep the history a little more easily scannable. I normally do that with git rebase -i HEAD^^^^^^^^^^^^^ (one caret for each of the 13 commits) then you'd need to git push --force the branch. Doing so won't affect the approve I just added.

Steps and sidecars can contain a script field. In linux tasks these scripts are copied into files which are made executable and then steps are added to the task to execute those files. This commit adds comparable functionality if scripts are used in a task which will run on a windows node. On a windows node the mechanics are different, due to how windows handles executable files. The key difference is that Tekton needs to know that a script will run on windows, and how to run the file (which interpreter to use). This is done through a ‘windows shebang’ line at the start of the script. The line must begin with ‘#!win’. After that the user needs to provide the interpreter to use, as well as any necessary arguments. The line must be written such that the name of the file containing the script to execute can be appended to the end. For example, to run the script in the file ‘test.ps1’ with powershell, the command would usually be ‘powershell -File test.ps1’ and so the shebang line must be ‘#!win powershell -File’. If no interpreter is provided (i.e. the shebang line is only ‘#!win’) then the script contents will be stored in a .cmd file and executed. Finally, since a pod cannot contain a mix of windows and linux containers a windows shell image has been added to the Images structure, which will be used in the place-scripts step when needed on a windows node. To maintain parity with other alpha features, task validation for tasks containing windows scripts will now require the 'enable-api-fields' flag to be 'alpha'. TaskRuns/Tasks that contain windows scripts will be rejected if this flag is not set, giving the user immediate feedback. The integration tests for windows scripts have been updated to reflect this alpha flag requirement.

tekton-robot · 2021-09-03T02:35: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/apis/pipeline/v1beta1/task_validation.go	97.0%	97.1%	0.1

DrWadsy · 2021-09-03T02:58:51Z

/retest

DrWadsy · 2021-09-05T22:43:46Z

@imjasonh I made some code changes and squashed this all down into a single commit, and so the PR lost the lgtm you added. Could you please check this over one more time? :)

dlorenc · 2021-09-06T00:18:24Z

@imjasonh is out for another week I think, so I took a look.

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 30, 2021

tekton-robot requested review from dlorenc and vdemeester July 30, 2021 05:31

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

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 30, 2021

tekton-robot assigned dibyom Jul 30, 2021

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 Jul 30, 2021

imjasonh reviewed Jul 30, 2021

View reviewed changes

aiden-deloryn mentioned this pull request Aug 2, 2021

Support Windows #1826

Closed

tekton-robot assigned imjasonh Aug 19, 2021

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

ghost reviewed Sep 1, 2021

View reviewed changes

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

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

DrWadsy force-pushed the windows-scripts branch from acfa9e9 to 32d8f2a Compare September 3, 2021 02:32

tekton-robot assigned dlorenc Sep 6, 2021

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

tekton-robot merged commit 25f3175 into tektoncd:main Sep 6, 2021


		`#!win <interpreter command> <args>`

		Unlike linux, we need to specify how to interpret the script file which is generated by Tekton. The example below shows how to execute a powershell script:

		@@ -0,0 +1 @@
		[{"key":"StartedAt","value":"2021-08-19T11:40:06.469+10:00","type":"InternalTektonResult"},{"key":"ExitCode","value":"2","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.504+10:00","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.519+10:00","type":"InternalTektonResult"},{"key":"StartedAt","value":"2021-08-19T11:40:06.531+10:00","type":"InternalTektonResult"}]

Add script mode support to windows tasks #4128

Add script mode support to windows tasks #4128

Conversation

DrWadsy commented Jul 30, 2021

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 30, 2021

DrWadsy commented Jul 30, 2021

vdemeester commented Jul 30, 2021

tekton-robot commented Jul 30, 2021

imjasonh 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

linux-foundation-easycla bot commented Aug 2, 2021 • edited Loading

tekton-robot commented Aug 2, 2021

DrWadsy commented Aug 9, 2021

imjasonh commented Aug 11, 2021

DrWadsy commented Aug 19, 2021

DrWadsy commented Aug 19, 2021

imjasonh commented Aug 19, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrWadsy Sep 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrWadsy commented Sep 2, 2021

ghost commented Sep 2, 2021

tekton-robot commented Sep 3, 2021

ghost commented Sep 3, 2021

tekton-robot commented Sep 3, 2021

ghost commented Sep 3, 2021

tekton-robot commented Sep 3, 2021

DrWadsy commented Sep 3, 2021

DrWadsy commented Sep 5, 2021

dlorenc commented Sep 6, 2021

linux-foundation-easycla bot commented Aug 2, 2021 •

edited

Loading

DrWadsy Sep 1, 2021 •

edited

Loading