Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix service account creation when no Values.serviceAccount.name is defined #562

Merged

Conversation

luong-komorebi
Copy link
Contributor

What was changed

Instead of checking for serviceAccount.name inside a helper template, I rely on serviceAccount.createand ensure a service account is always used instead of the service account default

Why?

Given a yaml values:

serviceAccount:
  create: true

Without this change, a service account is created with the default name (which is taken from release full name as documented in values yaml ) but this new service account will never be used because the helper template will never refer to it

Instead all the pods created by temporal will always use serviceAccountName: default and default will come with no configuration, such as annotations that people use on popular k8s platform like eks

              extraAnnotations:
                eks.amazonaws.com/role-arn: <some arn here>

Just because of a missing serviceaccount name, any other configuration for service account will never be applied

Extra context

I figured out this issue when trying to debug why archivals did not work and using temporal operator to enable archivals is giving back 403 in the server log
After enabling debugging log for archival s3store, I found

temporal-frontend -----------------------------------------------------
temporal-frontend 2024/09/24 08:18:47 DEBUG: Response s3/HeadBucket Details:
temporal-frontend ---[ RESPONSE ]--------------------------------------
temporal-frontend HTTP/1.1 403 Forbidden
temporal-frontend Connection: close
temporal-frontend Content-Type: application/xml
temporal-frontend Date: Tue, 24 Sep 2024 08:18:46 GMT
temporal-frontend Server: AmazonS3
temporal-frontend X-Amz-Bucket-Region: us-west-2
temporal-frontend X-Amz-Id-2: <stripped>
temporal-frontend X-Amz-Request-Id: GEQS26YQT7Q0WSCM
temporal-frontend
temporal-frontend
temporal-frontend -----------------------------------------------------

which hints at the permission issue with the role.

Checklist

  1. Closes [Feature Request] [Archival] [s3 provider] use IAM role + serviceAccount #464

  2. How was this tested: Manually on my platform. New pods will be created with

      serviceAccount: temporal
      serviceAccountName: temporal
  1. Any docs updates needed? No

@luong-komorebi luong-komorebi requested a review from a team as a code owner September 24, 2024 08:48
@CLAassistant
Copy link

CLAassistant commented Sep 24, 2024

CLA assistant check
All committers have signed the CLA.

@@ -42,7 +42,7 @@ Create the name of the service account
Define the service account as needed
*/}}
{{- define "temporal.serviceAccount" -}}
{{- if .Values.serviceAccount.name -}}
{{- if .Values.serviceAccount.create -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but we need to still use the service account if create is false but name is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed another commit but it will drastically changed from the original direction.
Based on your comment and how the repo has actually worked, I am now creating the serviceaccount as the default (no more create flag)
If no name is defined, then we will referred to the default naming scheme as we noted to the user
If a name is defined, then we will use that name

Let me know what you think @robholland

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents users from using a pre-made service account. Helm will fail trying to create the serviceAccount that already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robholland thanks for the feedback, I did not account for premade service account because they were not referenced anyway

I just moved the conditional check in a different area to support both

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand your fix. Now we might create a serviceAccount called "default". I don't understand the reasoning there. Please see if #574 fixes the behaviour you needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. I left a comment at #574 to further explain. @robholland
If it remains unclear, feel free to close this and move forward with your fix since it still acheives the same goal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we might create a serviceAccount called "default"

correction here: we dont create this serviceaccount. We just reference to it, in an explicit way.
https://github.com/temporalio/helm-charts/blob/main/charts/temporal/templates/serviceaccount.yaml is already guarded by the same check {{- if .Values.serviceAccount.create -}}

@robholland robholland added the needs revision Team has requested some changes label Sep 25, 2024
Copy link
Contributor

@robholland robholland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your explanation and patience :) This all makes sense to me now, I've tried to tweak the comment a little, but otherwise looks good!

charts/temporal/values.yaml Outdated Show resolved Hide resolved
@robholland robholland removed the needs revision Team has requested some changes label Oct 3, 2024
@robholland robholland merged commit f6e1903 into temporalio:main Oct 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants