fix mapping between Status.Steps and Status.TaskSpec.Steps #1188

jkhelil · 2024-08-26T14:57:18Z

Changes

this PR fixes the mapping between tr.Status.Steps and tr.Status.taskSpec.steps in this way:

they should have the same size in the last 15 versions at least of pipelines
if size are different, we fail altogether and do not try to sign anything
if size are the same, we validate that if a step is name unamed-…, it has an empty name in the other list, if doesnt we fail
fixes Tekton Chains Controller is in "CrashLoopBackOff" due to "index out of range" error #1185

Submitter Checklist

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

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2024-08-26T15:01:46Z

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

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	72.0%	75.9%	3.9

vdemeester

One question.

vdemeester · 2024-08-26T15:06:14Z

pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go

+			if len(tr.Status.TaskSpec.Steps) != len(tr.Status.Steps) {
+				logger.Errorf("Mismatch in number of steps for task run %s. TaskSpec steps: %d, Status steps: %d",
+					tr.Name, len(tr.Status.TaskSpec.Steps), len(tr.Status.Steps))
+				continue // Skip this task run entirely


Shouldn't we mark the TaskRun as "failed to sign" or something ? (so that the controller doesn't pick it up later on).

I think we dont have a way to do this today, i reference a discussion here about that #979

tekton-robot · 2024-08-27T07:34:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	72.0%	75.9%	3.9

jkhelil · 2024-08-27T10:52:01Z

/test pull-tekton-chains-unit-tests

jkhelil · 2024-08-27T13:42:56Z

/test pull-tekton-chains-unit-tests

jkhelil · 2024-08-28T13:58:44Z

@lcarva @vdemeester can you have a look please ?

jkhelil · 2024-09-10T06:38:21Z

Hi @lcarva, can you have a look to this please ?

lcarva

/lgtm

Thank you!

tekton-robot · 2024-09-10T12:20:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lcarva, 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~~ [lcarva]

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

…1188)

…1203) * fix mapping between Status.Steps and Status.TaskSpec.Steps (#1188) * .github: add github-actions to dependabot… (#1194) * .github: add github-actions to dependabot… … so that we also get dependabot actions updates. Today, we seems to be using deprecated actions. Signed-off-by: Vincent Demeester <vdemeest@redhat.com> * .github/workflows: update some action versions Signed-off-by: Vincent Demeester <vdemeest@redhat.com> --------- Signed-off-by: Vincent Demeester <vdemeest@redhat.com> --------- Signed-off-by: Vincent Demeester <vdemeest@redhat.com> Co-authored-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester · 2024-11-07T19:35:44Z

/cherry-pick release-v0.22.x

vdemeester · 2024-11-07T19:35:48Z

/cherry-pick release-v0.21.x

vdemeester · 2024-11-08T15:43:46Z

/cherry-pick release-v0.22.x

vdemeester · 2024-11-08T15:43:54Z

/cherry-pick release-v0.21.x

tekton-robot · 2024-11-08T15:44:26Z

@vdemeester: new pull request could not be created: failed to create pull request against tektoncd/chains#release-v0.22.x from head tekton-robot:cherry-pick-1188-to-release-v0.22.x: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between tektoncd:release-v0.22.x and tekton-robot:cherry-pick-1188-to-release-v0.22.x"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"}

In response to this:

/cherry-pick release-v0.22.x

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-11-08T15:44:33Z

@vdemeester: #1188 failed to apply on top of branch "release-v0.21.x":

Applying: fix mapping between Status.Steps and Status.TaskSpec.Steps
Using index info to reconstruct a base tree...
M	pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go
M	pkg/chains/formats/slsa/v1/pipelinerun/provenance_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/chains/formats/slsa/v1/pipelinerun/provenance_test.go
Auto-merging pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go
CONFLICT (content): Merge conflict in pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix mapping between Status.Steps and Status.TaskSpec.Steps
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.21.x

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 added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 26, 2024

tekton-robot requested review from chitrangpatel and wlynch August 26, 2024 14:57

vdemeester reviewed Aug 26, 2024

View reviewed changes

fix mapping between Status.Steps and Status.TaskSpec.Steps

cadb35e

jkhelil force-pushed the fix_1185 branch from 7377249 to cadb35e Compare August 27, 2024 07:29

vdemeester approved these changes Sep 10, 2024

View reviewed changes

lcarva approved these changes Sep 10, 2024

View reviewed changes

tekton-robot assigned lcarva Sep 10, 2024

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

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

tekton-robot merged commit a8cd5f6 into tektoncd:main Sep 10, 2024
18 checks passed

jkhelil added a commit to jkhelil/chains that referenced this pull request Sep 16, 2024

fix mapping between Status.Steps and Status.TaskSpec.Steps (tektoncd#…

9d7d110

…1188)

lcarva mentioned this pull request Nov 7, 2024

crash on startup #1243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix mapping between Status.Steps and Status.TaskSpec.Steps #1188

fix mapping between Status.Steps and Status.TaskSpec.Steps #1188

jkhelil commented Aug 26, 2024 •

edited

Loading

tekton-robot commented Aug 26, 2024

vdemeester left a comment

vdemeester Aug 26, 2024

jkhelil Aug 27, 2024

tekton-robot commented Aug 27, 2024

jkhelil commented Aug 27, 2024 •

edited

Loading

jkhelil commented Aug 27, 2024

jkhelil commented Aug 28, 2024

jkhelil commented Sep 10, 2024

lcarva left a comment

tekton-robot commented Sep 10, 2024

vdemeester commented Nov 7, 2024

vdemeester commented Nov 7, 2024

vdemeester commented Nov 8, 2024

vdemeester commented Nov 8, 2024

tekton-robot commented Nov 8, 2024

tekton-robot commented Nov 8, 2024

fix mapping between Status.Steps and Status.TaskSpec.Steps #1188

fix mapping between Status.Steps and Status.TaskSpec.Steps #1188

Conversation

jkhelil commented Aug 26, 2024 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 26, 2024

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Aug 26, 2024

Choose a reason for hiding this comment

jkhelil Aug 27, 2024

Choose a reason for hiding this comment

tekton-robot commented Aug 27, 2024

jkhelil commented Aug 27, 2024 • edited Loading

jkhelil commented Aug 27, 2024

jkhelil commented Aug 28, 2024

jkhelil commented Sep 10, 2024

lcarva left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2024

vdemeester commented Nov 7, 2024

vdemeester commented Nov 7, 2024

vdemeester commented Nov 8, 2024

vdemeester commented Nov 8, 2024

tekton-robot commented Nov 8, 2024

tekton-robot commented Nov 8, 2024

jkhelil commented Aug 26, 2024 •

edited

Loading

jkhelil commented Aug 27, 2024 •

edited

Loading