Fix PipelineRun serviceAccountNames for finally tasks #3560

ljupchokotev · 2020-11-24T20:34:28Z

Changes

Mapping a service account to a finally task in a PipelineRun's
serviceAccountNames field or taskRunSpecs would result in an error saying that the task doesn't
exist in the Pipeline.

This commit fixes the ValidateServiceaccountMapping and ValidateTasskRunSpecs functions by iterating over
the finally tasks in addition to the normal tasks of a Pipeline.

Also, added test cases for the bug.

/kind bug

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Fixes a bug where PipelineRun's serviceAccountNames and taskPodSpecs couldn't be applied on finally tasks and resulted in an error.

Mapping a service account to a finally task in a PipelineRun's serviceAccountNames field would result in an error saying that the task doesn't exist in the Pipeline. This commit fixes the ValidateServiceaccountMapping function by iterating over the finally tasks in addition to the normal tasks of a Pipeline.

tekton-robot · 2020-11-24T20:34:42Z

Hi @ljupchokotev. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ljupchokotev · 2020-11-24T20:36:35Z

/assign afrittoli pritidesai

pritidesai · 2020-11-24T21:27:22Z

/ok-to-test

tekton-robot · 2020-11-24T21:29:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	87.4%	88.0%	0.7

pritidesai · 2020-11-25T00:05:48Z

thanks @ljupchokotev for the fix.

/approve

Please note that the pipelineRun.Spec.ServiceAccountNames are deprecated and will be removed as early as May, 2021.

@vdemeester these changes are fixing finally task to use pipelineRun.Spec.ServiceAccountNames. It was not restricted at the time finally tasks were implemented but now since its deprecated, what is the procedure? Merge this now and drop the support while deprecating in May?

pritidesai · 2020-11-25T00:09:11Z

or avoid fixing it? putting it on hold until we clarify

/hold

ljupchokotev · 2020-11-25T08:33:57Z

@pritidesai just looked at the code and it seems that even by using taskRunSpecs you would have the same issue. See here, it doesn't iterate over the finally tasks which means you won't be able to set a service account for a finally task and fail with the same issue as what this PR tries to fix.

Should I fix that part and include it in this PR?

EDIT: I tested this using the following objects:

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: echo
spec:
  steps:
    - name: echo
      image: busybox:latest
      script: |
        echo foo
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: echo
spec:
  tasks:
    - name: echo
      taskRef:
        name: echo
  finally:
    - name: echo-final
      taskRef:
        name: echo
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: echo-run
spec:
  pipelineRef:
    name: echo
  taskRunSpecs:
    - pipelineTaskName: echo-final
      taskServiceAccountName: tester

The PipelineRun failed with

PipelineRun tekton-pipelines/echo-run doesn't define taskRunSpecs correctly: PipelineRun's taskrunSpecs defined wrong taskName: "echo-final", does not exist in Pipeline

vdemeester · 2020-11-25T09:31:33Z

@pritidesai just looked at the code and it seems that even by using taskRunSpecs you would have the same issue. See here, it doesn't iterate over the finally tasks which means you won't be able to set a service account for a finally task and fail with the same issue as what this PR tries to fix.

Should I fix that part and include it in this PR?

Definitely yes 😉

vdemeester · 2020-11-25T09:33:44Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

+	for _, task := range p.Finally {
+		pipelineTasks[task.Name] = task.Name
+	}
+
 	for _, name := range pr.Spec.ServiceAccountNames {


Interesting, this is a miss when we deprecated ServiceAccountNames 🤔 it shouldn't be looking here (or at least it should be sharing the same code as what the reconciler does)

I am still trying to understand this but it looks a bit weird so far 😕

Reconciler runs validation on pipelineRun.Spec.ServiceAccountNames and pipelineRun.Spec.taskRunSpecs, making sure the tasks specified in these sections exist under tasks and finally:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 443 in 4fdd018

if err := resources.ValidateServiceaccountMapping(pipelineSpec, pr); err != nil {

and

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 451 in 4fdd018

if err := resources.ValidateTaskRunSpecs(pipelineSpec, pr); err != nil {

But, resolvePipelineState resolves task with pr.Spec.ServiceAccountName:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 471 in 4fdd018

pipelineRunState, err := c.resolvePipelineState(ctx, tasks, pipelineMeta, pr, providedResources)

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 266 in 4fdd018

fn, _, err := tresources.GetTaskFunc(ctx, c.KubeClientSet, c.PipelineClientSet, task.TaskRef, pr.Namespace, pr.Spec.ServiceAccountName)

Only applies to bundle at this point, i.e. bundles does not honor pr.Spec.ServiceAccountNames or pr.Spec.TaskRunSpecs 🤔

pipeline/pkg/reconciler/taskrun/resources/taskref.go

Line 53 in 4fdd018

ServiceAccountName: saName,

Looking further 🔍

alright, createTaskRun resolves service account name in expected order:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 654 in 4fdd018

serviceAccountName, podTemplate := pr.GetTaskRunSpecs(rprt.PipelineTask.Name)

First, read serviceAccountName specified in pr.Spec.ServiceAccountNames:

pipeline/pkg/apis/pipeline/v1beta1/pipelinerun_types.go

Line 141 in 4fdd018

func (pr *PipelineRun) GetServiceAccountName(pipelineTaskName string) string {

which get replaced with pr.Spec.TaskRunSpecs if specified:

pipeline/pkg/apis/pipeline/v1beta1/pipelinerun_types.go

Lines 438 to 439 in 4fdd018

if task.TaskServiceAccountName != "" {

serviceAccountName = task.TaskServiceAccountName

it shouldn't be looking here pr.Spec.ServiceAccountNames (or at least it should be sharing the same code as what the reconciler does)

It's looking here to validate which is fine from the validation perspective, reconciler reads serviceAccountName, serviceAccountNames, and taskRunSpecs, and sets the service account name in expected order.

Using TaskRunSpecs on finally tasks resulted in an error because the validation function only checked the normal tasks. This commit updates the validation function so that it iterates over the finally tasks too fixing the issue. TaskRunSpecs can now be used to add a custom TaskPodTemplate and TaskServiceAccountName to a finally task.

tekton-robot · 2020-11-25T12:04:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	87.4%	91.9%	4.6

ljupchokotev · 2020-11-25T12:05:38Z

@pritidesai just looked at the code and it seems that even by using taskRunSpecs you would have the same issue. See here, it doesn't iterate over the finally tasks which means you won't be able to set a service account for a finally task and fail with the same issue as what this PR tries to fix.
Should I fix that part and include it in this PR?

Definitely yes wink

Done, also edited the PR description to match the current state

pritidesai · 2020-11-25T19:11:39Z

@vdemeester any more changes needed here?

tekton-robot · 2020-11-26T08:25:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pritidesai, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pritidesai,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pritidesai · 2020-11-30T18:40:04Z

/hold cancel

vdemeester

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Nov 24, 2020

tekton-robot requested review from dibyom and dlorenc November 24, 2020 20:34

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 24, 2020

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 24, 2020

tekton-robot assigned afrittoli and pritidesai Nov 24, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 24, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2020

vdemeester requested changes Nov 25, 2020

View reviewed changes

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2020

ljupchokotev requested a review from vdemeester November 25, 2020 17:48

vdemeester approved these changes Nov 26, 2020

View reviewed changes

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2020

vdemeester reviewed Dec 1, 2020

View reviewed changes

tekton-robot assigned vdemeester Dec 1, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

tekton-robot merged commit 2931d52 into tektoncd:master Dec 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PipelineRun serviceAccountNames for finally tasks #3560

Fix PipelineRun serviceAccountNames for finally tasks #3560

ljupchokotev commented Nov 24, 2020 •

edited

Loading

tekton-robot commented Nov 24, 2020

ljupchokotev commented Nov 24, 2020

pritidesai commented Nov 24, 2020

tekton-robot commented Nov 24, 2020

pritidesai commented Nov 25, 2020 •

edited

Loading

pritidesai commented Nov 25, 2020

ljupchokotev commented Nov 25, 2020 •

edited

Loading

vdemeester commented Nov 25, 2020

vdemeester Nov 25, 2020

pritidesai Nov 25, 2020

pritidesai Nov 25, 2020

pritidesai Nov 25, 2020

tekton-robot commented Nov 25, 2020

ljupchokotev commented Nov 25, 2020

pritidesai commented Nov 25, 2020

tekton-robot commented Nov 26, 2020

pritidesai commented Nov 30, 2020

vdemeester left a comment

	if task.TaskServiceAccountName != "" {
	serviceAccountName = task.TaskServiceAccountName

Fix PipelineRun serviceAccountNames for finally tasks #3560

Fix PipelineRun serviceAccountNames for finally tasks #3560

Conversation

ljupchokotev commented Nov 24, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Nov 24, 2020

ljupchokotev commented Nov 24, 2020

pritidesai commented Nov 24, 2020

tekton-robot commented Nov 24, 2020

pritidesai commented Nov 25, 2020 • edited Loading

pritidesai commented Nov 25, 2020

ljupchokotev commented Nov 25, 2020 • edited Loading

vdemeester commented Nov 25, 2020

vdemeester Nov 25, 2020

Choose a reason for hiding this comment

pritidesai Nov 25, 2020

Choose a reason for hiding this comment

pritidesai Nov 25, 2020

Choose a reason for hiding this comment

pritidesai Nov 25, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 25, 2020

ljupchokotev commented Nov 25, 2020

pritidesai commented Nov 25, 2020

tekton-robot commented Nov 26, 2020

pritidesai commented Nov 30, 2020

vdemeester left a comment

Choose a reason for hiding this comment

ljupchokotev commented Nov 24, 2020 •

edited

Loading

pritidesai commented Nov 25, 2020 •

edited

Loading

ljupchokotev commented Nov 25, 2020 •

edited

Loading