diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index e9d418c4d6c..a6365265dcc 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -216,11 +216,11 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { return err } // Since we are using the status subresource, it is not possible to update - // the status and labels simultaneously. - if !reflect.DeepEqual(original.ObjectMeta.Labels, pr.ObjectMeta.Labels) { - if _, err := c.updateLabels(pr); err != nil { - c.Logger.Warn("Failed to update PipelineRun labels", zap.Error(err)) - c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "PipelineRun failed to update labels") + // the status and labels/annotations simultaneously. + if !reflect.DeepEqual(original.ObjectMeta.Labels, pr.ObjectMeta.Labels) || !reflect.DeepEqual(original.ObjectMeta.Annotations, pr.ObjectMeta.Annotations) { + if _, err := c.updateLabelsAndAnnotations(pr); err != nil { + c.Logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err)) + c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "PipelineRun failed to update labels/annotations") return err } } @@ -281,6 +281,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er } pr.ObjectMeta.Labels[pipeline.GroupName+pipeline.PipelineLabelKey] = p.Name + // Propagate annotations from Pipeline to PipelineRun. + if pr.ObjectMeta.Annotations == nil { + pr.ObjectMeta.Annotations = make(map[string]string, len(p.ObjectMeta.Annotations)) + } + for key, value := range p.ObjectMeta.Annotations { + pr.ObjectMeta.Annotations[key] = value + } + pipelineState, err := resources.ResolvePipelineRun( *pr, func(name string) (v1alpha1.TaskInterface, error) { @@ -455,6 +463,12 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re } labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] = pr.Name + // Propagate annotations from PipelineRun to TaskRun. + annotations := make(map[string]string, len(pr.ObjectMeta.Annotations)+1) + for key, val := range pr.ObjectMeta.Annotations { + annotations[key] = val + } + tr, _ := c.taskRunLister.TaskRuns(pr.Namespace).Get(rprt.TaskRunName) if tr != nil { @@ -473,6 +487,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re Namespace: pr.Namespace, OwnerReferences: pr.GetOwnerReference(), Labels: labels, + Annotations: annotations, }, Spec: v1alpha1.TaskRunSpec{ TaskRef: &v1alpha1.TaskRef{ @@ -524,13 +539,14 @@ func (c *Reconciler) updateStatus(pr *v1alpha1.PipelineRun) (*v1alpha1.PipelineR return newPr, nil } -func (c *Reconciler) updateLabels(pr *v1alpha1.PipelineRun) (*v1alpha1.PipelineRun, error) { +func (c *Reconciler) updateLabelsAndAnnotations(pr *v1alpha1.PipelineRun) (*v1alpha1.PipelineRun, error) { newPr, err := c.pipelineRunLister.PipelineRuns(pr.Namespace).Get(pr.Name) if err != nil { - return nil, fmt.Errorf("Error getting PipelineRun %s when updating labels: %s", pr.Name, err) + return nil, fmt.Errorf("Error getting PipelineRun %s when updating labels/annotations: %s", pr.Name, err) } - if !reflect.DeepEqual(pr.ObjectMeta.Labels, newPr.ObjectMeta.Labels) { + if !reflect.DeepEqual(pr.ObjectMeta.Labels, newPr.ObjectMeta.Labels) || !reflect.DeepEqual(pr.ObjectMeta.Annotations, newPr.ObjectMeta.Annotations) { newPr.ObjectMeta.Labels = pr.ObjectMeta.Labels + newPr.ObjectMeta.Annotations = pr.ObjectMeta.Annotations return c.PipelineClientSet.TektonV1alpha1().PipelineRuns(pr.Namespace).Update(newPr) } return newPr, nil diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index 976fd4b8970..60bb0d37f82 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -892,3 +892,65 @@ func TestReconcileWithTimeoutAndRetry(t *testing.T) { }) } } + +func TestReconcilePropagateAnnotations(t *testing.T) { + names.TestingSeed() + + ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec( + tb.PipelineTask("hello-world-1", "hello-world"), + ))} + prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-with-annotations", "foo", + tb.PipelineRunAnnotation("PipelineRunAnnotation", "PipelineRunValue"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunServiceAccount("test-sa"), + ), + )} + ts := []*v1alpha1.Task{tb.Task("hello-world", "foo")} + + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + } + + // create fake recorder for testing + fr := record.NewFakeRecorder(2) + + testAssets := getPipelineRunController(t, d, fr) + c := testAssets.Controller + clients := testAssets.Clients + + err := c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-with-annotations") + if err != nil { + t.Errorf("Did not expect to see error when reconciling completed PipelineRun but saw %s", err) + } + + // Check that the PipelineRun was reconciled correctly + _, err = clients.Pipeline.Tekton().PipelineRuns("foo").Get("test-pipeline-run-with-annotations", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err) + } + + // Check that the expected TaskRun was created + actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun) + if actual == nil { + t.Errorf("Expected a TaskRun to be created, but it wasn't.") + } + expectedTaskRun := tb.TaskRun("test-pipeline-run-with-annotations-hello-world-1-9l9zj", "foo", + tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-with-annotations", + tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"), + tb.Controller, tb.BlockOwnerDeletion, + ), + tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-with-annotations"), + tb.TaskRunAnnotation("PipelineRunAnnotation", "PipelineRunValue"), + tb.TaskRunSpec( + tb.TaskRunTaskRef("hello-world"), + tb.TaskRunServiceAccount("test-sa"), + ), + ) + + if d := cmp.Diff(actual, expectedTaskRun); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, d) + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/pod.go b/pkg/reconciler/v1alpha1/taskrun/resources/pod.go index a44961ac937..ef94a8e57a0 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/pod.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/pod.go @@ -231,14 +231,6 @@ func TryGetPod(taskRunStatus v1alpha1.TaskRunStatus, gp GetPod) (*corev1.Pod, er // MakePod converts TaskRun and TaskSpec objects to a Pod which implements the taskrun specified // by the supplied CRD. func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient kubernetes.Interface, cache *entrypoint.Cache, logger *zap.SugaredLogger) (*corev1.Pod, error) { - // Copy annotations on the build through to the underlying pod to allow users - // to specify pod annotations. - annotations := map[string]string{} - for key, val := range taskRun.Annotations { - annotations[key] = val - } - annotations["sidecar.istio.io/inject"] = "false" - cred, secrets, err := makeCredentialInitializer(taskRun.Spec.ServiceAccount, taskRun.Namespace, kubeclient) if err != nil { return nil, err @@ -329,7 +321,7 @@ func MakePod(taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient k OwnerReferences: []metav1.OwnerReference{ *metav1.NewControllerRef(taskRun, groupVersionKind), }, - Annotations: annotations, + Annotations: makeAnnotations(taskRun), Labels: makeLabels(taskRun), }, Spec: corev1.PodSpec{ @@ -355,6 +347,16 @@ func makeLabels(s *v1alpha1.TaskRun) map[string]string { return labels } +// makeAnnotations constructs the annotations we will propagate from TaskRuns to Pods. +func makeAnnotations(s *v1alpha1.TaskRun) map[string]string { + annotations := make(map[string]string, len(s.ObjectMeta.Annotations)+1) + for k, v := range s.ObjectMeta.Annotations { + annotations[k] = v + } + annotations["sidecar.istio.io/inject"] = "false" + return annotations +} + // zeroNonMaxResourceRequests zeroes out the container's cpu, memory, or // ephemeral storage resource requests if the container does not have the // largest request out of all containers in the pod. This is done because Tekton diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index c0bc41a386d..021c34b9a87 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -197,10 +197,10 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { return err } // Since we are using the status subresource, it is not possible to update - // the status and labels simultaneously. + // the status and labels/annotations simultaneously. if !reflect.DeepEqual(original.ObjectMeta.Labels, tr.ObjectMeta.Labels) { - if _, err := c.updateLabels(tr); err != nil { - c.Logger.Warn("Failed to update TaskRun labels", zap.Error(err)) + if _, err := c.updateLabelsAndAnnotations(tr); err != nil { + c.Logger.Warn("Failed to update TaskRun labels/annotations", zap.Error(err)) return err } } @@ -264,6 +264,14 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error tr.ObjectMeta.Labels[pipeline.GroupName+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 + } + // Check if the TaskRun has timed out; if it is, this will set its status // accordingly. if timedOut, err := c.checkTimeout(tr, taskSpec, c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete); err != nil { @@ -491,13 +499,14 @@ func (c *Reconciler) updateStatus(taskrun *v1alpha1.TaskRun) (*v1alpha1.TaskRun, return newtaskrun, nil } -func (c *Reconciler) updateLabels(tr *v1alpha1.TaskRun) (*v1alpha1.TaskRun, error) { +func (c *Reconciler) updateLabelsAndAnnotations(tr *v1alpha1.TaskRun) (*v1alpha1.TaskRun, error) { newTr, err := c.taskRunLister.TaskRuns(tr.Namespace).Get(tr.Name) if err != nil { - return nil, fmt.Errorf("Error getting TaskRun %s when updating labels: %s", tr.Name, err) + return nil, fmt.Errorf("Error getting TaskRun %s when updating labels/annotations: %s", tr.Name, err) } - if !reflect.DeepEqual(tr.ObjectMeta.Labels, newTr.ObjectMeta.Labels) { + if !reflect.DeepEqual(tr.ObjectMeta.Labels, newTr.ObjectMeta.Labels) || !reflect.DeepEqual(tr.ObjectMeta.Annotations, newTr.ObjectMeta.Annotations) { newTr.ObjectMeta.Labels = tr.ObjectMeta.Labels + newTr.ObjectMeta.Annotations = tr.ObjectMeta.Annotations return c.PipelineClientSet.TektonV1alpha1().TaskRuns(tr.Namespace).Update(newTr) } return newTr, nil diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 03bcf8bc05d..d7884e5d545 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -304,6 +304,13 @@ func TestReconcile(t *testing.T) { ), ) + taskRunWithAnnotations := tb.TaskRun("test-taskrun-with-annotations", "foo", + tb.TaskRunAnnotation("TaskRunAnnotation", "TaskRunValue"), + tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name), + ), + ) + taskRunWithResourceRequests := tb.TaskRun("test-taskrun-with-resource-requests", "foo", tb.TaskRunSpec( tb.TaskRunTaskSpec( @@ -347,7 +354,7 @@ func TestReconcile(t *testing.T) { taskRunSuccess, taskRunWithSaSuccess, taskRunTemplating, taskRunInputOutput, taskRunWithTaskSpec, taskRunWithClusterTask, taskRunWithResourceSpecAndTaskSpec, - taskRunWithLabels, taskRunWithResourceRequests, taskRunTaskEnv, taskRunWithPod, + taskRunWithLabels, taskRunWithAnnotations, taskRunWithResourceRequests, taskRunTaskEnv, taskRunWithPod, } d := test.Data{ @@ -845,6 +852,42 @@ func TestReconcile(t *testing.T) { ), ), ), + }, { + name: "taskrun-with-annotations", + taskRun: taskRunWithAnnotations, + wantPod: tb.Pod("test-taskrun-with-annotations-pod-123456", "foo", + tb.PodAnnotation("sidecar.istio.io/inject", "false"), + tb.PodAnnotation("TaskRunAnnotation", "TaskRunValue"), + tb.PodLabel(taskNameLabelKey, "test-task"), + tb.PodLabel(taskRunNameLabelKey, "test-taskrun-with-annotations"), + tb.PodOwnerReference("TaskRun", "test-taskrun-with-annotations", + tb.OwnerReferenceAPIVersion(currentApiVersion)), + tb.PodSpec( + tb.PodVolumes(toolsVolume, workspaceVolume, homeVolume), + tb.PodRestartPolicy(corev1.RestartPolicyNever), + getCredentialsInitContainer("9l9zj"), + getPlaceToolsInitContainer(), + tb.PodContainer("build-step-simple-step", "foo", + tb.Command(entrypointLocation), + tb.Args("-wait_file", "", "-post_file", "/builder/tools/0", "-entrypoint", "/mycmd", "--"), + tb.WorkingDir(workspaceDir), + tb.EnvVar("HOME", "/builder/home"), + tb.VolumeMount("tools", "/builder/tools"), + tb.VolumeMount("workspace", workspaceDir), + tb.VolumeMount("home", "/builder/home"), + tb.Resources(tb.Requests( + tb.CPU("0"), + tb.Memory("0"), + tb.EphemeralStorage("0"), + )), + ), + tb.PodContainer("nop", "override-with-nop:latest", + tb.Command("/builder/tools/entrypoint"), + tb.Args("-wait_file", "/builder/tools/0", "-post_file", "/builder/tools/1", "-entrypoint", "/ko-app/nop", "--"), + tb.VolumeMount(entrypoint.MountName, entrypoint.MountPoint), + ), + ), + ), }, { name: "task-env", taskRun: taskRunTaskEnv, diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go index 34589df013e..8ac4c9ecef3 100644 --- a/test/builder/pipeline.go +++ b/test/builder/pipeline.go @@ -270,6 +270,16 @@ func PipelineRunLabel(key, value string) PipelineRunOp { } } +// PipelineRunAnnotations adds a annotation to the PipelineRun. +func PipelineRunAnnotation(key, value string) PipelineRunOp { + return func(pr *v1alpha1.PipelineRun) { + if pr.ObjectMeta.Annotations == nil { + pr.ObjectMeta.Annotations = map[string]string{} + } + pr.ObjectMeta.Annotations[key] = value + } +} + // PipelineRunResourceBinding adds bindings from actual instances to a Pipeline's declared resources. func PipelineRunResourceBinding(name string, ops ...PipelineResourceBindingOp) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { diff --git a/test/builder/task.go b/test/builder/task.go index e511a0ca63c..2891b3e5c7d 100644 --- a/test/builder/task.go +++ b/test/builder/task.go @@ -266,8 +266,9 @@ func ParamDefault(value string) TaskParamOp { func TaskRun(name, namespace string, ops ...TaskRunOp) *v1alpha1.TaskRun { tr := &v1alpha1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ - Namespace: namespace, - Name: name, + Namespace: namespace, + Name: name, + Annotations: map[string]string{}, }, } @@ -395,6 +396,15 @@ func TaskRunLabel(key, value string) TaskRunOp { } } +func TaskRunAnnotation(key, value string) TaskRunOp { + return func(tr *v1alpha1.TaskRun) { + if tr.ObjectMeta.Annotations == nil { + tr.ObjectMeta.Annotations = map[string]string{} + } + tr.ObjectMeta.Annotations[key] = value + } +} + // TaskRunSpec sets the specified spec of the TaskRun. // Any number of TaskRunSpec modifier can be passed to transform it. func TaskRunSpec(ops ...TaskRunSpecOp) TaskRunOp { diff --git a/test/builder/task_test.go b/test/builder/task_test.go index 215617389c9..4762568f457 100644 --- a/test/builder/task_test.go +++ b/test/builder/task_test.go @@ -117,7 +117,7 @@ func TestClusterTask(t *testing.T) { } } -func TestTaskRunWitTaskRef(t *testing.T) { +func TestTaskRunWithTaskRef(t *testing.T) { var trueB = true taskRun := tb.TaskRun("test-taskrun", "foo", tb.TaskRunOwnerReference("PipelineRun", "test", @@ -164,7 +164,8 @@ func TestTaskRunWitTaskRef(t *testing.T) { Controller: &trueB, BlockOwnerDeletion: &trueB, }}, - Labels: map[string]string{"label": "label-value"}, + Labels: map[string]string{"label": "label-value"}, + Annotations: map[string]string{}, }, Spec: v1alpha1.TaskRunSpec{ Inputs: v1alpha1.TaskRunInputs{ @@ -223,6 +224,7 @@ func TestTaskRunWithTaskSpec(t *testing.T) { expectedTaskRun := &v1alpha1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "test-taskrun", Namespace: "foo", + Annotations: map[string]string{}, }, Spec: v1alpha1.TaskRunSpec{ TaskSpec: &v1alpha1.TaskSpec{ diff --git a/test/pipelinerun_test.go b/test/pipelinerun_test.go index 97f170f1dcb..9ed7985b7f3 100644 --- a/test/pipelinerun_test.go +++ b/test/pipelinerun_test.go @@ -162,6 +162,8 @@ func TestPipelineRun(t *testing.T) { t.Logf("Checking that labels were propagated correctly for TaskRun %s", r.Name) checkLabelPropagation(t, c, namespace, prName, r) + t.Logf("Checking that annotations were propagated correctly for TaskRun %s", r.Name) + checkAnnotationPropagation(t, c, namespace, prName, r) } matchKinds := map[string][]string{"PipelineRun": {prName}, "TaskRun": expectedTaskRunNames} @@ -427,6 +429,45 @@ func checkLabelPropagation(t *testing.T, c *clients, namespace string, pipelineR } } +// checkAnnotationPropagation checks that annotations are correctly propagating from +// Pipelines, PipelineRuns, and Tasks to TaskRuns and Pods. +func checkAnnotationPropagation(t *testing.T, c *clients, namespace string, pipelineRunName string, tr *v1alpha1.TaskRun) { + annotations := make(map[string]string) + + // Check annotation propagation to PipelineRuns. + pr, err := c.PipelineRunClient.Get(pipelineRunName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Couldn't get expected PipelineRun for %s: %s", tr.Name, err) + } + p, err := c.PipelineClient.Get(pr.Spec.PipelineRef.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Couldn't get expected Pipeline for %s: %s", pr.Name, err) + } + for key, val := range p.ObjectMeta.Annotations { + annotations[key] = val + } + assertAnnotationsMatch(t, annotations, pr.ObjectMeta.Annotations) + + // Check annotation propagation to TaskRuns. + for key, val := range pr.ObjectMeta.Annotations { + annotations[key] = val + } + if tr.Spec.TaskRef != nil { + task, err := c.TaskClient.Get(tr.Spec.TaskRef.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Couldn't get expected Task for %s: %s", tr.Name, err) + } + for key, val := range task.ObjectMeta.Annotations { + annotations[key] = val + } + } + assertAnnotationsMatch(t, annotations, tr.ObjectMeta.Annotations) + + // Check annotation propagation to Pods. + pod := getPodForTaskRun(t, c.KubeClient, namespace, tr) + assertAnnotationsMatch(t, annotations, pod.ObjectMeta.Annotations) +} + func getPodForTaskRun(t *testing.T, kubeClient *knativetest.KubeClient, namespace string, tr *v1alpha1.TaskRun) *corev1.Pod { // The Pod name has a random suffix, so we filter by label to find the one we care about. pods, err := kubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{ @@ -448,3 +489,11 @@ func assertLabelsMatch(t *testing.T, expectedLabels, actualLabels map[string]str } } } + +func assertAnnotationsMatch(t *testing.T, expectedAnnotations, actualAnnotations map[string]string) { + for key, expectedVal := range expectedAnnotations { + if actualVal := actualAnnotations[key]; actualVal != expectedVal { + t.Errorf("Expected annotations containing %s=%s but annotations were %v", key, expectedVal, actualAnnotations) + } + } +}