From 93a8d359836782ad6a3752323794bb1073ccddec Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Wed, 1 Jun 2022 14:57:12 -0400 Subject: [PATCH] TEP-0111 - Propagating workspaces in Taskrun This POC illustrates propagating workspaces defined at the `pipelinerun` stage and directly referred at the `spec`. There is no need to specify it in the `pipelinespec` followed by `tasks` and `taskspec`. --- docs/install.md | 1 + docs/tasks.md | 2 + .../alpha/propagating_workspaces.yaml | 16 +++ pkg/apis/pipeline/v1/task_types.go | 55 +++++++++ pkg/apis/pipeline/v1/task_types_test.go | 75 +++++++++++++ .../v1beta1/pipelinerun_validation.go | 1 - pkg/apis/pipeline/v1beta1/task_types.go | 56 ++++++++++ pkg/apis/pipeline/v1beta1/task_types_test.go | 75 +++++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 1 + pkg/reconciler/taskrun/taskrun.go | 65 ++++++++--- pkg/reconciler/taskrun/taskrun_test.go | 104 +++++------------- pkg/substitution/substitution.go | 15 +-- pkg/substitution/substitution_test.go | 46 ++++++++ pkg/workspace/apply.go | 13 +++ pkg/workspace/apply_test.go | 49 +++++++++ pkg/workspace/validate_test.go | 10 ++ 16 files changed, 486 insertions(+), 98 deletions(-) create mode 100644 examples/v1beta1/taskruns/alpha/propagating_workspaces.yaml create mode 100644 pkg/apis/pipeline/v1/task_types_test.go create mode 100644 pkg/apis/pipeline/v1beta1/task_types_test.go diff --git a/docs/install.md b/docs/install.md index 009603025a4..eb37e379763 100644 --- a/docs/install.md +++ b/docs/install.md @@ -427,6 +427,7 @@ Features currently in "alpha" are: | [Isolated `Step` & `Sidecar` `Workspaces`](./workspaces.md#isolated-workspaces) | [TEP-0029](https://github.com/tektoncd/community/blob/main/teps/0029-step-workspaces.md) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | | | [Hermetic Execution Mode](./hermetic.md) | [TEP-0025](https://github.com/tektoncd/community/blob/main/teps/0025-hermekton.md) | [v0.25.0](https://github.com/tektoncd/pipeline/releases/tag/v0.25.0) | | | [Propagated `Parameters`](./taskruns.md#propagated-parameters) | [TEP-0107](https://github.com/tektoncd/community/blob/main/teps/0107-propagating-parameters.md) | [v0.36.0](https://github.com/tektoncd/pipeline/releases/tag/v0.36.0) | | +| [Propagated `Workspaces`](./pipelineruns.md#propagated-workspaces) | [TEP-0111](https://github.com/tektoncd/community/blob/main/teps/0111-propagating-workspaces.md) | | | | [Windows Scripts](./tasks.md#windows-scripts) | [TEP-0057](https://github.com/tektoncd/community/blob/main/teps/0057-windows-support.md) | [v0.28.0](https://github.com/tektoncd/pipeline/releases/tag/v0.28.0) | | | [Remote Tasks](./taskruns.md#remote-tasks) and [Remote Pipelines](./pipelineruns.md#remote-pipelines) | [TEP-0060](https://github.com/tektoncd/community/blob/main/teps/0060-remote-resolutiond.md) | | | | [Debug](./debug.md) | [TEP-0042](https://github.com/tektoncd/community/blob/main/teps/0042-taskrun-breakpoint-on-failure.md) | [v0.26.0](https://github.com/tektoncd/pipeline/releases/tag/v0.26.0) | | diff --git a/docs/tasks.md b/docs/tasks.md index 8f7a6eaaf2b..c51a0309ec7 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -726,6 +726,8 @@ spec: For more information, see [Using `Workspaces` in `Tasks`](workspaces.md#using-workspaces-in-tasks) and the [`Workspaces` in a `TaskRun`](../examples/v1beta1/taskruns/workspace.yaml) example YAML file. +Workspaces can be propagated to embedded task specs, not referenced Tasks. For more information, see [Referenced TaskRuns within Embedded PipelineRuns](pipelineruns.md#referenced-taskruns-within-embedded-pipelineruns). + ### Emitting `Results` A Task is able to emit string results that can be viewed by users and passed to other Tasks in a Pipeline. These diff --git a/examples/v1beta1/taskruns/alpha/propagating_workspaces.yaml b/examples/v1beta1/taskruns/alpha/propagating_workspaces.yaml new file mode 100644 index 00000000000..804367a6663 --- /dev/null +++ b/examples/v1beta1/taskruns/alpha/propagating_workspaces.yaml @@ -0,0 +1,16 @@ +apiVersion: tekton.dev/v1beta1 +kind: TaskRun +metadata: + generateName: propagating-workspaces- +spec: + taskSpec: + steps: + - name: simple-step + image: ubuntu + command: + - echo + args: + - $(workspaces.tr-workspace.path) + workspaces: + - emptyDir: {} + name: tr-workspace diff --git a/pkg/apis/pipeline/v1/task_types.go b/pkg/apis/pipeline/v1/task_types.go index 4283e8119cd..c2370c5f34a 100644 --- a/pkg/apis/pipeline/v1/task_types.go +++ b/pkg/apis/pipeline/v1/task_types.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/kmeta" ) @@ -95,6 +96,60 @@ type TaskSpec struct { Results []TaskResult `json:"results,omitempty"` } +func findWorkspaceSubstitutionLocationsInSidecars(sidecars []Sidecar) sets.String { + locationsToCheck := sets.NewString() + for _, sidecar := range sidecars { + locationsToCheck.Insert(sidecar.Script) + + for i := range sidecar.Args { + locationsToCheck.Insert(sidecar.Args[i]) + } + + for i := range sidecar.Command { + locationsToCheck.Insert(sidecar.Command[i]) + } + } + return locationsToCheck +} + +func findWorkspaceSubstitutionLocationsInSteps(steps []Step) sets.String { + locationsToCheck := sets.NewString() + for _, step := range steps { + locationsToCheck.Insert(step.Script) + + for i := range step.Args { + locationsToCheck.Insert(step.Args[i]) + } + + for i := range step.Command { + locationsToCheck.Insert(step.Command[i]) + } + } + return locationsToCheck +} + +func findWorkspaceSubstitutionLocationsInStepTemplate(stepTemplate *StepTemplate) sets.String { + locationsToCheck := sets.NewString() + + for i := range stepTemplate.Args { + locationsToCheck.Insert(stepTemplate.Args[i]) + } + + for i := range stepTemplate.Command { + locationsToCheck.Insert(stepTemplate.Command[i]) + } + return locationsToCheck +} + +// FindWorkspaceSubstitutionLocationsInTask returns a set of all the locations in the TaskSpec where we can apply substitutions. +func (ts *TaskSpec) FindWorkspaceSubstitutionLocationsInTask() sets.String { + locationsToCheck := sets.NewString() + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInSteps(ts.Steps).List()...) + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInSidecars(ts.Sidecars).List()...) + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInStepTemplate(ts.StepTemplate).List()...) + return locationsToCheck +} + // TaskList contains a list of Task // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object type TaskList struct { diff --git a/pkg/apis/pipeline/v1/task_types_test.go b/pkg/apis/pipeline/v1/task_types_test.go new file mode 100644 index 00000000000..81093a4ac6f --- /dev/null +++ b/pkg/apis/pipeline/v1/task_types_test.go @@ -0,0 +1,75 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1_test + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/test/diff" + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestTaskSpec_FindWorkspaceSubstitutionLocationsInTask(t *testing.T) { + tests := []struct { + name string + ts *v1.TaskSpec + want sets.String + }{{ + name: "completespec", + ts: &v1.TaskSpec{ + Steps: []v1.Step{{ + Name: "step-name", + Image: "step-image", + Script: "step-script", + Command: []string{"step-command"}, + Args: []string{"step-args"}, + }}, + Sidecars: []v1.Sidecar{{ + Name: "sidecar-name", + Image: "sidecar-image", + Script: "sidecar-script", + Command: []string{"sidecar-command"}, + Args: []string{"sidecar-args"}, + }}, + StepTemplate: &v1.StepTemplate{ + Image: "steptemplate-image", + Command: []string{"steptemplate-command"}, + Args: []string{"steptemplate-args"}, + }, + }, + want: sets.NewString( + "step-script", + "step-args", + "step-command", + "sidecar-script", + "sidecar-args", + "sidecar-command", + "steptemplate-args", + "steptemplate-command", + ), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.ts.FindWorkspaceSubstitutionLocationsInTask() + if d := cmp.Diff(tt.want, got); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 13779aa9b4a..13b734e1013 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -143,7 +143,6 @@ func (ps *PipelineRunSpec) validatePipelineRunParameters(ctx context.Context) (e } } } - return errs } diff --git a/pkg/apis/pipeline/v1beta1/task_types.go b/pkg/apis/pipeline/v1beta1/task_types.go index 957b0aef697..028ee1a1571 100644 --- a/pkg/apis/pipeline/v1beta1/task_types.go +++ b/pkg/apis/pipeline/v1beta1/task_types.go @@ -21,6 +21,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/kmeta" ) @@ -78,6 +79,61 @@ func (*Task) GetGroupVersionKind() schema.GroupVersionKind { return SchemeGroupVersion.WithKind(pipeline.TaskControllerName) } +func findWorkspaceSubstitutionLocationsInSidecars(sidecars []Sidecar) sets.String { + locationsToCheck := sets.NewString() + for _, sidecar := range sidecars { + locationsToCheck.Insert(sidecar.Script) + + for i := range sidecar.Args { + locationsToCheck.Insert(sidecar.Args[i]) + } + + for i := range sidecar.Command { + locationsToCheck.Insert(sidecar.Command[i]) + } + } + return locationsToCheck +} + +func findWorkspaceSubstitutionLocationsInSteps(steps []Step) sets.String { + locationsToCheck := sets.NewString() + for _, step := range steps { + locationsToCheck.Insert(step.Script) + + for i := range step.Args { + locationsToCheck.Insert(step.Args[i]) + } + + for i := range step.Command { + locationsToCheck.Insert(step.Command[i]) + } + } + return locationsToCheck +} + +func findWorkspaceSubstitutionLocationsInStepTemplate(stepTemplate *StepTemplate) sets.String { + locationsToCheck := sets.NewString() + + if stepTemplate != nil { + for i := range stepTemplate.Args { + locationsToCheck.Insert(stepTemplate.Args[i]) + } + for i := range stepTemplate.Command { + locationsToCheck.Insert(stepTemplate.Command[i]) + } + } + return locationsToCheck +} + +// FindWorkspaceSubstitutionLocationsInTask returns a set of all the locations in the TaskSpec where we can apply substitutions. +func (ts *TaskSpec) FindWorkspaceSubstitutionLocationsInTask() sets.String { + locationsToCheck := sets.NewString() + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInSteps(ts.Steps).List()...) + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInSidecars(ts.Sidecars).List()...) + locationsToCheck.Insert(findWorkspaceSubstitutionLocationsInStepTemplate(ts.StepTemplate).List()...) + return locationsToCheck +} + // TaskSpec defines the desired state of Task. type TaskSpec struct { // Resources is a list input and output resource to run the task diff --git a/pkg/apis/pipeline/v1beta1/task_types_test.go b/pkg/apis/pipeline/v1beta1/task_types_test.go new file mode 100644 index 00000000000..be9a315cd58 --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/task_types_test.go @@ -0,0 +1,75 @@ +/* +Copyright 2019 The Tekton Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1_test + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + "github.com/tektoncd/pipeline/test/diff" + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestTaskSpec_FindWorkspaceSubstitutionLocationsInTask(t *testing.T) { + tests := []struct { + name string + ts *v1beta1.TaskSpec + want sets.String + }{{ + name: "completespec", + ts: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Name: "step-name", + Image: "step-image", + Script: "step-script", + Command: []string{"step-command"}, + Args: []string{"step-args"}, + }}, + Sidecars: []v1beta1.Sidecar{{ + Name: "sidecar-name", + Image: "sidecar-image", + Script: "sidecar-script", + Command: []string{"sidecar-command"}, + Args: []string{"sidecar-args"}, + }}, + StepTemplate: &v1beta1.StepTemplate{ + Image: "steptemplate-image", + Command: []string{"steptemplate-command"}, + Args: []string{"steptemplate-args"}, + }, + }, + want: sets.NewString( + "step-script", + "step-args", + "step-command", + "sidecar-script", + "sidecar-args", + "sidecar-command", + "steptemplate-args", + "steptemplate-command", + ), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.ts.FindWorkspaceSubstitutionLocationsInTask() + if d := cmp.Diff(tt.want, got); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index c8f0fb5c9bb..e4d471f4f4c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -1020,6 +1020,7 @@ func getTaskrunWorkspaces(pr *v1beta1.PipelineRun, rpt *resources.ResolvedPipeli for _, binding := range pr.Spec.Workspaces { pipelineRunWorkspaces[binding.Name] = binding } + for _, ws := range rpt.PipelineTask.Workspaces { taskWorkspaceName, pipelineTaskSubPath, pipelineWorkspaceName := ws.Name, ws.SubPath, ws.Workspace diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 0548542b4ca..b41436ce50a 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -388,7 +388,21 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 return nil, nil, controller.NewPermanentError(err) } - if err := workspace.ValidateBindings(ctx, taskSpec.Workspaces, tr.Spec.Workspaces); err != nil { + var workspaceDeclarations []v1beta1.WorkspaceDeclaration + // Propagating workspaces allows users to skip declarations + // In order to validate the workspace bindings we create declarations based on + // the workspaces provided in the task run spec. This logic is hidden behind the + // alpha feature gate since propagating workspaces is behind that feature gate. + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + for _, ws := range tr.Spec.Workspaces { + wspaceDeclaration := v1beta1.WorkspaceDeclaration{Name: ws.Name} + workspaceDeclarations = append(workspaceDeclarations, wspaceDeclaration) + } + workspaceDeclarations = append(workspaceDeclarations, taskSpec.Workspaces...) + } else { + workspaceDeclarations = taskSpec.Workspaces + } + if err := workspace.ValidateBindings(ctx, workspaceDeclarations, tr.Spec.Workspaces); err != nil { logger.Errorf("TaskRun %q workspaces are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) return nil, nil, controller.NewPermanentError(err) @@ -427,13 +441,20 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re defer c.durationAndCountMetrics(ctx, tr) logger := logging.FromContext(ctx) recorder := controller.GetEventRecorder(ctx) + var err error - ts := updateTaskSpecParamsContextsResults(ctx, tr, rtr) + // Get the randomized volume names assigned to workspace bindings + workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces) + + ts, err := updateTaskSpecParamsContextsResults(ctx, tr, rtr, workspaceVolumes) + if err != nil { + logger.Errorf("Error updating task spec parameters, contexts, results and workspaces: %s", err) + return err + } tr.Status.TaskSpec = ts // Get the TaskRun's Pod if it should have one. Otherwise, create the Pod. var pod *corev1.Pod - var err error if tr.Status.PodName != "" { pod, err = c.podLister.Pods(tr.Namespace).Get(tr.Status.PodName) @@ -479,7 +500,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re // This is used by createPod below. Changes to the Spec are not updated. tr.Spec.Workspaces = taskRunWorkspaces } - pod, err = c.createPod(ctx, ts, tr, rtr) + pod, err = c.createPod(ctx, ts, tr, rtr, workspaceVolumes) if err != nil { newErr := c.handlePodCreationError(tr, err) logger.Errorf("Failed to create task run pod for taskrun %q: %v", tr.Name, newErr) @@ -658,7 +679,7 @@ func (c *Reconciler) failTaskRun(ctx context.Context, tr *v1beta1.TaskRun, reaso // createPod creates a Pod based on the Task's configuration, with pvcName as a volumeMount // TODO(dibyom): Refactor resource setup/substitution logic to its own function in the resources package -func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) (*corev1.Pod, error) { +func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources, workspaceVolumes map[string]corev1.Volume) (*corev1.Pod, error) { logger := logging.FromContext(ctx) inputResources, err := resourceImplBinding(rtr.Inputs, c.Images) if err != nil { @@ -694,12 +715,6 @@ func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1 ts = resources.ApplyResources(ts, inputResources, "inputs") ts = resources.ApplyResources(ts, outputResources, "outputs") - // Get the randomized volume names assigned to workspace bindings - workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces) - - // Apply workspace resource substitution - ts = resources.ApplyWorkspaces(ctx, ts, ts.Workspaces, tr.Spec.Workspaces, workspaceVolumes) - // By this time, params and workspaces should be propagated down so we can // validate that all parameter variables and workspaces used in the TaskSpec are declared by the Task. ctx = config.SkipValidationDueToPropagatedParametersAndWorkspaces(ctx, false) @@ -709,6 +724,7 @@ func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1 } ts, err = workspace.Apply(ctx, *ts, tr.Spec.Workspaces, workspaceVolumes) + if err != nil { logger.Errorf("Failed to create a pod for taskrun: %s due to workspace error %v", tr.Name, err) return nil, err @@ -750,7 +766,7 @@ func (c *Reconciler) createPod(ctx context.Context, ts *v1beta1.TaskSpec, tr *v1 return pod, err } -func updateTaskSpecParamsContextsResults(ctx context.Context, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) *v1beta1.TaskSpec { +func updateTaskSpecParamsContextsResults(ctx context.Context, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources, workspaceVolumes map[string]corev1.Volume) (*v1beta1.TaskSpec, error) { ts := rtr.TaskSpec.DeepCopy() var defaults []v1beta1.ParamSpec if len(ts.Params) > 0 { @@ -768,7 +784,30 @@ func updateTaskSpecParamsContextsResults(ctx context.Context, tr *v1beta1.TaskRu // Apply step exitCode path substitution ts = resources.ApplyStepExitCodePath(ts) - return ts + // Apply workspace resource substitution + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == config.AlphaAPIFields { + // propagate workspaces from taskrun to task. + twn := []string{} + for _, tw := range ts.Workspaces { + twn = append(twn, tw.Name) + } + + for _, trw := range tr.Spec.Workspaces { + skip := false + for _, tw := range twn { + if tw == trw.Name { + skip = true + break + } + } + if !skip { + ts.Workspaces = append(ts.Workspaces, v1beta1.WorkspaceDeclaration{Name: trw.Name}) + } + } + } + ts = resources.ApplyWorkspaces(ctx, ts, ts.Workspaces, tr.Spec.Workspaces, workspaceVolumes) + + return ts, nil } func isExceededResourceQuotaError(err error) bool { diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index bb7a771fa88..35e3ab4f9f8 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -44,6 +44,7 @@ import ( ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" + "github.com/tektoncd/pipeline/pkg/workspace" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" eventstest "github.com/tektoncd/pipeline/test/events" @@ -2467,97 +2468,41 @@ status: } } -func TestExpandMountPath(t *testing.T) { - expectedMountPath := "/temppath/replaced" - expectedReplacedArgs := fmt.Sprintf("replacedArgs - %s", expectedMountPath) - // The task's Workspace has a parameter variable - simpleTask := parse.MustParseTask(t, ` -metadata: - name: test-task - namespace: foo -spec: - params: - - name: source-path - type: string - - name: source-path-two - type: string - steps: - - args: - - replacedArgs - $(workspaces.tr-workspace.path) - command: - - echo - image: foo - name: simple-step - workspaces: - - description: a test task workspace - mountPath: /temppath/$(params.source-path) - name: tr-workspace - readOnly: true -`) - +func TestUpdateTaskSpecParamsContextsResults(t *testing.T) { taskRun := parse.MustParseTaskRun(t, ` metadata: name: test-taskrun-not-started namespace: foo spec: - params: - - name: source-path - value: replaced - taskRef: - name: test-task + taskSpec: + steps: + - args: + - replacedArgs - $(workspaces.tr-workspace.path) + command: + - echo + image: foo + name: simple-step workspaces: - emptyDir: {} name: tr-workspace `) - d := test.Data{ - TaskRuns: []*v1beta1.TaskRun{taskRun}, - Tasks: []*v1beta1.Task{simpleTask}, + wsDeclarations := []v1beta1.WorkspaceDeclaration{} + for _, ws := range taskRun.Spec.Workspaces { + wsDeclarations = append(wsDeclarations, v1beta1.WorkspaceDeclaration{Name: ws.Name}) } - d.ConfigMaps = []*corev1.ConfigMap{ - { - ObjectMeta: metav1.ObjectMeta{Name: config.GetDefaultsConfigName(), Namespace: system.Namespace()}, - Data: map[string]string{ - "default-cloud-events-sink": "http://synk:8080", - }, - }, - } - - testAssets, cancel := getTaskRunController(t, d) - defer cancel() - createServiceAccount(t, testAssets, "default", taskRun.Namespace) - - // Use the test assets to create a *Reconciler directly for focused testing. - r := &Reconciler{ - KubeClientSet: testAssets.Clients.Kube, - PipelineClientSet: testAssets.Clients.Pipeline, - Clock: testClock, - taskRunLister: testAssets.Informers.TaskRun.Lister(), - resourceLister: testAssets.Informers.PipelineResource.Lister(), - limitrangeLister: testAssets.Informers.LimitRange.Lister(), - cloudEventClient: testAssets.Clients.CloudEvents, - metrics: nil, // Not used - entrypointCache: nil, // Not used - pvcHandler: volumeclaim.NewPVCHandler(testAssets.Clients.Kube, testAssets.Logger), - } - rtr := &resources.ResolvedTaskResources{ TaskName: "test-task", Kind: "Task", - TaskSpec: &v1beta1.TaskSpec{Steps: simpleTask.Spec.Steps, Workspaces: simpleTask.Spec.Workspaces}, + TaskSpec: &v1beta1.TaskSpec{Steps: taskRun.Spec.TaskSpec.Steps, Workspaces: wsDeclarations}, } - taskSpec := updateTaskSpecParamsContextsResults(context.Background(), taskRun, rtr) - pod, err := r.createPod(testAssets.Ctx, taskSpec, taskRun, rtr) - + ctx := config.EnableAlphaAPIFields(context.Background()) + workspaceVolumes := workspace.CreateVolumes(taskRun.Spec.Workspaces) + taskSpec, err := updateTaskSpecParamsContextsResults(ctx, taskRun, rtr, workspaceVolumes) if err != nil { - t.Fatalf("create pod threw error %v", err) - } - - if vm := pod.Spec.Containers[0].VolumeMounts[0]; !strings.HasPrefix(vm.Name, "ws-9l9zj") || vm.MountPath != expectedMountPath { - t.Fatalf("failed to find expanded Workspace mountpath %v", expectedMountPath) + t.Fatalf("update task spec threw error %v", err) } - - if a := pod.Spec.Containers[0].Args; a[len(a)-1] != expectedReplacedArgs { - t.Fatalf("failed to replace Workspace mountpath variable, expected %s, actual: %s", expectedReplacedArgs, a) + if taskSpec.Steps[0].Args[0] != "replacedArgs - /workspace/tr-workspace" { + t.Fatalf(`Expected "replacedArgs - /workspace/tr-workspace" but got "%v"`, taskSpec.Steps[0].Args[0]) } } @@ -2649,8 +2594,13 @@ spec: TaskSpec: &v1beta1.TaskSpec{Steps: simpleTask.Spec.Steps, Workspaces: simpleTask.Spec.Workspaces}, } - taskSpec := updateTaskSpecParamsContextsResults(context.Background(), taskRun, rtr) - _, err := r.createPod(testAssets.Ctx, taskSpec, taskRun, rtr) + workspaceVolumes := workspace.CreateVolumes(taskRun.Spec.Workspaces) + ctx := config.EnableAlphaAPIFields(context.Background()) + taskSpec, err := updateTaskSpecParamsContextsResults(ctx, taskRun, rtr, workspaceVolumes) + if err != nil { + t.Errorf("update task spec threw an error: %v", err) + } + _, err = r.createPod(testAssets.Ctx, taskSpec, taskRun, rtr, workspaceVolumes) if err == nil || err.Error() != expectedError { t.Errorf("Expected to fail validation for duplicate Workspace mount paths, error was %v", err) diff --git a/pkg/substitution/substitution.go b/pkg/substitution/substitution.go index f98d03569b9..6db28980026 100644 --- a/pkg/substitution/substitution.go +++ b/pkg/substitution/substitution.go @@ -50,7 +50,7 @@ var intIndexRegex = regexp.MustCompile(intIndex) // ValidateVariable makes sure all variables in the provided string are known func ValidateVariable(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError { - if vs, present, _ := extractVariablesFromString(value, prefix); present { + if vs, present, _ := ExtractVariablesFromString(value, prefix); present { for _, v := range vs { v = strings.TrimSuffix(v, "[*]") if !vars.Has(v) { @@ -66,7 +66,7 @@ func ValidateVariable(name, value, prefix, locationName, path string, vars sets. // ValidateVariableP makes sure all variables for a parameter in the provided string are known func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError { - if vs, present, errString := extractVariablesFromString(value, prefix); present { + if vs, present, errString := ExtractVariablesFromString(value, prefix); present { if errString != "" { return &apis.FieldError{ Message: errString, @@ -90,7 +90,7 @@ func ValidateVariableP(value, prefix string, vars sets.String) *apis.FieldError // ValidateVariableProhibited verifies that variables matching the relevant string expressions do not reference any of the names present in vars. func ValidateVariableProhibited(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError { - if vs, present, _ := extractVariablesFromString(value, prefix); present { + if vs, present, _ := ExtractVariablesFromString(value, prefix); present { for _, v := range vs { v = strings.TrimSuffix(v, "[*]") if vars.Has(v) { @@ -106,7 +106,7 @@ func ValidateVariableProhibited(name, value, prefix, locationName, path string, // ValidateVariableProhibitedP verifies that variables for a parameter matching the relevant string expressions do not reference any of the names present in vars. func ValidateVariableProhibitedP(value, prefix string, vars sets.String) *apis.FieldError { - if vs, present, errString := extractVariablesFromString(value, prefix); present { + if vs, present, errString := ExtractVariablesFromString(value, prefix); present { if errString != "" { return &apis.FieldError{ Message: errString, @@ -155,7 +155,7 @@ func ValidateEntireVariableProhibitedP(value, prefix string, vars sets.String) * // ValidateVariableIsolated verifies that variables matching the relevant string expressions are completely isolated if present. func ValidateVariableIsolated(name, value, prefix, locationName, path string, vars sets.String) *apis.FieldError { - if vs, present, _ := extractVariablesFromString(value, prefix); present { + if vs, present, _ := ExtractVariablesFromString(value, prefix); present { firstMatch, _ := extractExpressionFromString(value, prefix) for _, v := range vs { v = strings.TrimSuffix(v, "[*]") @@ -174,7 +174,7 @@ func ValidateVariableIsolated(name, value, prefix, locationName, path string, va // ValidateVariableIsolatedP verifies that variables matching the relevant string expressions are completely isolated if present. func ValidateVariableIsolatedP(value, prefix string, vars sets.String) *apis.FieldError { - if vs, present, errString := extractVariablesFromString(value, prefix); present { + if vs, present, errString := ExtractVariablesFromString(value, prefix); present { if errString != "" { return &apis.FieldError{ Message: errString, @@ -234,7 +234,8 @@ func extractExpressionFromString(s, prefix string) (string, bool) { return match[0], true } -func extractVariablesFromString(s, prefix string) ([]string, bool, string) { +// ExtractVariablesFromString extracts variables from an input string s with the given prefix via regex matching. +func ExtractVariablesFromString(s, prefix string) ([]string, bool, string) { pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution) re := regexp.MustCompile(pattern) matches := re.FindAllStringSubmatch(s, -1) diff --git a/pkg/substitution/substitution_test.go b/pkg/substitution/substitution_test.go index 986f6b9ce4c..4ba4a1c133d 100644 --- a/pkg/substitution/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -606,3 +606,49 @@ func TestStripStarVarSubExpression(t *testing.T) { }) } } + +func TestExtractVariablesFromString(t *testing.T) { + tests := []struct { + name string + s string + prefix string + want []string + extracted bool + err string + }{{ + name: "complete match", + s: "--flag=$(inputs.params.baz)", + prefix: "inputs.params", + want: []string{"baz"}, + extracted: true, + err: "", + }, { + name: "no match", + s: "--flag=$(inputs.params.baz)", + prefix: "outputs", + want: []string{}, + extracted: false, + err: "", + }, { + name: "too many dots", + s: "--flag=$(inputs.params.foo.baz.bar)", + prefix: "inputs.params", + want: []string{""}, + extracted: true, + err: fmt.Sprintf(`Invalid referencing of parameters in "--flag=$(inputs.params.foo.baz.bar)"! Only two dot-separated components after the prefix "inputs.params" are allowed.`), + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, extracted, err := substitution.ExtractVariablesFromString(tt.s, tt.prefix) + if d := cmp.Diff(tt.want, got); d != "" { + t.Error(diff.PrintWantGot(d)) + } + if d := cmp.Diff(tt.extracted, extracted); d != "" { + t.Error(diff.PrintWantGot(d)) + } + if d := cmp.Diff(tt.err, err); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/workspace/apply.go b/pkg/workspace/apply.go index 822be5d50af..897d8a161db 100644 --- a/pkg/workspace/apply.go +++ b/pkg/workspace/apply.go @@ -125,6 +125,19 @@ func Apply(ctx context.Context, ts v1beta1.TaskSpec, wb []v1beta1.WorkspaceBindi } for i := range wb { + if alphaAPIEnabled { + // Propagate missing Workspaces + addWorkspace := true + for _, ws := range ts.Workspaces { + if ws.Name == wb[i].Name { + addWorkspace = false + break + } + } + if addWorkspace { + ts.Workspaces = append(ts.Workspaces, v1beta1.WorkspaceDeclaration{Name: wb[i].Name}) + } + } w, err := getDeclaredWorkspace(wb[i].Name, ts.Workspaces) if err != nil { return nil, err diff --git a/pkg/workspace/apply_test.go b/pkg/workspace/apply_test.go index c93bd1f4567..f08fc02a2c8 100644 --- a/pkg/workspace/apply_test.go +++ b/pkg/workspace/apply_test.go @@ -1001,6 +1001,55 @@ func TestApply_IsolatedWorkspaces(t *testing.T) { ReadOnly: true, }}, }, + }, { + name: "add workspaces to taskspec", + ts: v1beta1.TaskSpec{ + Sidecars: []v1beta1.Sidecar{{ + Name: "conflicting volume mount sidecar", + VolumeMounts: []corev1.VolumeMount{{ + Name: "mount-path-conflicts", + MountPath: "/my/fancy/mount/path", + }}, + }}, + }, + workspaces: []v1beta1.WorkspaceBinding{{ + Name: "custom", + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "mypvc", + }, + }}, + expectedTaskSpec: v1beta1.TaskSpec{ + StepTemplate: &v1beta1.StepTemplate{ + VolumeMounts: []corev1.VolumeMount{{ + Name: "ws-vr6ds", + MountPath: "/workspace/custom", + ReadOnly: false, + }}, + }, + Sidecars: []v1beta1.Sidecar{{ + Name: "conflicting volume mount sidecar", + VolumeMounts: []corev1.VolumeMount{{ + Name: "mount-path-conflicts", + MountPath: "/my/fancy/mount/path", + }, { + Name: "ws-vr6ds", + MountPath: "/workspace/custom", + }}, + }}, + Volumes: []corev1.Volume{{ + Name: "ws-vr6ds", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "mypvc", + }, + }, + }}, + Workspaces: []v1beta1.WorkspaceDeclaration{{ + Name: "custom", + MountPath: "", + ReadOnly: false, + }}, + }, }} { t.Run(tc.name, func(t *testing.T) { ctx := config.ToContext(context.Background(), &config.Config{ diff --git a/pkg/workspace/validate_test.go b/pkg/workspace/validate_test.go index 0859e6950ae..ac5f08f6fac 100644 --- a/pkg/workspace/validate_test.go +++ b/pkg/workspace/validate_test.go @@ -142,6 +142,16 @@ func TestValidateBindingsInvalid(t *testing.T) { Name: "beth", PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{}, }}, + }, { + name: "Mismatch between declarations and bindings", + declarations: []v1beta1.WorkspaceDeclaration{{ + Name: "Notbeth", + Optional: true, + }}, + bindings: []v1beta1.WorkspaceBinding{{ + Name: "beth", + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }}, }} { t.Run(tc.name, func(t *testing.T) { if err := ValidateBindings(context.Background(), tc.declarations, tc.bindings); err == nil {