From 3d63cf72db13d5c6d4ab4fd83b6c3051fc25e553 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 2 Nov 2023 20:24:40 +0000 Subject: [PATCH] [TEP-0142] Refactor extractStepActions This commit moves extractStepActions to a dedicated function called in Taskrun Reconciler, this can help the work for remote resolution of StepAction and also decouple the functions. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- pkg/reconciler/taskrun/resources/taskref.go | 4 +- pkg/reconciler/taskrun/resources/taskspec.go | 15 +- .../taskrun/resources/taskspec_test.go | 195 ++++++++++-------- pkg/reconciler/taskrun/taskrun.go | 19 +- 5 files changed, 134 insertions(+), 101 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 9df6aff28a1..aac9fba25ba 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -361,7 +361,7 @@ func (c *Reconciler) resolvePipelineState( pst, ) if err != nil { - if tresources.IsGetTaskErrTransient(err) { + if tresources.IsErrTransient(err) { return nil, err } if errors.Is(err, remote.ErrRequestInProgress) { diff --git a/pkg/reconciler/taskrun/resources/taskref.go b/pkg/reconciler/taskrun/resources/taskref.go index 03896f7976a..a22ee654e78 100644 --- a/pkg/reconciler/taskrun/resources/taskref.go +++ b/pkg/reconciler/taskrun/resources/taskref.go @@ -251,8 +251,8 @@ func (l *LocalStepActionRefResolver) GetStepAction(ctx context.Context, name str return stepAction, nil, nil } -// IsGetTaskErrTransient returns true if an error returned by GetTask is retryable. -func IsGetTaskErrTransient(err error) bool { +// IsErrTransient returns true if an error returned by GetTask/GetStepAction is retryable. +func IsErrTransient(err error) bool { return strings.Contains(err.Error(), errEtcdLeaderChange) } diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index 19954a78950..4cc85145bb9 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -23,6 +23,7 @@ import ( v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "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/trustedresources" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -51,7 +52,7 @@ type GetTaskRun func(string) (*v1.TaskRun, error) // GetTaskData will retrieve the Task metadata and Spec associated with the // provided TaskRun. This can come from a reference Task or from the TaskRun's // metadata and embedded TaskSpec. -func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, getStepAction GetStepAction) (*resolutionutil.ResolvedObjectMeta, *v1.TaskSpec, error) { +func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask) (*resolutionutil.ResolvedObjectMeta, *v1.TaskSpec, error) { taskMeta := metav1.ObjectMeta{} taskSpec := v1.TaskSpec{} var refSource *v1.RefSource @@ -89,13 +90,6 @@ func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, getS return nil, nil, fmt.Errorf("taskRun %s not providing TaskRef or TaskSpec", taskRun.Name) } - steps, err := extractStepActions(ctx, taskSpec, getStepAction) - if err != nil { - return nil, nil, err - } else { - taskSpec.Steps = steps - } - taskSpec.SetDefaults(ctx) return &resolutionutil.ResolvedObjectMeta{ ObjectMeta: &taskMeta, @@ -104,13 +98,14 @@ func GetTaskData(ctx context.Context, taskRun *v1.TaskRun, getTask GetTask, getS }, &taskSpec, nil } -// extractStepActions extracts the StepActions and merges them with the inlined Step specification. -func extractStepActions(ctx context.Context, taskSpec v1.TaskSpec, getStepAction GetStepAction) ([]v1.Step, error) { +// GetStepActionsData extracts the StepActions and merges them with the inlined Step specification. +func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, tr *v1.TaskRun, tekton clientset.Interface) ([]v1.Step, error) { steps := []v1.Step{} for _, step := range taskSpec.Steps { s := step.DeepCopy() if step.Ref != nil { s.Ref = nil + getStepAction := GetStepActionFunc(tekton, tr.Namespace) stepAction, _, err := getStepAction(ctx, step.Ref.Name) if err != nil { return nil, err diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index c5eaa6893b4..37c174bcb35 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -27,6 +27,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/client/clientset/versioned/fake" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/trustedresources" "github.com/tektoncd/pipeline/test/diff" @@ -34,20 +35,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func getStepAction(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) { - stepAction := &v1alpha1.StepAction{ - ObjectMeta: metav1.ObjectMeta{ - Name: "stepAction", - }, - Spec: v1alpha1.StepActionSpec{ - Image: "myimage", - Command: []string{"ls"}, - Args: []string{"-lh"}, - }, - } - return stepAction, nil, nil -} - func TestGetTaskSpec_Ref(t *testing.T) { task := &v1.Task{ ObjectMeta: metav1.ObjectMeta{ @@ -73,7 +60,7 @@ func TestGetTaskSpec_Ref(t *testing.T) { gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { return task, sampleRefSource.DeepCopy(), nil, nil } - resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt, getStepAction) + resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt) if err != nil { t.Fatalf("Did not expect error getting task spec but got: %s", err) @@ -107,7 +94,7 @@ func TestGetTaskSpec_Embedded(t *testing.T) { gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { return nil, nil, nil, errors.New("shouldn't be called") } - resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt, getStepAction) + resolvedObjectMeta, taskSpec, err := resources.GetTaskData(context.Background(), tr, gt) if err != nil { t.Fatalf("Did not expect error getting task spec but got: %s", err) @@ -136,7 +123,7 @@ func TestGetTaskSpec_Invalid(t *testing.T) { gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { return nil, nil, nil, errors.New("shouldn't be called") } - _, _, err := resources.GetTaskData(context.Background(), tr, gt, getStepAction) + _, _, err := resources.GetTaskData(context.Background(), tr, gt) if err == nil { t.Fatalf("Expected error resolving spec with no embedded or referenced task spec but didn't get error") } @@ -156,7 +143,7 @@ func TestGetTaskSpec_Error(t *testing.T) { gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { return nil, nil, nil, errors.New("something went wrong") } - _, _, err := resources.GetTaskData(context.Background(), tr, gt, getStepAction) + _, _, err := resources.GetTaskData(context.Background(), tr, gt) if err == nil { t.Fatalf("Expected error when unable to find referenced Task but got none") } @@ -200,7 +187,7 @@ func TestGetTaskData_ResolutionSuccess(t *testing.T) { }, sampleRefSource.DeepCopy(), nil, nil } ctx := context.Background() - resolvedMeta, resolvedSpec, err := resources.GetTaskData(ctx, tr, getTask, getStepAction) + resolvedMeta, resolvedSpec, err := resources.GetTaskData(ctx, tr, getTask) if err != nil { t.Fatalf("Unexpected error getting mocked data: %v", err) } @@ -234,7 +221,7 @@ func TestGetPipelineData_ResolutionError(t *testing.T) { return nil, nil, nil, errors.New("something went wrong") } ctx := context.Background() - _, _, err := resources.GetTaskData(ctx, tr, getTask, getStepAction) + _, _, err := resources.GetTaskData(ctx, tr, getTask) if err == nil { t.Fatalf("Expected error when unable to find referenced Task but got none") } @@ -257,7 +244,7 @@ func TestGetTaskData_ResolvedNilTask(t *testing.T) { return nil, nil, nil, nil } ctx := context.Background() - _, _, err := resources.GetTaskData(ctx, tr, getTask, getStepAction) + _, _, err := resources.GetTaskData(ctx, tr, getTask) if err == nil { t.Fatalf("Expected error when unable to find referenced Task but got none") } @@ -304,7 +291,7 @@ func TestGetTaskData_VerificationResult(t *testing.T) { Spec: *sourceSpec.DeepCopy(), }, nil, verificationResult, nil } - r, _, err := resources.GetTaskData(context.Background(), tr, getTask, getStepAction) + r, _, err := resources.GetTaskData(context.Background(), tr, getTask) if err != nil { t.Fatalf("Did not expect error but got: %s", err) } @@ -313,17 +300,18 @@ func TestGetTaskData_VerificationResult(t *testing.T) { } } -func TestGetStepAction(t *testing.T) { +func TestGetStepActionsData(t *testing.T) { tests := []struct { - name string - tr *v1.TaskRun - want *v1.TaskSpec - stepActionFunc func(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) + name string + tr *v1.TaskRun + stepAction *v1alpha1.StepAction + want []v1.Step }{{ name: "step-action-with-command-args", tr: &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Name: "mytaskrun", + Name: "mytaskrun", + Namespace: "default", }, Spec: v1.TaskRunSpec{ TaskSpec: &v1.TaskSpec{ @@ -337,21 +325,30 @@ func TestGetStepAction(t *testing.T) { }, }, }, - want: &v1.TaskSpec{ - Steps: []v1.Step{{ - Image: "myimage", - Command: []string{"ls"}, - Args: []string{"-lh"}, - WorkingDir: "/bar", - Timeout: &metav1.Duration{Duration: time.Hour}, - }}, + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ + Image: "myimage", + Command: []string{"ls"}, + Args: []string{"-lh"}, + }, }, - stepActionFunc: getStepAction, + want: []v1.Step{{ + Image: "myimage", + Command: []string{"ls"}, + Args: []string{"-lh"}, + WorkingDir: "/bar", + Timeout: &metav1.Duration{Duration: time.Hour}, + }}, }, { name: "step-action-with-script", tr: &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Name: "mytaskrun", + Name: "mytaskrun", + Namespace: "default", }, Spec: v1.TaskRunSpec{ TaskSpec: &v1.TaskSpec{ @@ -363,29 +360,26 @@ func TestGetStepAction(t *testing.T) { }, }, }, - want: &v1.TaskSpec{ - Steps: []v1.Step{{ + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepActionWithScript", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ Image: "myimage", Script: "ls", - }}, - }, - stepActionFunc: func(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) { - stepAction := &v1alpha1.StepAction{ - ObjectMeta: metav1.ObjectMeta{ - Name: "stepActionWithScript", - }, - Spec: v1alpha1.StepActionSpec{ - Image: "myimage", - Script: "ls", - }, - } - return stepAction, nil, nil + }, }, + want: []v1.Step{{ + Image: "myimage", + Script: "ls", + }}, }, { name: "step-action-with-env", tr: &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Name: "mytaskrun", + Name: "mytaskrun", + Namespace: "default", }, Spec: v1.TaskRunSpec{ TaskSpec: &v1.TaskSpec{ @@ -397,37 +391,75 @@ func TestGetStepAction(t *testing.T) { }, }, }, - want: &v1.TaskSpec{ - Steps: []v1.Step{{ + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepActionWithEnv", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ Image: "myimage", Env: []corev1.EnvVar{{ Name: "env1", Value: "value1", }}, - }}, + }, }, - stepActionFunc: func(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) { - stepAction := &v1alpha1.StepAction{ - ObjectMeta: metav1.ObjectMeta{ - Name: "stepActionWithEnv", - }, - Spec: v1alpha1.StepActionSpec{ - Image: "myimage", - Env: []corev1.EnvVar{{ - Name: "env1", - Value: "value1", + want: []v1.Step{{ + Image: "myimage", + Env: []corev1.EnvVar{{ + Name: "env1", + Value: "value1", + }}, + }}, + }, { + name: "inline and ref StepAction", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{{ + Ref: &v1.Ref{ + Name: "stepAction", + }, + WorkingDir: "/bar", + Timeout: &metav1.Duration{Duration: time.Hour}, + }, { + Image: "foo", + Command: []string{"ls"}, }}, }, - } - return stepAction, nil, nil + }, + }, + stepAction: &v1alpha1.StepAction{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stepAction", + Namespace: "default", + }, + Spec: v1alpha1.StepActionSpec{ + Image: "myimage", + Command: []string{"ls"}, + Args: []string{"-lh"}, + }, }, + want: []v1.Step{{ + Image: "myimage", + Command: []string{"ls"}, + Args: []string{"-lh"}, + WorkingDir: "/bar", + Timeout: &metav1.Duration{Duration: time.Hour}, + }, { + Image: "foo", + Command: []string{"ls"}, + }}, }} for _, tt := range tests { - gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { - return nil, nil, nil, nil - } + ctx := context.Background() + tektonclient := fake.NewSimpleClientset(tt.stepAction) - _, got, err := resources.GetTaskData(context.Background(), tt.tr, gt, tt.stepActionFunc) + got, err := resources.GetStepActionsData(ctx, *tt.tr.Spec.TaskSpec, tt.tr, tektonclient) if err != nil { t.Errorf("Did not expect an error but got : %s", err) } @@ -437,14 +469,14 @@ func TestGetStepAction(t *testing.T) { } } -func TestGetStepAction_Error(t *testing.T) { +func TestGetStepActionsData_Error(t *testing.T) { tests := []struct { name string tr *v1.TaskRun - stepActionFunc func(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) + stepActionFunc func(ctx context.Context, ref *v1.Ref) (*v1alpha1.StepAction, *v1.RefSource, error) expectedError error }{{ - name: "step-action-error", + name: "namespace missing error", tr: &v1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "mytaskrun", @@ -459,17 +491,10 @@ func TestGetStepAction_Error(t *testing.T) { }, }, }, - stepActionFunc: func(ctx context.Context, n string) (*v1alpha1.StepAction, *v1.RefSource, error) { - return nil, nil, fmt.Errorf("Error fetching Step Action.") - }, - expectedError: fmt.Errorf("Error fetching Step Action."), + expectedError: fmt.Errorf("must specify namespace to resolve reference to step action stepActionError"), }} for _, tt := range tests { - gt := func(ctx context.Context, n string) (*v1.Task, *v1.RefSource, *trustedresources.VerificationResult, error) { - return nil, nil, nil, nil - } - - _, _, err := resources.GetTaskData(context.Background(), tt.tr, gt, tt.stepActionFunc) + _, err := resources.GetStepActionsData(context.Background(), *tt.tr.Spec.TaskSpec, tt.tr, nil) if err == nil { t.Fatalf("Expected to get an error but did not find any.") } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index fd0b2553468..049365e897e 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -332,9 +332,8 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, return nil, nil, fmt.Errorf("failed to list VerificationPolicies from namespace %s with error %w", tr.Namespace, err) } getTaskfunc := resources.GetTaskFuncFromTaskRun(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, tr, vp) - getStepActionfunc := resources.GetStepActionFunc(c.PipelineClientSet, tr.Namespace) - taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskfunc, getStepActionfunc) + taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskfunc) switch { case errors.Is(err, remote.ErrRequestInProgress): message := fmt.Sprintf("TaskRun %s/%s awaiting remote resource", tr.Namespace, tr.Name) @@ -347,7 +346,7 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, return nil, nil, err case err != nil: logger.Errorf("Failed to determine Task spec to use for taskrun %s: %v", tr.Name, err) - if resources.IsGetTaskErrTransient(err) { + if resources.IsErrTransient(err) { return nil, nil, err } tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) @@ -359,6 +358,20 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1.TaskRun) (*v1.TaskSpec, } } + steps, err := resources.GetStepActionsData(ctx, *taskSpec, tr, c.PipelineClientSet) + switch { + case err != nil: + logger.Errorf("Failed to determine StepAction to use for TaskRun %s: %v", tr.Name, err) + if resources.IsErrTransient(err) { + return nil, nil, err + } + tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) + return nil, nil, controller.NewPermanentError(err) + default: + // Store the fetched StepActions to TaskSpec + taskSpec.Steps = steps + } + if taskMeta.VerificationResult != nil { switch taskMeta.VerificationResult.VerificationResultType { case trustedresources.VerificationError: