Allow customizing the ServiceAccount for workload of PipeCD Helm chart #3535

mugioka · 2022-04-14T17:13:55Z

What this PR does / why we need it:
Hi team, PipeCD is very good software.
Currently, the default service account annotation must be edited in order to use GCP Workload Identity or AWS WebIdentity.
This change should improve convenience by allowing k8s service accounts dedicated to PipeCD Server to be created from the helm chart.

Which issue(s) this PR fixes:
none

Does this PR introduce a user-facing change?:

Allow customizing the ServiceAccount for workload of PipeCD Helm chart

pipecd-bot · 2022-04-14T17:13:57Z

Welcome @mugioka!

Thanks for your contributing. It looks like this is your first PR to this repository 🎉. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). Please read the CLA and leave a /cla sign comment on this pull request to sign the CLA.

mugioka · 2022-04-14T17:15:26Z

/cla sign

pipecd-bot · 2022-04-14T17:15:27Z

Thank you for signing the CLA. Your signing has been successfully committed.

…elm chart. Hi team, PipeCD is very good software. Currently, the default service account annotation must be edited in order to use GCP Workload Identity or AWS WebIdentity. This change should improve convenience by allowing k8s service accounts dedicated to PipeCD Server to be created from the helm chart. Signed-off-by: mugioka <[email protected]>

khanhtc1202 · 2022-04-15T01:34:38Z

/trigger presubmit

khanhtc1202 · 2022-04-15T01:35:12Z

/trigger presubmits

pipecd-bot · 2022-04-15T01:35:15Z

@khanhtc1202: Your requested presubmits has been scheduled in response to this comment.

pipecd-bot · 2022-04-15T01:40:41Z

Code coverage for golang is 35.67%. This pull request does not change code coverage.

nghialv · 2022-04-15T02:11:12Z

manifests/pipecd/values.yaml

+  # Specifies whether a service account should be created
+  create: true
+  # Annotations to add to the service account
+  annotations: {}


Could you add a name field to allow users to use an existing Service Account if they want?

Same as the chart for Piped Agent. https://github.com/pipe-cd/pipecd/blob/master/manifests/piped/values.yaml#L89-L93

nghialv · 2022-04-15T02:12:32Z

manifests/pipecd/templates/deployment.yaml

+      {{- if .Values.serviceAccount.create -}}
+      serviceAccountName: {{ include "pipecd.fullname" . }}-server
+      {{- end }}


Follow this comment

https://github.com/pipe-cd/pipecd/blob/master/manifests/piped/templates/deployment.yaml#L22-L24
https://github.com/pipe-cd/pipecd/blob/master/manifests/piped/templates/_helpers.tpl#L76-L85

nghialv · 2022-04-15T02:13:24Z

@mugioka Hi, Thank you for your pull request.

It is really nice. I just have left some nits. PTAL.

Signed-off-by: mugioka <[email protected]>

mugioka

@nghialv
I fixed.
Please re-review.

mugioka · 2022-04-15T04:48:45Z

manifests/piped/templates/deployment.yaml

@@ -19,9 +19,7 @@ spec:
        sidecar.istio.io/inject: "false"
        rollme: {{ randAlphaNum 5 | quote }}
    spec:
-      {{- if .Values.serviceAccount.create }}


refs: #3535 (comment)

to use existing service account.

manifests/pipecd/templates/serviceaccount.yaml

Co-authored-by: Le Van Nghia <[email protected]>

nghialv · 2022-04-15T06:02:32Z

manifests/pipecd/values.yaml

+# ServiceAccount
+serviceAccount:
+  # Specifies whether a service account should be created
+  create: true


@mugioka Please change the default value to false to avoid adding an unexpected service account for existing users.

I fixed it.

nghialv · 2022-04-15T06:03:07Z

@mugioka Sorry, I have just found a thing. PTAL #3535 (comment)

nghialv · 2022-04-15T06:08:13Z

manifests/pipecd/templates/deployment.yaml

@@ -84,6 +84,7 @@ spec:
        {{- include "pipecd.selectorLabels" . | nindent 8 }}
        app.kubernetes.io/component: server
    spec:
+      serviceAccountName: {{ include "pipecd.serviceAccountName" . }}


@mugioka Since the default value has been changed to false, I think we need to add a check condition here.

OK!
I added the default value to service account name.
d763a3d

@mugioka I got your point. I think that way is good as well.
But to be honest, I want to introduce the changes to end-users as little as possible.
For existing users, their deployment resource is not containing the serviceAccountName but after getting this release, their deployment resource will be updated to add serviceAccountName: default.
In most cases, there will be no difference between them but to be safe we should not introduce that change.

So I still prefer adding a check condition over specifying the "default" name as d763a3d.

WDYT?

In most cases, there will be no difference between them but to be safe we should not introduce that change.

I see, that makes sense.
I will add check statement.

I added the if statement.

Oh, sorry, an unnecessary file was committed.
I will fix it.

I fixed it.

nghialv · 2022-04-15T06:55:54Z

/trigger presubmits

pipecd-bot · 2022-04-15T06:55:58Z

@nghialv: Your requested presubmits has been scheduled in response to this comment.

nghialv · 2022-04-15T06:56:13Z

Nice improvement. Thank you.

/lgtm

pipecd-bot · 2022-04-15T07:01:57Z

Code coverage for golang is 35.56%. This pull request does not change code coverage.

khanhtc1202 · 2022-04-15T07:10:48Z

Thanks for contribution 🙌
/approve

pipecd-bot · 2022-04-15T07:10:51Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

pipecd-bot added do-not-merge/release-note-needed do-not-merge/cla-not-signed trigger/needs-ok-to-test area/manifests size/S labels Apr 14, 2022

pipecd-bot requested a review from khanhtc1202 April 14, 2022 17:13

pipecd-bot requested review from knanao and nghialv April 14, 2022 17:13

pipecd-bot added release-note-none and removed do-not-merge/release-note-needed labels Apr 14, 2022

pipecd-bot removed the do-not-merge/cla-not-signed label Apr 14, 2022

mugioka force-pushed the feat/be-able-to-create-dedicated-k8s-sa-for-pipecd-with-helm-chart branch from 6c510b6 to 61008aa Compare April 14, 2022 17:19

mugioka force-pushed the feat/be-able-to-create-dedicated-k8s-sa-for-pipecd-with-helm-chart branch from 61008aa to cb15699 Compare April 14, 2022 17:26

nghialv reviewed Apr 15, 2022

View reviewed changes

fix: piped/pipecd chart to be able to use existed k8s sa.

bc3ef54

Signed-off-by: mugioka <[email protected]>

pipecd-bot added size/M and removed size/S labels Apr 15, 2022

mugioka commented Apr 15, 2022

View reviewed changes

nghialv reviewed Apr 15, 2022

View reviewed changes

manifests/pipecd/templates/serviceaccount.yaml Outdated Show resolved Hide resolved

Update manifests/pipecd/templates/serviceaccount.yaml

5084a6b

Co-authored-by: Le Van Nghia <[email protected]>

nghialv changed the title ~~feat: be able to create the dedicated k8s sa for pipecd server with helm chart.~~ Allow customizing the ServiceAccount for workload of PipeCD Helm chart Apr 15, 2022

pipecd-bot removed the release-note-none label Apr 15, 2022

nghialv reviewed Apr 15, 2022

View reviewed changes

fix: change default values from true to false.

3c7440e

nghialv reviewed Apr 15, 2022

View reviewed changes

mugioka added 2 commits April 15, 2022 15:13

feat: add default value.

d763a3d

feat: add if statement.

1d38878

mugioka force-pushed the feat/be-able-to-create-dedicated-k8s-sa-for-pipecd-with-helm-chart branch from ca3f87a to 1d38878 Compare April 15, 2022 06:46

pipecd-bot added the lgtm label Apr 15, 2022

pipecd-bot added the approved label Apr 15, 2022

pipecd-bot merged commit 56cfdb4 into pipe-cd:master Apr 15, 2022

mugioka deleted the feat/be-able-to-create-dedicated-k8s-sa-for-pipecd-with-helm-chart branch April 15, 2022 07:14

pipecd-bot mentioned this pull request May 10, 2022

Remove seal key secret management type from piped configuration #3603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow customizing the ServiceAccount for workload of PipeCD Helm chart #3535

Allow customizing the ServiceAccount for workload of PipeCD Helm chart #3535

mugioka commented Apr 14, 2022 •

edited by nghialv

Loading

pipecd-bot commented Apr 14, 2022

mugioka commented Apr 14, 2022

pipecd-bot commented Apr 14, 2022

khanhtc1202 commented Apr 15, 2022

khanhtc1202 commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

nghialv Apr 15, 2022

nghialv Apr 15, 2022

nghialv commented Apr 15, 2022

mugioka left a comment

mugioka Apr 15, 2022

nghialv Apr 15, 2022

mugioka Apr 15, 2022

nghialv commented Apr 15, 2022

nghialv Apr 15, 2022

mugioka Apr 15, 2022

nghialv Apr 15, 2022

mugioka Apr 15, 2022

nghialv Apr 15, 2022

mugioka Apr 15, 2022

mugioka Apr 15, 2022

mugioka Apr 15, 2022

nghialv commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

nghialv commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

khanhtc1202 commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

Allow customizing the ServiceAccount for workload of PipeCD Helm chart #3535

Allow customizing the ServiceAccount for workload of PipeCD Helm chart #3535

Conversation

mugioka commented Apr 14, 2022 • edited by nghialv Loading

pipecd-bot commented Apr 14, 2022

mugioka commented Apr 14, 2022

pipecd-bot commented Apr 14, 2022

khanhtc1202 commented Apr 15, 2022

khanhtc1202 commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nghialv commented Apr 15, 2022

mugioka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nghialv commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nghialv commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

nghialv commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

khanhtc1202 commented Apr 15, 2022

pipecd-bot commented Apr 15, 2022

mugioka commented Apr 14, 2022 •

edited by nghialv

Loading