Skip to content

Commit

Permalink
Fix Pipeline/Task to *Run label/annotation propagation 🥨
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@vdemeester @tekton-robot
vdemeester authored and tekton-robot committed Jan 19, 2022
1 parent 766fb25 commit d8425c2
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 60 deletions.
39 changes: 20 additions & 19 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,27 +343,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
Expand Down Expand Up @@ -1146,10 +1129,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
}
Expand Down
28 changes: 21 additions & 7 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6188,26 +6188,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))
}
}
Expand Down
48 changes: 23 additions & 25 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,33 +294,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 {
Expand Down Expand Up @@ -793,10 +770,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
}
Expand Down
28 changes: 19 additions & 9 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4031,10 +4031,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",
Expand All @@ -4046,23 +4050,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))
}
}
Expand Down

0 comments on commit d8425c2

Please sign in to comment.