Dereference the TaskSpec into TaskRun.Status. #2444

dlorenc · 2020-04-20T15:08:28Z

Changes

The Task definition used for a TaskRun can change after the TaskRun
has started. This poses problems for auditability post-run. Rather
than chase down every part of a Task that we might like to audit later,
let's just add the entire thing here.

This is a replacement for #2399

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

The TaskRun.Status now contains a TaskSpec field, including the full dereferenced Task used to instantiate the run.

afrittoli · 2020-04-20T15:37:48Z

Nice, I was going to work on this after I finished #2421 :)
This is both a benefit to auditability, but it also should improve the reconcile performance, since we now fetch the task on every reconcile cycle.
Once we start fetching tasks from an OCI registry, it will be even more relevant.

One though I have is that we should also store resources in the status.

dlorenc · 2020-04-20T15:44:12Z

/test pull-tekton-pipeline-integration-tests

afrittoli

Thanks for this!
I have one comment on this.
Even if only use it for audit for now, I would still add the task to the status when we first obtain the task

afrittoli · 2020-04-20T16:00:17Z

pkg/pod/status.go

@@ -116,6 +118,12 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1alpha1.TaskRun, pod *core
 trs.Steps = []v1alpha1.StepState{}
 trs.Sidecars = []v1alpha1.SidecarState{}

+ ts := v1beta1.TaskSpec{}


We might want to store this into the TaskRun status when we first fetch it, i.e.

pipeline/pkg/reconciler/taskrun/taskrun.go

Lines 207 to 208 in 9344e4f

getTaskFunc, kind := c.getTaskFunc(tr)

taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskFunc)

We could then modify that could to fetch the task only if not available in the status, and else use that from status

Whoa, yeah thanks. Do you think it's enough to only set this once, the first time?

Yes, I think that would be great! Especially when the task spec is not embedded in the taskrun spec, this avoids issues when the task definition is updated during the run.

The only case where is use the change to the spec for something is cancel, so we would have to find an alternative for that. Perhaps the tkn client could reset the task definition in status to force a reload? @vdemeester what do you think?

Oh, actually it would not break cancel since that goes in the spec of the taskrun, not of the task - so fetching the task only once would work just fine!

dlorenc · 2020-04-20T17:25:20Z

One though I have is that we should also store resources in the status.

I hesitated on this one for now. We get some of the resource info in the status now (under containers):

{
      "container": "step-git-source-skaffold-git-build-push-kaniko-p28nk",
      "imageID": "docker-pullable://gcr.io/dlorenc-vmtest2/git-init-4874978a9786b6625dd8b6ef2a21aa70@sha256:99f410cc098b4300384ad056c8febf5bb79e4749417b5e9621fc529514c4a0c3",
      "name": "git-source-skaffold-git-build-push-kaniko-p28nk",
      "terminated": {
        "containerID": "docker://b96396af17dea856526547ef5e140478148d4d672c8bda8a95e6010380e7cfab",
        "exitCode": 0,
        "finishedAt": "2020-04-20T15:32:20Z",
        "message": "[{\"key\":\"commit\",\"value\":\"6ed7aad5e8a36052ee5f6079fc91368e362121f7\",\"resourceRef\":{\"name\":\"skaffold-git-build-push-kaniko\"}}]",
        "reason": "Completed",
        "startedAt": "2020-04-20T15:32:18Z"
      }
    },

This doesn't include commands/arguments, but it's close. We also get resourcesResults.

Should we add them to the v1beta1 API, given we don't know the future of them yet? #2447

afrittoli

/lgtm

pkg/pod/status.go

linux-foundation-easycla · 2020-04-21T14:38:14Z

The committers are authorized under a signed CLA.

✅ dlorenc (1f9c107)

afrittoli

Thanks for this!
We should do the same for pipelines :)
/approve

pkg/reconciler/taskrun/taskrun.go

tekton-robot · 2020-04-21T20:21:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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~~ [afrittoli]

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

bobcatfish · 2020-04-23T14:01:08Z

docs/taskruns.md

-```
-kubectl get taskrun <name>
-```
+The exact Task Spec used to instantiate the TaskRun is also included in the Status for full auditability.


dlorenc · 2020-04-23T14:04:03Z

/test pull-tekton-pipeline-build-tests

The Task definition used for a TaskRun can change after the TaskRun has started. This poses problems for auditability post-run. Rather than chase down every part of a Task that we might like to audit later, let's just add the entire thing here. This is a replacement for tektoncd#2399

dlorenc · 2020-04-23T14:18:11Z

/test pull-tekton-pipeline-integration-tests

dibyom · 2020-04-23T15:06:07Z

/lgtm

dlorenc · 2020-04-23T15:10:50Z

/test pull-tekton-pipeline-integration-tests

skaegi · 2020-04-23T16:21:25Z

Yep. Apart from my own selfish utility concerns ;) size in etcd is the only real concern. This is still fairly safe and not going to break anything for us.

dlorenc · 2020-04-23T17:01:01Z

/test pull-tekton-pipeline-integration-tests

dibyom · 2020-04-23T20:48:33Z

/test pull-tekton-pipeline-build-tests

The Pipeline definition used for a PipelineRun can change after the PipelineRun has started. This poses problems for auditability post-run. Rather than chase down every part of a Pipeline that we might like to audit later, let's just add the entire thing here. This is a follow up to tektoncd#2444.

The Pipeline definition used for a PipelineRun can change after the PipelineRun has started. This poses problems for auditability post-run. Rather than chase down every part of a Pipeline that we might like to audit later, let's just add the entire thing here. This is a follow up to #2444.

bobcatfish · 2020-08-27T14:01:56Z

I believe this and #2489 fixed #279

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 20, 2020

tekton-robot requested review from afrittoli and dibyom April 20, 2020 15:08

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 20, 2020

afrittoli reviewed Apr 20, 2020

View reviewed changes

dlorenc force-pushed the deref branch from 907ffaa to 8de5547 Compare April 20, 2020 17:16

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2020

afrittoli reviewed Apr 21, 2020

View reviewed changes

pkg/pod/status.go Outdated Show resolved Hide resolved

tekton-robot assigned afrittoli Apr 21, 2020

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

dlorenc force-pushed the deref branch from 8de5547 to 4360852 Compare April 21, 2020 14:38

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2020

dlorenc force-pushed the deref branch 2 times, most recently from 1f9c107 to 12304b5 Compare April 21, 2020 19:03

afrittoli reviewed Apr 21, 2020

View reviewed changes

pkg/reconciler/taskrun/taskrun.go Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2020

dlorenc force-pushed the deref branch 2 times, most recently from 9ff346b to b6cedcb Compare April 22, 2020 15:48

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2020

dlorenc force-pushed the deref branch 2 times, most recently from fb4ddc3 to 5cff4f6 Compare April 22, 2020 15:56

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 22, 2020

bobcatfish reviewed Apr 23, 2020

View reviewed changes

dlorenc force-pushed the deref branch from 5cff4f6 to ea2a4dd Compare April 23, 2020 14:08

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020

tekton-robot assigned dibyom Apr 23, 2020

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

dibyom mentioned this pull request Apr 23, 2020

Pick up the cloudevents v2 RC1 #2357

Closed

tekton-robot merged commit 0c2798e into tektoncd:master Apr 23, 2020

dlorenc deleted the deref branch April 24, 2020 13:26

dlorenc mentioned this pull request Apr 24, 2020

Dereference the PipelineSpec into PipelineRun.Status #2489

Merged

3 tasks

afrittoli added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 30, 2020

SaschaSchwarze0 mentioned this pull request May 6, 2020

As developer, I'd like to see build spec snapshot for each buildrun shipwright-io/build#151

Closed

This was referenced May 11, 2020

Add metadata to pipelinerun to capture the complete pipeline definition #994

Closed

Capture User Information with PipelineRun/TaskRun Creation #2045

Closed

vdemeester mentioned this pull request Jul 22, 2020

Embed pipeline in pipelinerun on start tektoncd/cli#726

Closed

qu1queee mentioned this pull request Jul 24, 2020

Upgrade the Tekton version to the latest v0.12 version shipwright-io/build#221

Closed

This was referenced Aug 27, 2020

PipelineRun Status bloat and excessive updates #3140

Closed

Storing and updating PipelineSpec #3145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dereference the TaskSpec into TaskRun.Status. #2444

Dereference the TaskSpec into TaskRun.Status. #2444

dlorenc commented Apr 20, 2020

afrittoli commented Apr 20, 2020

dlorenc commented Apr 20, 2020

afrittoli left a comment

afrittoli Apr 20, 2020

dlorenc Apr 20, 2020

afrittoli Apr 21, 2020

afrittoli Apr 21, 2020

vdemeester Apr 23, 2020

dlorenc commented Apr 20, 2020

afrittoli left a comment

linux-foundation-easycla bot commented Apr 21, 2020 •

edited

Loading

afrittoli left a comment

tekton-robot commented Apr 21, 2020

bobcatfish Apr 23, 2020

dlorenc commented Apr 23, 2020

dlorenc commented Apr 23, 2020

dibyom commented Apr 23, 2020

dlorenc commented Apr 23, 2020

skaegi commented Apr 23, 2020

dlorenc commented Apr 23, 2020

dibyom commented Apr 23, 2020

bobcatfish commented Aug 27, 2020

	getTaskFunc, kind := c.getTaskFunc(tr)
	taskMeta, taskSpec, err := resources.GetTaskData(ctx, tr, getTaskFunc)

Dereference the TaskSpec into TaskRun.Status. #2444

Dereference the TaskSpec into TaskRun.Status. #2444

Conversation

dlorenc commented Apr 20, 2020

Changes

Submitter Checklist

Reviewer Notes

Release Notes

afrittoli commented Apr 20, 2020

dlorenc commented Apr 20, 2020

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Apr 20, 2020

Choose a reason for hiding this comment

dlorenc Apr 20, 2020

Choose a reason for hiding this comment

afrittoli Apr 21, 2020

Choose a reason for hiding this comment

afrittoli Apr 21, 2020

Choose a reason for hiding this comment

vdemeester Apr 23, 2020

Choose a reason for hiding this comment

dlorenc commented Apr 20, 2020

afrittoli left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Apr 21, 2020 • edited Loading

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Apr 21, 2020

bobcatfish Apr 23, 2020

Choose a reason for hiding this comment

dlorenc commented Apr 23, 2020

dlorenc commented Apr 23, 2020

dibyom commented Apr 23, 2020

dlorenc commented Apr 23, 2020

skaegi commented Apr 23, 2020

dlorenc commented Apr 23, 2020

dibyom commented Apr 23, 2020

bobcatfish commented Aug 27, 2020

linux-foundation-easycla bot commented Apr 21, 2020 •

edited

Loading