fix: ensure default type for params in remote tasks to prevent pipeline failures #7776

l-qing · 2024-03-19T07:17:31Z

analyze

In the existing logic, resources used for ConvertTo should have default values set. Otherwise, there could be issues with incorrect parameter types being set (e.g., an array type being treated as a string type).

pipeline/pkg/apis/pipeline/v1beta1/param_conversion.go

Lines 25 to 31 in f3dd478

    
           func (p ParamSpec) convertTo(ctx context.Context, sink *v1.ParamSpec) { 
        
           	sink.Name = p.Name 
        
           	if p.Type != "" { 
        
           		sink.Type = v1.ParamType(p.Type) 
        
           	} else { 
        
           		sink.Type = v1.ParamType(ParamTypeString) 
        
           	}

However, resources fetched from remote sources haven't undergone the SetDefaults operation. If we directly invoke the ConvertTo operation, it might result in erroneous outcomes.

pipeline/pkg/apis/pipeline/v1beta1/param_types.go

Lines 72 to 101 in f3dd478

    
           // SetDefaults set the default type 
        
           func (pp *ParamSpec) SetDefaults(context.Context) { 
        
           	if pp == nil { 
        
           		return 
        
           	} 
        
           	// Propagate inferred type to the parent ParamSpec's type, and default type to the PropertySpec's type 
        
           	// The sequence to look at is type in ParamSpec -> properties -> type in default -> array/string/object value in default 
        
           	// If neither `properties` or `default` section is provided, ParamTypeString will be the default type. 
        
           	switch { 
        
           	case pp.Type != "": 
        
           		// If param type is provided by the author, do nothing but just set default type for PropertySpec in case `properties` section is provided. 
        
           		pp.setDefaultsForProperties() 
        
           	case pp.Properties != nil: 
        
           		pp.Type = ParamTypeObject 
        
           		// Also set default type for PropertySpec 
        
           		pp.setDefaultsForProperties() 
        
           	case pp.Default == nil: 
        
           		// ParamTypeString is the default value (when no type can be inferred from the default value) 
        
           		pp.Type = ParamTypeString 
        
           	case pp.Default.Type != "": 
        
           		pp.Type = pp.Default.Type 
        
           	case pp.Default.ArrayVal != nil: 
        
           		pp.Type = ParamTypeArray 
        
           	case pp.Default.ObjectVal != nil: 
        
           		pp.Type = ParamTypeObject 
        
           	default: 
        
           		pp.Type = ParamTypeString 
        
           	} 
        
           }

For instance, a v1beta1 ClusterTask that undergoes a direct ConvertTo to convert the resource into a v1 Task for validation might be mistakenly considered invalid.

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Lines 212 to 219 in f3dd478

    
           case *v1beta1.ClusterTask: 
        
           	t, err := convertClusterTaskToTask(ctx, *obj) 
        
           	// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks 
        
           	// without actually creating the Task on the cluster 
        
           	if err := apiserver.DryRunValidate(ctx, namespace, t, tekton); err != nil { 
        
           		return nil, nil, err 
        
           	} 
        
           	return t, nil, err

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Lines 305 to 314 in f3dd478

    
           v1Task := &v1.Task{ 
        
           	TypeMeta: metav1.TypeMeta{ 
        
           		Kind:       "Task", 
        
           		APIVersion: "tekton.dev/v1", 
        
           	}, 
        
           } 
        
           if err := t.ConvertTo(ctx, v1Task); err != nil { 
        
           	return nil, err 
        
           }

Additionally, even if a v1beta1 Task passes validation, the process of converting it to a v1 Task could still incorrectly set default parameter types, leading to errors during execution.

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Lines 194 to 211 in f3dd478

    
           case *v1beta1.Task: 
        
           	// Verify the Task once we fetch from the remote resolution, mutating, validation and conversion of the task should happen after the verification, since signatures are based on the remote task contents 
        
           	vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies) 
        
           	// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks 
        
           	// without actually creating the Task on the cluster. 
        
           	if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil { 
        
           		return nil, nil, err 
        
           	} 
        
           	t := &v1.Task{ 
        
           		TypeMeta: metav1.TypeMeta{ 
        
           			Kind:       "Task", 
        
           			APIVersion: "tekton.dev/v1", 
        
           		}, 
        
           	} 
        
           	if err := obj.ConvertTo(ctx, t); err != nil { 
        
           		return nil, nil, fmt.Errorf("failed to convert obj %s into Pipeline", obj.GetObjectKind().GroupVersionKind().String()) 
        
           	} 
        
           	return t, &vr, nil

Tips

This issue has been present since this commit 445734d92, affecting versions v0.51 - v0.57.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

fix: resolve issues that may cause pipeline failures when using remote resources

/kind bug

tekton-robot · 2024-03-19T07:17:40Z

Hi @l-qing. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2024-03-19T07:27:50Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

vdemeester

/ok-to-test
/cc @tektoncd/core-maintainers @abayer

tekton-robot · 2024-03-19T10:58:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot · 2024-03-19T11:03:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

tekton-robot · 2024-03-19T14:37:15Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

tekton-robot · 2024-03-19T15:00:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

tekton-robot · 2024-03-21T16:03:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

tekton-robot · 2024-03-21T16:05:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2

tekton-robot · 2024-04-03T06:10:03Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2
test/parse/yaml.go	12.3%	24.1%	11.7
test/parse/yaml.go	Do not exist	47.5%
test/parse/yaml.go	Do not exist	7.6%

tekton-robot · 2024-04-03T06:20:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2
test/parse/yaml.go	12.3%	73.4%	61.1
test/parse/yaml.go	Do not exist	24.1%
test/parse/yaml.go	Do not exist	11.1%

tekton-robot · 2024-04-09T06:38:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2
test/parse/yaml.go	66.7%	24.1%	-42.6
test/parse/yaml.go	Do not exist	47.5%
test/parse/yaml.go	Do not exist	7.6%

tekton-robot · 2024-04-09T06:38:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	92.6%	92.8%	0.2
test/parse/yaml.go	66.7%	20.3%	-46.4
test/parse/yaml.go	Do not exist	13.9%
test/parse/yaml.go	Do not exist	26.3%

tekton-robot · 2024-04-13T07:50:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	66.7%	24.1%	-42.6
test/parse/yaml.go	Do not exist	47.5%
test/parse/yaml.go	Do not exist	7.6%

tekton-robot · 2024-04-13T07:51:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	66.7%	20.3%	-46.4
test/parse/yaml.go	Do not exist	13.9%
test/parse/yaml.go	Do not exist	26.3%

tekton-robot · 2024-04-23T15:17:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	66.7%	20.3%	-46.4
test/parse/yaml.go	Do not exist	13.9%
test/parse/yaml.go	Do not exist	26.3%

tekton-robot · 2024-04-23T15:39:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	66.7%	50.6%	-16.0
test/parse/yaml.go	Do not exist	20.3%
test/parse/yaml.go	Do not exist	11.1%

…ne failures fix tektoncd#7775 In the existing logic, resources used for ConvertTo should have default values set. Otherwise, there could be issues with incorrect parameter types being set (e.g., an array type being treated as a string type). However, resources fetched from remote sources haven't undergone the SetDefaults operation. If we directly invoke the ConvertTo operation, it might result in erroneous outcomes. For instance, a v1beta1 ClusterTask that undergoes a direct ConvertTo to convert the resource into a v1 Task for validation might be mistakenly considered invalid. Additionally, even if a v1beta1 Task passes validation, the process of converting it to a v1 Task could still incorrectly set default parameter types, leading to errors during execution.

tekton-robot · 2024-04-24T15:40:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	11.1%	20.3%	9.1
test/parse/yaml.go	Do not exist	13.9%
test/parse/yaml.go	Do not exist	26.3%

tekton-robot · 2024-04-24T15:43:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	91.8%	91.5%	-0.3
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.2%	93.5%	0.3
pkg/reconciler/taskrun/resources/taskref.go	94.0%	94.2%	0.1
test/parse/yaml.go	11.1%	24.1%	12.9
test/parse/yaml.go	Do not exist	47.5%
test/parse/yaml.go	Do not exist	7.6%

afrittoli

/lgtm

tekton-robot requested review from afrittoli and dibyom March 19, 2024 07:17

vdemeester approved these changes Mar 19, 2024

View reviewed changes

tekton-robot requested review from abayer and a team March 19, 2024 10:57

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 19, 2024

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2024

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from 8cdd12d to cf4aa54 Compare March 19, 2024 14:31

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from cf4aa54 to 8610fc1 Compare March 21, 2024 15:57

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from 8610fc1 to 703310f Compare April 3, 2024 05:59

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from 703310f to a3aa1bc Compare April 9, 2024 06:25

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from a3aa1bc to fe42657 Compare April 13, 2024 07:36

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from fe42657 to 34fcb87 Compare April 23, 2024 15:06

l-qing force-pushed the fix/default-param-type-in-remote-task2 branch from 34fcb87 to d8e6d8b Compare April 24, 2024 15:29

afrittoli reviewed Apr 25, 2024

View reviewed changes

tekton-robot assigned afrittoli Apr 25, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024

tekton-robot merged commit 30e389b into tektoncd:main Apr 25, 2024
13 checks passed

l-qing deleted the fix/default-param-type-in-remote-task2 branch April 25, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure default type for params in remote tasks to prevent pipeline failures #7776

fix: ensure default type for params in remote tasks to prevent pipeline failures #7776

l-qing commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

vdemeester left a comment

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 21, 2024

tekton-robot commented Mar 21, 2024

tekton-robot commented Apr 3, 2024

tekton-robot commented Apr 3, 2024

tekton-robot commented Apr 9, 2024

tekton-robot commented Apr 9, 2024

tekton-robot commented Apr 13, 2024

tekton-robot commented Apr 13, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 24, 2024

tekton-robot commented Apr 24, 2024

afrittoli left a comment

	func (p ParamSpec) convertTo(ctx context.Context, sink *v1.ParamSpec) {
	sink.Name = p.Name
	if p.Type != "" {
	sink.Type = v1.ParamType(p.Type)
	} else {
	sink.Type = v1.ParamType(ParamTypeString)
	}

	// SetDefaults set the default type
	func (pp *ParamSpec) SetDefaults(context.Context) {
	if pp == nil {
	return
	}

	// Propagate inferred type to the parent ParamSpec's type, and default type to the PropertySpec's type
	// The sequence to look at is type in ParamSpec -> properties -> type in default -> array/string/object value in default
	// If neither `properties` or `default` section is provided, ParamTypeString will be the default type.
	switch {
	case pp.Type != "":
	// If param type is provided by the author, do nothing but just set default type for PropertySpec in case `properties` section is provided.
	pp.setDefaultsForProperties()
	case pp.Properties != nil:
	pp.Type = ParamTypeObject
	// Also set default type for PropertySpec
	pp.setDefaultsForProperties()
	case pp.Default == nil:
	// ParamTypeString is the default value (when no type can be inferred from the default value)
	pp.Type = ParamTypeString
	case pp.Default.Type != "":
	pp.Type = pp.Default.Type
	case pp.Default.ArrayVal != nil:
	pp.Type = ParamTypeArray
	case pp.Default.ObjectVal != nil:
	pp.Type = ParamTypeObject
	default:
	pp.Type = ParamTypeString
	}
	}

	case *v1beta1.ClusterTask:
	t, err := convertClusterTaskToTask(ctx, *obj)
	// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks
	// without actually creating the Task on the cluster
	if err := apiserver.DryRunValidate(ctx, namespace, t, tekton); err != nil {
	return nil, nil, err
	}
	return t, nil, err

	v1Task := &v1.Task{
	TypeMeta: metav1.TypeMeta{
	Kind: "Task",
	APIVersion: "tekton.dev/v1",
	},
	}

	if err := t.ConvertTo(ctx, v1Task); err != nil {
	return nil, err
	}

	case *v1beta1.Task:
	// Verify the Task once we fetch from the remote resolution, mutating, validation and conversion of the task should happen after the verification, since signatures are based on the remote task contents
	vr := trustedresources.VerifyResource(ctx, obj, k8s, refSource, verificationPolicies)
	// Issue a dry-run request to create the remote Task, so that it can undergo validation from validating admission webhooks
	// without actually creating the Task on the cluster.
	if err := apiserver.DryRunValidate(ctx, namespace, obj, tekton); err != nil {
	return nil, nil, err
	}
	t := &v1.Task{
	TypeMeta: metav1.TypeMeta{
	Kind: "Task",
	APIVersion: "tekton.dev/v1",
	},
	}
	if err := obj.ConvertTo(ctx, t); err != nil {
	return nil, nil, fmt.Errorf("failed to convert obj %s into Pipeline", obj.GetObjectKind().GroupVersionKind().String())
	}
	return t, &vr, nil

fix: ensure default type for params in remote tasks to prevent pipeline failures #7776

fix: ensure default type for params in remote tasks to prevent pipeline failures #7776

Conversation

l-qing commented Mar 19, 2024

analyze

Tips

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 19, 2024

tekton-robot commented Mar 21, 2024

tekton-robot commented Mar 21, 2024

tekton-robot commented Apr 3, 2024

tekton-robot commented Apr 3, 2024

tekton-robot commented Apr 9, 2024

tekton-robot commented Apr 9, 2024

tekton-robot commented Apr 13, 2024

tekton-robot commented Apr 13, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 23, 2024

tekton-robot commented Apr 24, 2024

tekton-robot commented Apr 24, 2024

afrittoli left a comment

Choose a reason for hiding this comment