From c1b6abca77683e14b135a0091fffbe7ce44811ef Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Tue, 7 Nov 2023 21:49:11 +0000 Subject: [PATCH] [TEP-0142] Add VolumeMounts to StepAction This commit adds VolumeMounts to StepAction, the VolumeMount.Name should use string param reference, and cannot be set both with Step from Task/TaskRun. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipeline-api.md | 30 +++ docs/stepactions.md | 26 +++ go.mod | 2 +- pkg/apis/pipeline/v1/param_types.go | 2 +- pkg/apis/pipeline/v1/task_validation.go | 6 + pkg/apis/pipeline/v1/task_validation_test.go | 19 +- .../pipeline/v1alpha1/openapi_generated.go | 23 +- .../pipeline/v1alpha1/stepaction_types.go | 7 + .../v1alpha1/stepaction_validation.go | 33 +++ .../v1alpha1/stepaction_validation_test.go | 211 +++++++++++++++--- pkg/apis/pipeline/v1alpha1/swagger.json | 11 + .../v1alpha1/zz_generated.deepcopy.go | 7 + pkg/apis/pipeline/v1beta1/param_types.go | 2 +- pkg/apis/pipeline/v1beta1/task_validation.go | 6 + .../pipeline/v1beta1/task_validation_test.go | 18 +- pkg/reconciler/taskrun/resources/taskspec.go | 3 + .../taskrun/resources/taskspec_test.go | 8 + pkg/substitution/substitution.go | 18 +- pkg/substitution/substitution_test.go | 42 +++- 19 files changed, 436 insertions(+), 38 deletions(-) diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index fd0e5b73880..fdb95ffb338 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -6629,6 +6629,21 @@ More info: +[]Kubernetes core/v1.VolumeMount + + + + +(Optional) +

Volumes to mount into the Step’s filesystem. +Cannot be updated.

+ + @@ -7505,6 +7520,21 @@ More info: +[]Kubernetes core/v1.VolumeMount + + + + +(Optional) +

Volumes to mount into the Step’s filesystem. +Cannot be updated.

+ +

VerificationPolicySpec diff --git a/docs/stepactions.md b/docs/stepactions.md index 85ac1d5e51f..561a582e2c8 100644 --- a/docs/stepactions.md +++ b/docs/stepactions.md @@ -41,6 +41,7 @@ A `StepAction` definition supports the following fields: - [`params`](#declaring-params) - [`results`](#declaring-results) - [`securityContext`](#declaring-securitycontext) + - [`volumeMounts`](#declaring-volumemounts) The non-functional example below demonstrates the use of most of the above-mentioned fields: @@ -134,6 +135,30 @@ spec: Note that the `securityContext` from `StepAction` will overwrite the `securityContext` from [`TaskRun`](./taskruns.md/#example-of-running-step-containers-as-a-non-root-user). +### Declaring VolumeMounts + +You can define `VolumeMounts` in `StepActions`. The `name` of the `VolumeMount` MUST be a single reference to a string `Parameter`. For example, `$(params.registryConfig)` is valid while `$(params.registryConfig)-foo` and `"unparametrized-name"` are invalid. This is to ensure reusability of `StepActions` such that `Task` authors have control of which `Volumes` they bind to the `VolumeMounts`. + +```yaml +apiVersion: tekton.dev/v1alpha1 +kind: StepAction +metadata: + name: myStep +spec: + params: + - name: registryConfig + - name: otherConfig + volumeMounts: + - name: $(params.registryConfig) + mountPath: /registry-config + volumeMounts: + - name: $(params.otherConfig) + mountPath: /other-config + image: ... + script: ... +``` + + ## Referencing a StepAction `StepActions` can be referenced from the `Step` using the `ref` field, as follows: @@ -191,6 +216,7 @@ If a `Step` is referencing a `StepAction`, it cannot contain the fields supporte - `args` - `script` - `env` +- `volumeMounts` Using any of the above fields and referencing a `StepAction` in the same `Step` is not allowed and will cause an validation error. diff --git a/go.mod b/go.mod index 6ce453422a9..06965015b16 100644 --- a/go.mod +++ b/go.mod @@ -51,6 +51,7 @@ require ( github.com/goccy/kpoward v0.1.0 github.com/google/cel-go v0.18.1 github.com/google/go-containerregistry/pkg/authn/k8schain v0.0.0-20230625233257-b8504803389b + github.com/google/go-containerregistry/pkg/authn/kubernetes v0.0.0-20230516205744-dbecb1de8cfa github.com/sigstore/sigstore/pkg/signature/kms/aws v1.7.5 github.com/sigstore/sigstore/pkg/signature/kms/azure v1.7.5 github.com/sigstore/sigstore/pkg/signature/kms/gcp v1.7.5 @@ -102,7 +103,6 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/golang-jwt/jwt/v5 v5.0.0 // indirect github.com/google/gnostic v0.6.9 // indirect - github.com/google/go-containerregistry/pkg/authn/kubernetes v0.0.0-20230516205744-dbecb1de8cfa // indirect github.com/google/s2a-go v0.1.7 // indirect github.com/googleapis/enterprise-certificate-proxy v0.3.1 // indirect github.com/googleapis/gax-go/v2 v2.12.0 // indirect diff --git a/pkg/apis/pipeline/v1/param_types.go b/pkg/apis/pipeline/v1/param_types.go index 9a964641bc0..c5973544221 100644 --- a/pkg/apis/pipeline/v1/param_types.go +++ b/pkg/apis/pipeline/v1/param_types.go @@ -329,7 +329,7 @@ func (ps ParamSpecs) ExtractDefaultParamArrayLengths() map[string]int { // it would return ["$(params.array-param[1])", "$(params.other-array-param[2])"]. func extractArrayIndexingParamRefs(paramReference string) []string { l := []string{} - list := substitution.ExtractParamsExpressions(paramReference) + list := substitution.ExtractArrayIndexingParamsExpressions(paramReference) for _, val := range list { indexString := substitution.ExtractIndexString(val) if indexString != "" { diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 4f3ba830c75..7fb4c70be9d 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -299,6 +299,12 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi Paths: []string{"env"}, }) } + if len(s.VolumeMounts) > 0 { + errs = errs.Also(&apis.FieldError{ + Message: "volumeMounts cannot be used with Ref", + Paths: []string{"volumeMounts"}, + }) + } } else { if len(s.Params) > 0 { errs = errs.Also(&apis.FieldError{ diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 16554c7e36f..5bc80e018fc 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1513,7 +1513,24 @@ func TestTaskSpecValidateErrorWithStepActionRef(t *testing.T) { Message: "params cannot be used without Ref", Paths: []string{"steps[0].params"}, }, - }} + }, { + name: "Cannot use volumeMounts with Ref", + Steps: []v1.Step{{ + Ref: &v1.Ref{ + Name: "stepAction", + }, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/registry-config", + }}, + }}, + enableStepActions: true, + expectedError: apis.FieldError{ + Message: "volumeMounts cannot be used with Ref", + Paths: []string{"steps[0].volumeMounts"}, + }, + }, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ts := v1.TaskSpec{ diff --git a/pkg/apis/pipeline/v1alpha1/openapi_generated.go b/pkg/apis/pipeline/v1alpha1/openapi_generated.go index 48b42c1663f..cb18896855e 100644 --- a/pkg/apis/pipeline/v1alpha1/openapi_generated.go +++ b/pkg/apis/pipeline/v1alpha1/openapi_generated.go @@ -927,11 +927,32 @@ func schema_pkg_apis_pipeline_v1alpha1_StepActionSpec(ref common.ReferenceCallba Ref: ref("k8s.io/api/core/v1.SecurityContext"), }, }, + "volumeMounts": { + VendorExtensible: spec.VendorExtensible{ + Extensions: spec.Extensions{ + "x-kubernetes-list-type": "atomic", + "x-kubernetes-patch-merge-key": "mountPath", + "x-kubernetes-patch-strategy": "merge", + }, + }, + SchemaProps: spec.SchemaProps{ + Description: "Volumes to mount into the Step's filesystem. Cannot be updated.", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: map[string]interface{}{}, + Ref: ref("k8s.io/api/core/v1.VolumeMount"), + }, + }, + }, + }, + }, }, }, }, Dependencies: []string{ - "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1.ParamSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.StepActionResult", "k8s.io/api/core/v1.EnvVar", "k8s.io/api/core/v1.SecurityContext"}, + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1.ParamSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.StepActionResult", "k8s.io/api/core/v1.EnvVar", "k8s.io/api/core/v1.SecurityContext", "k8s.io/api/core/v1.VolumeMount"}, } } diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_types.go b/pkg/apis/pipeline/v1alpha1/stepaction_types.go index 4e9efb77714..97752a2b421 100644 --- a/pkg/apis/pipeline/v1alpha1/stepaction_types.go +++ b/pkg/apis/pipeline/v1alpha1/stepaction_types.go @@ -127,6 +127,13 @@ type StepActionSpec struct { // The value set in StepAction will take precedence over the value from Task. // +optional SecurityContext *corev1.SecurityContext `json:"securityContext,omitempty" protobuf:"bytes,15,opt,name=securityContext"` + // Volumes to mount into the Step's filesystem. + // Cannot be updated. + // +optional + // +patchMergeKey=mountPath + // +patchStrategy=merge + // +listType=atomic + VolumeMounts []corev1.VolumeMount `json:"volumeMounts,omitempty" patchStrategy:"merge" patchMergeKey:"mountPath" protobuf:"bytes,9,rep,name=volumeMounts"` } // StepActionObject is implemented by StepAction diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go index 47674cfc62f..f170410674a 100644 --- a/pkg/apis/pipeline/v1alpha1/stepaction_validation.go +++ b/pkg/apis/pipeline/v1alpha1/stepaction_validation.go @@ -23,6 +23,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/validate" "github.com/tektoncd/pipeline/pkg/substitution" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/apis" "knative.dev/pkg/webhook/resourcesemantics" @@ -67,6 +68,7 @@ func (ss *StepActionSpec) Validate(ctx context.Context) (errs *apis.FieldError) errs = errs.Also(validateParameterVariables(ctx, *ss, ss.Params)) errs = errs.Also(validateStepActionResultsVariables(ctx, *ss)) errs = errs.Also(validateResults(ctx, ss.Results).ViaField("results")) + errs = errs.Also(validateVolumeMounts(ss.VolumeMounts, ss.Params).ViaField("volumeMounts")) return errs } @@ -89,6 +91,28 @@ func validateResults(ctx context.Context, results []StepActionResult) (errs *api return errs } +func validateVolumeMounts(volumeMounts []corev1.VolumeMount, params v1.ParamSpecs) (errs *apis.FieldError) { + if len(volumeMounts) == 0 { + return + } + paramNames := sets.String{} + for _, p := range params { + paramNames.Insert(p.Name) + } + for idx, v := range volumeMounts { + matches, _ := substitution.ExtractVariableExpressions(v.Name, "params") + if len(matches) != 1 { + errs = errs.Also(apis.ErrInvalidValue(v.Name, "name", "expect the Name to be a single param reference").ViaIndex(idx)) + return errs + } else if matches[0] != v.Name { + errs = errs.Also(apis.ErrInvalidValue(v.Name, "name", "expect the Name to be a single param reference").ViaIndex(idx)) + return errs + } + errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(v.Name, "params", paramNames).ViaIndex(idx)) + } + return errs +} + // validateParameterVariables validates all variables within a slice of ParamSpecs against a StepAction func validateParameterVariables(ctx context.Context, sas StepActionSpec, params v1.ParamSpecs) *apis.FieldError { var errs *apis.FieldError @@ -133,6 +157,9 @@ func validateStepActionObjectUsageAsWhole(sas StepActionSpec, prefix string, var for _, env := range sas.Env { errs = errs.Also(substitution.ValidateNoReferencesToEntireProhibitedVariables(env.Value, prefix, vars).ViaFieldKey("env", env.Name)) } + for i, vm := range sas.VolumeMounts { + errs = errs.Also(substitution.ValidateNoReferencesToEntireProhibitedVariables(vm.Name, prefix, vars).ViaFieldIndex("volumeMounts", i)) + } return errs } @@ -149,6 +176,9 @@ func validateStepActionArrayUsage(sas StepActionSpec, prefix string, arrayParamN for _, env := range sas.Env { errs = errs.Also(substitution.ValidateNoReferencesToProhibitedVariables(env.Value, prefix, arrayParamNames).ViaFieldKey("env", env.Name)) } + for i, vm := range sas.VolumeMounts { + errs = errs.Also(substitution.ValidateNoReferencesToProhibitedVariables(vm.Name, prefix, arrayParamNames).ViaFieldIndex("volumeMounts", i)) + } return errs } @@ -165,6 +195,9 @@ func validateStepActionVariables(ctx context.Context, sas StepActionSpec, prefix for _, env := range sas.Env { errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(env.Value, prefix, vars).ViaFieldKey("env", env.Name)) } + for i, vm := range sas.VolumeMounts { + errs = errs.Also(substitution.ValidateNoReferencesToUnknownVariables(vm.Name, prefix, vars).ViaFieldIndex("volumeMounts", i)) + } return errs } diff --git a/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go b/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go index 9e371c1edba..2288ea4fdae 100644 --- a/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/stepaction_validation_test.go @@ -61,13 +61,14 @@ func TestStepActionValidate(t *testing.T) { func TestStepActionSpecValidate(t *testing.T) { type fields struct { - Image string - Command []string - Args []string - Script string - Env []corev1.EnvVar - Params []v1.ParamSpec - Results []v1alpha1.StepActionResult + Image string + Command []string + Args []string + Script string + Env []corev1.EnvVar + Params []v1.ParamSpec + Results []v1alpha1.StepActionResult + VolumeMounts []corev1.VolumeMount } tests := []struct { name string @@ -250,17 +251,47 @@ func TestStepActionSpecValidate(t *testing.T) { }, }}, }, + }, { + name: "valid volumeMounts", + fields: fields{ + Image: "my-image", + Args: []string{"arg"}, + Params: []v1.ParamSpec{{ + Name: "foo", + }, { + Name: "array-params", + Type: v1.ParamTypeArray, + }, { + Name: "object-params", + Type: v1.ParamTypeObject, + Properties: map[string]v1.PropertySpec{ + "key": {Type: "string"}, + }, + }, + }, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/config", + }, { + Name: "$(params.array-params[0])", + MountPath: "/config", + }, { + Name: "$(params.object-params.key)", + MountPath: "/config", + }}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { sa := &v1alpha1.StepActionSpec{ - Image: tt.fields.Image, - Command: tt.fields.Command, - Args: tt.fields.Args, - Script: tt.fields.Script, - Env: tt.fields.Env, - Params: tt.fields.Params, - Results: tt.fields.Results, + Image: tt.fields.Image, + Command: tt.fields.Command, + Args: tt.fields.Args, + Script: tt.fields.Script, + Env: tt.fields.Env, + Params: tt.fields.Params, + Results: tt.fields.Results, + VolumeMounts: tt.fields.VolumeMounts, } ctx := context.Background() sa.SetDefaults(ctx) @@ -273,13 +304,14 @@ func TestStepActionSpecValidate(t *testing.T) { func TestStepActionValidateError(t *testing.T) { type fields struct { - Image string - Command []string - Args []string - Script string - Env []corev1.EnvVar - Params []v1.ParamSpec - Results []v1alpha1.StepActionResult + Image string + Command []string + Args []string + Script string + Env []corev1.EnvVar + Params []v1.ParamSpec + Results []v1alpha1.StepActionResult + VolumeMounts []corev1.VolumeMount } tests := []struct { name string @@ -399,19 +431,142 @@ func TestStepActionValidateError(t *testing.T) { Message: `non-existent variable in "$(params.foo) && $(params.inexistent)"`, Paths: []string{"spec.args[0]"}, }, + }, { + name: "invalid param reference in volumeMount.Name - not a param reference", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "foo", + }}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "params.foo", + MountPath: "/path", + }}, + }, + expectedError: apis.FieldError{ + Message: `invalid value: params.foo`, + Paths: []string{"spec.volumeMounts[0].name"}, + Details: `expect the Name to be a single param reference`, + }, + }, { + name: "invalid param reference in volumeMount.Name - nested reference", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "foo", + }}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)-foo", + MountPath: "/path", + }}, + }, + expectedError: apis.FieldError{ + Message: `invalid value: $(params.foo)-foo`, + Paths: []string{"spec.volumeMounts[0].name"}, + Details: `expect the Name to be a single param reference`, + }, + }, { + name: "invalid param reference in volumeMount.Name - multiple params references", + fields: fields{ + Image: "myimage", + Params: []v1.ParamSpec{{ + Name: "foo", + }, { + Name: "bar", + }}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)$(params.bar)", + MountPath: "/path", + }}, + }, + expectedError: apis.FieldError{ + Message: `invalid value: $(params.foo)$(params.bar)`, + Paths: []string{"spec.volumeMounts[0].name"}, + Details: `expect the Name to be a single param reference`, + }, + }, { + name: "invalid param reference in volumeMount.Name - not defined in params", + fields: fields{ + Image: "myimage", + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/path", + }}, + }, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.foo)"`, + Paths: []string{"spec.volumeMounts[0]"}, + }, + }, { + name: "invalid param reference in volumeMount.Name - array used in a volumeMounts name field", + fields: fields{ + Params: []v1.ParamSpec{{ + Name: "gitrepo", + Type: v1.ParamTypeArray, + }}, + Image: "image", + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.gitrepo)", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo)"`, + Paths: []string{"spec.volumeMounts[0]"}, + }, + }, { + name: "invalid param reference in volumeMount.Name - object used in a volumeMounts name field", + fields: fields{ + Params: []v1.ParamSpec{{ + Name: "gitrepo", + Type: v1.ParamTypeObject, + Properties: map[string]v1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Image: "image", + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.gitrepo)", + }}, + }, + expectedError: apis.FieldError{ + Message: `variable type invalid in "$(params.gitrepo)"`, + Paths: []string{"spec.volumeMounts[0]"}, + }, + }, { + name: "invalid param reference in volumeMount.Name - object key not existent in params", + fields: fields{ + Params: []v1.ParamSpec{{ + Name: "gitrepo", + Type: v1.ParamTypeObject, + Properties: map[string]v1.PropertySpec{ + "url": {}, + "commit": {}, + }, + }}, + Image: "image", + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.gitrepo.foo)", + }}, + }, + expectedError: apis.FieldError{ + Message: `non-existent variable in "$(params.gitrepo.foo)"`, + Paths: []string{"spec.volumeMounts[0]"}, + }, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { sa := &v1alpha1.StepAction{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: v1alpha1.StepActionSpec{ - Image: tt.fields.Image, - Command: tt.fields.Command, - Args: tt.fields.Args, - Script: tt.fields.Script, - Env: tt.fields.Env, - Params: tt.fields.Params, - Results: tt.fields.Results, + Image: tt.fields.Image, + Command: tt.fields.Command, + Args: tt.fields.Args, + Script: tt.fields.Script, + Env: tt.fields.Env, + Params: tt.fields.Params, + Results: tt.fields.Results, + VolumeMounts: tt.fields.VolumeMounts, }, } ctx := context.Background() diff --git a/pkg/apis/pipeline/v1alpha1/swagger.json b/pkg/apis/pipeline/v1alpha1/swagger.json index aa6a859ecdc..eea6d5d9a67 100644 --- a/pkg/apis/pipeline/v1alpha1/swagger.json +++ b/pkg/apis/pipeline/v1alpha1/swagger.json @@ -476,6 +476,17 @@ "securityContext": { "description": "SecurityContext defines the security options the Step should be run with. If set, the fields of SecurityContext override the equivalent fields of PodSecurityContext. More info: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/ The value set in StepAction will take precedence over the value from Task.", "$ref": "#/definitions/v1.SecurityContext" + }, + "volumeMounts": { + "description": "Volumes to mount into the Step's filesystem. Cannot be updated.", + "type": "array", + "items": { + "default": {}, + "$ref": "#/definitions/v1.VolumeMount" + }, + "x-kubernetes-list-type": "atomic", + "x-kubernetes-patch-merge-key": "mountPath", + "x-kubernetes-patch-strategy": "merge" } } }, diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index 592f5961a91..4e02d12fe6d 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -340,6 +340,13 @@ func (in *StepActionSpec) DeepCopyInto(out *StepActionSpec) { *out = new(v1.SecurityContext) (*in).DeepCopyInto(*out) } + if in.VolumeMounts != nil { + in, out := &in.VolumeMounts, &out.VolumeMounts + *out = make([]v1.VolumeMount, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } return } diff --git a/pkg/apis/pipeline/v1beta1/param_types.go b/pkg/apis/pipeline/v1beta1/param_types.go index 689770d663e..f0634f8c580 100644 --- a/pkg/apis/pipeline/v1beta1/param_types.go +++ b/pkg/apis/pipeline/v1beta1/param_types.go @@ -283,7 +283,7 @@ func (ps ParamSpecs) ExtractDefaultParamArrayLengths() map[string]int { // it would return ["$(params.array-param[1])", "$(params.other-array-param[2])"]. func extractArrayIndexingParamRefs(paramReference string) []string { l := []string{} - list := substitution.ExtractParamsExpressions(paramReference) + list := substitution.ExtractArrayIndexingParamsExpressions(paramReference) for _, val := range list { indexString := substitution.ExtractIndexString(val) if indexString != "" { diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index e623acbe115..127d20cd788 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -305,6 +305,12 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi Paths: []string{"env"}, }) } + if len(s.VolumeMounts) > 0 { + errs = errs.Also(&apis.FieldError{ + Message: "volumeMounts cannot be used with Ref", + Paths: []string{"volumeMounts"}, + }) + } } else { if len(s.Params) > 0 { errs = errs.Also(&apis.FieldError{ diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 04688954224..936b32ca2bb 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1526,7 +1526,23 @@ func TestTaskSpecValidateErrorWithStepActionRef(t *testing.T) { Message: "params cannot be used without Ref", Paths: []string{"steps[0].params"}, }, - }} + }, { + name: "Cannot use volumeMounts with Ref", + Steps: []v1beta1.Step{{ + Ref: &v1beta1.Ref{ + Name: "stepAction", + }, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/registry-config", + }}, + }}, + enableStepActions: true, + expectedError: apis.FieldError{ + Message: "volumeMounts cannot be used with Ref", + Paths: []string{"steps[0].volumeMounts"}, + }}, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ts := v1beta1.TaskSpec{ diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index ac6c9845f92..cf24b19b977 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -128,6 +128,9 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T if stepActionSpec.Env != nil { s.Env = stepActionSpec.Env } + if len(stepActionSpec.VolumeMounts) > 0 { + s.VolumeMounts = stepActionSpec.VolumeMounts + } if err := validateStepHasStepActionParameters(s.Params, stepActionSpec.Params); err != nil { return nil, err } diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index 95b82a8b377..33e11ae9e9b 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -336,6 +336,10 @@ func TestGetStepActionsData(t *testing.T) { Image: "myimage", Command: []string{"ls"}, Args: []string{"-lh"}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/path", + }}, }, }, want: []v1.Step{{ @@ -344,6 +348,10 @@ func TestGetStepActionsData(t *testing.T) { Args: []string{"-lh"}, WorkingDir: "/bar", Timeout: &metav1.Duration{Duration: time.Hour}, + VolumeMounts: []corev1.VolumeMount{{ + Name: "$(params.foo)", + MountPath: "/path", + }}, }}, }, { name: "step-action-with-script", diff --git a/pkg/substitution/substitution.go b/pkg/substitution/substitution.go index 69b2c0d827d..8e1acab2fe1 100644 --- a/pkg/substitution/substitution.go +++ b/pkg/substitution/substitution.go @@ -334,11 +334,25 @@ func TrimArrayIndex(s string) string { return arrayIndexingRegex.ReplaceAllString(s, "") } -// ExtractParamsExpressions will find all `$(params.paramName[int])` expressions -func ExtractParamsExpressions(s string) []string { +// ExtractArrayIndexingParamsExpressions will find all `$(params.paramName[int])` expressions +func ExtractArrayIndexingParamsExpressions(s string) []string { return paramIndexingRegex.FindAllString(s, -1) } +func ExtractVariableExpressions(s, prefix string) ([]string, error) { + pattern := fmt.Sprintf(braceMatchingRegex, prefix, parameterSubstitution, parameterSubstitution, parameterSubstitution) + re, err := regexp.Compile(pattern) + if err != nil { + return nil, fmt.Errorf("failed to parse regex pattern: %w", err) + } + + matches := re.FindAllString(s, -1) + if len(matches) == 0 { + return []string{}, nil + } + return matches, nil +} + // ExtractIndexString will find the leftmost match of `[int]` func ExtractIndexString(s string) string { return intIndexRegex.FindString(s) diff --git a/pkg/substitution/substitution_test.go b/pkg/substitution/substitution_test.go index ff8c236f13f..c265510d195 100644 --- a/pkg/substitution/substitution_test.go +++ b/pkg/substitution/substitution_test.go @@ -542,7 +542,7 @@ func TestApplyArrayReplacements(t *testing.T) { } } -func TestExtractParamsExpressions(t *testing.T) { +func TestExtractArrayParamsExpressionsExpressions(t *testing.T) { tests := []struct { name string input string @@ -567,7 +567,45 @@ func TestExtractParamsExpressions(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := substitution.ExtractParamsExpressions(tt.input) + got := substitution.ExtractArrayIndexingParamsExpressions(tt.input) + if d := cmp.Diff(tt.want, got); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} + +func TestExtractVariableExpressions(t *testing.T) { + tests := []struct { + name string + input string + prefix string + want []string + }{{ + name: "normal string", + input: "hello world", + prefix: "params", + want: []string{}, + }, { + name: "param reference", + input: "$(params.paramName)", + prefix: "params", + want: []string{"$(params.paramName)"}, + }, { + name: "param star reference", + input: "$(params.paramName[*])", + prefix: "params", + want: []string{"$(params.paramName[*])"}, + }, { + name: "param index reference", + input: "$(params.paramName[1])", + prefix: "params", + want: []string{"$(params.paramName[1])"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _ := substitution.ExtractVariableExpressions(tt.input, tt.prefix) if d := cmp.Diff(tt.want, got); d != "" { t.Error(diff.PrintWantGot(d)) }