fix(server): correct Kubernetes label key length validation#398
fix(server): correct Kubernetes label key length validation#398zerone0x wants to merge 1 commit intoalibaba:mainfrom
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.
| """Label prefix (DNS subdomain) exceeding 253 characters should be rejected.""" | ||
| # Build a prefix that is longer than 253 chars: 5 labels of 62 chars = 314 chars | ||
| label_part = "a" * 62 | ||
| long_prefix = ".".join([label_part] * 5) # 62*5 + 4 = 314 chars |
There was a problem hiding this comment.
Consider adding a direct assertion for the constructed prefix length (e.g., assert len(long_prefix) > 253 or == 314). It makes the intent of the test more self-documenting and guards against accidental edits to the construction logic.
| long_prefix = ".".join([label_part] * 5) # 62*5 + 4 = 314 chars | |
| long_prefix = ".".join([label_part] * 5) # 62*5 + 4 = 314 chars | |
| assert len(long_prefix) == 314 |
| with pytest.raises(HTTPException) as exc_info: | ||
| ensure_metadata_labels({long_name: "value"}) | ||
| assert exc_info.value.status_code == 400 | ||
| assert exc_info.value.detail["code"] == SandboxErrorCodes.INVALID_METADATA_LABEL |
There was a problem hiding this comment.
Several new tests repeat the same pytest.raises(HTTPException) + status_code + detail['code'] assertions. Consider consolidating the invalid cases into a single @pytest.mark.parametrize test to reduce duplication and make it easier to extend the matrix of invalid inputs over time.
|
|
|
Thanks for fixes. Please sign CLA and I will review and merge those changes as soon as possible. |
|
@Pangjiping already signed! |
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