Provide a way to specify default service accounts #1227

sthaha · 2019-08-21T13:39:43Z

This PR allows defaults service-account (configurable though the configmap)
to be set when none is specified in a PipelineRun.

Fixes: #1213
Signed-off-by: Sunil Thaha [email protected]

Changes

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

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 and TaskRun 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

If pipeline controllers are deployed with a config-map that has `default-service-account` key set to a non-empty string, pipeline-runs that do not specify a ServiceAccount will be modified (mutated) to  the value of the `default-service-account`.

tekton-robot · 2019-08-21T13:41:36Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	77.8%	81.8%	4.0

vdemeester

SGTM

/cc @bobcatfish @imjasonh

sthaha · 2019-08-21T14:00:27Z

@vdemeester @bobcatfish does it make sense to implement the same for taskruns as well?

tekton-robot · 2019-08-21T14:10:41Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	77.8%	81.8%	4.0

vdemeester · 2019-08-22T16:32:30Z

@vdemeester @bobcatfish does it make sense to implement the same for taskruns as well?

I would think so yes

bobcatfish

Thanks for adding this @sthaha and thanks for your patience! 🙏 A few requests:

Can we add some docs on how to configure this? Maybe at https://github.com/tektoncd/pipeline/blob/master/docs/install.md#configuring-tekton-pipelines
Can we add a test that ensures that the serviceAccount and serviceAccounts fields will override the configured default? (https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#service-accounts) Looking at the way you implemented it, I bet it will work, but just to be sure :) (could be a reconciler test, or an end to end test, or even an example)
Can we update https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#service-account and https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#service-account to indicate that the service account used when those fields aren't configured won't necessarily be the default account

Before this change, if there was no serviceAccount or serviceAccounts specified in a PipelineRun or a TaskRun, I could be reasonably sure that the service account default was used, but that is no longer completely obvious.. And if the configuration of the controller was changed, I wouldn't be able to tell from looking at a previous Run which service account was used. I wonder if it would make sense to include this in the status field? I'm not 100% sure this is a good idea tho - maybe we can open a follow up issue and discuss? Or wait to see if ppl actually care (curious what you think @imjasonh @vdemeester )

(The coverage reporting seems a bit odd, opened #1248)

bobcatfish · 2019-08-27T18:56:39Z

pkg/apis/config/default.go

@@ -30,17 +30,20 @@ const (
 	DefaultTimeoutMinutes    = 60
 	NoTimeoutDuration        = 0 * time.Minute
 	defaultTimeoutMinutesKey = "default-timeout-minutes"
+	defaultServiceAccountKey = "default-service-account"


I'm a bit confused why in config/config-defaults.yaml the default service account is default, but if that file isn't use, the default will be default-service-account - would it make sense to use default in both cases?

@bobcatfish default-service-account is the "config" key to get the value for the default-service-account

@bobcatfish like @vdemeester mentioned, defaultServiceAccountKey (default-service-account) refers to the key part of the configmap e.g.

data: default-service-account: tekton

which if unspecified would spawn the pods with the default ServiceAccount.

ah whoops, thanks for the explanation!

sthaha · 2019-08-28T13:05:23Z

Can we add some docs on how to configure this? Maybe at https://github.com/tektoncd/pipeline/blob/master/docs/install.md#configuring-tekton-pipelines

👍 will do

Can we add a test that ensures that the serviceAccount and serviceAccounts fields will override the configured default? (https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#service-accounts) Looking at the way you implemented it, I bet it will work, but just to be sure :) (could be a reconciler test, or an end to end test, or even an example)

Nice! didn't think of this test 🤗

Can we update https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#service-account and https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#service-account to indicate that the service account used when those fields aren't configured won't necessarily be the default account

👍

Before this change, if there was no serviceAccount or serviceAccounts specified in a PipelineRun or a TaskRun, I could be reasonably sure that the service account default was used, but that is no longer completely obvious.. And if the configuration of the controller was changed, I wouldn't be able to tell from looking at a previous Run which service account was used. I wonder if it would make sense to include this in the status field? I'm not 100% sure this is a good idea tho - maybe we can open a follow up issue and discuss? Or wait to see if ppl actually care (curious what you think @imjasonh @vdemeester )

I agree that this can be taken up as a follow up issue. On a related note, would metadata.annotations.run be a good place to store this?

bobcatfish · 2019-08-29T20:11:35Z

On a related note, would metadata.annotations.run be a good place to store this?

I'm not sure! I feel like the status field is where we have been storing most information like this up until this point. I know there is a lot of precedent for using the metadata for this kind of thing. In this case I don't have a strong opinion either way!

sthaha · 2019-09-18T05:59:09Z

@bobcatfish

Before this change, if there was no serviceAccount or serviceAccounts specified in a PipelineRun or a TaskRun, I could be reasonably sure that the service account default was used, but that is no longer completely obvious.. And if the configuration of the controller was changed, I wouldn't be able to tell from looking at a previous Run which service account was used. I wonder if it would make sense to include this in the status field? I'm not 100% sure this is a good idea tho - maybe we can open a follow up issue and discuss? Or wait to see if ppl actually care (curious what you think @imjasonh @vdemeester )

IIUC, the serviceAccount set by the mutating webhook can still be seen when you inspect the pipelinerun or the taskrun

sthaha · 2019-09-18T10:51:44Z

@bobcatfish

Can we add a test that ensures that the serviceAccount and serviceAccounts fields will override the configured default? (https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#service-accounts) Looking at the way you implemented it, I bet it will work, but just to be sure :) (could be a reconciler test, or an end to end test, or even an example)

This requires the test to

setup: create an SA account say foobar
setup: run webhook with the config-defaults.default-service-account set to foobar
test: run a pipelinerun with the serviceAccount explicitly set to something-else

I was looking for a precedence (may be in the case of default-timeout-minutes) but can't find any. Do you have any pointers?

tekton-robot · 2019-09-18T12:30:01Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	77.8%	81.8%	4.0

bobcatfish · 2019-09-18T19:47:39Z

Hey @sthaha ! probably the easiest way would be to add a reconciler level test.

For example this test case is making sure the right service account is used:

pipeline/pkg/reconciler/taskrun/taskrun_test.go

Lines 460 to 491 in eb6edee

    
           name:    "serviceaccount", 
        
           taskRun: taskRunWithSaSuccess, 
        
           wantPod: tb.Pod("test-taskrun-with-sa-run-success-pod-123456", "foo", 
        
           	tb.PodAnnotation("tekton.dev/ready", ""), 
        
           	tb.PodLabel(taskNameLabelKey, "test-with-sa"), 
        
           	tb.PodLabel(taskRunNameLabelKey, "test-taskrun-with-sa-run-success"), 
        
           	tb.PodLabel(resources.ManagedByLabelKey, resources.ManagedByLabelValue), 
        
           	tb.PodOwnerReference("TaskRun", "test-taskrun-with-sa-run-success", 
        
           		tb.OwnerReferenceAPIVersion(currentApiVersion)), 
        
           	tb.PodSpec( 
        
           		tb.PodServiceAccountName("test-sa"), 
        
           		tb.PodVolumes(toolsVolume, downward, workspaceVolume, homeVolume), 
        
           		tb.PodRestartPolicy(corev1.RestartPolicyNever), 
        
           		getCredentialsInitContainer("9l9zj"), 
        
           		getPlaceToolsInitContainer(), 
        
           		tb.PodContainer("step-sa-step", "foo", 
        
           			tb.Command(entrypointLocation), 
        
           			tb.Args("-wait_file", "/builder/downward/ready", "-post_file", "/builder/tools/0", "-wait_file_content", "-entrypoint", "/mycmd", "--"), 
        
           			tb.WorkingDir(workspaceDir), 
        
           			tb.EnvVar("HOME", "/builder/home"), 
        
           			tb.VolumeMount("tools", "/builder/tools"), 
        
           			tb.VolumeMount("downward", "/builder/downward"), 
        
           			tb.VolumeMount("workspace", workspaceDir), 
        
           			tb.VolumeMount("home", "/builder/home"), 
        
           			tb.Resources(tb.Requests( 
        
           				tb.CPU("0"), 
        
           				tb.Memory("0"), 
        
           				tb.EphemeralStorage("0"), 
        
           			)), 
        
           		), 
        
           	), 
        
           ),

pipeline/pkg/reconciler/taskrun/taskrun_test.go

Lines 1050 to 1061 in eb6edee

    
           saName := tc.taskRun.Spec.ServiceAccount 
        
           if saName == "" { 
        
           	saName = "default" 
        
           } 
        
           if _, err := clients.Kube.CoreV1().ServiceAccounts(tc.taskRun.Namespace).Create(&corev1.ServiceAccount{ 
        
           	ObjectMeta: metav1.ObjectMeta{ 
        
           		Name:      saName, 
        
           		Namespace: tc.taskRun.Namespace, 
        
           	}, 
        
           }); err != nil { 
        
           	t.Fatal(err) 
        
           }

It looks like the reconciler that this test instantiates passes a config map watcher into the reconciler, so i think if you use those fake kube clients + configure a config map, the test reconciler would use it:

pipeline/pkg/reconciler/taskrun/taskrun_test.go

Lines 256 to 273 in eb6edee

    
           func getTaskRunController(t *testing.T, d test.Data) (test.TestAssets, func()) { 
        
           	ctx, _ := rtesting.SetupFakeContext(t) 
        
           	ctx, cancel := context.WithCancel(ctx) 
        
           	cloudEventClientBehaviour := cloudevent.FakeClientBehaviour{ 
        
           		SendSuccessfully: true, 
        
           	} 
        
           	ctx = cloudevent.WithClient(ctx, &cloudEventClientBehaviour) 
        
           	entrypointCache, _ = entrypoint.NewCache() 
        
           	c, _ := test.SeedTestData(t, ctx, d) 
        
           	configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace()) 
        
           	return test.TestAssets{ 
        
           		Controller: NewController( 
        
           			ctx, 
        
           			configMapWatcher, 
        
           		), 
        
           		Clients: c, 
        
           	}, cancel 
        
           }

So what I would do is probably make a completely new test function, copying a lot of the body of the Run function from the test I linked above. (Sorry writing reconciler tests is kind of tedious!!)

This PR allows defaults service-account (configurable though the configmap) to be set when none is specified in a PipelineRun. Fixes: tektoncd#1213 Signed-off-by: Sunil Thaha <[email protected]>

sthaha · 2019-09-27T05:45:56Z

pkg/reconciler/taskrun/taskrun_test.go

+	}
+
+	defaultSAName := "pipelines"
+	defaultCfg := &config.Config{


@bobcatfish @vdemeester Does this suffice? Has 2 tests

verifies that default service account specified in defaults is used

verifies that if SA is set ( test-sa ) then that will be used

yes 😻 We may want to refactor some of this, but I think that's the job of a follow-up

tekton-robot · 2019-09-27T05:46:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/config/default.go	77.8%	81.8%	4.0

vdemeester

/lgtm
/hold
^^ to let @bobcatfish review 👼

vdemeester · 2019-09-27T06:59:56Z

pkg/reconciler/taskrun/taskrun_test.go

+	}
+
+	defaultSAName := "pipelines"
+	defaultCfg := &config.Config{


yes 😻 We may want to refactor some of this, but I think that's the job of a follow-up

tekton-robot · 2019-09-27T07:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sthaha, 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~~ [vdemeester]

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

vdemeester · 2019-10-02T14:50:07Z

/hold cancel

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2019

tekton-robot requested review from afrittoli and dlorenc August 21, 2019 13:39

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Aug 21, 2019

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 21, 2019

vdemeester approved these changes Aug 21, 2019

View reviewed changes

tekton-robot requested review from bobcatfish and imjasonh August 21, 2019 13:57

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2019

sthaha changed the title ~~WIP: Provide a way to specify default service accounts~~ Provide a way to specify default service accounts Aug 21, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2019

sthaha force-pushed the allow-default-sa branch from b2308d7 to f80266a Compare August 21, 2019 14:07

bobcatfish mentioned this pull request Aug 27, 2019

Seems like coverage reporter isn't reporting on all files #1248

Closed

bobcatfish reviewed Aug 27, 2019

View reviewed changes

vdemeester added this to the Pipelines 0.7 🐱 milestone Sep 16, 2019

sthaha force-pushed the allow-default-sa branch from f80266a to bdf371b Compare September 18, 2019 12:28

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 Sep 18, 2019

afrittoli modified the milestones: Pipelines 0.7 🐱, Pipelines 0.8 🐱 Sep 19, 2019

vdemeester self-assigned this Sep 23, 2019

Provide a way to specify default service accounts

9d96e7b

This PR allows defaults service-account (configurable though the configmap) to be set when none is specified in a PipelineRun. Fixes: tektoncd#1213 Signed-off-by: Sunil Thaha <[email protected]>

sthaha force-pushed the allow-default-sa branch from bdf371b to 9d96e7b Compare September 27, 2019 05:43

sthaha commented Sep 27, 2019

View reviewed changes

vdemeester approved these changes Sep 27, 2019

View reviewed changes

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 27, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2019

tekton-robot merged commit 5563e21 into tektoncd:master Oct 2, 2019

christianvogt mentioned this pull request Oct 10, 2019

Ability to Specify Service Account for Pipeline Run via Pipelines View openshift/console#2955

Closed

nikhil-thomas mentioned this pull request Dec 11, 2019

Monitor task with 0.8 openshift-pipelines operator picks up wrong service account name openshift/tektoncd-pipeline-operator#139

Closed

bobcatfish mentioned this pull request Sep 2, 2020

fixing nightly unit test failure #3160

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to specify default service accounts #1227

Provide a way to specify default service accounts #1227

sthaha commented Aug 21, 2019

tekton-robot commented Aug 21, 2019

vdemeester left a comment

sthaha commented Aug 21, 2019

tekton-robot commented Aug 21, 2019

vdemeester commented Aug 22, 2019

bobcatfish left a comment

bobcatfish Aug 27, 2019

vdemeester Aug 28, 2019

sthaha Aug 28, 2019

bobcatfish Aug 29, 2019

sthaha commented Aug 28, 2019

bobcatfish commented Aug 29, 2019

sthaha commented Sep 18, 2019

sthaha commented Sep 18, 2019 •

edited

Loading

tekton-robot commented Sep 18, 2019

bobcatfish commented Sep 18, 2019

sthaha Sep 27, 2019

vdemeester Sep 27, 2019

tekton-robot commented Sep 27, 2019

vdemeester left a comment

vdemeester Sep 27, 2019

tekton-robot commented Sep 27, 2019

vdemeester commented Oct 2, 2019

Provide a way to specify default service accounts #1227

Provide a way to specify default service accounts #1227

Conversation

sthaha commented Aug 21, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 21, 2019

vdemeester left a comment

Choose a reason for hiding this comment

sthaha commented Aug 21, 2019

tekton-robot commented Aug 21, 2019

vdemeester commented Aug 22, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 27, 2019

Choose a reason for hiding this comment

vdemeester Aug 28, 2019

Choose a reason for hiding this comment

sthaha Aug 28, 2019

Choose a reason for hiding this comment

bobcatfish Aug 29, 2019

Choose a reason for hiding this comment

sthaha commented Aug 28, 2019

bobcatfish commented Aug 29, 2019

sthaha commented Sep 18, 2019

sthaha commented Sep 18, 2019 • edited Loading

tekton-robot commented Sep 18, 2019

bobcatfish commented Sep 18, 2019

sthaha Sep 27, 2019

Choose a reason for hiding this comment

vdemeester Sep 27, 2019

Choose a reason for hiding this comment

tekton-robot commented Sep 27, 2019

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Sep 27, 2019

Choose a reason for hiding this comment

tekton-robot commented Sep 27, 2019

vdemeester commented Oct 2, 2019

sthaha commented Sep 18, 2019 •

edited

Loading