diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 4c7cafd8ac7..b3d676a24ca 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -81,6 +81,9 @@ const ( // parameter(s) declared in the PipelineRun do not have the some declared type as the // parameters(s) declared in the Pipeline that they are supposed to override. ReasonParameterTypeMismatch = "ParameterTypeMismatch" + // ReasonObjectParameterMissKeys indicates that the object param value provided from PipelineRun spec + // misses some keys required for the object param declared in Pipeline spec. + ReasonObjectParameterMissKeys = "ObjectParameterMissKeys" // ReasonCouldntGetTask indicates that the reason for the failure status is that the // associated Pipeline's Tasks couldn't all be retrieved ReasonCouldntGetTask = "CouldntGetTask" @@ -429,8 +432,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get // Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type. // Weird substitution issues can occur if this is not validated (ApplyParameters() does not verify type). - err = resources.ValidateParamTypesMatching(pipelineSpec, pr) - if err != nil { + if err = resources.ValidateParamTypesMatching(pipelineSpec, pr); err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it pr.Status.MarkFailed(ReasonParameterTypeMismatch, "PipelineRun %s/%s parameters have mismatching types with Pipeline %s/%s's parameters: %s", @@ -438,6 +440,15 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get return controller.NewPermanentError(err) } + // Ensure that the keys of an object param declared in PipelineSpec are not missed in the PipelineRunSpec + if err = resources.ValidateObjectParamRequiredKeys(pipelineSpec.Params, pr.Spec.Params); err != nil { + // This Run has failed, so we need to mark it as failed and stop reconciling it + pr.Status.MarkFailed(ReasonObjectParameterMissKeys, + "PipelineRun %s/%s parameters is missing object keys required by Pipeline %s/%s's parameters: %s", + pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err) + return controller.NewPermanentError(err) + } + // Ensure that the workspaces expected by the Pipeline are provided by the PipelineRun. if err := resources.ValidateWorkspaceBindings(pipelineSpec, pr); err != nil { pr.Status.MarkFailed(ReasonInvalidWorkspaceBinding, diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 313b9a7bd8f..c1bd72c4e0a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -914,6 +914,18 @@ spec: type: %s `, v1beta1.ParamTypeArray)), parse.MustParseTask(t, fmt.Sprintf(` +metadata: + name: a-task-that-needs-object-params + namespace: foo +spec: + params: + - name: some-param + type: %s + properties: + key1: {} + key2: {} +`, v1beta1.ParamTypeObject)), + parse.MustParseTask(t, fmt.Sprintf(` metadata: name: a-task-that-needs-a-resource namespace: foo @@ -989,6 +1001,22 @@ spec: taskRef: name: a-task-that-needs-array-params `, v1beta1.ParamTypeArray)), + parse.MustParsePipeline(t, fmt.Sprintf(` +metadata: + name: a-pipeline-with-object-params + namespace: foo +spec: + params: + - name: some-param + type: %s + properties: + key1: {type: string} + key2: {type: string} + tasks: + - name: some-task + taskRef: + name: a-task-that-needs-object-params +`, v1beta1.ParamTypeObject)), } for _, tc := range []struct { @@ -1118,6 +1146,26 @@ spec: "Normal Started", "Warning Failed PipelineRun foo/pipeline-mismatching-param-type parameters have mismatching types", }, + }, { + name: "invalid-pipeline-missing-object-keys", + pipelineRun: parse.MustParsePipelineRun(t, ` +metadata: + name: pipeline-missing-object-param-keys + namespace: foo +spec: + pipelineRef: + name: a-pipeline-with-object-params + params: + - name: some-param + value: + key1: "a" +`), + reason: ReasonObjectParameterMissKeys, + permanentError: true, + wantEvents: []string{ + "Normal Started", + fmt.Sprintf("Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters: %v", map[string][]string{"some-param": {"key2"}}), + }, }, { name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling", pipelineRun: parse.MustParsePipelineRun(t, fmt.Sprintf(` @@ -1254,10 +1302,13 @@ spec: }, }} { t.Run(tc.name, func(t *testing.T) { + cms := []*corev1.ConfigMap{withEnabledAlphaAPIFields(newFeatureFlagsConfigMap())} + d := test.Data{ PipelineRuns: []*v1beta1.PipelineRun{tc.pipelineRun}, Pipelines: ps, Tasks: ts, + ConfigMaps: cms, } prt := newPipelineRunTest(d, t) defer prt.Cancel() @@ -7326,7 +7377,7 @@ spec: - pipelineTaskName: hello-world-1 metadata: labels: - PipelineTaskRunSpecLabel: PipelineTaskRunSpecValue + PipelineTaskRunSpecLabel: PipelineTaskRunSpecValue annotations: PipelineTaskRunSpecAnnotation: PipelineTaskRunSpecValue taskServiceAccountName: custom-sa @@ -7377,7 +7428,7 @@ spec: image: foo-image metadata: labels: - TestPrecedenceLabel: PipelineTaskSpecValue + TestPrecedenceLabel: PipelineTaskSpecValue annotations: TestPrecedenceAnnotation: PipelineTaskSpecValue `)} @@ -7387,9 +7438,9 @@ metadata: namespace: foo metadata: labels: - TestPrecedenceLabel: PipelineRunValue + TestPrecedenceLabel: PipelineRunValue annotations: - TestPrecedenceAnnotation: PipelineRunValue + TestPrecedenceAnnotation: PipelineRunValue spec: pipelineRef: name: test-pipeline @@ -7397,7 +7448,7 @@ spec: - pipelineTaskName: hello-world-1 metadata: labels: - TestPrecedenceLabel: PipelineTaskRunSpecValue + TestPrecedenceLabel: PipelineTaskRunSpecValue annotations: TestPrecedenceAnnotation: PipelineTaskRunSpecValue taskServiceAccountName: custom-sa diff --git a/pkg/reconciler/pipelinerun/resources/validate_params.go b/pkg/reconciler/pipelinerun/resources/validate_params.go index febfe3ee8e3..dca9cb6ae9c 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params.go @@ -21,6 +21,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/list" + "github.com/tektoncd/pipeline/pkg/reconciler/taskrun" ) // ValidateParamTypesMatching validate that parameters in PipelineRun override corresponding parameters in Pipeline of the same type. @@ -73,3 +74,13 @@ func ValidateRequiredParametersProvided(pipelineParameters *[]v1beta1.ParamSpec, } return nil } + +// ValidateObjectParamRequiredKeys validates that the required keys of all the object parameters expected by the Pipeline are provided by the PipelineRun. +func ValidateObjectParamRequiredKeys(pipelineParameters []v1beta1.ParamSpec, pipelineRunParameters []v1beta1.Param) error { + missings := taskrun.MissingKeysObjectParamNames(pipelineParameters, pipelineRunParameters) + if len(missings) != 0 { + return fmt.Errorf("PipelineRun missing object keys for parameters: %v", missings) + } + + return nil +} diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go index 17ddc77b590..7e5fefbab3f 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_params_test.go +++ b/pkg/reconciler/pipelinerun/resources/validate_params_test.go @@ -188,3 +188,110 @@ func TestValidateRequiredParametersProvided_Invalid(t *testing.T) { }) } } + +func TestValidateObjectParamRequiredKeys_Invalid(t *testing.T) { + for _, tc := range []struct { + name string + pp []v1beta1.ParamSpec + prp []v1beta1.Param + }{{ + name: "miss all required keys", + pp: []v1beta1.ParamSpec{ + { + Name: "an-object-param", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }, + }, + prp: []v1beta1.Param{ + { + Name: "an-object-param", + Value: *v1beta1.NewObject(map[string]string{ + "foo": "val1", + })}, + }, + }, { + name: "miss one of the required keys", + pp: []v1beta1.ParamSpec{ + { + Name: "an-object-param", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }, + }, + prp: []v1beta1.Param{ + { + Name: "an-object-param", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "foo", + })}, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + if err := ValidateObjectParamRequiredKeys(tc.pp, tc.prp); err == nil { + t.Errorf("Expected to see error when validating invalid object parameter keys but saw none") + } + }) + } +} + +func TestValidateObjectParamRequiredKeys_Valid(t *testing.T) { + for _, tc := range []struct { + name string + pp []v1beta1.ParamSpec + prp []v1beta1.Param + }{{ + name: "all keys are provided with a value", + pp: []v1beta1.ParamSpec{ + { + Name: "an-object-param", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }, + }, + prp: []v1beta1.Param{ + { + Name: "an-object-param", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "val1", + "key2": "val2", + })}, + }, + }, { + name: "extra keys are provided", + pp: []v1beta1.ParamSpec{ + { + Name: "an-object-param", + Type: v1beta1.ParamTypeObject, + Properties: map[string]v1beta1.PropertySpec{ + "key1": {Type: "string"}, + "key2": {Type: "string"}, + }, + }, + }, + prp: []v1beta1.Param{ + { + Name: "an-object-param", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "val1", + "key2": "val2", + "key3": "val3", + })}, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + if err := ValidateObjectParamRequiredKeys(tc.pp, tc.prp); err != nil { + t.Errorf("Didn't expect to see error when validating invalid object parameter keys but got: %v", err) + } + }) + } +} diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index 96d3a3589bb..f03405f1972 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -81,7 +81,7 @@ func validateParams(ctx context.Context, paramSpecs []v1beta1.ParamSpec, params if wrongTypeParamNames := wrongTypeParamsNames(params, matrix, neededParamsTypes); len(wrongTypeParamNames) != 0 { return fmt.Errorf("param types don't match the user-specified type: %s", wrongTypeParamNames) } - if missingKeysObjectParamNames := missingKeysObjectParamNames(paramSpecs, params); len(missingKeysObjectParamNames) != 0 { + if missingKeysObjectParamNames := MissingKeysObjectParamNames(paramSpecs, params); len(missingKeysObjectParamNames) != 0 { return fmt.Errorf("missing keys for these params which are required in ParamSpec's properties %v", missingKeysObjectParamNames) } @@ -157,9 +157,8 @@ func wrongTypeParamsNames(params []v1beta1.Param, matrix []v1beta1.Param, needed return wrongTypeParamNames } -// missingKeysObjectParamNames checks if all required keys of object type params are provided in taskrun params. -// TODO (@chuangw6): This might be refactored out to support single pair validation. -func missingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) []string { +// MissingKeysObjectParamNames checks if all required keys of object type params are provided in taskrun params. +func MissingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) map[string][]string { neededKeys := make(map[string][]string) providedKeys := make(map[string][]string) @@ -185,17 +184,16 @@ func missingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta } // validateObjectKeys checks if objects have missing keys in its provider (either taskrun value or result value) -func validateObjectKeys(neededObjectKeys, providedObjectKeys map[string][]string) []string { - missings := []string{} +func validateObjectKeys(neededObjectKeys, providedObjectKeys map[string][]string) map[string][]string { + missings := map[string][]string{} for p, keys := range providedObjectKeys { if _, ok := neededObjectKeys[p]; !ok { - // Ignore any missing objects - this happens when extra objects were - // passed that aren't being used. + // Ignore any missing objects - this happens when object param is provided with default continue } missedKeys := list.DiffLeft(neededObjectKeys[p], keys) if len(missedKeys) != 0 { - missings = append(missings, p) + missings[p] = missedKeys } }