Uh oh!
There was an error while loading. Please reload this page.
fix: make _dnsify honor the 63-character DNS_LABEL contract - #3466
Open
Daksha1611 wants to merge 1 commit into
Open
fix: make _dnsify honor the 63-character DNS_LABEL contract#3466Daksha1611 wants to merge 1 commit into
Daksha1611 wants to merge 1 commit into
Conversation
`_dnsify` documents that its result is a valid Kubernetes DNS_LABEL: at most 63 characters, lower-case alphanumerics and '-', never leading or trailing with '-'. Two bugs let it return values that are neither. The hash-and-truncate step ran before the character conversion, so the cap was enforced on the input rather than on the result. The conversion grows the string -- every upper-case character has a '-' inserted before it -- so a 62-character camelCase name passed the input check and came out at 70 characters. The `len(res) < 62` guards did not help: they only suppressed the separator, while the character itself was still appended, so `res` kept growing past 63. They also dropped separators from the tail of the name, turning `...SweepStage` into `...sweepstage`. Separately, each of '_', '-' and '.' appended a '-' without checking whether the previous character was already one, and only a single trailing '-' was stripped, so two adjacent separators at the end of a name produced a label ending in '-'. Enforce the length on the converted value instead, keeping the hash over the original input so distinct values sharing a suffix still do not collide; collapse runs of separators; and strip every trailing '-'. `_dnsify` is applied to every node ID in `tools/translator.py`, to `node_name` overrides in `core/node.py`, and to eager execution names in `core/worker_queue.py`, and nothing downstream re-validates the result. Fixesflyteorg/flyte#7994 Signed-off-by: Daksha1611 <mehtadaksha1611@gmail.com>
Daksha1611
requested review from
cosmicBboy, davidmirror-ops, kumare3, machichima, pingsutw, samhita-alla and wild-endeavor
as code ownersSeptember 7, 2026 06:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking issue
Closesflyteorg/flyte#7994
Why are the changes needed?
flytekit.core.utils._dnsifyis the only place flytekit enforces the Kubernetes DNS_LABEL contract. Its docstring states the result "must only consist of alphanumeric (lower-case a-z, and 0-9) and not exceed 63 characters. It's permitted to have '-' character as long as it's not in the first or last positions." It violates both halves of that.1. The 63-character cap was applied to the input, not the output. The hash-and-truncate step ran before the character conversion, but the conversion grows the string — every upper-case character has a
-inserted before it. A 62-character camelCase name passed the input check and came out at 70 characters:The
len(res) < 62guards did not bound anything: they only suppressed the separator, while the character itself was still appended unconditionally. They also silently corrupted the tail of the name — notesweepstageabove, where the separator beforeStagewas dropped.2. Consecutive separators could leave a trailing
-. Each of_,-,.appended a-without checking whether the previous character was already one, and only a single trailing-was stripped, so_dnsify("test..")returned'test-'— not a valid DNS_LABEL._dnsifyis applied to every node ID inflytekit/tools/translator.py, tonode_nameoverrides inflytekit/core/node.py, and to eager execution names inflytekit/core/worker_queue.py. Nothing downstream re-validates length or shape, so an over-long or trailing--identifier is serialized into the workflow spec and registered as-is.What changes were proposed in this pull request?
In
flytekit/core/utils.py:"t" * 64 -> "da4b348ebe-" + "t" * 52behaviour is unchanged._,-and.no longer append a-when the result already ends in one.-rather than just one, and drop a leading-from the truncated tail so the hashed form does not read ashash--tail.len(res) < 62guards, which only dropped separators without bounding the length.Only inputs whose converted form reaches 63 characters change shape, and those were already producing invalid identifiers. Names that convert to 62 characters or fewer are untouched.
How was this patch tested?
tests/flytekit/unit/core/test_utils.py:test_dnsifyparametrization for consecutive separators ("test..","test_-","my_task-.name..").test_dnsify_is_a_valid_dns_label— asserts the output is<= 63characters and matches^[a-z0-9]([-a-z0-9]*[a-z0-9])?$for nine representative inputs (camelCase, all-upper, alternating case, separator-only).test_dnsify_node_name_override_is_a_valid_dns_label— end-to-end regression through the public API, checking the node ID produced bywith_overrides(node_name=...).All ten pre-existing
test_dnsifycases pass unchanged. Twelve of the new assertions fail onmasterand pass with this change.Setup process
Full
tests/flytekit/unit/core+tests/flytekit/unit/test_translator.pyrun before and after: identical failure set (33 pre-existing, environment-related failures on both sides), 1407 -> 1420 passing, no new failures.Additionally, a fuzz sweep over 300k random inputs drawn from
[A-Za-z0-9_-.$ ](lengths 0–200) produces 431 outputs that are not valid DNS_LABELs onmasterand 0 with this change, with a maximum output length of 63.Check all the applicable boxes
_dnsifydocstring already states the contract; this makes the code match it.)