TEP-0075: Relax object param key validation

Related to #5270. Prior to this change, if users want to provide default value for an object param, they have to provide values for ALL keys, but can't provide values for a subset of required keys that are declared in `properties` section. Same restriction applies to the `value` provided from run level. In this PR, we relax the validation for object param keys to allow users to provide values for an object param in a more flexible way i.e. a subset of keys are provided in the spec's default, and the rest of the keys are provided in run level's value. Signed-off-by: Chuang Wang <chuangw@google.com>
tektoncd · Sep 12, 2022 · 80fc393 · 80fc393
1 parent 3733e13
commit 80fc393
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 133 deletions.
diff --git a/docs/tasks.md b/docs/tasks.md
@@ -532,6 +532,7 @@ Each declared parameter has a `type` field, which can be set to `string`, `array
   > NOTE: 
   > - `object` param is an `alpha` feature and gated by the `alpha` feature flag.
   > - `object` param must specify the `properties` section to define the schema i.e. what keys are available for this object param. See how to define `properties` section in the following example and the [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#defaulting-to-string-types-for-values).
+  > - When providing value for an `object` param, one may provide values for just a subset of keys in spec's `default`, and provide values for the rest of keys at runtime ([example](../examples/v1beta1/taskruns/alpha/object-param-result.yaml)).
   > - When using object in variable replacement, users can only access its individual key ("child" member) of the object by its name i.e. `$(params.gitrepo.url)`. Using an entire object as a value is only allowed when the value is also an object like [this example](https://github.com/tektoncd/pipeline/blob/55665765e4de35b3a4fb541549ae8cdef0996641/examples/v1beta1/pipelineruns/alpha/pipeline-object-param-and-result.yaml#L64-L65). See more details about using object param from the [TEP-0075](https://github.com/tektoncd/community/blob/main/teps/0075-object-param-and-result-types.md#using-objects-in-variable-replacement).
 
 - `array` type 

diff --git a/examples/v1beta1/taskruns/alpha/object-param-result.yaml b/examples/v1beta1/taskruns/alpha/object-param-result.yaml
@@ -6,7 +6,6 @@ spec:
   params:
     - name: gitrepo
       value:
-        url: "xyz.com"
         commit: "sha123"
   taskSpec:
     params:
@@ -15,6 +14,8 @@ spec:
         properties:
           url: {type: string}
           commit: {type: string}
+        default:
+          url: "github.example.com"
     results:
       - name: object-results
         type: object

diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go
@@ -27,7 +27,6 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
 	"github.com/tektoncd/pipeline/pkg/apis/version"
-	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/substitution"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -366,7 +365,6 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para
 		errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
 	}
 	errs = errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
-	errs = errs.Also(validateObjectDefault(objectParamSpecs))
 	return errs.Also(validateObjectUsage(ctx, steps, objectParamSpecs))
 }
 
@@ -404,45 +402,6 @@ func validateObjectUsage(ctx context.Context, steps []Step, params []ParamSpec)
 	return errs.Also(validateObjectUsageAsWhole(steps, "params", objectParameterNames))
 }
 
-// validateObjectDefault validates the keys of all the object params within a
-// slice of ParamSpecs are provided in default iff the default section is provided.
-func validateObjectDefault(objectParams []ParamSpec) (errs *apis.FieldError) {
-	for _, p := range objectParams {
-		errs = errs.Also(ValidateObjectKeys(p.Properties, p.Default).ViaField(p.Name))
-	}
-	return errs
-}
-
-// ValidateObjectKeys validates if object keys defined in properties are all provided in its value provider iff the provider is not nil.
-func ValidateObjectKeys(properties map[string]PropertySpec, propertiesProvider *ParamValue) (errs *apis.FieldError) {
-	if propertiesProvider == nil || propertiesProvider.ObjectVal == nil {
-		return nil
-	}
-
-	neededKeys := []string{}
-	providedKeys := []string{}
-
-	// collect all needed keys
-	for key := range properties {
-		neededKeys = append(neededKeys, key)
-	}
-
-	// collect all provided keys
-	for key := range propertiesProvider.ObjectVal {
-		providedKeys = append(providedKeys, key)
-	}
-
-	missings := list.DiffLeft(neededKeys, providedKeys)
-	if len(missings) != 0 {
-		return &apis.FieldError{
-			Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", missings),
-			Paths:   []string{"properties", "default"},
-		}
-	}
-
-	return nil
-}
-
 // validateObjectUsageAsWhole makes sure the object params are not used as whole when providing values for strings
 // i.e. param.objectParam, param.objectParam[*]
 func validateObjectUsageAsWhole(steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) {

diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go
@@ -189,6 +189,17 @@ func TestTaskSpecValidate(t *testing.T) {
 					"key1": {},
 					"key2": {},
 				},
+			}, {
+				Name:        "myobjWithDefaultPartialKeys",
+				Type:        v1.ParamTypeObject,
+				Description: "param",
+				Properties: map[string]v1.PropertySpec{
+					"key1": {},
+					"key2": {},
+				},
+				Default: v1.NewObject(map[string]string{
+					"key1": "default",
+				}),
 			}},
 			Steps: validSteps,
 		},
@@ -686,27 +697,6 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
 			Paths:   []string{"params.task.properties"},
 		},
-	}, {
-		name: "keys defined in properties are missed in default",
-		fields: fields{
-			Params: []v1.ParamSpec{{
-				Name:        "myobjectParam",
-				Description: "param",
-				Properties: map[string]v1.PropertySpec{
-					"key1": {},
-					"key2": {},
-				},
-				Default: v1.NewObject(map[string]string{
-					"key1": "var1",
-					"key3": "var1",
-				}),
-			}},
-			Steps: validSteps,
-		},
-		expectedError: apis.FieldError{
-			Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", []string{"key2"}),
-			Paths:   []string{"myobjectParam.properties", "myobjectParam.default"},
-		},
 	}, {
 		name: "invalid step",
 		fields: fields{

diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go
@@ -27,7 +27,6 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
 	"github.com/tektoncd/pipeline/pkg/apis/version"
-	"github.com/tektoncd/pipeline/pkg/list"
 	"github.com/tektoncd/pipeline/pkg/substitution"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -368,7 +367,6 @@ func ValidateParameterVariables(ctx context.Context, steps []Step, params []Para
 		errs = errs.Also(validateVariables(ctx, steps, "params", allParameterNames))
 	}
 	errs = errs.Also(validateArrayUsage(steps, "params", arrayParameterNames))
-	errs = errs.Also(validateObjectDefault(objectParamSpecs))
 	return errs.Also(validateObjectUsage(ctx, steps, objectParamSpecs))
 }
 
@@ -425,45 +423,6 @@ func validateObjectUsage(ctx context.Context, steps []Step, params []ParamSpec)
 	return errs.Also(validateObjectUsageAsWhole(steps, "params", objectParameterNames))
 }
 
-// validateObjectDefault validates the keys of all the object params within a
-// slice of ParamSpecs are provided in default iff the default section is provided.
-func validateObjectDefault(objectParams []ParamSpec) (errs *apis.FieldError) {
-	for _, p := range objectParams {
-		errs = errs.Also(ValidateObjectKeys(p.Properties, p.Default).ViaField(p.Name))
-	}
-	return errs
-}
-
-// ValidateObjectKeys validates if object keys defined in properties are all provided in its value provider iff the provider is not nil.
-func ValidateObjectKeys(properties map[string]PropertySpec, propertiesProvider *ParamValue) (errs *apis.FieldError) {
-	if propertiesProvider == nil || propertiesProvider.ObjectVal == nil {
-		return nil
-	}
-
-	neededKeys := []string{}
-	providedKeys := []string{}
-
-	// collect all needed keys
-	for key := range properties {
-		neededKeys = append(neededKeys, key)
-	}
-
-	// collect all provided keys
-	for key := range propertiesProvider.ObjectVal {
-		providedKeys = append(providedKeys, key)
-	}
-
-	missings := list.DiffLeft(neededKeys, providedKeys)
-	if len(missings) != 0 {
-		return &apis.FieldError{
-			Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", missings),
-			Paths:   []string{fmt.Sprintf("properties"), fmt.Sprintf("default")},
-		}
-	}
-
-	return nil
-}
-
 // validateObjectUsageAsWhole makes sure the object params are not used as whole when providing values for strings
 // i.e. param.objectParam, param.objectParam[*]
 func validateObjectUsageAsWhole(steps []Step, prefix string, vars sets.String) (errs *apis.FieldError) {

diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go
@@ -225,6 +225,17 @@ func TestTaskSpecValidate(t *testing.T) {
 					"key1": {},
 					"key2": {},
 				},
+			}, {
+				Name:        "myobjWithDefaultPartialKeys",
+				Type:        v1beta1.ParamTypeObject,
+				Description: "param",
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {},
+					"key2": {},
+				},
+				Default: v1beta1.NewObject(map[string]string{
+					"key1": "default",
+				}),
 			}},
 			Steps: validSteps,
 		},
@@ -798,27 +809,6 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
 			Paths:   []string{"params.task.properties"},
 		},
-	}, {
-		name: "keys defined in properties are missed in default",
-		fields: fields{
-			Params: []v1beta1.ParamSpec{{
-				Name:        "myobjectParam",
-				Description: "param",
-				Properties: map[string]v1beta1.PropertySpec{
-					"key1": {},
-					"key2": {},
-				},
-				Default: v1beta1.NewObject(map[string]string{
-					"key1": "var1",
-					"key3": "var1",
-				}),
-			}},
-			Steps: validSteps,
-		},
-		expectedError: apis.FieldError{
-			Message: fmt.Sprintf("Required key(s) %s are missing in the value provider.", []string{"key2"}),
-			Paths:   []string{"myobjectParam.properties", "myobjectParam.default"},
-		},
 	}, {
 		name: "invalid step",
 		fields: fields{

diff --git a/pkg/reconciler/pipelinerun/resources/validate_params_test.go b/pkg/reconciler/pipelinerun/resources/validate_params_test.go
@@ -253,6 +253,31 @@ func TestValidateObjectParamRequiredKeys_Valid(t *testing.T) {
 		pp   []v1beta1.ParamSpec
 		prp  []v1beta1.Param
 	}{{
+		name: "some keys are provided by default, and the rest are provided in value",
+		pp: []v1beta1.ParamSpec{
+			{
+				Name: "an-object-param",
+				Type: v1beta1.ParamTypeObject,
+				Properties: map[string]v1beta1.PropertySpec{
+					"key1": {Type: "string"},
+					"key2": {Type: "string"},
+				},
+				Default: &v1beta1.ParamValue{
+					Type: v1beta1.ParamTypeObject,
+					ObjectVal: map[string]string{
+						"key1": "val1",
+					},
+				},
+			},
+		},
+		prp: []v1beta1.Param{
+			{
+				Name: "an-object-param",
+				Value: *v1beta1.NewObject(map[string]string{
+					"key2": "val2",
+				})},
+		},
+	}, {
 		name: "all keys are provided with a value",
 		pp: []v1beta1.ParamSpec{
 			{

diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go
@@ -172,21 +172,28 @@ 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.
+// MissingKeysObjectParamNames checks if all required keys of object type params are provided in taskrun params or taskSpec's default.
 func MissingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta1.Param) map[string][]string {
 	neededKeys := make(map[string][]string)
 	providedKeys := make(map[string][]string)
 
-	// collect needed keys for object parameters
 	for _, spec := range paramSpecs {
 		if spec.Type == v1beta1.ParamTypeObject {
+			// collect required keys from properties section
 			for key := range spec.Properties {
 				neededKeys[spec.Name] = append(neededKeys[spec.Name], key)
 			}
+
+			// collect provided keys from default
+			if spec.Default != nil && spec.Default.ObjectVal != nil {
+				for key := range spec.Default.ObjectVal {
+					providedKeys[spec.Name] = append(providedKeys[spec.Name], key)
+				}
+			}
 		}
 	}
 
-	// collect provided keys for object parameters
+	// collect provided keys from run level value
 	for _, p := range params {
 		if p.Value.Type == v1beta1.ParamTypeObject {
 			for key := range p.Value.ObjectVal {
@@ -198,7 +205,7 @@ func MissingKeysObjectParamNames(paramSpecs []v1beta1.ParamSpec, params []v1beta
 	return findMissingKeys(neededKeys, providedKeys)
 }
 
-// findMissingKeys checks if objects have missing keys in its provider (either taskrun value or result value)
+// findMissingKeys checks if objects have missing keys in its providers (taskrun value and default)
 func findMissingKeys(neededKeys, providedKeys map[string][]string) map[string][]string {
 	missings := map[string][]string{}
 	for p, keys := range providedKeys {