Fix TaskRunSpec overrides when empty 🗜

In case of TaskRunSpec being present on a PipelineRun, we override the
PipelineTask PodTemplate and ServiceAccount blindly, even if those
values are empty (empty string, nil point)

This fixes this behavior by overriding PodTemplate and/or
ServiceAccountName only when they are not empty values.

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
(cherry picked from commit 695d117)
Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

pkg/apis/pipeline/v1beta1/pipelinerun_types.go

@@ -413,8 +413,12 @@ func (pr *PipelineRun) GetTaskRunSpecs(pipelineTaskName string) (string, *PodTem
  taskPodTemplate := pr.Spec.PodTemplate
  for _, task := range pr.Spec.TaskRunSpecs {
  if task.PipelineTaskName == pipelineTaskName {
- taskPodTemplate = task.TaskPodTemplate
- serviceAccountName = task.TaskServiceAccountName
+ if task.TaskPodTemplate != nil {
+ taskPodTemplate = task.TaskPodTemplate
+ }
+ if task.TaskServiceAccountName != "" {
+ serviceAccountName = task.TaskServiceAccountName
+ }
  }
  }
  return serviceAccountName, taskPodTemplate

pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go

@@ -338,8 +338,7 @@ func TestPipelineRunGetPodSpecSABackcompatibility(t *testing.T) {
  "unknown": "defaultSA",
  "taskName": "newTaskSA",
  },
- },
- {
+ }, {
  name: "mixed default SA backward compatibility",
  pr: &v1beta1.PipelineRun{
  ObjectMeta: metav1.ObjectMeta{Name: "pr"},
@@ -360,6 +359,26 @@ func TestPipelineRunGetPodSpecSABackcompatibility(t *testing.T) {
  "taskNameOne": "TaskSAOne",
  "taskNameTwo": "newTaskTwo",
  },
+ }, {
+ name: "mixed SA and TaskRunSpec",
+ pr: &v1beta1.PipelineRun{
+ ObjectMeta: metav1.ObjectMeta{Name: "pr"},
+ Spec: v1beta1.PipelineRunSpec{
+ PipelineRef: &v1beta1.PipelineRef{Name: "prs"},
+ ServiceAccountName: "defaultSA",
+ TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{
+ PipelineTaskName: "taskNameOne",
+ }, {
+ PipelineTaskName: "taskNameTwo",
+ TaskServiceAccountName: "newTaskTwo",
+ }},
+ },
+ },
+ expectedSAs: map[string]string{
+ "unknown": "defaultSA",
+ "taskNameOne": "defaultSA",
+ "taskNameTwo": "newTaskTwo",
+ },
  },
  } {
  for taskName, expected := range tt.expectedSAs {

test/serviceaccount_test.go

@@ -0,0 +1,304 @@
+// +build e2e
+
+/*
+Copyright 2019 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+ corev1 "k8s.io/api/core/v1"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ knativetest "knative.dev/pkg/test"
+)
+
+func TestPipelineRunWithServiceAccounts(t *testing.T) {
+ c, namespace := setup(t)
+ t.Parallel()
+
+ knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf)
+ defer tearDown(t, c, namespace)
+
+ saPerTask := map[string]string{
+ "task1": "sa1",
+ "task2": "sa2",
+ "task3": "sa3",
+ }
+
+ // Create Secrets and Service Accounts
+ secrets := []corev1.Secret{{
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "secret1",
+ },
+ Type: "Opaque",
+ Data: map[string][]byte{
+ "foo1": []byte("bar1"),
+ },
+ }, {
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "secret2",
+ },
+ Type: "Opaque",
+ Data: map[string][]byte{
+ "foo2": []byte("bar2"),
+ },
+ }, {
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "secret3",
+ },
+ Type: "Opaque",
+ Data: map[string][]byte{
+ "foo2": []byte("bar3"),
+ },
+ }}
+ serviceAccounts := []corev1.ServiceAccount{{
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "sa1",
+ },
+ Secrets: []corev1.ObjectReference{{
+ Name: "secret1",
+ }},
+ }, {
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "sa2",
+ },
+ Secrets: []corev1.ObjectReference{{
+ Name: "secret2",
+ }},
+ }, {
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "sa3",
+ },
+ Secrets: []corev1.ObjectReference{{
+ Name: "secret3",
+ }},
+ }}
+ for _, secret := range secrets {
+ if _, err := c.KubeClient.Kube.CoreV1().Secrets(namespace).Create(&secret); err != nil {
+ t.Fatalf("Failed to create secret `%s`: %s", secret.Name, err)
+ }
+ }
+ for _, serviceAccount := range serviceAccounts {
+ if _, err := c.KubeClient.Kube.CoreV1().ServiceAccounts(namespace).Create(&serviceAccount); err != nil {
+ t.Fatalf("Failed to create SA `%s`: %s", serviceAccount.Name, err)
+ }
+ }
+
+ // Create a Pipeline with multiple tasks
+ pipeline := &v1beta1.Pipeline{
+ ObjectMeta: metav1.ObjectMeta{Name: "pipelinewithsas", Namespace: namespace},
+ Spec: v1beta1.PipelineSpec{
+ Tasks: []v1beta1.PipelineTask{{
+ Name: "task1",
+ TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: &v1beta1.TaskSpec{
+ Steps: []v1beta1.Step{{
+ Container: corev1.Container{
+ Image: "ubuntu",
+ },
+ Script: `echo task1`,
+ }},
+ }},
+ }, {
+ Name: "task2",
+ TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: &v1beta1.TaskSpec{
+ Steps: []v1beta1.Step{{
+ Container: corev1.Container{
+ Image: "ubuntu",
+ },
+ Script: `echo task2`,
+ }},
+ }},
+ }, {
+ Name: "task3",
+ TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: &v1beta1.TaskSpec{
+ Steps: []v1beta1.Step{{
+ Container: corev1.Container{
+ Image: "ubuntu",
+ },
+ Script: `echo task3`,
+ }},
+ }},
+ }},
+ },
+ }
+ if _, err := c.PipelineClient.Create(pipeline); err != nil {
+ t.Fatalf("Failed to create Pipeline `%s`: %s", pipeline.Name, err)
+ }
+
+ // Create a PipelineRun that uses those ServiceAccount
+ pipelineRun := &v1beta1.PipelineRun{
+ ObjectMeta: metav1.ObjectMeta{Name: "pipelinerunwithasas", Namespace: namespace},
+ Spec: v1beta1.PipelineRunSpec{
+ PipelineRef: &v1beta1.PipelineRef{Name: "pipelinewithsas"},
+ ServiceAccountName: "sa1",
+ ServiceAccountNames: []v1beta1.PipelineRunSpecServiceAccountName{{
+ TaskName: "task2", ServiceAccountName: "sa2",
+ }},
+ TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{
+ PipelineTaskName: "task3", TaskServiceAccountName: "sa3",
+ }},
+ },
+ }
+
+ _, err := c.PipelineRunClient.Create(pipelineRun)
+ if err != nil {
+ t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
+ }
+
+ t.Logf("Waiting for PipelineRun %s in namespace %s to complete", pipelineRun.Name, namespace)
+ if err := WaitForPipelineRunState(c, pipelineRun.Name, pipelineRunTimeout, PipelineRunSucceed(pipelineRun.Name), "PipelineRunSuccess"); err != nil {
+ t.Fatalf("Error waiting for PipelineRun %s to finish: %s", pipelineRun.Name, err)
+ }
+
+ // Verify it used those serviceAccount
+ taskRuns, err := c.TaskRunClient.List(metav1.ListOptions{LabelSelector: fmt.Sprintf("tekton.dev/pipelineRun=%s", pipelineRun.Name)})
+ if err != nil {
+ t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+ }
+ for _, taskRun := range taskRuns.Items {
+ sa := taskRun.Spec.ServiceAccountName
+ taskName := taskRun.Labels["tekton.dev/pipelineTask"]
+ expectedSA := saPerTask[taskName]
+ if sa != expectedSA {
+ t.Fatalf("TaskRun %s expected SA %s, got %s", taskRun.Name, expectedSA, sa)
+ }
+ pods, err := c.KubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{LabelSelector: fmt.Sprintf("tekton.dev/taskRun=%s", taskRun.Name)})
+ if err != nil {
+ t.Fatalf("Error listing Pods for TaskRun %s: %s", taskRun.Name, err)
+ }
+ if len(pods.Items) != 1 {
+ t.Fatalf("TaskRun %s should have only 1 pod association, got %+v", taskRun.Name, pods.Items)
+ }
+ podSA := pods.Items[0].Spec.ServiceAccountName
+ if podSA != expectedSA {
+ t.Fatalf("TaskRun's pod %s expected SA %s, got %s", pods.Items[0].Name, expectedSA, podSA)
+ }
+ }
+}
+
+func TestPipelineRunWithServiceAccountNameAndTaskRunSpec(t *testing.T) {
+ c, namespace := setup(t)
+ t.Parallel()
+
+ knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf)
+ defer tearDown(t, c, namespace)
+
+ // Create Secrets and Service Accounts
+ secret := &corev1.Secret{
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "secret1",
+ },
+ Type: "Opaque",
+ Data: map[string][]byte{
+ "foo1": []byte("bar1"),
+ },
+ }
+ serviceAccount := &corev1.ServiceAccount{
+ ObjectMeta: metav1.ObjectMeta{
+ Namespace: namespace,
+ Name: "sa1",
+ },
+ Secrets: []corev1.ObjectReference{{
+ Name: "secret1",
+ }},
+ }
+ if _, err := c.KubeClient.Kube.CoreV1().Secrets(namespace).Create(secret); err != nil {
+ t.Fatalf("Failed to create secret `%s`: %s", secret.Name, err)
+ }
+ if _, err := c.KubeClient.Kube.CoreV1().ServiceAccounts(namespace).Create(serviceAccount); err != nil {
+ t.Fatalf("Failed to create SA `%s`: %s", serviceAccount.Name, err)
+ }
+
+ // Create a Pipeline with multiple tasks
+ pipeline := &v1beta1.Pipeline{
+ ObjectMeta: metav1.ObjectMeta{Name: "pipelinewithsas", Namespace: namespace},
+ Spec: v1beta1.PipelineSpec{
+ Tasks: []v1beta1.PipelineTask{{
+ Name: "task1",
+ TaskSpec: &v1beta1.EmbeddedTask{TaskSpec: &v1beta1.TaskSpec{
+ Steps: []v1beta1.Step{{
+ Container: corev1.Container{
+ Image: "ubuntu",
+ },
+ Script: `echo task1`,
+ }},
+ }},
+ }},
+ },
+ }
+ if _, err := c.PipelineClient.Create(pipeline); err != nil {
+ t.Fatalf("Failed to create Pipeline `%s`: %s", pipeline.Name, err)
+ }
+
+ // Create a PipelineRun that uses those ServiceAccount
+ pipelineRun := &v1beta1.PipelineRun{
+ ObjectMeta: metav1.ObjectMeta{Name: "pipelinerunwithasas", Namespace: namespace},
+ Spec: v1beta1.PipelineRunSpec{
+ PipelineRef: &v1beta1.PipelineRef{Name: "pipelinewithsas"},
+ ServiceAccountName: "sa1",
+ TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{{
+ PipelineTaskName: "task1",
+ TaskPodTemplate: &v1beta1.PodTemplate{
+ HostNetwork: true,
+ },
+ }},
+ },
+ }
+
+ _, err := c.PipelineRunClient.Create(pipelineRun)
+ if err != nil {
+ t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
+ }
+
+ t.Logf("Waiting for PipelineRun %s in namespace %s to complete", pipelineRun.Name, namespace)
+ if err := WaitForPipelineRunState(c, pipelineRun.Name, pipelineRunTimeout, PipelineRunSucceed(pipelineRun.Name), "PipelineRunSuccess"); err != nil {
+ t.Fatalf("Error waiting for PipelineRun %s to finish: %s", pipelineRun.Name, err)
+ }
+
+ // Verify it used those serviceAccount
+ taskRuns, err := c.TaskRunClient.List(metav1.ListOptions{LabelSelector: fmt.Sprintf("tekton.dev/pipelineRun=%s", pipelineRun.Name)})
+ if err != nil {
+ t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+ }
+ for _, taskRun := range taskRuns.Items {
+ sa := taskRun.Spec.ServiceAccountName
+ expectedSA := "sa1"
+ if sa != expectedSA {
+ t.Fatalf("TaskRun %s expected SA %s, got %s", taskRun.Name, expectedSA, sa)
+ }
+ pods, err := c.KubeClient.Kube.CoreV1().Pods(namespace).List(metav1.ListOptions{LabelSelector: fmt.Sprintf("tekton.dev/taskRun=%s", taskRun.Name)})
+ if err != nil {
+ t.Fatalf("Error listing Pods for TaskRun %s: %s", taskRun.Name, err)
+ }
+ if len(pods.Items) != 1 {
+ t.Fatalf("TaskRun %s should have only 1 pod association, got %+v", taskRun.Name, pods.Items)
+ }
+ podSA := pods.Items[0].Spec.ServiceAccountName
+ if podSA != expectedSA {
+ t.Fatalf("TaskRun's pod %s expected SA %s, got %s", pods.Items[0].Name, expectedSA, podSA)
+ }
+ }
+}