Skip to content

Commit

Permalink
Remove deprecated serviceAccount field from *Run 🌳
Browse files Browse the repository at this point in the history 
This removes the deprecated `serviceAccount` field from TaskRun and
PipelineRun in favor of the newly added `serviceAccountName` (in
previous release) to match more closely the kubernetes API.

Signed-off-by: Vincent Demeester <[email protected]>
  • Loading branch information
@vdemeester
vdemeester committed Nov 26, 2019
1 parent 40a827d commit dc5b2a8
Show file tree
Hide file tree
Showing 13 changed files with 11 additions and 346 deletions.
24 changes: 0 additions & 24 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,6 @@ type PipelineRunSpec struct {
Params []Param `json:"params,omitempty"`
// +optional
ServiceAccountName string `json:"serviceAccountName,omitempty"`
// DeprecatedServiceAccount is a depreciated alias for ServiceAccountName.
// Deprecated: Use serviceAccountName instead.
// +optional
DeprecatedServiceAccount string `json:"serviceAccount,omitempty"`
// +optional
DeprecatedServiceAccounts []DeprecatedPipelineRunSpecServiceAccount `json:"serviceAccounts,omitempty"`
// +optional
ServiceAccountNames []PipelineRunSpecServiceAccountName `json:"serviceAccountNames,omitempty"`
// Used for cancelling a pipelinerun (and maybe more later on)
Expand Down Expand Up @@ -156,16 +150,6 @@ func (pr *PipelineRunStatus) InitializeConditions() {
pipelineRunCondSet.Manage(pr).InitializeConditions()
}

// DeprecatedPipelineRunSpecServiceAccount can be used to configure specific
// ServiceAccount for a concrete Task
// Deprecated: Use pipelineRunSpecServiceAccountName instead.
type DeprecatedPipelineRunSpecServiceAccount struct {
TaskName string `json:"taskName,omitempty"`
// DeprecatedServiceAccount is a depreciated alias for ServiceAccountName.
// Deprecated: Use serviceAccountName instead.
DeprecatedServiceAccount string `json:"serviceAccount,omitempty"`
}

// PipelineRunSpecServiceAccountName can be used to configure specific
// ServiceAccountName for a concrete Task
type PipelineRunSpecServiceAccountName struct {
Expand Down Expand Up @@ -279,14 +263,6 @@ func (pr *PipelineRun) IsTimedOut() bool {
// PipelineTask if configured, otherwise it returns the PipelineRun's serviceAccountName.
func (pr *PipelineRun) GetServiceAccountName(pipelineTaskName string) string {
serviceAccountName := pr.Spec.ServiceAccountName
if serviceAccountName == "" {
serviceAccountName = pr.Spec.DeprecatedServiceAccount
}
for _, sa := range pr.Spec.DeprecatedServiceAccounts {
if sa.TaskName == pipelineTaskName {
serviceAccountName = sa.DeprecatedServiceAccount
}
}
for _, sa := range pr.Spec.ServiceAccountNames {
if sa.TaskName == pipelineTaskName {
serviceAccountName = sa.ServiceAccountName
Expand Down
23 changes: 5 additions & 18 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,31 +235,18 @@ func TestPipelineRunGetServiceAccountName(t *testing.T) {
"taskName": "taskSA",
},
},
{
"deprecated default SA",
tb.PipelineRun("pr", "ns",
tb.PipelineRunSpec("prs",
tb.PipelineRunDeprecatedServiceAccountName("", "deprecatedSA"),
tb.PipelineRunDeprecatedServiceAccountTask("taskName", "deprecatedTaskSA"))),
map[string]string{
"unknown": "deprecatedSA",
"taskName": "deprecatedTaskSA",
},
},
{
"mixed default SA",
tb.PipelineRun("defaultSA", "defaultSA",
tb.PipelineRunSpec("defaultSA",
tb.PipelineRunDeprecatedServiceAccountName("defaultSA", "deprecatedSA"),
tb.PipelineRunServiceAccountName("defaultSA"),
tb.PipelineRunServiceAccountNameTask("task1", "task1SA"),
tb.PipelineRunServiceAccountNameTask("task2", "task2SA"),
tb.PipelineRunDeprecatedServiceAccountTask("deprecatedTask3", "deprecatedTask3SA"),
tb.PipelineRunDeprecatedServiceAccountTask("task2", "deprecated"))),
)),
map[string]string{
"unknown": "defaultSA",
"task1": "task1SA",
"task2": "task2SA",
"deprecatedTask3": "deprecatedTask3SA",
"unknown": "defaultSA",
"task1": "task1SA",
"task2": "task2SA",
},
},
} {
Expand Down
13 changes: 0 additions & 13 deletions pkg/apis/pipeline/v1alpha1/taskrun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ type TaskRunSpec struct {
Outputs TaskRunOutputs `json:"outputs,omitempty"`
// +optional
ServiceAccountName string `json:"serviceAccountName"`
// DeprecatedServiceAccount is a depreciated alias for ServiceAccountName.
// Deprecated: Use serviceAccountName instead.
// +optional
DeprecatedServiceAccount string `json:"serviceAccount,omitempty"`
// no more than one of the TaskRef and TaskSpec may be specified.
// +optional
TaskRef *TaskRef `json:"taskRef,omitempty"`
Expand Down Expand Up @@ -291,15 +287,6 @@ func (tr *TaskRun) GetRunKey() string {
return fmt.Sprintf("%s/%p", "TaskRun", tr)
}

func (tr *TaskRun) GetServiceAccountName() string {
name := tr.Spec.ServiceAccountName
if name == "" {
name = tr.Spec.DeprecatedServiceAccount
}
return name

}

// IsPartOfPipeline return true if TaskRun is a part of a Pipeline.
// It also return the name of Pipeline and PipelineRun
func (tr *TaskRun) IsPartOfPipeline() (bool, string, string) {
Expand Down
27 changes: 0 additions & 27 deletions pkg/apis/pipeline/v1alpha1/taskrun_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,33 +153,6 @@ func TestTaskRunHasStarted(t *testing.T) {
}
}

func TestTaskRunGetServiceAccountName(t *testing.T) {
for _, tt := range []struct {
name string
tr *v1alpha1.TaskRun
expectedSA string
}{{
"service account",
tb.TaskRun("name", "ns", tb.TaskRunSpec(tb.TaskRunServiceAccountName("defaultSA"))),
"defaultSA",
},
{
"deprecated SA",
tb.TaskRun("name", "ns", tb.TaskRunSpec(tb.TaskRunDeprecatedServiceAccount("", "deprecatedSA"))),
"deprecatedSA",
},
{
"both SA",
tb.TaskRun("name", "ns", tb.TaskRunSpec(tb.TaskRunDeprecatedServiceAccount("defaultSA", "deprecatedSA"))),
"defaultSA",
},
} {
if e, a := tt.expectedSA, tt.tr.GetServiceAccountName(); e != a {
t.Errorf("%s: wrong service account name: got: %q want: %q", tt.name, a, e)
}
}
}

func TestTaskRunIsOfPipelinerun(t *testing.T) {
tests := []struct {
name string
Expand Down
21 changes: 0 additions & 21 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

132 changes: 0 additions & 132 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1073,138 +1073,6 @@ func TestReconcileWithDifferentServiceAccounts(t *testing.T) {

}

func TestReconcileWithDeprecatedServiceAccounts(t *testing.T) {
names.TestingSeed()

ps := []*v1alpha1.Pipeline{
tb.Pipeline("test-sa-0", "foo", tb.PipelineSpec(tb.PipelineTask("deprecated-sa-0", "hello-world-task"))),
tb.Pipeline("test-sa-1", "foo", tb.PipelineSpec(tb.PipelineTask("sa-1", "hello-world-task"))),
tb.Pipeline("test-sa-2", "foo", tb.PipelineSpec(tb.PipelineTask("task-deprecated-sa-2", "hello-world-task"))),
tb.Pipeline("test-sa-3", "foo", tb.PipelineSpec(tb.PipelineTask("task-sa-3", "hello-world-task"))),
}
prs := []*v1alpha1.PipelineRun{
tb.PipelineRun("test-pipeline-run-deprecated-sa-0", "foo",
tb.PipelineRunSpec("test-sa-0",
tb.PipelineRunDeprecatedServiceAccountName("", "sa-0"),
),
),
tb.PipelineRun("test-pipeline-run-sa-1", "foo",
tb.PipelineRunSpec("test-sa-1",
tb.PipelineRunDeprecatedServiceAccountName("sa-1", "sa-0"),
),
),
tb.PipelineRun("test-pipeline-run-task-deprecated-sa-2", "foo",
tb.PipelineRunSpec("test-sa-2",
tb.PipelineRunServiceAccountName("wrong-sa"),
tb.PipelineRunDeprecatedServiceAccountTask("task-deprecated-sa-2", "sa-2"),
),
),
tb.PipelineRun("test-pipeline-run-task-sa-3", "foo",
tb.PipelineRunSpec("test-sa-3",
tb.PipelineRunServiceAccountName("wrong-sa"),
tb.PipelineRunServiceAccountNameTask("task-sa-3", "sa-3"),
tb.PipelineRunDeprecatedServiceAccountTask("task-sa-3", "wrong-deprecated-sa"),
),
),
}

ts := []*v1alpha1.Task{
tb.Task("hello-world-task", "foo"),
}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
}

testAssets, cancel := getPipelineRunController(t, d)
defer cancel()
c := testAssets.Controller
clients := testAssets.Clients

for _, pr := range prs {
err := c.Reconciler.Reconcile(context.Background(), fmt.Sprintf("%s/%s", pr.GetNamespace(), pr.GetName()))
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(pr.GetNamespace()).Get(pr.GetName(), metav1.GetOptions{})
if err != nil {
t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err)
}
}

expectedTaskRuns := []*v1alpha1.TaskRun{
tb.TaskRun("test-pipeline-run-deprecated-sa-0-deprecated-sa-0-9l9zj", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-deprecated-sa-0",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world-task"),
tb.TaskRunServiceAccountName("sa-0"),
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-sa-0"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-deprecated-sa-0"),
tb.TaskRunLabel("tekton.dev/pipelineTask", "deprecated-sa-0"),
),
tb.TaskRun("test-pipeline-run-sa-1-sa-1-mz4c7", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-sa-1",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world-task"),
tb.TaskRunServiceAccountName("sa-1"),
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-sa-1"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-sa-1"),
tb.TaskRunLabel("tekton.dev/pipelineTask", "sa-1"),
),
tb.TaskRun("test-pipeline-run-task-deprecated-sa-2-task-deprecated-sa-mssqb", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-task-deprecated-sa-2",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world-task"),
tb.TaskRunServiceAccountName("sa-2"),
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-sa-2"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-task-deprecated-sa-2"),
tb.TaskRunLabel("tekton.dev/pipelineTask", "task-deprecated-sa-2"),
),
tb.TaskRun("test-pipeline-run-task-sa-3-task-sa-3-78c5n", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-task-sa-3",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world-task"),
tb.TaskRunServiceAccountName("sa-3"),
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-sa-3"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-task-sa-3"),
tb.TaskRunLabel("tekton.dev/pipelineTask", "task-sa-3"),
),
}

for _, taskRun := range expectedTaskRuns {
// Check that the expected TaskRun was created
actual, err := clients.Pipeline.Tekton().TaskRuns(taskRun.GetNamespace()).Get(taskRun.GetName(), metav1.GetOptions{})
if err != nil {
t.Fatalf("Expected a TaskRun to be created, but it wasn't: %s", err)
}
if d := cmp.Diff(actual, taskRun); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", taskRun, d)
}

}

}

func TestReconcileWithTimeoutAndRetry(t *testing.T) {

tcs := []struct {
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/taskrun/entrypoint/entrypoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ func getRemoteImage(image string, kubeclient kubernetes.Interface, taskRun *v1al

kc, err := k8schain.New(kubeclient, k8schain.Options{
Namespace: taskRun.Namespace,
ServiceAccountName: taskRun.GetServiceAccountName(),
ServiceAccountName: taskRun.Spec.ServiceAccountName,
})
if err != nil {
return nil, xerrors.Errorf("Failed to create k8schain: %w", err)
Expand Down
3 changes: 1 addition & 2 deletions pkg/reconciler/taskrun/entrypoint/entrypoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,7 @@ func TestGetRemoteEntrypointWithNonDefaultSA(t *testing.T) {
for _, tt := range []func(taskRun *v1alpha1.TaskRun) *v1alpha1.TaskRun{
func(tr *v1alpha1.TaskRun) *v1alpha1.TaskRun { return tr },
func(tr *v1alpha1.TaskRun) *v1alpha1.TaskRun {
tr.Spec.ServiceAccountName = ""
tr.Spec.DeprecatedServiceAccount = "some-other-sa"
tr.Spec.ServiceAccountName = "some-other-sa"
return tr
},
} {
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/taskrun/resources/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha
var initContainers []corev1.Container
var volumes []corev1.Volume

if credsInitContainer, secretsVolumes, err := pod.CredsInit(images.CredsImage, taskRun.GetServiceAccountName(), taskRun.Namespace, kubeclient, implicitVolumeMounts, implicitEnvVars); err != nil {
if credsInitContainer, secretsVolumes, err := pod.CredsInit(images.CredsImage, taskRun.Spec.ServiceAccountName, taskRun.Namespace, kubeclient, implicitVolumeMounts, implicitEnvVars); err != nil {
return nil, err
} else if credsInitContainer != nil {
initContainers = append(initContainers, *credsInitContainer)
Expand Down Expand Up @@ -280,7 +280,7 @@ cat > ${tmpfile} << '%s'
RestartPolicy: corev1.RestartPolicyNever,
InitContainers: initContainers,
Containers: mergedPodContainers,
ServiceAccountName: taskRun.GetServiceAccountName(),
ServiceAccountName: taskRun.Spec.ServiceAccountName,
Volumes: volumes,
NodeSelector: taskRun.Spec.PodTemplate.NodeSelector,
Tolerations: taskRun.Spec.PodTemplate.Tolerations,
Expand Down
Loading

0 comments on commit dc5b2a8

Please sign in to comment.