fix(server): correct Kubernetes label key length validation#398
Merged
Pangjiping merged 1 commit intoalibaba:mainfrom Mar 11, 2026
Merged
Conversation
The previous implementation of _is_valid_label_key checked whether the total key length exceeded 253 characters. This incorrectly rejected valid label keys where the prefix portion was within the allowed 253-char limit but the combined prefix+"/"+name exceeded 253 characters. According to Kubernetes label key constraints: - The optional prefix must be a DNS subdomain with at most 253 characters - The name segment must be at most 63 characters - There is no constraint on the combined total length (max: 253+1+63=317) This commit removes the total-length check and replaces it with a direct prefix-length check, which matches the Kubernetes specification. Regression coverage is added for: - Valid key rejected by the old check (prefix=251 chars, total>253) - Prefix that exceeds 253 chars (should be rejected) - Invalid prefix format - Name that exceeds 63 chars - Value that exceeds 63 chars - Non-string metadata keys - Key with empty prefix (starts with '/')
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
Fixes Kubernetes label key validation to correctly allow label keys whose combined prefix/name length exceeds 253 characters, while still enforcing the prefix (DNS subdomain) ≤ 253 and name ≤ 63.
Changes:
- Remove incorrect total-length guard from
_is_valid_label_keyand validate prefix length explicitly. - Add regression tests covering long-but-valid prefixed keys and multiple invalid key/value cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/src/services/validators.py | Corrects label key validation logic to align with Kubernetes spec (prefix length vs total key length). |
| server/tests/test_validators.py | Adds regression tests for long prefix/name combinations and invalid metadata label inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Thanks for fixes. Please sign CLA and I will review and merge those changes as soon as possible. |
Contributor
Author
|
@Pangjiping already signed! |
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 free
to 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.
Summary
The
_is_valid_label_keyfunction inserver/src/services/validators.pyincorrectly rejected valid Kubernetes label keys when the total key length (prefix +/+ name) exceeded 253 characters — even if the prefix alone was within the allowed 253-character limit.Root Cause
The old guard condition:
was evaluated (due to Python operator precedence) as:
The first clause
len(key) > 253applied to all keys, including prefixed ones. Because of this, a key whose prefix was valid (≤ 253 chars) but whose combined length exceeded 253 would be incorrectly rejected.Fix
Per the Kubernetes label key specification:
The fix removes the total-length check and uses an explicit
len(prefix) > 253check instead.Testing
All existing tests continue to pass. New regression tests are added for:
Checklist