From 0025bb54a04aedb72ec01a13290f73adb7750dd9 Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Fri, 24 Apr 2020 09:43:37 -0500 Subject: [PATCH] Dereference the PipelineSpec into PipelineRun.Status 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. --- docs/pipelineruns.md | 5 ++++ go.mod | 1 - .../pipeline/v1beta1/pipelinerun_types.go | 3 ++ .../pipeline/v1beta1/zz_generated.deepcopy.go | 5 ++++ pkg/reconciler/pipelinerun/pipelinerun.go | 14 ++++++++++ .../pipelinerun/pipelinerun_test.go | 28 +++++++++++++++++++ 6 files changed, 55 insertions(+), 1 deletion(-) diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index 05b2a69469f..5e773b5e146 100644 --- a/docs/pipelineruns.md +++ b/docs/pipelineruns.md @@ -30,6 +30,11 @@ executed successfully or a failure occurs. **Note:** A `PipelineRun` automatically creates corresponding `TaskRuns` for every `Task` in your `Pipeline`. +The `Status` field tracks the current state of a `PipelineRun`, and can be used to monitor +progress. +This field contains the status of every `TaskRun`, as well as the full `PipelineSpec` used +to instantiate this `PipelineRun`, for full auditability. + ## Configuring a `PipelineRun` A `PipelineRun` definition supports the following fields: diff --git a/go.mod b/go.mod index 730f0bb19f2..bca5bcafaab 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,6 @@ 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 // 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/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index 62c03d32758..a850f4f01db 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -255,6 +255,9 @@ type PipelineRunStatusFields struct { // PipelineResults are the list of results written out by the pipeline task's containers // +optional PipelineResults []PipelineRunResult `json:"pipelineResults,omitempty"` + + // PipelineRunSpec contains the exact spec used to instantiate the run + PipelineSpec *PipelineSpec `json:"pipelineSpec,omitempty"` } // PipelineRunResult used to describe the results of a pipeline diff --git a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go index 1d92712e04a..40fc616861b 100644 --- a/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go @@ -672,6 +672,11 @@ func (in *PipelineRunStatusFields) DeepCopyInto(out *PipelineRunStatusFields) { *out = make([]PipelineRunResult, len(*in)) copy(*out, *in) } + if in.PipelineSpec != nil { + in, out := &in.PipelineSpec, &out.PipelineSpec + *out = new(PipelineSpec) + (*in).DeepCopyInto(*out) + } return } diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index eaae85c631a..39061f486a8 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -359,6 +359,11 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er return nil } + // Store the fetched PipelineSpec on the PipelineRun for auditing + if err := storePipelineSpec(ctx, pr, pipelineSpec); err != nil { + c.Logger.Errorf("Failed to store PipelineSpec on PipelineRun.Status for pipelinerun %s: %v", pr.Name, err) + } + // Propagate labels from Pipeline to PipelineRun. if pr.ObjectMeta.Labels == nil { pr.ObjectMeta.Labels = make(map[string]string, len(pipelineMeta.Labels)+1) @@ -906,3 +911,12 @@ func (c *Reconciler) makeConditionCheckContainer(rprt *resources.ResolvedPipelin cc := v1alpha1.ConditionCheck(*cctr) return &cc, err } + +func storePipelineSpec(ctx context.Context, pr *v1alpha1.PipelineRun, ps *v1alpha1.PipelineSpec) error { + // Only store the PipelineSpec once, if it has never been set before. + if pr.Status.PipelineSpec == nil { + pr.Status.PipelineSpec = &v1beta1.PipelineSpec{} + return ps.ConvertTo(ctx, pr.Status.PipelineSpec) + } + return nil +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index dac1952ade5..6c798c592f5 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -2042,3 +2042,31 @@ func TestReconcileWithPipelineResults(t *testing.T) { t.Errorf("expected to see pipeline run results created. -want, +got: Diff %s", d) } } + +func Test_storePipelineSpec(t *testing.T) { + ctx := context.Background() + pr := tb.PipelineRun("foo") + + ps := tb.Pipeline("some-pipeline", tb.PipelineSpec(tb.PipelineDescription("foo-pipeline"))).Spec + want := &v1beta1.PipelineSpec{} + if err := ps.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 := storePipelineSpec(ctx, pr, &ps); err != nil { + t.Errorf("storePipelineSpec() error = %v", err) + } + if d := cmp.Diff(pr.Status.PipelineSpec, want); d != "" { + t.Fatalf("-want, +got: %v", d) + } + + ps.Description = "new-pipeline" + // The next time, it should not get overwritten + if err := storePipelineSpec(ctx, pr, &ps); err != nil { + t.Errorf("storePipelineSpec() error = %v", err) + } + if d := cmp.Diff(pr.Status.PipelineSpec, want); d != "" { + t.Fatalf("-want, +got: %v", d) + } +}