From 9882a30342511c3e84a60ca62932825fb1e3891a Mon Sep 17 00:00:00 2001 From: Xinru Zhang Date: Wed, 3 May 2023 17:07:04 -0400 Subject: [PATCH] Refactor Sidecar Containers Construction If Script Exists Prior to this commit, in order to reuse the code, we convert v1beta1 Sidecar object to v1beta1.Step and then construct containers out of those steps when the script field is specified. As we are switching the storage version to v1, some fields are deprecated in v1.Step (but not Sidecar), thus it does not make sense to convert sidercar to step. While reusing as much code as possible, this commit refactor the code to seperate the container construction process for steps and sidecars. --- pkg/pod/script.go | 211 ++++++++++++++++++++++------------------- pkg/pod/script_test.go | 172 +++++++++++++++++++++++++++++++-- 2 files changed, 277 insertions(+), 106 deletions(-) diff --git a/pkg/pod/script.go b/pkg/pod/script.go index 23498a0b0d9..a984cd72ca8 100644 --- a/pkg/pod/script.go +++ b/pkg/pod/script.go @@ -25,7 +25,6 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/names" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( @@ -71,7 +70,6 @@ var ( // convertScripts converts any steps and sidecars that specify a Script field into a normal 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 // /tekton/scripts directory which would be later used by the step containers // as a Command @@ -96,16 +94,6 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta } breakpoints := []string{} - sideCarSteps := []v1beta1.Step{} - for _, sidecar := range sidecars { - c := sidecar.ToK8sContainer() - sidecarStep := v1beta1.Step{ - Script: sidecar.Script, - Timeout: &metav1.Duration{}, - } - sidecarStep.SetContainerFields(*c) - sideCarSteps = append(sideCarSteps, sidecarStep) - } // Add mounts for debug if debugConfig != nil && len(debugConfig.Breakpoint) > 0 { @@ -113,73 +101,84 @@ func convertScripts(shellImageLinux string, shellImageWin string, steps []v1beta placeScriptsInit.VolumeMounts = append(placeScriptsInit.VolumeMounts, debugScriptsVolumeMount) } - convertedStepContainers := convertListOfSteps(steps, &placeScriptsInit, &placeScripts, breakpoints, "script") + convertedStepContainers := convertListOfSteps(steps, &placeScriptsInit, breakpoints, "script") + sidecarContainers := convertListOfSidecars(sidecars, &placeScriptsInit, "sidecar-script") - // Pass empty breakpoint list in "sidecar step to container" converter to not rewrite the scripts and add breakpoints to sidecar - sidecarContainers := convertListOfSteps(sideCarSteps, &placeScriptsInit, &placeScripts, []string{}, "sidecar-script") - if placeScripts { + if hasScripts(steps, sidecars, breakpoints) { return &placeScriptsInit, convertedStepContainers, sidecarContainers } return nil, convertedStepContainers, sidecarContainers } -// convertListOfSteps does the heavy lifting for convertScripts. -// -// It iterates through the list of steps (or sidecars), generates the script file name and heredoc termination string, +// convertListOfSidecars iterates through the list of sidecars, generates the script file name and heredoc termination string, // adds an entry to the init container args, sets up the step container to run the script, and sets the volume mounts. -func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, placeScripts *bool, breakpoints []string, namePrefix string) []corev1.Container { +func convertListOfSidecars(sidecars []v1beta1.Sidecar, initContainer *corev1.Container, namePrefix string) []corev1.Container { containers := []corev1.Container{} - for i, s := range steps { - // Add debug mounts if breakpoints are present - if len(breakpoints) > 0 { - debugInfoVolumeMount := corev1.VolumeMount{ - Name: debugInfoVolumeName, - MountPath: filepath.Join(debugInfoDir, fmt.Sprintf("%d", i)), - } - steps[i].VolumeMounts = append(steps[i].VolumeMounts, debugScriptsVolumeMount, debugInfoVolumeMount) + for i, s := range sidecars { + c := s.ToK8sContainer() + if s.Script != "" { + placeScriptInContainer(s.Script, getScriptFile(scriptsDir, fmt.Sprintf("%s-%d", namePrefix, i)), c, initContainer) } + containers = append(containers, *c) + } + return containers +} - if s.Script == "" { - // Nothing to convert. - containers = append(containers, *steps[i].ToK8sContainer()) - continue +// convertListOfSteps iterates through the list of steps, generates the script file name and heredoc termination string, +// adds an entry to the init container args, sets up the step container to run the script, and sets the volume mounts. +func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, breakpoints []string, namePrefix string) []corev1.Container { + containers := []corev1.Container{} + for i, s := range steps { + c := steps[i].ToK8sContainer() + if s.Script != "" { + placeScriptInContainer(s.Script, getScriptFile(scriptsDir, fmt.Sprintf("%s-%d", namePrefix, i)), c, initContainer) } + containers = append(containers, *c) + } + if len(breakpoints) > 0 { + placeDebugScriptInContainers(containers, initContainer) + } + return containers +} - // Check for a shebang, and add a default if it's not set. - // The shebang must be the first non-empty line. - cleaned := strings.TrimSpace(s.Script) - hasShebang := strings.HasPrefix(cleaned, "#!") - requiresWindows := strings.HasPrefix(cleaned, "#!win") +func getScriptFile(scriptsDir, scriptName string) string { + return filepath.Join(scriptsDir, names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(scriptName)) +} - script := s.Script - if !hasShebang { - script = defaultScriptPreamble + s.Script - } +// placeScriptInContainer given a piece of script to be executed, placeScriptInContainer firstly modifies initContainer +// so that it capsules the target script into scriptFile, then it modifies the container so that it can execute the scriptFile +// in runtime. +func placeScriptInContainer(script, scriptFile string, c *corev1.Container, initContainer *corev1.Container) { + if script == "" { + return + } + cleaned := strings.TrimSpace(script) + hasShebang := strings.HasPrefix(cleaned, "#!") + requiresWindows := strings.HasPrefix(cleaned, "#!win") - // At least one step uses a script, so we should return a - // non-nil init container. - *placeScripts = true + if !hasShebang { + script = defaultScriptPreamble + script + } - // Append to the place-scripts script to place the - // script file in a known location in the scripts volume. - scriptFile := filepath.Join(scriptsDir, names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-%d", namePrefix, i))) - if requiresWindows { - command, args, script, scriptFile := extractWindowsScriptComponents(script, scriptFile) - initContainer.Args[1] += fmt.Sprintf(`@" + // Append to the place-scripts script to place the + // script file in a known location in the scripts volume. + if requiresWindows { + command, args, script, scriptFile := extractWindowsScriptComponents(script, scriptFile) + initContainer.Args[1] += fmt.Sprintf(`@" %s "@ | Out-File -FilePath %s `, script, scriptFile) - steps[i].Command = command - // Append existing args field to end of derived args - args = append(args, steps[i].Args...) - steps[i].Args = args - } else { - // Only encode the script for linux scripts - // The decode-script subcommand of the entrypoint does not work under windows - script = encodeScript(script) - heredoc := "_EOF_" // underscores because base64 doesnt include them in its alphabet - initContainer.Args[1] += fmt.Sprintf(`scriptfile="%s" + c.Command = command + // Append existing args field to end of derived args + args = append(args, c.Args...) + c.Args = args + } else { + // Only encode the script for linux scripts + // The decode-script subcommand of the entrypoint does not work under windows + script = encodeScript(script) + heredoc := "_EOF_" // underscores because base64 doesn't include them in its alphabet + initContainer.Args[1] += fmt.Sprintf(`scriptfile="%s" touch ${scriptfile} && chmod +x ${scriptfile} cat > ${scriptfile} << '%s' %s @@ -187,53 +186,67 @@ cat > ${scriptfile} << '%s' /tekton/bin/entrypoint decode-script "${scriptfile}" `, scriptFile, heredoc, script, heredoc) - // Set the command to execute the correct script in the mounted - // volume. - // A previous merge with stepTemplate may have populated - // Command and Args, even though this is not normally valid, so - // we'll clear out the Args and overwrite Command. - steps[i].Command = []string{scriptFile} - } - steps[i].VolumeMounts = append(steps[i].VolumeMounts, scriptsVolumeMount) - - containers = append(containers, *steps[i].ToK8sContainer()) + // Set the command to execute the correct script in the mounted volume. + // A previous merge with stepTemplate may have populated + // Command and Args, even though this is not normally valid, so + // we'll clear out the Args and overwrite Command. + c.Command = []string{scriptFile} } + c.VolumeMounts = append(c.VolumeMounts, scriptsVolumeMount) +} - // Place debug scripts if breakpoints are enabled - if len(breakpoints) > 0 { - // If breakpoint is not nil then should add the init container - // to write debug script files - *placeScripts = true +// encodeScript encodes a script field into a format that avoids kubernetes' built-in processing of container args, +// which can mangle dollar signs and unexpectedly replace variable references in the user's script. +func encodeScript(script string) string { + return base64.StdEncoding.EncodeToString([]byte(script)) +} - type script struct { - name string - content string - } - debugScripts := []script{{ - name: "continue", - content: defaultScriptPreamble + fmt.Sprintf(debugContinueScriptTemplate, len(steps), debugInfoDir, RunDir), - }, { - name: "fail-continue", - content: defaultScriptPreamble + fmt.Sprintf(debugFailScriptTemplate, len(steps), debugInfoDir, RunDir), - }} - - // Add debug or breakpoint related scripts to /tekton/debug/scripts - // Iterate through the debugScripts and add routine for each of them in the initContainer for their creation - for _, debugScript := range debugScripts { - tmpFile := filepath.Join(debugScriptsDir, fmt.Sprintf("%s-%s", "debug", debugScript.name)) - heredoc := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-%s-heredoc-randomly-generated", "debug", debugScript.name)) - - initContainer.Args[1] += fmt.Sprintf(initScriptDirective, tmpFile, heredoc, debugScript.content, heredoc) +// placeDebugScriptInContainers inserts debug scripts into containers. It capsules those scripts to files in initContainer, +// then executes those scripts in target containers. +func placeDebugScriptInContainers(containers []corev1.Container, initContainer *corev1.Container) { + for i := 0; i < len(containers); i++ { + debugInfoVolumeMount := corev1.VolumeMount{ + Name: debugInfoVolumeName, + MountPath: filepath.Join(debugInfoDir, fmt.Sprintf("%d", i)), } + (&containers[i]).VolumeMounts = append((&containers[i]).VolumeMounts, debugScriptsVolumeMount, debugInfoVolumeMount) } - return containers + type script struct { + name string + content string + } + debugScripts := []script{{ + name: "continue", + content: defaultScriptPreamble + fmt.Sprintf(debugContinueScriptTemplate, len(containers), debugInfoDir, RunDir), + }, { + name: "fail-continue", + content: defaultScriptPreamble + fmt.Sprintf(debugFailScriptTemplate, len(containers), debugInfoDir, RunDir), + }} + + // Add debug or breakpoint related scripts to /tekton/debug/scripts + // Iterate through the debugScripts and add routine for each of them in the initContainer for their creation + for _, debugScript := range debugScripts { + tmpFile := filepath.Join(debugScriptsDir, fmt.Sprintf("%s-%s", "debug", debugScript.name)) + heredoc := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-%s-heredoc-randomly-generated", "debug", debugScript.name)) + + initContainer.Args[1] += fmt.Sprintf(initScriptDirective, tmpFile, heredoc, debugScript.content, heredoc) + } } -// encodeScript encodes a script field into a format that avoids kubernetes' built-in processing of container args, -// which can mangle dollar signs and unexpectedly replace variable references in the user's script. -func encodeScript(script string) string { - return base64.StdEncoding.EncodeToString([]byte(script)) +// hasScripts determines if we need to generate scripts in InitContainer given steps, sidecars and breakpoints. +func hasScripts(steps []v1beta1.Step, sidecars []v1beta1.Sidecar, breakpoints []string) bool { + for _, s := range steps { + if s.Script != "" { + return true + } + } + for _, s := range sidecars { + if s.Script != "" { + return true + } + } + return len(breakpoints) > 0 } func checkWindowsRequirement(steps []v1beta1.Step, sidecars []v1beta1.Sidecar) bool { diff --git a/pkg/pod/script_test.go b/pkg/pod/script_test.go index f341be86d42..3f9f8d0be08 100644 --- a/pkg/pod/script_test.go +++ b/pkg/pod/script_test.go @@ -105,7 +105,7 @@ func TestConvertScripts_NothingToConvert_WithSidecar(t *testing.T) { } } -func TestConvertScripts(t *testing.T) { +func TestConvertScripts_Steps(t *testing.T) { names.TestingSeed() preExistingVolumeMounts := []corev1.VolumeMount{{ @@ -194,6 +194,165 @@ _EOF_ } } +func TestConvertScripts_Sidecars(t *testing.T) { + names.TestingSeed() + + preExistingVolumeMounts := []corev1.VolumeMount{{ + Name: "pre-existing-volume-mount", + MountPath: "/mount/path", + }, { + Name: "another-one", + MountPath: "/another/one", + }} + + for _, tc := range []struct { + name string + sidecars []v1beta1.Sidecar + wantInitScripts string + wantContainers []corev1.Container + }{{ + name: "simple sidecar", + sidecars: []v1beta1.Sidecar{{ + Script: `#!/bin/sh +script-1`, + Image: "sidecar-1", + }}, + wantInitScripts: `scriptfile="/tekton/scripts/sidecar-script-0-9l9zj" +touch ${scriptfile} && chmod +x ${scriptfile} +cat > ${scriptfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC0x +_EOF_ +/tekton/bin/entrypoint decode-script "${scriptfile}" +`, + wantContainers: []corev1.Container{{ + Image: "sidecar-1", + Command: []string{"/tekton/scripts/sidecar-script-0-9l9zj"}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + }}, + }, { + name: "no script", + sidecars: []v1beta1.Sidecar{{ + Image: "sidecar-2", + }}, + wantInitScripts: "", + wantContainers: []corev1.Container{{ + Image: "sidecar-2", + }}, + }, { + name: "pre-existing volumeMounts and args", + sidecars: []v1beta1.Sidecar{{ + Script: `no-shebang`, + Image: "sidecar-3", + VolumeMounts: preExistingVolumeMounts, + Args: []string{"my", "args"}, + }}, + wantInitScripts: `scriptfile="/tekton/scripts/sidecar-script-0-mz4c7" +touch ${scriptfile} && chmod +x ${scriptfile} +cat > ${scriptfile} << '_EOF_' +IyEvYmluL3NoCnNldCAtZQpuby1zaGViYW5n +_EOF_ +/tekton/bin/entrypoint decode-script "${scriptfile}" +`, + wantContainers: []corev1.Container{{ + Image: "sidecar-3", + Command: []string{"/tekton/scripts/sidecar-script-0-mz4c7"}, + Args: []string{"my", "args"}, + VolumeMounts: []corev1.VolumeMount{ + {Name: "pre-existing-volume-mount", MountPath: "/mount/path"}, + {Name: "another-one", MountPath: "/another/one"}, + scriptsVolumeMount, + }, + }}, + }, { + name: "multiple sidecars", + sidecars: []v1beta1.Sidecar{{ + Script: `#!/bin/sh +script-4`, + Image: "sidecar-4", + }, { + Script: `#!/bin/sh +script-5`, + Image: "sidecar-5", + }, { + Image: "sidecar-6", + }}, + wantInitScripts: `scriptfile="/tekton/scripts/sidecar-script-0-mssqb" +touch ${scriptfile} && chmod +x ${scriptfile} +cat > ${scriptfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC00 +_EOF_ +/tekton/bin/entrypoint decode-script "${scriptfile}" +scriptfile="/tekton/scripts/sidecar-script-1-78c5n" +touch ${scriptfile} && chmod +x ${scriptfile} +cat > ${scriptfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC01 +_EOF_ +/tekton/bin/entrypoint decode-script "${scriptfile}" +`, + wantContainers: []corev1.Container{{ + Image: "sidecar-4", + Command: []string{"/tekton/scripts/sidecar-script-0-mssqb"}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + }, { + Image: "sidecar-5", + Command: []string{"/tekton/scripts/sidecar-script-1-78c5n"}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + }, { + Image: "sidecar-6", + }}, + }, { + name: "sidecar with deprecated fields in step", + sidecars: []v1beta1.Sidecar{{ + Image: "sidecar-7", + LivenessProbe: &corev1.Probe{InitialDelaySeconds: 1}, + ReadinessProbe: &corev1.Probe{InitialDelaySeconds: 2}, + Ports: []corev1.ContainerPort{{Name: "port"}}, + StartupProbe: &corev1.Probe{InitialDelaySeconds: 3}, + Lifecycle: &corev1.Lifecycle{PostStart: &corev1.LifecycleHandler{Exec: &corev1.ExecAction{ + Command: []string{"lifecycle command"}, + }}}, + TerminationMessagePath: "path", + TerminationMessagePolicy: corev1.TerminationMessagePolicy("policy"), + Stdin: true, + StdinOnce: true, + TTY: true, + }}, + wantInitScripts: "", + wantContainers: []corev1.Container{{ + Image: "sidecar-7", + LivenessProbe: &corev1.Probe{InitialDelaySeconds: 1}, + ReadinessProbe: &corev1.Probe{InitialDelaySeconds: 2}, + Ports: []corev1.ContainerPort{{Name: "port"}}, + StartupProbe: &corev1.Probe{InitialDelaySeconds: 3}, + Lifecycle: &corev1.Lifecycle{PostStart: &corev1.LifecycleHandler{Exec: &corev1.ExecAction{ + Command: []string{"lifecycle command"}, + }}}, + TerminationMessagePath: "path", + TerminationMessagePolicy: corev1.TerminationMessagePolicy("policy"), + Stdin: true, + StdinOnce: true, + TTY: true, + }}, + }} { + t.Run(tc.name, func(t *testing.T) { + gotInit, gotSteps, gotSidecars := convertScripts(images.ShellImage, images.ShellImageWin, []v1beta1.Step{}, tc.sidecars, nil) + gotInitScripts := "" + if gotInit != nil { + gotInitScripts = gotInit.Args[1] + } + if d := cmp.Diff(tc.wantInitScripts, gotInitScripts); d != "" { + t.Errorf("Init Container Diff %s", diff.PrintWantGot(d)) + } + if d := cmp.Diff(tc.wantContainers, gotSidecars); d != "" { + t.Errorf("Containers Diff %s", diff.PrintWantGot(d)) + } + if len(gotSteps) != 0 { + t.Errorf("Expected zero steps, got %v", len(gotSteps)) + } + }) + } +} + func TestConvertScripts_WithBreakpoint_OnFailure(t *testing.T) { names.TestingSeed() @@ -301,8 +460,8 @@ debug-fail-continue-heredoc-randomly-generated-6nl7g want := []corev1.Container{{ Image: "step-1", Command: []string{"/tekton/scripts/script-0-9l9zj"}, - VolumeMounts: []corev1.VolumeMount{debugScriptsVolumeMount, - {Name: debugInfoVolumeName, MountPath: "/tekton/debug/info/0"}, scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, debugScriptsVolumeMount, + {Name: debugInfoVolumeName, MountPath: "/tekton/debug/info/0"}}, }, { Image: "step-2", VolumeMounts: []corev1.VolumeMount{ @@ -312,9 +471,8 @@ debug-fail-continue-heredoc-randomly-generated-6nl7g Image: "step-3", Command: []string{"/tekton/scripts/script-2-mz4c7"}, Args: []string{"my", "args"}, - VolumeMounts: append(preExistingVolumeMounts, debugScriptsVolumeMount, - corev1.VolumeMount{Name: debugInfoVolumeName, MountPath: "/tekton/debug/info/2"}, - scriptsVolumeMount), + VolumeMounts: append(preExistingVolumeMounts, scriptsVolumeMount, debugScriptsVolumeMount, + corev1.VolumeMount{Name: debugInfoVolumeName, MountPath: "/tekton/debug/info/2"}), }, { Image: "step-3", Command: []string{"/tekton/scripts/script-3-mssqb"}, @@ -322,9 +480,9 @@ debug-fail-continue-heredoc-randomly-generated-6nl7g VolumeMounts: []corev1.VolumeMount{ {Name: "pre-existing-volume-mount", MountPath: "/mount/path"}, {Name: "another-one", MountPath: "/another/one"}, + scriptsVolumeMount, debugScriptsVolumeMount, {Name: debugInfoVolumeName, MountPath: "/tekton/debug/info/3"}, - scriptsVolumeMount, }, }}