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

chore: remove 16 characters from -service-account #2567

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

kahirokunn
Copy link
Contributor

The service account must be less than 63 characters. Currently, a normal install will result in ${INSTALLATION_NAME}-gha-runner-scale-set-kube-mode-service-account and 47 characters are reserved.
This is too long, so we will remove 16 characters from -service-account

The service account must be less than 63 characters.
Currently, a normal install will result in ${INSTALLATION_NAME}-gha-runner-scale-set-kube-mode-service-account
and 47 characters are reserved.
This is too long, so we will remove 16 characters from -service-account

Signed-off-by: kahirokunn <okinakahiro@gmail.com>
@Link- Link- added needs triage Requires review from the maintainers community Community contribution gha-runner-scale-set Related to the gha-runner-scale-set mode labels May 8, 2023
@kahirokunn
Copy link
Contributor Author

@Link- How about this?

@bjw-s
Copy link

bjw-s commented May 18, 2023

I'm also quite interested in seeing this being reviewed/implemented. The current limitation forces my ARC runner deployments to have super short names since otherwise the SA name will become too long. Alternatively, allowing us to set a fullnameOverride (as the default helm create template also implements) could help a lot with this in a more generic way.

@nikola-jokic
Copy link
Member

nikola-jokic commented May 18, 2023

@Link- this is a good suggestion (in my opinion) that we might want to apply to all resources.
When you search for the role, you know that the kind is Role. We don't really need a suffix in the name.
If you are troubleshooting, what I would usually do is kubectl get roles. The -role or -service-account suffix does not really provide much value, but it limits the name used as a label

@Link-
Copy link
Collaborator

Link- commented May 18, 2023

@nikola-jokic alright, let's take this on. It's on our board.

@Link- Link- added attention Requires attention and removed needs triage Requires review from the maintainers labels May 18, 2023
@kahirokunn
Copy link
Contributor Author

I need this 👍

@nikola-jokic
Copy link
Member

Hey @kahirokunn,

The change seems good to me, but can you help me understand where is this causing you problems? I'm trying to reproduce an issue where the name of the service account is going to cause problems, so I registered the scale set with the name length of 45 characters. The no-permission service account did not cause any troubles there since we used its name as an annotation in cleanup (not having the 63 character limit), and the name of the resource has the 253 characters in the form of the DNS name

@kahirokunn
Copy link
Contributor Author

@nikola-jokic
The problem lies in this link.
cilium/cilium#16579 (comment)
cilium is something that many vendors, such as GKE, provide by default and share.
But this is not just an issue on the cilium side, it should be able to be adjusted to not exceed the dns rule of 63 characters.

@nikola-jokic
Copy link
Member

Oh, I see, thanks!

Then we might want to consider different naming conventions that will allow shorter names (@Link-)
I updated your branch to trigger chart tests but for now. I think that we should merge this change before a new release. But it seems like the naming limitation of 45 characters is not correct for situations like this one

Copy link
Member

@nikola-jokic nikola-jokic left a comment

Choose a reason for hiding this comment

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

LGTM

@nikola-jokic nikola-jokic merged commit 78a9356 into actions:master Jun 27, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention Requires attention community Community contribution gha-runner-scale-set Related to the gha-runner-scale-set mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants