drone Allow using a non-default kubernetes service account #31

james-mcgoodwin · 2021-01-22T15:01:43Z

This PR lets the user choose to not use the built in 'default' Kubernetes service account in the namespace drone is being deployed to.

Our use case for this is we create specific KSA's for each of our services, while optionally attaching it to GCPs Workload Identity system via annotations.

If left unchanged by the user, the default KSA will still be used and no new KSA will be generated.

If the user overrides the serviceAccount: object they can do the following:

name: [string] - Select the name of a KSA to use instead of 'default'
create: [boolean] - Optionally create a new KSA if it does not exist
annotations: [object] - Apply service account annotations to the optionally created KSA

Note that annotations will not be applied to any KSA if the create bool is false.

I feel like this PR dove tales with the Allow custom webserver port PR.

My ultimate goal is a drone running with out root (using port 8080), with a KSA that we choose (named predictably 'drone')

This will permit users to select a custom kubernetes service account via helm chart values, instead of using the built in 'default' ksa. If the serviceAccount.create boolean is set to true, a new helm chart 'serviceaccounts.yaml' is produced which describes the KSA to be created, and annotated with the users desired annotations. If no overrides are provided, the default values.yaml will select the kubernetes service account named 'default', which is the same net effect seen via the drone helm chart today.

james-mcgoodwin · 2021-01-22T15:05:25Z

It looks like the CI linting system is out of commission, it's failing for other PR's on the bitnami charts URL.

Error: looks like "https://charts.bitnami.com" is not a valid chart repository or cannot be reached: failed to fetch https://charts.bitnami.com/index.yaml : 403 Forbidden
--

CI fails to pass because values.yaml contains an invalid null string. Running 'helm lint ./drone' fails Even calling helm lint with an additional '-f ./drone/ci/test-values.yaml' still fails to validate values.yaml $ helm lint ./drone/ -f ./drone/ci/test-values.yaml ==> Linting ./drone/ [ERROR] values.yaml: - env.DRONE_SERVER_HOST: String length must be greater than or equal to 3 Error: 1 chart(s) linted, 1 chart(s) failed I have updated values.yaml with the example string used in the drone documentation page for the DRONE_SERVER_HOST value: https://readme.drone.io/server/reference/drone-server-host/

james-mcgoodwin · 2021-01-26T19:34:49Z

Popping this commit message out since it changes a default value in values.yaml:

CI fails to pass because values.yaml contains an invalid null string.
Running 'helm lint ./drone' fails
Even calling helm lint with an additional
'-f ./drone/ci/test-values.yaml' still fails to validate values.yaml

$  helm lint ./drone/ -f ./drone/ci/test-values.yaml
==> Linting ./drone/
[ERROR] values.yaml: - env.DRONE_SERVER_HOST: String length must be greater than or equal to 3

Error: 1 chart(s) linted, 1 chart(s) failed

I have updated values.yaml with the example string used in the drone
documentation page for the DRONE_SERVER_HOST value:
https://readme.drone.io/server/reference/drone-server-host/

cognifloyd

This needs to be updated with master.

cognifloyd · 2021-09-02T15:46:20Z

charts/drone/templates/deployment.yaml

+      serviceAccount: {{ .Values.serviceAccount.name }}
+      serviceAccountName: {{ .Values.serviceAccount.name }}


Suggested change

serviceAccount: {{ .Values.serviceAccount.name }}

serviceAccountName: {{ .Values.serviceAccount.name }}

serviceAccountName: {{ .Values.serviceAccount.name }}

I think you only need serviceAccountName.

cognifloyd · 2021-09-02T15:57:51Z

charts/drone/templates/serviceaccount.yaml

+metadata:
+  name: {{ .Values.serviceAccount.name }}
+  annotations:
+{{ toYaml .Values.serviceAccount.annotations | indent 4 }}


I like using nindent to keep the template indentation aligned with the yaml indentation.

Suggested change

{{ toYaml .Values.serviceAccount.annotations | indent 4 }}

{{- toYaml .Values.serviceAccount.annotations | nindent 4 }}

cognifloyd · 2021-09-02T15:59:16Z

charts/drone/values.schema.json

+    "serviceAccount": {
+      "$id": "#/properties/serviceAccount",
+      "type": "object",
+	  "required": [
+		  "name",
+		  "create"
+	  ],
+	  "properties": {
+		  "name": {
+			  "$id": "#/properties/serviceAccount/name",
+			  "type": "string"
+		  },
+		  "create": {
+			  "$id": "#/properties/serviceAccount/create",
+			  "type": "boolean"
+		  }
+	  } 
+    },


whitespacing is inconsistent. You've got tabs and a trailing space.
Also, I recommend adding annotations here as well.

Suggested change

"serviceAccount": {

"$id": "#/properties/serviceAccount",

"type": "object",

"required": [

"name",

"create"

],

"properties": {

"name": {

"$id": "#/properties/serviceAccount/name",

"type": "string"

},

"create": {

"$id": "#/properties/serviceAccount/create",

"type": "boolean"

}

}

},

"serviceAccount": {

"$id": "#/properties/serviceAccount",

"type": "object",

"required": [

"name",

"create"

],

"properties": {

"name": {

"$id": "#/properties/serviceAccount/properties/name",

"type": "string"

},

"create": {

"$id": "#/properties/serviceAccount/properties/create",

"type": "boolean"

},

"annotations": {

"$id": "#/properties/serviceAccount/properties/annotations",

"type": "object"

}

}

},

ashtonian · 2021-09-02T21:48:04Z

would like this to use a sa with server for eks irsa for s3 access.

bkk-bcd · 2022-06-16T12:09:04Z

would like this to use a sa with server for eks irsa for s3 access.

We need this for the same reason. Helm create generates all the service account goodness one needs, it shouldn't have been removed to begin with.

@jeanlucmongrain can you get the conflicts resolved and then we can we can try to get this merged?

stekole · 2022-06-16T12:31:54Z

@jimsheldon - can we get someone to look at this PR please

jimsheldon · 2022-06-16T13:12:04Z

I'll be looking into this asap. Can someone fix the conflict? Thanks

bkk-bcd · 2022-06-16T13:20:27Z

Thanks guys. If ~~@jimsheldon~~ @james-mcgoodwin is busy I can send up another PR

bkk-bcd · 2022-06-16T23:58:30Z

@jimsheldon try #76

jimsheldon · 2022-06-24T11:56:42Z

#76 has been merged, closing this out

James McGoodwin added 2 commits January 21, 2021 16:01

Some small tweaks. Remove unneccesary if condition, fix yaml doc header

bfab5fd

stekole approved these changes Jan 22, 2021

View reviewed changes

Fix CI pipeline

d0ab50e

james-mcgoodwin changed the title ~~Allow using a non-default kubernetes service account~~ drone Allow using a non-default kubernetes service account Jan 26, 2021

James McGoodwin added 3 commits January 26, 2021 11:07

Bump helm chart version

3f62bc5

Gobble trailing spaces

a7fa1f3

cognifloyd reviewed Sep 2, 2021

View reviewed changes

cognifloyd added a commit to cognifloyd/drone-charts that referenced this pull request Sep 2, 2021

Merge upstream PR drone#31

2f040a4

Merge branch 'drone:master' into master

bf9a013

bkk-bcd mentioned this pull request Jun 16, 2022

service account support for drone server #76

Merged

jimsheldon closed this Jun 24, 2022

		serviceAccount: {{ .Values.serviceAccount.name }}
		serviceAccountName: {{ .Values.serviceAccount.name }}

	{{ toYaml .Values.serviceAccount.annotations \| indent 4 }}
	{{- toYaml .Values.serviceAccount.annotations \| nindent 4 }}

drone Allow using a non-default kubernetes service account #31

drone Allow using a non-default kubernetes service account #31

Uh oh!

Conversation

james-mcgoodwin commented Jan 22, 2021

Uh oh!

james-mcgoodwin commented Jan 22, 2021

Uh oh!

james-mcgoodwin commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cognifloyd left a comment

Choose a reason for hiding this comment

Uh oh!

cognifloyd Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

cognifloyd Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

cognifloyd Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashtonian commented Sep 2, 2021

Uh oh!

bkk-bcd commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stekole commented Jun 16, 2022

Uh oh!

jimsheldon commented Jun 16, 2022

Uh oh!

bkk-bcd commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkk-bcd commented Jun 16, 2022

Uh oh!

jimsheldon commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

james-mcgoodwin commented Jan 26, 2021 •

edited

Loading

cognifloyd Sep 2, 2021 •

edited

Loading

bkk-bcd commented Jun 16, 2022 •

edited

Loading

bkk-bcd commented Jun 16, 2022 •

edited

Loading