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

Proposal: remove -serviceaccount suffix from KSA names in helm chart #10892

Merged
merged 9 commits into from
Sep 15, 2020
Merged

Proposal: remove -serviceaccount suffix from KSA names in helm chart #10892

merged 9 commits into from
Sep 15, 2020

Conversation

jaketf
Copy link
Contributor

@jaketf jaketf commented Sep 12, 2020

It's quite annoying to have -serviceaccount in each service account name as this is a useless 15 characters that provides no additional information about the account's purpose.

"why do you care so much about naming convention Jake?"
Actually there is a practical reason: GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.
If i have a simple release name like airflow I already violate the 30 char limit with airflow-scheduler-serviceaccount (32 chars)

R: @ashb
CC: @potiuk @mik-laj (in our project I'm just going to truncate GSA names to 30 chars as I imagine we may get push back on a PR like this which could have adverse affects for others in the community who may rely on the current SA naming convention)

Jacob Ferriero added 9 commits September 11, 2020 17:07
It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.
@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Sep 12, 2020
@ashb
Copy link
Member

ashb commented Sep 12, 2020

I'm okay with this change; are there any adverse effects when upgrading from before to after?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. @dimberman ?

@potiuk
Copy link
Member

potiuk commented Sep 12, 2020

I'm okay with this change; are there any adverse effects when upgrading from before to after?

There are known problems with Helm Updating various resources in Kubernetes. Particularly Service Account name upgrade will not work it seems helm/helm#5473 and helm/helm#4429

Seems there is an open "umbrella" issue to make this work for different kinds of resources that helm manages in Kubernetes: helm/helm#5915

If we had the chart released - that would be a breaking change. I'd say definitely when we release it, we should have a separate chart/UPDATING.md or smth, and this change should be explained there IMHO. But for now we do not even have a version that we can refer to as "when it changed" :). So the question is - do we want to start keeping track of those and other similar changes already or do we start after releasing it for the first time?

@mik-laj
Copy link
Member

mik-laj commented Sep 14, 2020

@schnie @dimberman Can you take a look at it? This helps us to create a secure environment on GKE. If we use Workload Identity, then we don't have to use long-lived credentials keys.

Copy link
Contributor

@schnie schnie left a comment

Choose a reason for hiding this comment

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

Looks good to me, makes sense!

@mik-laj
Copy link
Member

mik-laj commented Sep 15, 2020

do we want to start keeping track of those and other similar changes already or do we start after releasing it for the first time?

Only after the first version is released, we can maintain the changelog. Now we are in a phase of intense development, so breaking changes are natural. We don't maintain semver or any other versioning, so maintaining this type of documentation would be difficult.

@mik-laj mik-laj merged commit 23768f6 into apache:master Sep 15, 2020
potiuk pushed a commit that referenced this pull request Nov 15, 2020
…10892)

* [WIP] remove -serviceaccount suffix in helm chart

It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

(cherry picked from commit 23768f6)
@potiuk potiuk added this to the Airflow 1.10.13 milestone Nov 15, 2020
@potiuk potiuk added the type:improvement Changelog: Improvements label Nov 15, 2020
potiuk pushed a commit that referenced this pull request Nov 16, 2020
…10892)

* [WIP] remove -serviceaccount suffix in helm chart

It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

(cherry picked from commit 23768f6)
potiuk pushed a commit that referenced this pull request Nov 16, 2020
…10892)

* [WIP] remove -serviceaccount suffix in helm chart

It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

(cherry picked from commit 23768f6)
kaxil pushed a commit that referenced this pull request Nov 18, 2020
…10892)

* [WIP] remove -serviceaccount suffix in helm chart

It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

(cherry picked from commit 23768f6)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
…pache#10892)

* [WIP] remove -serviceaccount suffix in helm chart

It's quite annoying to have `-serviceaccount` in each service account name as this is a useless 15 characters that provides no additional information.
"why is this so frustrating to you Jake?"
GCP service accounts have 30 char name limit https://cloud.google.com/iam/docs/creating-managing-service-accounts#creating
For manageability / clarity I'd like to keep KSA and GSA names exactly the same when using workload identity which maps KSA<>GSA 1:1 https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity.

(cherry picked from commit 23768f6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants