Helm: sanitize resource name lengths

[mruoss]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR addresses a but in the Helm chart. Some of the resources generated by the Helm chart are very long. Combined with the release name, they can easily get too long.

Example

Let's look at the resource {{ include "kueue.fullname" . }}-controller-manager-metrics-service of kind: Service. When generated for a somewhat long release name like project-foo-cluster-bar-kueue, the resulting resource name is project-foo-cluster-bar-kueue-controller-manager-metrics-service which is 65 characters long and we get the following error when trying to apply/install:

Service "project-foo-cluster-bar-kueue-controller-manager-metrics-service" is invalid: metadata.name: Invalid value: "project-foo-cluster-bar-kueue-controller-manager-metrics-service": must be no more than 63 characters

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE (it only affects names for resources that would have been too long and therefore failed anyway. Existing resources remain unchanged)

Helm: Sanitize resource names by truncating them to a maximum length of 63 characters

[linux-foundation-easycla]

• ✅login: mruoss / (516f0b2)

The committers listed above are authorized under a signed CLA.

[k8s-ci-robot]

Welcome @mruoss!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kueue has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @mruoss. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	516f0b2
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6710b77ded65cf00084c000a
😎 Deploy Preview	https://deploy-preview-3250--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mimowo]

LGTM, but I would prefer to also give it a pass by @mbobrovskyi or @tenzen-y

[tenzen-y]

/ok-to-test

[tenzen-y]

Thank you!
/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 349bf9f82635964031b89b1094f32c5883f92094

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mruoss, tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mbobrovskyi]

/hold

I tried to test with "project-foo-cluster-bar-kueue-controller-manager-metrics-service" and it's not working.

Error: INSTALLATION FAILED: clusterroles.rbac.authorization.k8s.io "kueue-project-foo-cluster-bar-kueue-controller-manager-metrics" already exists

[mbobrovskyi]

I think we need to add some e2e test case for helm.

@mimowo @tenzen-y WDYT?

[mimowo]

+1 on e2e tests, feel free to open the issue and follow up. I guess they could have a dedicated run like mk, but we can discuss details later.

We often have issues with helm which are hard to see just by static code analysis. Thanks for checking.

[mruoss]

I tried to test with "project-foo-cluster-bar-kueue-controller-manager-metrics-service" and it's not working.

Yeah okay, if the release name itself is longer than 63 chars, all resources are named the same (first 63 chars of the release name). I haven't seen any chart accounting for that, though. But one could change the order (start with the specific resource name and suffix it with the release name). Or obviously: truncate the release name to a max length instead of the resulting resource name...

I think we need to add some e2e test case for helm.

Agreed. Especially these bulk search-and-replace fixes don't come without risks... Chart tests would however already be a start.

[mbobrovskyi]

Or obviously: truncate the release name to a max length instead of the resulting resource name...

We already do that:

kueue/charts/kueue/templates/_helpers.tpl

Lines 8 to 24 in 9332a5a

	{{/*
	Create a default fully qualified app name.
	We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
	If release name contains chart name it will be used as a full name.
	*/}}
	{{- define "kueue.fullname" -}}
	{{- if .Values.fullnameOverride }}
	{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
	{{- else }}
	{{- $name := default .Chart.Name .Values.nameOverride }}
	{{- if contains $name .Release.Name }}
	{{- .Release.Name | trunc 63 | trimSuffix "-" }}
	{{- else }}
	{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
	{{- end }}
	{{- end }}
	{{- end }}

[mruoss]

Or obviously: truncate the release name to a max length instead of the resulting resource name...

We already do that https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/templates/_helpers.tpl#L21.

I see. But it should be truncated to a length that is 63 - charlength(longsest_suffix) if you know what I mean. This would make the changes in this PR redundant. But it's harder to maintain.

[mbobrovskyi]

Or obviously: truncate the release name to a max length instead of the resulting resource name...

We already do that https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/templates/_helpers.tpl#L21.

I see. But it should be truncated to a length that is 63 - charlength(longsest_suffix) if you know what I mean. This would make the changes in this PR redundant. But it's harder to maintain.

Can we calculate charlength(longsest_suffix)?

[mruoss]

Can we calculate charlength(longsest_suffix)?

I don't think you can do that globally in _helpers.tpl. You could probably do it in each resource's template...

[mbobrovskyi]

Can we calculate charlength(longsest_suffix)?

I don't think you can do that globally in _helpers.tpl. You could probably do it in each resource's template...

We can probably calculate it in update-helm.sh when generating the templates, and then update longest_suffix_length in _helpers.tpl.

[alculquicondor]

I'm not convinced that we should be doing this.
For one, it's a lot more (and repetitive) code to maintain. Maybe you can submit a PR to helm to support a method called resource-name-sanitize (or something along those lines) that can be used in templates.
Second, a user might still come and put invalid characters in their value.yaml file and not be able to install Kueue.

So I would categorize this as user-error unless helm provides some better mechanism to prevent this issues.

[alculquicondor]

would this actually work?

Doesn't it always have to finish in .svc or .svc.{ClusterDomain}?

Do we need to worry about length for dns names?

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

@mruoss: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kueue-verify-main	516f0b2	link	true	/test pull-kueue-verify-main
pull-kueue-test-tas-e2e-main	516f0b2	link	true	/test pull-kueue-test-tas-e2e-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.