From 111d059313a62a9b64421f785e54c3d402a30003 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Fri, 14 Jan 2022 15:41:06 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20Pipeline/Task=20to=20*Run=20label/annotat?= =?UTF-8?q?ion=20propagation=20=F0=9F=A5=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this fix, we are "forgetting" the Task and Pipeline metadata (Labels/Annotations) when using them in a Run (PipelineRun or TaskRun). It is however documented that those metadata get propagated to the generated Pod, and even the code after storing the Spec is written that way. Because we only store the spec, the second time we were reconciling the TaskRun or PipelineRun, we wouldn't fetch the referenced Task or Pipeline anymore, and thus, we would not propagate them. Signed-off-by: Vincent Demeester --- pkg/reconciler/pipelinerun/pipelinerun.go | 39 +++++++-------- .../pipelinerun/pipelinerun_test.go | 28 ++++++++--- pkg/reconciler/taskrun/taskrun.go | 48 +++++++++---------- pkg/reconciler/taskrun/taskrun_test.go | 28 +++++++---- 4 files changed, 83 insertions(+), 60 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index be94fce2174..01de154f90a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -344,27 +344,10 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get } // Store the fetched PipelineSpec on the PipelineRun for auditing - if err := storePipelineSpec(ctx, pr, pipelineSpec); err != nil { + if err := storePipelineSpecAndMergeMeta(pr, pipelineSpec, pipelineMeta); err != nil { 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) - } - for key, value := range pipelineMeta.Labels { - pr.ObjectMeta.Labels[key] = value - } - pr.ObjectMeta.Labels[pipeline.PipelineLabelKey] = pipelineMeta.Name - - // Propagate annotations from Pipeline to PipelineRun. - if pr.ObjectMeta.Annotations == nil { - pr.ObjectMeta.Annotations = make(map[string]string, len(pipelineMeta.Annotations)) - } - for key, value := range pipelineMeta.Annotations { - pr.ObjectMeta.Annotations[key] = value - } - d, err := dag.Build(v1beta1.PipelineTaskList(pipelineSpec.Tasks), v1beta1.PipelineTaskList(pipelineSpec.Tasks).Deps()) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it @@ -1147,10 +1130,28 @@ func (c *Reconciler) makeConditionCheckContainer(ctx context.Context, rprt *reso return &cc, err } -func storePipelineSpec(ctx context.Context, pr *v1beta1.PipelineRun, ps *v1beta1.PipelineSpec) error { +func storePipelineSpecAndMergeMeta(pr *v1beta1.PipelineRun, ps *v1beta1.PipelineSpec, meta *metav1.ObjectMeta) error { // Only store the PipelineSpec once, if it has never been set before. if pr.Status.PipelineSpec == nil { pr.Status.PipelineSpec = ps + + // Propagate labels from Pipeline to PipelineRun. + if pr.ObjectMeta.Labels == nil { + pr.ObjectMeta.Labels = make(map[string]string, len(meta.Labels)+1) + } + for key, value := range meta.Labels { + pr.ObjectMeta.Labels[key] = value + } + pr.ObjectMeta.Labels[pipeline.PipelineLabelKey] = meta.Name + + // Propagate annotations from Pipeline to PipelineRun. + if pr.ObjectMeta.Annotations == nil { + pr.ObjectMeta.Annotations = make(map[string]string, len(meta.Annotations)) + } + for key, value := range meta.Annotations { + pr.ObjectMeta.Annotations[key] = value + } + } return nil } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index a6cde9178be..465662b5e0b 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -6166,26 +6166,40 @@ func TestReconcileWithPipelineResults(t *testing.T) { } func Test_storePipelineSpec(t *testing.T) { - ctx := context.Background() - pr := &v1beta1.PipelineRun{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} + labels := map[string]string{"lbl": "value"} + annotations := map[string]string{"io.annotation": "value"} + pr := &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Labels: labels, + Annotations: annotations, + }, + } ps := v1beta1.PipelineSpec{Description: "foo-pipeline"} ps1 := v1beta1.PipelineSpec{Description: "bar-pipeline"} - want := ps.DeepCopy() + + want := pr.DeepCopy() + want.Status = v1beta1.PipelineRunStatus{ + PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{ + PipelineSpec: ps.DeepCopy(), + }, + } + want.ObjectMeta.Labels["tekton.dev/pipeline"] = pr.ObjectMeta.Name // The first time we set it, it should get copied. - if err := storePipelineSpec(ctx, pr, &ps); err != nil { + if err := storePipelineSpecAndMergeMeta(pr, &ps, &pr.ObjectMeta); err != nil { t.Errorf("storePipelineSpec() error = %v", err) } - if d := cmp.Diff(pr.Status.PipelineSpec, want); d != "" { + if d := cmp.Diff(pr, want); d != "" { t.Fatalf(diff.PrintWantGot(d)) } // The next time, it should not get overwritten - if err := storePipelineSpec(ctx, pr, &ps1); err != nil { + if err := storePipelineSpecAndMergeMeta(pr, &ps1, &metav1.ObjectMeta{}); err != nil { t.Errorf("storePipelineSpec() error = %v", err) } - if d := cmp.Diff(pr.Status.PipelineSpec, want); d != "" { + if d := cmp.Diff(pr, want); d != "" { t.Fatalf(diff.PrintWantGot(d)) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 65bf2339601..ec8ec078ba9 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -295,33 +295,10 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 } // Store the fetched TaskSpec on the TaskRun for auditing - if err := storeTaskSpec(ctx, tr, taskSpec); err != nil { + if err := storeTaskSpecAndMergeMeta(tr, taskSpec, taskMeta); err != nil { 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) - } - for key, value := range taskMeta.Labels { - tr.ObjectMeta.Labels[key] = value - } - if tr.Spec.TaskRef != nil { - if tr.Spec.TaskRef.Kind == "ClusterTask" { - tr.ObjectMeta.Labels[pipeline.ClusterTaskLabelKey] = taskMeta.Name - } else { - tr.ObjectMeta.Labels[pipeline.TaskLabelKey] = taskMeta.Name - } - } - - // Propagate annotations from Task to TaskRun. - if tr.ObjectMeta.Annotations == nil { - tr.ObjectMeta.Annotations = make(map[string]string, len(taskMeta.Annotations)) - } - for key, value := range taskMeta.Annotations { - tr.ObjectMeta.Annotations[key] = value - } - inputs := []v1beta1.TaskResourceBinding{} outputs := []v1beta1.TaskResourceBinding{} if tr.Spec.Resources != nil { @@ -794,10 +771,31 @@ func applyVolumeClaimTemplates(workspaceBindings []v1beta1.WorkspaceBinding, own return taskRunWorkspaceBindings } -func storeTaskSpec(ctx context.Context, tr *v1beta1.TaskRun, ts *v1beta1.TaskSpec) error { +func storeTaskSpecAndMergeMeta(tr *v1beta1.TaskRun, ts *v1beta1.TaskSpec, meta *metav1.ObjectMeta) error { // Only store the TaskSpec once, if it has never been set before. if tr.Status.TaskSpec == nil { tr.Status.TaskSpec = ts + // Propagate annotations from Task to TaskRun. + if tr.ObjectMeta.Annotations == nil { + tr.ObjectMeta.Annotations = make(map[string]string, len(meta.Annotations)) + } + for key, value := range meta.Annotations { + tr.ObjectMeta.Annotations[key] = value + } + // Propagate labels from Task to TaskRun. + if tr.ObjectMeta.Labels == nil { + tr.ObjectMeta.Labels = make(map[string]string, len(meta.Labels)+1) + } + for key, value := range meta.Labels { + tr.ObjectMeta.Labels[key] = value + } + if tr.Spec.TaskRef != nil { + if tr.Spec.TaskRef.Kind == "ClusterTask" { + tr.ObjectMeta.Labels[pipeline.ClusterTaskLabelKey] = meta.Name + } else { + tr.ObjectMeta.Labels[pipeline.TaskLabelKey] = meta.Name + } + } } return nil } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 68708f75716..daa7f3de722 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -4111,10 +4111,14 @@ func TestFailTaskRun(t *testing.T) { } func Test_storeTaskSpec(t *testing.T) { - - ctx, _ := ttesting.SetupFakeContext(t) + labels := map[string]string{"lbl": "value"} + annotations := map[string]string{"io.annotation": "value"} tr := &v1beta1.TaskRun{ - ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Labels: labels, + Annotations: annotations, + }, Spec: v1beta1.TaskRunSpec{ TaskRef: &v1beta1.TaskRef{ Name: "foo-task", @@ -4126,23 +4130,29 @@ func Test_storeTaskSpec(t *testing.T) { Description: "foo-task", } ts1 := v1beta1.TaskSpec{ - Description: "foo-task", + Description: "bar-task", + } + want := tr.DeepCopy() + want.Status = v1beta1.TaskRunStatus{ + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + TaskSpec: ts.DeepCopy(), + }, } - want := ts.DeepCopy() + want.ObjectMeta.Labels["tekton.dev/task"] = tr.ObjectMeta.Name // The first time we set it, it should get copied. - if err := storeTaskSpec(ctx, tr, &ts); err != nil { + if err := storeTaskSpecAndMergeMeta(tr, &ts, &tr.ObjectMeta); err != nil { t.Errorf("storeTaskSpec() error = %v", err) } - if d := cmp.Diff(tr.Status.TaskSpec, want); d != "" { + if d := cmp.Diff(tr, want); d != "" { t.Fatalf(diff.PrintWantGot(d)) } // The next time, it should not get overwritten - if err := storeTaskSpec(ctx, tr, &ts1); err != nil { + if err := storeTaskSpecAndMergeMeta(tr, &ts1, &metav1.ObjectMeta{}); err != nil { t.Errorf("storeTaskSpec() error = %v", err) } - if d := cmp.Diff(tr.Status.TaskSpec, want); d != "" { + if d := cmp.Diff(tr, want); d != "" { t.Fatalf(diff.PrintWantGot(d)) } }