From 0c2798e9054094b319f0a1473a27db64d6f4093c Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Mon, 20 Apr 2020 09:53:48 -0500 Subject: [PATCH] Dereference the TaskSpec into TaskRun.Status. 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 https://github.com/tektoncd/pipeline/pull/2399 --- docs/taskruns.md | 6 ++-- go.mod | 2 +- pkg/apis/pipeline/v1beta1/taskrun_types.go | 3 ++ .../pipeline/v1beta1/zz_generated.deepcopy.go | 5 +++ pkg/reconciler/taskrun/taskrun.go | 14 ++++++++ pkg/reconciler/taskrun/taskrun_test.go | 32 +++++++++++++++++++ test/taskrun_test.go | 4 +++ 7 files changed, 62 insertions(+), 4 deletions(-) diff --git a/docs/taskruns.md b/docs/taskruns.md index ab119633a9d..ce27a5b457d 100644 --- a/docs/taskruns.md +++ b/docs/taskruns.md @@ -316,9 +316,9 @@ If multiple `Steps` are defined in the `Task` invoked by the `TaskRun`, you can status in the `steps.results` field using the following command, where `` is the name of the target `TaskRun`: -``` -kubectl get taskrun -``` +The exact Task Spec used to instantiate the TaskRun is also included in the Status for full auditability. + +### Steps The corresponding statuses appear in the `status.steps` list in the order in which the `Steps` have been specified in the `Task` definition. diff --git a/go.mod b/go.mod index e324709c8f8..730f0bb19f2 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,7 @@ require ( golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect golang.org/x/tools v0.0.0-20200214144324-88be01311a71 // indirect gomodules.xyz/jsonpatch/v2 v2.1.0 - google.golang.org/api v0.15.0 + google.golang.org/api v0.15.0 // indirect google.golang.org/appengine v1.6.5 // indirect k8s.io/api v0.17.3 k8s.io/apiextensions-apiserver v0.17.3 // indirect diff --git a/pkg/apis/pipeline/v1beta1/taskrun_types.go b/pkg/apis/pipeline/v1beta1/taskrun_types.go index 08cefb8d195..79b08c498d6 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_types.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_types.go @@ -177,6 +177,9 @@ type TaskRunStatusFields struct { // The list has one entry per sidecar in the manifest. Each entry is // represents the imageid of the corresponding sidecar. Sidecars []SidecarState `json:"sidecars,omitempty"` + + // TaskSpec contains the Spec from the dereferenced Task definition used to instantiate this TaskRun. + TaskSpec *TaskSpec `json:"taskSpec,omitempty"` } // TaskRunResult used to describe the results of a task diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 1d53fb0f78e..1d92712e04a 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -1481,6 +1481,11 @@ func (in *TaskRunStatusFields) DeepCopyInto(out *TaskRunStatusFields) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.TaskSpec != nil { + in, out := &in.TaskSpec, &out.TaskSpec + *out = new(TaskSpec) + (*in).DeepCopyInto(*out) + } return } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index df632b4b075..82abcefd334 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -216,6 +216,11 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error return nil } + // Store the fetched TaskSpec on the TaskRun for auditing + if err := storeTaskSpec(ctx, tr, taskSpec); err != nil { + c.Logger.Errorf("Failed to store TaskSpec on TaskRun.Statusfor taskrun %s: %v", tr.Name, err) + } + // Propagate labels from Task to TaskRun. if tr.ObjectMeta.Labels == nil { tr.ObjectMeta.Labels = make(map[string]string, len(taskMeta.Labels)+1) @@ -698,3 +703,12 @@ func applyVolumeClaimTemplates(workspaceBindings []v1alpha1.WorkspaceBinding, ow } return taskRunWorkspaceBindings } + +func storeTaskSpec(ctx context.Context, tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec) error { + // Only store the TaskSpec once, if it has never been set before. + if tr.Status.TaskSpec == nil { + tr.Status.TaskSpec = &v1beta1.TaskSpec{} + return ts.ConvertTo(ctx, tr.Status.TaskSpec) + } + return nil +} diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index ad3eb2a4255..2d8248b88f4 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -29,6 +29,8 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" + podconvert "github.com/tektoncd/pipeline/pkg/pod" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/cloudevent" ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing" @@ -38,6 +40,7 @@ import ( test "github.com/tektoncd/pipeline/test/v1alpha1" corev1 "k8s.io/api/core/v1" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -2339,3 +2342,32 @@ func TestFailTaskRun(t *testing.T) { }) } } + +func Test_storeTaskSpec(t *testing.T) { + + ctx := context.Background() + tr := tb.TaskRun("foo", tb.TaskRunSpec(tb.TaskRunTaskRef("foo-task"))) + + ts := tb.Task("some-task", tb.TaskSpec(tb.TaskDescription("foo-task"))).Spec + want := &v1beta1.TaskSpec{} + if err := ts.ConvertTo(ctx, want); err != nil { + t.Errorf("error converting to v1beta1: %v", err) + } + + // The first time we set it, it should get copied. + if err := storeTaskSpec(ctx, tr, &ts); err != nil { + t.Errorf("storeTaskSpec() error = %v", err) + } + if d := cmp.Diff(tr.Status.TaskSpec, want); d != "" { + t.Fatalf("-want, +got: %v", d) + } + + ts.Description = "new-task" + // The next time, it should not get overwritten + if err := storeTaskSpec(ctx, tr, &ts); err != nil { + t.Errorf("storeTaskSpec() error = %v", err) + } + if d := cmp.Diff(tr.Status.TaskSpec, want); d != "" { + t.Fatalf("-want, +got: %v", d) + } +} diff --git a/test/taskrun_test.go b/test/taskrun_test.go index afec2d1fd99..62aa13e0ee8 100644 --- a/test/taskrun_test.go +++ b/test/taskrun_test.go @@ -181,4 +181,8 @@ func TestTaskRunStatus(t *testing.T) { if !strings.HasSuffix(taskrun.Status.Steps[0].ImageID, fqImageName) { t.Fatalf("`ImageID: %s` does not end with `%s`", taskrun.Status.Steps[0].ImageID, fqImageName) } + + if d := cmp.Diff(taskrun.Status.TaskSpec, &task.Spec); d != "" { + t.Fatalf("-got, +want: %v", d) + } }