From 2f169707bd6af75dfb53c6275493e96e49db644e Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Fri, 10 Nov 2023 16:18:13 -0500 Subject: [PATCH] TEP-0142: Surface step results via termination message This PR surfaces step results (i.e results written to $(step.results..path)) via termination messages. A followup PR will handle surfacing the results via sidecar logs. --- cmd/entrypoint/main.go | 4 + docs/stepactions.md | 83 ++++++- .../v1/taskruns/alpha/stepaction-results.yaml | 56 +++++ pkg/entrypoint/entrypointer.go | 29 ++- pkg/pod/entrypoint.go | 60 +++++- pkg/pod/entrypoint_test.go | 204 +++++++++++++++++- pkg/pod/pod.go | 4 +- pkg/pod/status.go | 35 ++- pkg/pod/status_test.go | 148 +++++++++++++ pkg/reconciler/taskrun/resources/apply.go | 15 +- pkg/reconciler/taskrun/resources/taskspec.go | 6 +- .../taskrun/resources/taskspec_test.go | 70 ++++++ pkg/result/result.go | 4 + pkg/result/result_test.go | 4 + 14 files changed, 694 insertions(+), 28 deletions(-) create mode 100644 examples/v1/taskruns/alpha/stepaction-results.yaml diff --git a/cmd/entrypoint/main.go b/cmd/entrypoint/main.go index 54422d789e5..813f48e3747 100644 --- a/cmd/entrypoint/main.go +++ b/cmd/entrypoint/main.go @@ -49,6 +49,8 @@ var ( postFile = flag.String("post_file", "", "If specified, file to write upon completion") terminationPath = flag.String("termination_path", "/tekton/termination", "If specified, file to write upon termination") results = flag.String("results", "", "If specified, list of file names that might contain task results") + stepResults = flag.String("step_results", "", "step results if specified") + stepName = flag.String("step_name", "", "Name of the step") timeout = flag.Duration("timeout", time.Duration(0), "If specified, sets timeout for step") stdoutPath = flag.String("stdout_path", "", "If specified, file to copy stdout to") stderrPath = flag.String("stderr_path", "", "If specified, file to copy stderr to") @@ -159,11 +161,13 @@ func main() { }, PostWriter: &realPostWriter{}, Results: strings.Split(*results, ","), + StepResults: strings.Split(*stepResults, ","), Timeout: timeout, BreakpointOnFailure: *breakpointOnFailure, OnError: *onError, StepMetadataDir: *stepMetadataDir, SpireWorkloadAPI: spireWorkloadAPI, + StepName: *stepName, ResultExtractionMethod: *resultExtractionMethod, } diff --git a/docs/stepactions.md b/docs/stepactions.md index 88af514d4c5..4e7aca51b11 100644 --- a/docs/stepactions.md +++ b/docs/stepactions.md @@ -39,7 +39,7 @@ A `StepAction` definition supports the following fields: - cannot be used at the same time as using `command`. - `env` - [`params`](#declaring-params) - - [`results`](#declaring-results) + - [`results`](#emitting-results) - [`securityContext`](#declaring-securitycontext) - [`volumeMounts`](#declaring-volumemounts) @@ -93,7 +93,7 @@ spec: ] ``` -### Declaring Results +### Emitting Results A `StepAction` also declares the results that it will emit. @@ -115,6 +115,85 @@ spec: date | tee $(results.current-date-human-readable.path) ``` +In the above example, it is possible that when the same `StepAction` is used multiple times in the same `Task` or if multiple `StepActions` in the same `Task` produce a result that has the same name, resolving the names becomes critical otherwise there could be unexpected outcomes. The `Task` needs to be able to resolve these `Result` names clashes by mapping it to a differen result name. For this reason, we introduce the capability to store results on a `Step` level. + +The above `StepAction` can also emit results to `$(step.results..path)`. + +```yaml +apiVersion: tekton.dev/v1alpha1 +kind: StepAction +metadata: + name: stepaction-declaring-results +spec: + results: + - name: current-date-unix-timestamp + description: The current date in unix timestamp format + - name: current-date-human-readable + description: The current date in human readable format + image: bash:latest + script: | + #!/usr/bin/env bash + date +%s | tee $(step.results.current-date-unix-timestamp.path) + date | tee $(step.results.current-date-human-readable.path) +``` + +`Results` from the above `StepAciton` can be [fetched by the `Task`](#fetching-emitted-results-from-step-actions) using this `StepAction` via `$(steps..results.)`. + +#### Fetching Emitted Results from StepActions + +A `Task` can fetch `Results` produced by the `StepActions` (i.e. only results emitted to `$(step.results..path)`, `NOT` $(results..path)) using variable replacement syntax. We introduce a field to [`Task Results`](./tasks.md#emitting-results) called `Value` whose value can be set to the variable `$(steps..results.)`. + +```yaml +apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: task-fetching-results +spec: + results: + - name: git-url + description: "url of git repo" + value: $(steps.git-clone.results.url) + - name: registry-url + description: "url of docker registry" + value: $(steps.kaniko.results.url) + steps: + - name: git-clone + ref: + name: clone-step-action + - name: kaniko + ref: + name: kaniko-step-action +``` + +`Results` emitted to `$(step.results..path)` are not automatically available as `TaskRun Results`. The `Task` must explicitly fetch it from the underlying `Step` referencing `StepActions`. + +For example, lets assume that in the previous example, the "kaniko" `StepAction` also produced a result called "digest". In that case, the `Task` should also fetch the "digest" from "kaniko" `Step`. + +```yaml +apiVersion: tekton.dev/v1 +kind: Task +metadata: + name: task-fetching-results +spec: + results: + - name: git-url + description: "url of git repo" + value: $(steps.git-clone.results.url) + - name: registry-url + description: "url of docker registry" + value: $(steps.kaniko.results.url) + - nane: digest + description: "digest of the image" + value: $(steps.kaniko.results.digest) + steps: + - name: git-clone + ref: + name: clone-step-action + - name: kaniko + ref: + name: kaniko-step-action +``` + ### Declaring SecurityContext You can declare `securityContext` in a `StepAction`: diff --git a/examples/v1/taskruns/alpha/stepaction-results.yaml b/examples/v1/taskruns/alpha/stepaction-results.yaml new file mode 100644 index 00000000000..b59a4e31978 --- /dev/null +++ b/examples/v1/taskruns/alpha/stepaction-results.yaml @@ -0,0 +1,56 @@ +apiVersion: tekton.dev/v1alpha1 +kind: StepAction +metadata: + name: step-action-uri +spec: + results: + - name: uri + params: + - name: uri + image: alpine + script: | + echo $(params.uri) > $(step.results.uri.path) +--- +apiVersion: tekton.dev/v1alpha1 +kind: StepAction +metadata: + name: step-action-uri-digest +spec: + results: + - name: digest + - name: uri + params: + - name: uri + - name: digest + image: alpine + script: | + echo $(params.digest) > $(step.results.digest.path) + echo $(params.uri) > $(step.results.uri.path) +--- +apiVersion: tekton.dev/v1 +kind: TaskRun +metadata: + name: step-action-run +spec: + TaskSpec: + results: + - name: step-1-uri + value: $(steps.step1.results.uri) + - name: step-2-uri + value: $(steps.step2.results.uri) + - name: digest + steps: + - name: step1 + ref: + name: step-action-uri + params: + - name: uri + value: "https://github.com/tektoncd/pipeline" + - name: step2 + ref: + name: step-action-uri-digest + params: + - name: uri + value: "https://github.com/tektoncd/other" + - name: digest + value: "c8381846241cac4c93c30b6a5ac04cac51fa0a6e" diff --git a/pkg/entrypoint/entrypointer.go b/pkg/entrypoint/entrypointer.go index ac7baf16dfb..01363b01837 100644 --- a/pkg/entrypoint/entrypointer.go +++ b/pkg/entrypoint/entrypointer.go @@ -96,6 +96,8 @@ type Entrypointer struct { // PostWriter encapsulates writing files when complete. PostWriter PostWriter + // StepResults is the set of files that might contain step results + StepResults []string // Results is the set of files that might contain task results Results []string // Timeout is an optional user-specified duration within which the Step must complete @@ -110,6 +112,8 @@ type Entrypointer struct { StepMetadataDir string // SpireWorkloadAPI connects to spire and does obtains SVID based on taskrun SpireWorkloadAPI spire.EntrypointerAPIClient + // StepName is the name of the step + StepName string // ResultsDirectory is the directory to find results, defaults to pipeline.DefaultResultPath ResultsDirectory string // ResultExtractionMethod is the method using which the controller extracts the results from the task pod. @@ -136,6 +140,7 @@ type PostWriter interface { // Go optionally waits for a file, runs the command, and writes a // post file. func (e Entrypointer) Go() error { + var err error prod, _ := zap.NewProduction() logger := prod.Sugar() @@ -147,6 +152,11 @@ func (e Entrypointer) Go() error { _ = logger.Sync() }() + if e.StepName != "" && e.StepResults != nil { + if err := os.MkdirAll(filepath.Join(e.StepMetadataDir, "results"), os.ModePerm); err != nil { + return err + } + } for _, f := range e.WaitFiles { if err := e.Waiter.Wait(context.Background(), f, e.WaitFileContent, e.BreakpointOnFailure); err != nil { // An error happened while waiting, so we bail @@ -170,7 +180,6 @@ func (e Entrypointer) Go() error { ResultType: result.InternalTektonResultType, }) - var err error if e.Timeout != nil && *e.Timeout < time.Duration(0) { err = fmt.Errorf("negative timeout specified") } @@ -232,7 +241,7 @@ func (e Entrypointer) Go() error { // strings.Split(..) with an empty string returns an array that contains one element, an empty string. // This creates an error when trying to open the result folder as a file. - if len(e.Results) >= 1 && e.Results[0] != "" { + if (len(e.Results) >= 1 && e.Results[0] != "") || (len(e.StepResults) >= 1 && e.StepResults[0] != "") { resultPath := pipeline.DefaultResultPath if e.ResultsDirectory != "" { resultPath = e.ResultsDirectory @@ -264,6 +273,22 @@ func (e Entrypointer) readResultsFromDisk(ctx context.Context, resultDir string) ResultType: result.TaskRunResultType, }) } + for _, resultFile := range e.StepResults { + if resultFile == "" { + continue + } + fileContents, err := os.ReadFile(filepath.Join(pipeline.StepsDir, fmt.Sprintf("%s%s", pod.StepPrefix, e.StepName), "results", resultFile)) + if os.IsNotExist(err) { + continue + } else if err != nil { + return err + } + output = append(output, result.RunResult{ + Key: fmt.Sprintf("%s.%s", e.StepName, resultFile), + Value: string(fileContents), + ResultType: result.StepResultType, + }) + } if e.SpireWorkloadAPI != nil { signed, err := e.SpireWorkloadAPI.Sign(ctx, output) if err != nil { diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index d7cd3506405..f3a37d2ac01 100644 --- a/pkg/pod/entrypoint.go +++ b/pkg/pod/entrypoint.go @@ -57,7 +57,8 @@ const ( readyAnnotation = "tekton.dev/ready" readyAnnotationValue = "READY" - stepPrefix = "step-" + // StepPrefix is the prefix applied to the container name of a step container. + StepPrefix = "step-" sidecarPrefix = "sidecar-" downwardMountCancelFile = "cancel" @@ -124,7 +125,7 @@ var ( // command, we must have fetched the image's ENTRYPOINT before calling this // method, using entrypoint_lookup.go. // Additionally, Step timeouts are added as entrypoint flag. -func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1.TaskSpec, breakpointConfig *v1.TaskRunDebug, waitForReadyAnnotation, enableKeepPodOnCancel bool) ([]corev1.Container, error) { +func orderContainers(ctx context.Context, commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1.TaskSpec, breakpointConfig *v1.TaskRunDebug, waitForReadyAnnotation, enableKeepPodOnCancel bool) ([]corev1.Container, error) { if len(steps) == 0 { return nil, errors.New("No steps specified") } @@ -149,6 +150,14 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe "-termination_path", terminationPath, "-step_metadata_dir", filepath.Join(RunDir, idx, "status"), ) + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions { + stepName := s.Name + if s.Name == "" { + stepName = fmt.Sprintf("unnamed-%d", i) + } + argsForEntrypoint = append(argsForEntrypoint, "-step_name", stepName) + } + argsForEntrypoint = append(argsForEntrypoint, commonExtraEntrypointArgs...) if taskSpec != nil { if taskSpec.Steps != nil && len(taskSpec.Steps) >= i+1 { @@ -170,6 +179,13 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe } } argsForEntrypoint = append(argsForEntrypoint, resultArgument(steps, taskSpec.Results)...) + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions { + stepResultArgs, err := stepResultArgument(taskSpec.Results, s.Name) + if err != nil { + return nil, err + } + argsForEntrypoint = append(argsForEntrypoint, stepResultArgs...) + } } if breakpointConfig != nil && breakpointConfig.NeedsDebugOnFailure() { @@ -199,6 +215,34 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe return steps, nil } +func stepResultArgument(results []v1.TaskResult, stepName string) ([]string, error) { + if len(results) == 0 { + return nil, nil + } + if stepName == "" { + return nil, nil + } + res := []string{} + for _, r := range results { + if r.Value != nil { + if r.Value.StringVal != "" { + sName, resultName, err := v1.ExtractStepResultName(r.Value.StringVal) + if err != nil { + return nil, err + } + if stepName == sName { + res = append(res, resultName) + } + } + } + } + // No step results need fetching + if len(res) == 0 { + return nil, nil + } + return []string{"-step_results", strings.Join(res, ",")}, nil +} + func resultArgument(steps []corev1.Container, results []v1.TaskResult) []string { if len(results) == 0 { return nil @@ -209,7 +253,9 @@ func resultArgument(steps []corev1.Container, results []v1.TaskResult) []string func collectResultsName(results []v1.TaskResult) string { var resultNames []string for _, r := range results { - resultNames = append(resultNames, r.Name) + if r.Value == nil { + resultNames = append(resultNames, r.Name) + } } return strings.Join(resultNames, ",") } @@ -318,14 +364,14 @@ func IsSidecarStatusRunning(tr *v1.TaskRun) bool { // IsContainerStep returns true if the container name indicates that it // represents a step. -func IsContainerStep(name string) bool { return strings.HasPrefix(name, stepPrefix) } +func IsContainerStep(name string) bool { return strings.HasPrefix(name, StepPrefix) } // isContainerSidecar returns true if the container name indicates that it // represents a sidecar. func isContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } // trimStepPrefix returns the container name, stripped of its step prefix. -func trimStepPrefix(name string) string { return strings.TrimPrefix(name, stepPrefix) } +func trimStepPrefix(name string) string { return strings.TrimPrefix(name, StepPrefix) } // TrimSidecarPrefix returns the container name, stripped of its sidecar // prefix. @@ -335,7 +381,7 @@ func TrimSidecarPrefix(name string) string { return strings.TrimPrefix(name, sid // returns "step-unnamed-" if not specified func StepName(name string, i int) string { if name != "" { - return fmt.Sprintf("%s%s", stepPrefix, name) + return fmt.Sprintf("%s%s", StepPrefix, name) } - return fmt.Sprintf("%sunnamed-%d", stepPrefix, i) + return fmt.Sprintf("%sunnamed-%d", StepPrefix, i) } diff --git a/pkg/pod/entrypoint_test.go b/pkg/pod/entrypoint_test.go index 56d57c8a146..0b9acfbbc98 100644 --- a/pkg/pod/entrypoint_test.go +++ b/pkg/pod/entrypoint_test.go @@ -19,6 +19,7 @@ package pod import ( "context" "errors" + "fmt" "testing" "time" @@ -95,7 +96,7 @@ func TestOrderContainers(t *testing.T) { }, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, nil, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, nil, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -104,6 +105,60 @@ func TestOrderContainers(t *testing.T) { } } +func TestOrderContainersStepActions(t *testing.T) { + steps := []corev1.Container{{ + Name: "named-step", + Image: "step-1", + Command: []string{"cmd"}, + Args: []string{"arg1", "arg2"}, + }, { + Image: "step-1", + Command: []string{"cmd"}, + Args: []string{"arg1", "arg2"}, + }} + want := []corev1.Container{{ + Name: "named-step", + Image: "step-1", + Command: []string{entrypointBinary}, + Args: []string{ + "-wait_file", "/tekton/downward/ready", + "-wait_file_content", + "-post_file", "/tekton/run/0/out", + "-termination_path", "/tekton/termination", + "-step_metadata_dir", "/tekton/run/0/status", + "-step_name", "named-step", + "-entrypoint", "cmd", "--", + "arg1", "arg2", + }, + VolumeMounts: []corev1.VolumeMount{downwardMount}, + TerminationMessagePath: "/tekton/termination", + }, { + Image: "step-1", + Command: []string{entrypointBinary}, + Args: []string{ + "-wait_file", "/tekton/run/0/out", + "-post_file", "/tekton/run/1/out", + "-termination_path", "/tekton/termination", + "-step_metadata_dir", "/tekton/run/1/status", + "-step_name", "unnamed-1", + "-entrypoint", "cmd", "--", + "arg1", "arg2", + }, + TerminationMessagePath: "/tekton/termination", + }} + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableStepActions: true, + }, + }) + got, err := orderContainers(ctx, []string{}, steps, nil, nil, true, false) + if err != nil { + t.Fatalf("orderContainers: %v", err) + } + if d := cmp.Diff(want, got); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } +} func TestOrderContainersWithResultsSidecarLogs(t *testing.T) { steps := []corev1.Container{{ Image: "step-1", @@ -163,7 +218,7 @@ func TestOrderContainersWithResultsSidecarLogs(t *testing.T) { }, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{"-dont_send_results_to_termination_path"}, steps, nil, nil, true, false) + got, err := orderContainers(context.Background(), []string{"-dont_send_results_to_termination_path"}, steps, nil, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -209,7 +264,7 @@ func TestOrderContainersWithNoWait(t *testing.T) { VolumeMounts: []corev1.VolumeMount{volumeMount}, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, nil, nil, false, false) + got, err := orderContainers(context.Background(), []string{}, steps, nil, nil, false, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -245,7 +300,7 @@ func TestOrderContainersWithDebugOnFailure(t *testing.T) { OnFailure: "enabled", }, } - got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, nil, taskRunDebugConfig, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -273,7 +328,134 @@ func TestOrderContainersWithEnabelKeepPodOnCancel(t *testing.T) { VolumeMounts: []corev1.VolumeMount{downwardMount}, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, nil, nil, false, true) + got, err := orderContainers(context.Background(), []string{}, steps, nil, nil, false, true) + if err != nil { + t.Fatalf("orderContainers: %v", err) + } + if d := cmp.Diff(want, got); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } +} + +func TestStepResultArgument(t *testing.T) { + for _, tc := range []struct { + name string + results []v1.TaskResult + stepName string + want []string + }{{ + name: "no results", + stepName: "named-step", + }, { + name: "no stepName", + results: []v1.TaskResult{{ + Name: "sum", + Description: "This is the sum result of the task", + }}, + }, { + name: "fetched step result", + results: []v1.TaskResult{{ + Name: "sum", + Description: "This is the sum result of the task", + }, { + Name: "sub", + Description: "This is the sub result of the task", + Value: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(steps.named-step.results.sub)", + }, + }}, + stepName: "named-step", + want: []string{"-step_results", "sub"}, + }} { + t.Run(tc.name, func(t *testing.T) { + got, err := stepResultArgument(tc.results, tc.stepName) + if err != nil { + t.Fatalf("stepResultArgument(): Did not expect an error but got %v", err) + } + if d := cmp.Diff(tc.want, got); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } + }) + } +} + +func TestStepResultArgumentError(t *testing.T) { + for _, tc := range []struct { + name string + results []v1.TaskResult + stepName string + err error + }{{ + name: "invalid result", + results: []v1.TaskResult{{ + Name: "subNad", + Description: "invalid result", + Value: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "invalid", + }, + }}, + stepName: "named-step", + err: fmt.Errorf(`Could not extract step name and result name. Expected value to look like $(steps..results.) but got "invalid"`), + }} { + t.Run(tc.name, func(t *testing.T) { + got, err := stepResultArgument(tc.results, tc.stepName) + if got != nil { + t.Fatalf("stepResultArgument(): Did not expect an output but got %v", got) + } + if d := cmp.Diff(tc.err.Error(), err.Error()); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } + }) + } +} +func TestEntryPointStepActionResults(t *testing.T) { + taskSpec := v1.TaskSpec{ + Results: []v1.TaskResult{{ + Name: "sum", + Description: "This is the sum result of the task", + }, { + Name: "sub", + Description: "This is the sub result of the task", + Value: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(steps.named-step.results.sub)", + }, + }}, + } + + steps := []corev1.Container{{ + Name: "named-step", + Image: "step-1", + Command: []string{"cmd"}, + Args: []string{"arg1", "arg2"}, + }} + want := []corev1.Container{{ + Name: "named-step", + Image: "step-1", + Command: []string{entrypointBinary}, + Args: []string{ + "-wait_file", "/tekton/downward/ready", + "-wait_file_content", + "-post_file", "/tekton/run/0/out", + "-termination_path", "/tekton/termination", + "-step_metadata_dir", "/tekton/run/0/status", + "-step_name", "named-step", + "-results", "sum", + "-step_results", "sub", + "-entrypoint", "cmd", "--", + "arg1", "arg2", + }, + VolumeMounts: []corev1.VolumeMount{downwardMount}, + TerminationMessagePath: "/tekton/termination", + }} + ctx := config.ToContext(context.Background(), &config.Config{ + FeatureFlags: &config.FeatureFlags{ + EnableStepActions: true, + }, + }) + got, err := orderContainers(ctx, []string{}, steps, &taskSpec, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -351,7 +533,7 @@ func TestEntryPointResults(t *testing.T) { }, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, &taskSpec, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, &taskSpec, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -392,7 +574,7 @@ func TestEntryPointResultsSingleStep(t *testing.T) { VolumeMounts: []corev1.VolumeMount{downwardMount}, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, &taskSpec, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, &taskSpec, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -429,7 +611,7 @@ func TestEntryPointSingleResultsSingleStep(t *testing.T) { VolumeMounts: []corev1.VolumeMount{downwardMount}, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, &taskSpec, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, &taskSpec, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -500,7 +682,7 @@ func TestEntryPointOnError(t *testing.T) { err: errors.New("task step onError must be either \"continue\" or \"stopAndFail\" but it is set to an invalid value \"invalid-on-error\""), }} { t.Run(tc.desc, func(t *testing.T) { - got, err := orderContainers([]string{}, steps, &tc.taskSpec, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, &tc.taskSpec, nil, true, false) if len(tc.wantContainers) == 0 { if err == nil { t.Fatalf("expected an error for an invalid value for onError but received none") @@ -599,7 +781,7 @@ func TestEntryPointStepOutputConfigs(t *testing.T) { }, TerminationMessagePath: "/tekton/termination", }} - got, err := orderContainers([]string{}, steps, &taskSpec, nil, true, false) + got, err := orderContainers(context.Background(), []string{}, steps, &taskSpec, nil, true, false) if err != nil { t.Fatalf("orderContainers: %v", err) } @@ -726,7 +908,7 @@ const nopImage = "nop-image" // image. func TestStopSidecars(t *testing.T) { stepContainer := corev1.Container{ - Name: stepPrefix + "my-step", + Name: StepPrefix + "my-step", Image: "foo", } sidecarContainer := corev1.Container{ diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index cfc7d69d683..04a0a2dec0e 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -239,9 +239,9 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1.TaskRun, taskSpec v1.Ta readyImmediately := isPodReadyImmediately(*featureFlags, taskSpec.Sidecars) if alphaAPIEnabled { - stepContainers, err = orderContainers(commonExtraEntrypointArgs, stepContainers, &taskSpec, taskRun.Spec.Debug, !readyImmediately, enableKeepPodOnCancel) + stepContainers, err = orderContainers(ctx, commonExtraEntrypointArgs, stepContainers, &taskSpec, taskRun.Spec.Debug, !readyImmediately, enableKeepPodOnCancel) } else { - stepContainers, err = orderContainers(commonExtraEntrypointArgs, stepContainers, &taskSpec, nil, !readyImmediately, enableKeepPodOnCancel) + stepContainers, err = orderContainers(ctx, commonExtraEntrypointArgs, stepContainers, &taskSpec, nil, !readyImmediately, enableKeepPodOnCancel) } if err != nil { return nil, err diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 7c52ca4ce32..8a9a778435b 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -273,8 +273,20 @@ func filterResults(results []result.RunResult, specResults []v1.TaskResult) ([]v var taskResults []v1.TaskRunResult var filteredResults []result.RunResult neededTypes := make(map[string]v1.ResultsType) + neededStepTypes := make(map[string]v1.ResultsType) + neededStepResults := make(map[string]string) for _, r := range specResults { neededTypes[r.Name] = r.Type + if r.Value != nil { + if r.Value.StringVal != "" { + stepName, resultName, err := v1.ExtractStepResultName(r.Value.StringVal) + if err != nil { + continue + } + neededStepResults[fmt.Sprintf("%s.%s", stepName, resultName)] = r.Name + neededStepTypes[fmt.Sprintf("%s.%s", stepName, resultName)] = r.Type + } + } } for _, r := range results { switch r.ResultType { @@ -300,6 +312,28 @@ func filterResults(results []result.RunResult, specResults []v1.TaskResult) ([]v } taskResults = append(taskResults, taskRunResult) filteredResults = append(filteredResults, r) + case result.StepResultType: + var taskRunResult v1.TaskRunResult + if neededStepTypes[r.Key] == v1.ResultsTypeString { + taskRunResult = v1.TaskRunResult{ + Name: neededStepResults[r.Key], + Type: v1.ResultsTypeString, + Value: *v1.NewStructuredValues(r.Value), + } + } else { + v := v1.ResultValue{} + err := v.UnmarshalJSON([]byte(r.Value)) + if err != nil { + continue + } + taskRunResult = v1.TaskRunResult{ + Name: neededStepResults[r.Key], + Type: v1.ResultsType(v.Type), + Value: v, + } + } + taskResults = append(taskResults, taskRunResult) + filteredResults = append(filteredResults, r) case result.InternalTektonResultType: // Internal messages are ignored because they're not used as external result continue @@ -307,7 +341,6 @@ func filterResults(results []result.RunResult, specResults []v1.TaskResult) ([]v filteredResults = append(filteredResults, r) } } - return taskResults, filteredResults } diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 77c0176399a..660c689de58 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -189,6 +189,154 @@ func TestSetTaskRunStatusBasedOnStepStatus_sidecar_logs(t *testing.T) { } } +func TestMakeTaskRunStatus_StepResults(t *testing.T) { + for _, c := range []struct { + desc string + podStatus corev1.PodStatus + pod corev1.Pod + tr v1.TaskRun + want v1.TaskRunStatus + }{{ + desc: "step results string type", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-one", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"one.uri","value":"https://foo.bar\n","type":4}]`, + }, + }, + }}, + }, + tr: v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-run", + Namespace: "foo", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Results: []v1.TaskResult{{ + Name: "task-result", + Type: v1.ResultsTypeString, + Value: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(steps.one.results.uri)", + }, + }}, + }, + }, + }, + want: v1.TaskRunStatus{ + Status: statusSuccess(), + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"one.uri","value":"https://foo.bar\n","type":4}]`, + }}, + Name: "one", + Container: "step-one", + }}, + Sidecars: []v1.SidecarState{}, + Results: []v1.TaskRunResult{{ + Name: "task-result", + Type: v1.ResultsTypeString, + Value: *v1.NewStructuredValues("https://foo.bar\n"), + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "step results array type", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-one", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"one.array","value":"[\"hello\",\"world\"]","type":4}]`, + }, + }, + }}, + }, + tr: v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-run", + Namespace: "foo", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Results: []v1.TaskResult{{ + Name: "resultName", + Type: v1.ResultsTypeArray, + Value: &v1.ParamValue{ + Type: v1.ParamTypeString, + StringVal: "$(steps.one.results.array)", + }, + }}, + }, + }, + }, + want: v1.TaskRunStatus{ + Status: statusSuccess(), + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"one.array","value":"[\"hello\",\"world\"]","type":4}]`, + }}, + Name: "one", + Container: "step-one", + }}, + Sidecars: []v1.SidecarState{}, + Results: []v1.TaskRunResult{{ + Name: "resultName", + Type: v1.ResultsTypeArray, + Value: *v1.NewStructuredValues("hello", "world"), + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }} { + t.Run(c.desc, func(t *testing.T) { + now := metav1.Now() + if cmp.Diff(c.pod, corev1.Pod{}) == "" { + c.pod = corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Namespace: "foo", + CreationTimestamp: now, + }, + Status: c.podStatus, + } + } + + logger, _ := logging.NewLogger("", "status") + kubeclient := fakek8s.NewSimpleClientset() + got, err := MakeTaskRunStatus(context.Background(), logger, c.tr, &c.pod, kubeclient, &v1.TaskSpec{}) + if err != nil { + t.Errorf("MakeTaskRunResult: %s", err) + } + + // Common traits, set for test case brevity. + c.want.PodName = "pod" + + ensureTimeNotNil := cmp.Comparer(func(x, y *metav1.Time) bool { + if x == nil { + return y == nil + } + return y != nil + }) + if d := cmp.Diff(c.want, got, ignoreVolatileTime, ensureTimeNotNil); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestMakeTaskRunStatus(t *testing.T) { for _, c := range []struct { desc string diff --git a/pkg/reconciler/taskrun/resources/apply.go b/pkg/reconciler/taskrun/resources/apply.go index 0af13115fda..5f67bead047 100644 --- a/pkg/reconciler/taskrun/resources/apply.go +++ b/pkg/reconciler/taskrun/resources/apply.go @@ -24,6 +24,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/container" "github.com/tektoncd/pipeline/pkg/pod" "github.com/tektoncd/pipeline/pkg/substitution" @@ -44,14 +45,20 @@ var ( // FIXME(vdemeester) Remove that with deprecating v1beta1 "inputs.params.%s", } + stepActionResultPatterns = []string{ + "step.results.%s.path", + "step.results[%q].path", + "step.results['%s'].path", + } ) // applyStepActionParameters applies the params from the Task and the underlying Step to the referenced StepAction. -func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, stepParams v1.Params, defaults []v1.ParamSpec) *v1.Step { +func applyStepActionParametersAndResults(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, stepParams v1.Params, stepActionSpec v1alpha1.StepActionSpec, stepName string) *v1.Step { if stepParams != nil { stringR, arrayR, objectR := getTaskParameters(spec, tr, spec.Params...) stepParams = stepParams.ReplaceVariables(stringR, arrayR, objectR) } + defaults := stepActionSpec.Params // Set params from StepAction defaults stringReplacements, arrayReplacements, _ := replacementsFromDefaultParams(defaults) @@ -63,6 +70,12 @@ func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, for k, v := range stepArrays { arrayReplacements[k] = v } + // Overwrite step result path reference + for _, result := range stepActionSpec.Results { + for _, pattern := range stepActionResultPatterns { + stringReplacements[fmt.Sprintf(pattern, result.Name)] = filepath.Join(pipeline.StepsDir, stepName, "results", result.Name) + } + } container.ApplyStepReplacements(step, stringReplacements, arrayReplacements) return step diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index cf24b19b977..4dc2587eea3 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -25,6 +25,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" resolutionutil "github.com/tektoncd/pipeline/pkg/internal/resolution" + "github.com/tektoncd/pipeline/pkg/pod" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" "github.com/tektoncd/pipeline/pkg/trustedresources" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -103,7 +104,7 @@ func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask) (*re // GetStepActionsData extracts the StepActions and merges them with the inlined Step specification. func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskRun, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) ([]v1.Step, error) { steps := []v1.Step{} - for _, step := range taskSpec.Steps { + for i, step := range taskSpec.Steps { s := step.DeepCopy() if step.Ref != nil { getStepAction := GetStepActionFunc(tekton, k8s, requester, taskRun, s) @@ -134,7 +135,8 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T if err := validateStepHasStepActionParameters(s.Params, stepActionSpec.Params); err != nil { return nil, err } - s = applyStepActionParameters(s, &taskSpec, taskRun, s.Params, stepActionSpec.Params) + stepName := pod.StepName(s.Name, i) + s = applyStepActionParametersAndResults(s, &taskSpec, taskRun, s.Params, stepActionSpec, stepName) s.Params = nil s.Ref = nil steps = append(steps, *s) diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index 33e11ae9e9b..73d71c9183a 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -786,6 +786,76 @@ func TestGetStepActionsData(t *testing.T) { Image: "myimage", Args: []string{"taskrun string param", "taskspec", "array", "taskspec", "array", "param", "taskrun key", "taskspec key2", "step action key3"}, }}, + }, { + name: "an unnamed step emitting results to step result path", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{ + Ref: &v1.Ref{ + Name: "stepAction", + }, + }}, + }, + }, + }, + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ + Image: "myimage", + Results: []v1alpha1.StepActionResult{{ + Name: "resultName", + }}, + Script: "echo hello | tee $(step.results.resultName.path)", + }, + }, + want: []v1.Step{{ + Image: "myimage", + Script: "echo hello | tee /tekton/steps/step-unnamed-0/results/resultName", + }}, + }, { + name: "a named step emitting results to step result path", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{ + Name: "mystep", + Ref: &v1.Ref{ + Name: "stepAction", + }, + }}, + }, + }, + }, + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ + Image: "myimage", + Results: []v1alpha1.StepActionResult{{ + Name: "resultName", + }}, + Script: "echo hello | tee $(step.results.resultName.path)", + }, + }, + want: []v1.Step{{ + Name: "mystep", + Image: "myimage", + Script: "echo hello | tee /tekton/steps/step-mystep/results/resultName", + }}, }} for _, tt := range tests { ctx := context.Background() diff --git a/pkg/result/result.go b/pkg/result/result.go index cfcbc3e90a2..515fe9a6025 100644 --- a/pkg/result/result.go +++ b/pkg/result/result.go @@ -33,6 +33,8 @@ const ( InternalTektonResultType = 3 // UnknownResultType default unknown result type value UnknownResultType = 10 + // StepResultType default step result value + StepResultType ResultType = 4 ) // RunResult is used to write key/value pairs to TaskRun pod termination messages. @@ -80,6 +82,8 @@ func (r *ResultType) UnmarshalJSON(data []byte) error { } switch asString { + case "StepResult": + *r = StepResultType case "TaskRunResult": *r = TaskRunResultType case "InternalTektonResult": diff --git a/pkg/result/result_test.go b/pkg/result/result_test.go index ee2e5c366c4..41006aed079 100644 --- a/pkg/result/result_test.go +++ b/pkg/result/result_test.go @@ -33,6 +33,10 @@ func TestRunResult_UnmarshalJSON(t *testing.T) { name: "type defined as string - TaskRunResult", data: "{\"key\":\"resultName\",\"value\":\"resultValue\", \"type\": \"TaskRunResult\"}", pr: RunResult{Key: "resultName", Value: "resultValue", ResultType: TaskRunResultType}, + }, { + name: "type defined as string - StepResult", + data: "{\"key\":\"resultName\",\"value\":\"resultValue\", \"type\": \"StepResult\"}", + pr: RunResult{Key: "resultName", Value: "resultValue", ResultType: StepResultType}, }, { name: "type defined as string - InternalTektonResult",