Skip to content

fix(kuberay): preserve useful characters when truncating K8s label values#356

Merged
danielgafni merged 1 commit into
danielgafni:mainfrom
anam-org:peter/fix-k8s-label-truncation-order
Apr 23, 2026
Merged

fix(kuberay): preserve useful characters when truncating K8s label values#356
danielgafni merged 1 commit into
danielgafni:mainfrom
anam-org:peter/fix-k8s-label-truncation-order

Conversation

@peterroelants

@peterroelants peterroelants commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Problem

normalize_k8s_label_values strips leading/trailing non-alphanumeric chars before truncating to 63. For long inputs (e.g. a branch name stuck into dagster/git-branch), the 63rd character can be _, -, or ., which fails the K8s label-value regex (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?. This breaks RayClusterResource creation with HTTP 422, cascading into a 404 at cleanup.

Fix

Split the leading/trailing strip into two steps around the truncation:

  1. Strip leading non-alphanumerics — so a leading _ / . run doesn't eat the 63-char budget on characters we'd immediately strip anyway.
  2. Truncate to 63.
  3. Strip trailing non-alphanumerics — handles whatever character truncation lands on.

Then validate the result against the K8s label-value regex and raise if it doesn't match, so any future regression in this function surfaces at normalize time instead of at the K8s API.

Tests

  • The existing snapshot test now also checks every output against the K8s label-value regex.
  • test_normalize_k8s_label_values_truncated_tail_is_alphanumeric is extended with long leading _ / . runs and the §-style leading-garbage case from the review.
  • New test_normalize_k8s_label_values_preserves_useful_tail_after_leading_garbage pins the "useful tail survives" property.
  • New test_normalize_k8s_label_values_rejects_unexpectedly_invalid_output patches the trailing-strip to a no-op and asserts the K8s-regex validation raises.

@danielgafni

danielgafni commented Apr 23, 2026

Copy link
Copy Markdown
Owner

@peterroelants could you please update the PR description to something more concise (or I could do it if you're ok with it)

I'm a bit hesitant about this fix since the initial truncation may drop a lot of useful characters. Imagine this input string: §§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§§<x63>-finally-something-useful. It would not work with the new order of doing things.

If the problem is with just the very last character then perhaps we could fix just it? It seems like how we handle it with _LEADING_TRAILING_NON_ALNUM_PATTERN is the problem in the first place.

We should also update our test suite with the correct k8s regexp to prevent this kind of problems in the future. Ideally if we verify regex matching at label creation time as well.

@peterroelants
peterroelants force-pushed the peter/fix-k8s-label-truncation-order branch from 7b7eb9e to 0a27101 Compare April 23, 2026 10:51
@peterroelants peterroelants changed the title fix(kuberay): strip trailing non-alphanumeric chars after truncating K8s label values fix(kuberay): preserve useful characters when truncating K8s label values Apr 23, 2026
…lues

`normalize_k8s_label_values` stripped leading/trailing non-alphanumeric
characters before truncating to 63 chars. For long inputs (e.g. git
branch names used as `dagster/git-branch`-style labels), truncation
could then land on `_`, `-`, or `.` and the resulting label would fail
K8s's label-value regex `(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?`,
causing RayCluster creation to fail with HTTP 422.

Split the leading/trailing strip into two steps around the truncation:
strip leading non-alphanumerics (so a leading `_`/`.` run doesn't eat
the 63-char budget), truncate to 63, then strip trailing
non-alphanumerics (handling whatever character truncation lands on).
Also validate the final value against the K8s label-value regex and
raise if it doesn't match, so future regressions in this function
surface at normalize time instead of at the K8s API.

Extend the tests with leading `_`/`.` runs and the §-style
leading-garbage case from the PR review, assert the K8s label-value
regex against every normalized output, and cover the validation path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@peterroelants
peterroelants force-pushed the peter/fix-k8s-label-truncation-order branch from 0a27101 to 3ae2736 Compare April 23, 2026 11:08
@peterroelants

Copy link
Copy Markdown
Contributor Author

@danielgafni I should've addressed your comments

@danielgafni danielgafni left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks!

@peterroelants

Copy link
Copy Markdown
Contributor Author

Thanks!

Great, you're welcome.

Btw, I don't think I have merge permissions on this, so I'll leave merging this in up to you.

@danielgafni
danielgafni merged commit 9c236d7 into danielgafni:main Apr 23, 2026
17 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.

2 participants