Skip to content

Commit

Permalink
Fix TaskRunSpec overrides when empty 🗜
Browse files Browse the repository at this point in the history 
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 <[email protected]>
(cherry picked from commit 695d117)
Signed-off-by: Vincent Demeester <[email protected]>
  • Loading branch information
@vdemeester
vdemeester committed Oct 23, 2020
1 parent 5313762 commit ca8b286
Show file tree
Hide file tree
Showing 3 changed files with 340 additions and 4 deletions.
8 changes: 6 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipelinerun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 21 additions & 2 deletions pkg/apis/pipeline/v1beta1/pipelinerun_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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 {
Expand Down
313 changes: 313 additions & 0 deletions test/serviceaccount_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
// +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 (
"context"
"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) {
ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)
defer cancel()

c, namespace := setup(ctx, t)
t.Parallel()

knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
defer tearDown(ctx, 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(ctx, &secret, metav1.CreateOptions{}); 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(ctx, &serviceAccount, metav1.CreateOptions{}); 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(ctx, pipeline, metav1.CreateOptions{}); 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(ctx, pipelineRun, metav1.CreateOptions{})
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(ctx, 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(ctx, 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(ctx, 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) {
ctx := context.Background()
ctx, cancel := context.WithCancel(ctx)
defer cancel()

c, namespace := setup(ctx, t)
t.Parallel()

knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
defer tearDown(ctx, 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(ctx, secret, metav1.CreateOptions{}); err != nil {
t.Fatalf("Failed to create secret `%s`: %s", secret.Name, err)
}
if _, err := c.KubeClient.Kube.CoreV1().ServiceAccounts(namespace).Create(ctx, serviceAccount, metav1.CreateOptions{}); 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(ctx, pipeline, metav1.CreateOptions{}); 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(ctx, pipelineRun, metav1.CreateOptions{})
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(ctx, 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(ctx, 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(ctx, 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)
}
}
}

0 comments on commit ca8b286

Please sign in to comment.