KEP-4671: Add Declarative Validation to Workload API#135164
Merged
k8s-ci-robot merged 14 commits intokubernetes:masterfrom Jan 16, 2026
Merged
KEP-4671: Add Declarative Validation to Workload API#135164k8s-ci-robot merged 14 commits intokubernetes:masterfrom
k8s-ci-robot merged 14 commits intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
area/apiserver
area/code-generation
area/kubectl
area/test
kind/api-change
k8s-ci-robot
added
sig/cli
k8s-ci-robot
added
the
sig/testing
thockin
reviewed
Nov 6, 2025
Member
thockin
left a comment
thockin
left a comment
There was a problem hiding this comment.
I didn't go into depth, but it looks on the right track!
@lalitc375 @aaron-prindle @yongruilin
The way I would ask this to be structured is probably:
- 1 commit for wiring it up and activating it, no tags
- N commits with either 1 tag per commit (e.g. all the optionals) or 1 field per commit or even 1 single field-tag per commit.
This makes it much easier to review the conversion - we can see the generated code and which manual changes you made to handwritten code, and can evaluate the test cases one by one. The goal is to end up with better tests which are easier to comprehend, but it's hard to review an omnibus commit.
Given how this is new API, we should aim for 100% coverage, or else some issues one what was missing to reach 100%
In 1.36 we may be able to ONLY do DV for this.1
yongruilin
reviewed
Nov 6, 2025
macsko
reviewed
Nov 6, 2025
Contributor
|
LGTM label has been added. DetailsGit tree hash: 46c18750893a6bb17caf67bd0515e6e49aef653d |
thockin
reviewed
Dec 11, 2025
Member
There was a problem hiding this comment.
Comments on tests.
One of the side-quests of DV is scrubbing string-format validations and consolidating to a single library. I prepared a commit to move the path-segment stuff. I can either followup on this PR with my own, or I can just get you to paste it into your PR as distinct commit, and call the newly named func instead of the old (content.IsPathSegmentName() vs path.IsValidPathSegmentName())
commit 6bd2283ae502135ebaea0fdaac42b3313869cdcf
Good "git" signature for [email protected] with ED25519 key SHA256:M2+rk0pn34LtCBIVneq+UkpS+MLUTLylIUjHPq6OGSE
Author: Tim Hockin <[email protected]>
Date: Wed Dec 10 18:43:09 2025 -0800
Move path-segment validation to pkg content
This is where all the "scrubbed" validation helpers are going.
Note: This does NOT check for "" or too-long inputs, and changing it now
would be a breaking change.
diff --git pkg/apis/admissionregistration/validation/validation.go pkg/apis/admissionregistration/validation/validation.go
index 815f51a497a..56ab3e5af92 100644
--- pkg/apis/admissionregistration/validation/validation.go
+++ pkg/apis/admissionregistration/validation/validation.go
@@ -24,8 +24,8 @@ import (
"sync"
"k8s.io/apimachinery/pkg/api/equality"
+ "k8s.io/apimachinery/pkg/api/validate/content"
genericvalidation "k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -948,7 +948,7 @@ func validateNamedRuleWithOperations(n *admissionregistration.NamedRuleWithOpera
var allErrors field.ErrorList
resourceNames := sets.NewString()
for i, rName := range n.ResourceNames {
- for _, msg := range path.ValidatePathSegmentName(rName, false) {
+ for _, msg := range content.IsPathSegmentName(rName) {
allErrors = append(allErrors, field.Invalid(fldPath.Child("resourceNames").Index(i), rName, msg))
}
if resourceNames.Has(rName) {
@@ -1202,7 +1202,7 @@ func validateParamRef(pr *admissionregistration.ParamRef, fldPath *field.Path) f
}
if len(pr.Name) > 0 {
- for _, msg := range path.ValidatePathSegmentName(pr.Name, false) {
+ for _, msg := range content.IsPathSegmentName(pr.Name) {
allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), pr.Name, msg))
}
diff --git pkg/apis/autoscaling/validation/validation.go pkg/apis/autoscaling/validation/validation.go
index 524212f53a1..de542aac63a 100644
--- pkg/apis/autoscaling/validation/validation.go
+++ pkg/apis/autoscaling/validation/validation.go
@@ -19,8 +19,8 @@ package validation
import (
"fmt"
+ "k8s.io/apimachinery/pkg/api/validate/content"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
- pathvalidation "k8s.io/apimachinery/pkg/api/validation/path"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -84,7 +84,7 @@ func ValidateCrossVersionObjectReference(ref autoscaling.CrossVersionObjectRefer
if len(ref.Kind) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("kind"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Kind) {
+ for _, msg := range content.IsPathSegmentName(ref.Kind) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), ref.Kind, msg))
}
}
@@ -92,7 +92,7 @@ func ValidateCrossVersionObjectReference(ref autoscaling.CrossVersionObjectRefer
if len(ref.Name) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Name) {
+ for _, msg := range content.IsPathSegmentName(ref.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), ref.Name, msg))
}
}
@@ -472,7 +472,7 @@ func validateMetricIdentifier(id autoscaling.MetricIdentifier, fldPath *field.Pa
if len(id.Name) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("name"), "must specify a metric name"))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(id.Name) {
+ for _, msg := range content.IsPathSegmentName(id.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), id.Name, msg))
}
}
diff --git pkg/apis/networking/validation/validation.go pkg/apis/networking/validation/validation.go
index 31e5b29a2f5..066171598a0 100644
--- pkg/apis/networking/validation/validation.go
+++ pkg/apis/networking/validation/validation.go
@@ -20,8 +20,8 @@ import (
"fmt"
"strings"
+ "k8s.io/apimachinery/pkg/api/validate/content"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
- pathvalidation "k8s.io/apimachinery/pkg/api/validation/path"
unversionedvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
@@ -605,7 +605,7 @@ func validateIngressTypedLocalObjectReference(params *api.TypedLocalObjectRefere
if params.Kind == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("kind"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(params.Kind) {
+ for _, msg := range content.IsPathSegmentName(params.Kind) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), params.Kind, msg))
}
}
@@ -613,7 +613,7 @@ func validateIngressTypedLocalObjectReference(params *api.TypedLocalObjectRefere
if params.Name == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(params.Name) {
+ for _, msg := range content.IsPathSegmentName(params.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), params.Name, msg))
}
}
@@ -785,7 +785,7 @@ func validateIPAddressParentReference(params *networking.ParentReference, fldPat
if params.Resource == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("resource"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(params.Resource) {
+ for _, msg := range content.IsPathSegmentName(params.Resource) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("resource"), params.Resource, msg))
}
}
@@ -794,14 +794,14 @@ func validateIPAddressParentReference(params *networking.ParentReference, fldPat
if params.Name == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(params.Name) {
+ for _, msg := range content.IsPathSegmentName(params.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), params.Name, msg))
}
}
// namespace is optional
if params.Namespace != "" {
- for _, msg := range pathvalidation.IsValidPathSegmentName(params.Namespace) {
+ for _, msg := range content.IsPathSegmentName(params.Namespace) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), params.Namespace, msg))
}
}
diff --git pkg/apis/rbac/validation/validation.go pkg/apis/rbac/validation/validation.go
index fcc672dbd2a..2de94a9749e 100644
--- pkg/apis/rbac/validation/validation.go
+++ pkg/apis/rbac/validation/validation.go
@@ -17,7 +17,7 @@ limitations under the License.
package validation
import (
- "k8s.io/apimachinery/pkg/api/validation/path"
+ "k8s.io/apimachinery/pkg/api/validate/content"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
unversionedvalidation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -30,7 +30,7 @@ import (
// * https://github.com/kubernetes/kubernetes/blob/60db507b279ce45bd16ea3db49bf181f2aeb3c3d/pkg/api/validation/name.go
// * https://github.com/openshift/origin/blob/388478c40e751c4295dcb9a44dd69e5ac65d0e3b/pkg/api/helpers.go
func ValidateRBACName(name string, prefix bool) []string {
- return path.IsValidPathSegmentName(name)
+ return content.IsPathSegmentName(name)
}
func ValidateRole(role *rbac.Role) field.ErrorList {
diff --git pkg/apis/scheduling/validation/validation.go pkg/apis/scheduling/validation/validation.go
index 6ca22364544..e824bec3b72 100644
--- pkg/apis/scheduling/validation/validation.go
+++ pkg/apis/scheduling/validation/validation.go
@@ -20,8 +20,8 @@ import (
"fmt"
"strings"
+ "k8s.io/apimachinery/pkg/api/validate/content"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
- pathvalidation "k8s.io/apimachinery/pkg/api/validation/path"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -101,14 +101,14 @@ func validateControllerRef(ref *scheduling.TypedLocalObjectReference, fldPath *f
if ref.Kind == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("kind"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Kind) {
+ for _, msg := range content.IsPathSegmentName(ref.Kind) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), ref.Kind, msg))
}
}
if ref.Name == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
} else {
- for _, msg := range pathvalidation.IsValidPathSegmentName(ref.Name) {
+ for _, msg := range content.IsPathSegmentName(ref.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), ref.Name, msg))
}
}
diff --git staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation.go staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation.go
index 569c1f257a7..ce4a6036d3f 100644
--- staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation.go
+++ staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation.go
@@ -20,8 +20,8 @@ import (
"strings"
structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
+ "k8s.io/apimachinery/pkg/api/validate/content"
metavalidation "k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
"k8s.io/apimachinery/pkg/runtime/schema"
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -75,6 +75,13 @@ func validate(pth *field.Path, x interface{}, s *structuralschema.Structural) fi
return allErrs
}
+func validatePathSegment(name string, prefix bool) []string {
+ if prefix {
+ return content.IsPathSegmentPrefix(name)
+ }
+ return content.IsPathSegmentName(name)
+}
+
func validateEmbeddedResource(pth *field.Path, x map[string]interface{}, s *structuralschema.Structural) field.ErrorList {
var allErrs field.ErrorList
@@ -112,7 +119,7 @@ func validateEmbeddedResource(pth *field.Path, x map[string]interface{}, s *stru
if len(meta.Name) == 0 {
meta.Name = "fakename" // we have to set something to avoid an error
}
- allErrs = append(allErrs, metavalidation.ValidateObjectMeta(meta, len(meta.Namespace) > 0, path.ValidatePathSegmentName, pth.Child("metadata"))...)
+ allErrs = append(allErrs, metavalidation.ValidateObjectMeta(meta, len(meta.Namespace) > 0, validatePathSegment, pth.Child("metadata"))...)
}
}
}
diff --git staging/src/k8s.io/apimachinery/pkg/api/validate/content/path.go staging/src/k8s.io/apimachinery/pkg/api/validate/content/path.go
new file mode 100644
index 00000000000..c41b1d47312
--- /dev/null
+++ staging/src/k8s.io/apimachinery/pkg/api/validate/content/path.go
@@ -0,0 +1,63 @@
+/*
+Copyright 2015 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package content
+
+import (
+ "fmt"
+ "strings"
+)
+
+// Strings that cannot be used as names specified as path segments (like the
+// REST API or etcd store).
+var pathSegmentNameMayNotBe = []string{".", ".."}
+
+// Substrings that cannot be used in names specified as path segments (like the
+// REST API or etcd store).
+var pathSegmentNameMayNotContain = []string{"/", "%"}
+
+// IsPathSegmentName validates the name can be safely encoded as a path
+// segment.
+//
+// Note that, for historical reason, this function does not check for
+// empty strings or impose a limit on the length of the input.
+func IsPathSegmentName(name string) []string {
+ for _, illegalName := range pathSegmentNameMayNotBe {
+ if name == illegalName {
+ return []string{fmt.Sprintf(`may not be '%s'`, illegalName)}
+ }
+ }
+
+ return IsPathSegmentPrefix(name)
+}
+
+// IsPathSegmentPrefix validates the name can be used as a prefix for a
+// name which will be encoded as a path segment It does not check for exact
+// matches with disallowed names, since an arbitrary suffix might make the name
+// valid.
+//
+// Note that, for historical reason, this function does not check for
+// empty strings or impose a limit on the length of the input.
+func IsPathSegmentPrefix(name string) []string {
+ var errors []string
+ for _, illegalContent := range pathSegmentNameMayNotContain {
+ if strings.Contains(name, illegalContent) {
+ errors = append(errors, fmt.Sprintf(`may not contain '%s'`, illegalContent))
+ }
+ }
+
+ return errors
+}
diff --git staging/src/k8s.io/apimachinery/pkg/api/validation/path/name_test.go staging/src/k8s.io/apimachinery/pkg/api/validate/content/path_test.go
similarity index 58%
rename from staging/src/k8s.io/apimachinery/pkg/api/validation/path/name_test.go
rename to staging/src/k8s.io/apimachinery/pkg/api/validate/content/path_test.go
index 42890527388..16a1119e820 100644
--- staging/src/k8s.io/apimachinery/pkg/api/validation/path/name_test.go
+++ staging/src/k8s.io/apimachinery/pkg/api/validate/content/path_test.go
@@ -14,155 +14,145 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
-package path
+package content
import (
"strings"
"testing"
)
-func TestValidatePathSegmentName(t *testing.T) {
+func TestIsPathSegmentPrefix(t *testing.T) {
testcases := map[string]struct {
Name string
- Prefix bool
ExpectedMsg string
}{
"empty": {
Name: "",
- Prefix: false,
+ ExpectedMsg: "", // NOTE: this probably should fail
+ },
+ // NOTE: no validation of max length
+ "valid short": {
+ Name: "foo",
ExpectedMsg: "",
},
- "empty,prefix": {
- Name: "",
- Prefix: true,
- ExpectedMsg: "",
- },
-
- "valid": {
+ "valid long": {
+ Name: "foo.bar.baz",
+ ExpectedMsg: "",
+ },
+ // Make sure mixed case, non DNS subdomain characters are tolerated
+ "valid complex": {
+ Name: "sha256:ABCDEF012345@ABCDEF012345",
+ ExpectedMsg: "",
+ },
+ // Make sure non-ascii characters are tolerated
+ "valid extended charset": {
+ Name: "Iñtërnâtiônàlizætiøn",
+ ExpectedMsg: "",
+ },
+ "dot": {
+ Name: ".",
+ ExpectedMsg: "",
+ },
+ "dot leading": {
+ Name: ".test",
+ ExpectedMsg: "",
+ },
+ "dot dot": {
+ Name: "..",
+ ExpectedMsg: "",
+ },
+ "dot dot leading": {
+ Name: "..test",
+ ExpectedMsg: "",
+ },
+ "slash": {
+ Name: "foo/bar",
+ ExpectedMsg: "/",
+ },
+ "percent": {
+ Name: "foo%bar",
+ ExpectedMsg: "%",
+ },
+ }
+
+ for k, tc := range testcases {
+ msgs := IsPathSegmentPrefix(tc.Name)
+ if len(tc.ExpectedMsg) == 0 && len(msgs) > 0 {
+ t.Errorf("%s: expected no error, got %v", k, msgs)
+ }
+ if len(tc.ExpectedMsg) > 0 && len(msgs) == 0 {
+ t.Errorf("%s: expected error, got none", k)
+ }
+ if len(tc.ExpectedMsg) > 0 && !strings.Contains(msgs[0], tc.ExpectedMsg) {
+ t.Errorf("%s: expected error containing %q, got %v", k, tc.ExpectedMsg, msgs[0])
+ }
+ }
+}
+
+func TestIsPathSegmentName(t *testing.T) {
+ testcases := map[string]struct {
+ Name string
+ ExpectedMsg string
+ }{
+ "empty": {
+ Name: "",
+ ExpectedMsg: "", // NOTE: this probably should fail
+ },
+ // NOTE: no validation of max length
+ "valid short": {
+ Name: "foo",
+ ExpectedMsg: "",
+ },
+ "valid long": {
Name: "foo.bar.baz",
- Prefix: false,
ExpectedMsg: "",
},
- "valid,prefix": {
- Name: "foo.bar.baz",
- Prefix: true,
- ExpectedMsg: "",
- },
-
// Make sure mixed case, non DNS subdomain characters are tolerated
"valid complex": {
Name: "sha256:ABCDEF012345@ABCDEF012345",
- Prefix: false,
ExpectedMsg: "",
},
// Make sure non-ascii characters are tolerated
"valid extended charset": {
Name: "Iñtërnâtiônàlizætiøn",
- Prefix: false,
ExpectedMsg: "",
},
-
"dot": {
Name: ".",
- Prefix: false,
ExpectedMsg: ".",
},
"dot leading": {
Name: ".test",
- Prefix: false,
ExpectedMsg: "",
},
- "dot,prefix": {
- Name: ".",
- Prefix: true,
- ExpectedMsg: "",
- },
-
"dot dot": {
Name: "..",
- Prefix: false,
ExpectedMsg: "..",
},
"dot dot leading": {
Name: "..test",
- Prefix: false,
ExpectedMsg: "",
},
- "dot dot,prefix": {
- Name: "..",
- Prefix: true,
- ExpectedMsg: "",
- },
-
"slash": {
Name: "foo/bar",
- Prefix: false,
ExpectedMsg: "/",
},
- "slash,prefix": {
- Name: "foo/bar",
- Prefix: true,
- ExpectedMsg: "/",
- },
-
"percent": {
Name: "foo%bar",
- Prefix: false,
- ExpectedMsg: "%",
- },
- "percent,prefix": {
- Name: "foo%bar",
- Prefix: true,
ExpectedMsg: "%",
},
}
for k, tc := range testcases {
- msgs := ValidatePathSegmentName(tc.Name, tc.Prefix)
+ msgs := IsPathSegmentName(tc.Name)
if len(tc.ExpectedMsg) == 0 && len(msgs) > 0 {
- t.Errorf("%s: expected no message, got %v", k, msgs)
+ t.Errorf("%s: expected no error, got %v", k, msgs)
}
if len(tc.ExpectedMsg) > 0 && len(msgs) == 0 {
- t.Errorf("%s: expected error message, got none", k)
+ t.Errorf("%s: expected error, got none", k)
}
if len(tc.ExpectedMsg) > 0 && !strings.Contains(msgs[0], tc.ExpectedMsg) {
- t.Errorf("%s: expected message containing %q, got %v", k, tc.ExpectedMsg, msgs[0])
- }
- }
-}
-
-func TestValidateWithMultiErrors(t *testing.T) {
- testcases := map[string]struct {
- Name string
- Prefix bool
- ExpectedMsg []string
- }{
- "slash,percent": {
- Name: "foo//bar%",
- Prefix: false,
- ExpectedMsg: []string{"may not contain '/'", "may not contain '%'"},
- },
- "slash,percent,prefix": {
- Name: "foo//bar%",
- Prefix: true,
- ExpectedMsg: []string{"may not contain '/'", "may not contain '%'"},
- },
- }
-
- for k, tc := range testcases {
- msgs := ValidatePathSegmentName(tc.Name, tc.Prefix)
- if len(tc.ExpectedMsg) == 0 && len(msgs) > 0 {
- t.Errorf("%s: expected no message, got %v", k, msgs)
- }
- if len(tc.ExpectedMsg) > 0 && len(msgs) == 0 {
- t.Errorf("%s: expected error message, got none", k)
- }
- if len(tc.ExpectedMsg) > 0 {
- for i := 0; i < len(tc.ExpectedMsg); i++ {
- if msgs[i] != tc.ExpectedMsg[i] {
- t.Errorf("%s: expected message containing %q, got %v", k, tc.ExpectedMsg[i], msgs[i])
- }
- }
+ t.Errorf("%s: expected error containing %q, got %v", k, tc.ExpectedMsg, msgs[0])
}
}
}
diff --git staging/src/k8s.io/apimachinery/pkg/api/validation/path/name.go staging/src/k8s.io/apimachinery/pkg/api/validation/path/name.go
index ffb9f56d800..c0dee8108a2 100644
--- staging/src/k8s.io/apimachinery/pkg/api/validation/path/name.go
+++ staging/src/k8s.io/apimachinery/pkg/api/validation/path/name.go
@@ -17,52 +17,26 @@ limitations under the License.
package path
import (
- "fmt"
- "strings"
+ "k8s.io/apimachinery/pkg/api/validate/content"
)
-// NameMayNotBe specifies strings that cannot be used as names specified as path segments (like the REST API or etcd store)
-var NameMayNotBe = []string{".", ".."}
-
-// NameMayNotContain specifies substrings that cannot be used in names specified as path segments (like the REST API or etcd store)
-var NameMayNotContain = []string{"/", "%"}
-
// IsValidPathSegmentName validates the name can be safely encoded as a path segment
-func IsValidPathSegmentName(name string) []string {
- for _, illegalName := range NameMayNotBe {
- if name == illegalName {
- return []string{fmt.Sprintf(`may not be '%s'`, illegalName)}
- }
- }
-
- var errors []string
- for _, illegalContent := range NameMayNotContain {
- if strings.Contains(name, illegalContent) {
- errors = append(errors, fmt.Sprintf(`may not contain '%s'`, illegalContent))
- }
- }
-
- return errors
-}
+//
+// Deprecated: use content.IsPathSegmentName directly.
+var IsValidPathSegmentName = content.IsPathSegmentName
// IsValidPathSegmentPrefix validates the name can be used as a prefix for a name which will be encoded as a path segment
// It does not check for exact matches with disallowed names, since an arbitrary suffix might make the name valid
-func IsValidPathSegmentPrefix(name string) []string {
- var errors []string
- for _, illegalContent := range NameMayNotContain {
- if strings.Contains(name, illegalContent) {
- errors = append(errors, fmt.Sprintf(`may not contain '%s'`, illegalContent))
- }
- }
-
- return errors
-}
+//
+// Deprecated: use content.IsPathSegmentPrefix directly.
+var IsValidPathSegmentPrefix = content.IsPathSegmentPrefix
// ValidatePathSegmentName validates the name can be safely encoded as a path segment
+//
+// Deprecated: use a locally defined function.
func ValidatePathSegmentName(name string, prefix bool) []string {
if prefix {
return IsValidPathSegmentPrefix(name)
}
-
return IsValidPathSegmentName(name)
}
diff --git staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go
index d6bf062a19b..1cf14d892fd 100644
--- staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go
+++ staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go
@@ -22,7 +22,7 @@ import (
"net/http"
"strings"
- "k8s.io/apimachinery/pkg/api/validation/path"
+ "k8s.io/apimachinery/pkg/api/validate/content"
metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
metainternalversionscheme "k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -246,7 +246,7 @@ func (r *RequestInfoFactory) NewRequestInfo(req *http.Request) (*RequestInfo, er
if opts.FieldSelector != nil {
if name, ok := opts.FieldSelector.RequiresExactMatch("metadata.name"); ok {
- if len(path.IsValidPathSegmentName(name)) == 0 {
+ if len(content.IsPathSegmentName(name)) == 0 {
requestInfo.Name = name
}
}
diff --git staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go
index 722a312270a..906cdd7cdbb 100644
--- staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go
+++ staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go
@@ -27,8 +27,8 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
+ "k8s.io/apimachinery/pkg/api/validate/content"
"k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
@@ -286,7 +286,7 @@ func NamespaceKeyFunc(ctx context.Context, prefix string, name string) (string,
if len(name) == 0 {
return "", apierrors.NewBadRequest("Name parameter required.")
}
- if msgs := path.IsValidPathSegmentName(name); len(msgs) != 0 {
+ if msgs := content.IsPathSegmentName(name); len(msgs) != 0 {
return "", apierrors.NewBadRequest(fmt.Sprintf("Name parameter invalid: %q: %s", name, strings.Join(msgs, ";")))
}
key = key + "/" + name
@@ -299,7 +299,7 @@ func NoNamespaceKeyFunc(ctx context.Context, prefix string, name string) (string
if len(name) == 0 {
return "", apierrors.NewBadRequest("Name parameter required.")
}
- if msgs := path.IsValidPathSegmentName(name); len(msgs) != 0 {
+ if msgs := content.IsPathSegmentName(name); len(msgs) != 0 {
return "", apierrors.NewBadRequest(fmt.Sprintf("Name parameter invalid: %q: %s", name, strings.Join(msgs, ";")))
}
key := prefix + "/" + name
diff --git staging/src/k8s.io/apiserver/pkg/registry/rest/create.go staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
index b3e57d0a457..87b2c9d81bf 100644
--- staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
+++ staging/src/k8s.io/apiserver/pkg/registry/rest/create.go
@@ -22,8 +22,8 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
+ "k8s.io/apimachinery/pkg/api/validate/content"
genericvalidation "k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -89,6 +89,13 @@ type RESTCreateStrategy interface {
Canonicalize(obj runtime.Object)
}
+func validatePathSegment(name string, prefix bool) []string {
+ if prefix {
+ return content.IsPathSegmentPrefix(name)
+ }
+ return content.IsPathSegmentName(name)
+}
+
// BeforeCreate ensures that common operations for all resources are performed on creation. It only returns
// errors that can be converted to api.Status. It invokes PrepareForCreate, then Validate.
// It returns nil if the object should be created.
@@ -126,7 +133,7 @@ func BeforeCreate(strategy RESTCreateStrategy, ctx context.Context, obj runtime.
// Custom validation (including name validation) passed
// Now run common validation on object meta
// Do this *after* custom validation so that specific error messages are shown whenever possible
- if errs := genericvalidation.ValidateObjectMetaAccessor(objectMeta, strategy.NamespaceScoped(), path.ValidatePathSegmentName, field.NewPath("metadata")); len(errs) > 0 {
+ if errs := genericvalidation.ValidateObjectMetaAccessor(objectMeta, strategy.NamespaceScoped(), validatePathSegment, field.NewPath("metadata")); len(errs) > 0 {
return errors.NewInvalid(kind.GroupKind(), objectMeta.GetName(), errs)
}
diff --git staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go
index 000bb52497a..39833f1cbf7 100644
--- staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go
+++ staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go
@@ -28,7 +28,6 @@ import (
apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
- "k8s.io/apimachinery/pkg/api/validation/path"
metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/conversion"
@@ -449,7 +448,8 @@ func (t *Tester) testCreateIgnoresMismatchedNamespace(valid runtime.Object, opts
}
func (t *Tester) testCreateValidatesNames(valid runtime.Object, opts metav1.CreateOptions) {
- for _, invalidName := range path.NameMayNotBe {
+ invalidPathElements := []string{".", ".."}
+ for _, invalidName := range invalidPathElements {
objCopy := valid.DeepCopyObject()
objCopyMeta := t.getObjectMetaOrFail(objCopy)
objCopyMeta.SetName(invalidName)
@@ -461,7 +461,8 @@ func (t *Tester) testCreateValidatesNames(valid runtime.Object, opts metav1.Crea
}
}
- for _, invalidSuffix := range path.NameMayNotContain {
+ invalidInPath := []string{"/", "%"}
+ for _, invalidSuffix := range invalidInPath {
objCopy := valid.DeepCopyObject()
objCopyMeta := t.getObjectMetaOrFail(objCopy)
objCopyMeta.SetName(objCopyMeta.GetName() + invalidSuffix)
diff --git staging/src/k8s.io/apiserver/pkg/registry/rest/update.go staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
index 00f42cfe7c7..32de351aa7a 100644
--- staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
+++ staging/src/k8s.io/apiserver/pkg/registry/rest/update.go
@@ -23,7 +23,6 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
genericvalidation "k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -94,7 +93,7 @@ func validateCommonFields(obj, old runtime.Object, strategy RESTUpdateStrategy)
if err != nil {
return nil, fmt.Errorf("failed to get old object metadata: %v", err)
}
- allErrs = append(allErrs, genericvalidation.ValidateObjectMetaAccessor(objectMeta, strategy.NamespaceScoped(), path.ValidatePathSegmentName, field.NewPath("metadata"))...)
+ allErrs = append(allErrs, genericvalidation.ValidateObjectMetaAccessor(objectMeta, strategy.NamespaceScoped(), validatePathSegment, field.NewPath("metadata"))...)
allErrs = append(allErrs, genericvalidation.ValidateObjectMetaAccessorUpdate(objectMeta, oldObjectMeta, field.NewPath("metadata"))...)
return allErrs, nil
diff --git staging/src/k8s.io/apiserver/pkg/storage/util.go staging/src/k8s.io/apiserver/pkg/storage/util.go
index 3ca9acf3bd2..941422ef5e2 100644
--- staging/src/k8s.io/apiserver/pkg/storage/util.go
+++ staging/src/k8s.io/apiserver/pkg/storage/util.go
@@ -22,7 +22,7 @@ import (
"sync/atomic"
"k8s.io/apimachinery/pkg/api/meta"
- "k8s.io/apimachinery/pkg/api/validation/path"
+ "k8s.io/apimachinery/pkg/api/validate/content"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
@@ -46,7 +46,7 @@ func NamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
return "", err
}
name := meta.GetName()
- if msgs := path.IsValidPathSegmentName(name); len(msgs) != 0 {
+ if msgs := content.IsPathSegmentName(name); len(msgs) != 0 {
return "", fmt.Errorf("invalid name: %v", msgs)
}
return prefix + meta.GetNamespace() + "/" + name, nil
@@ -61,7 +61,7 @@ func NoNamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
return "", err
}
name := meta.GetName()
- if msgs := path.IsValidPathSegmentName(name); len(msgs) != 0 {
+ if msgs := content.IsPathSegmentName(name); len(msgs) != 0 {
return "", fmt.Errorf("invalid name: %v", msgs)
}
return prefix + name, nil
diff --git staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/validation/validation.go staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/validation/validation.go
index 82754d622a0..4e44494dfc3 100644
--- staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/validation/validation.go
+++ staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/validation/validation.go
@@ -20,8 +20,8 @@ import (
"fmt"
"strings"
+ "k8s.io/apimachinery/pkg/api/validate/content"
"k8s.io/apimachinery/pkg/api/validation"
- "k8s.io/apimachinery/pkg/api/validation/path"
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -34,7 +34,7 @@ func ValidateAPIService(apiService *apiregistration.APIService) field.ErrorList
allErrs := validation.ValidateObjectMeta(&apiService.ObjectMeta, false,
func(name string, prefix bool) []string {
- if minimalFailures := path.IsValidPathSegmentName(name); len(minimalFailures) > 0 {
+ if minimalFailures := content.IsPathSegmentName(name); len(minimalFailures) > 0 {
return minimalFailures
}
// the name *must* be version.group| obj.Spec.PodGroups[0].Name = "Invalid_Name" | ||
| }), | ||
| expectedErrs: field.ErrorList{ | ||
| field.Invalid(field.NewPath("spec", "podGroups").Index(0).Child("name"), "Invalid_Name", "a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')").WithOrigin("format=k8s-short-name"), |
Member
There was a problem hiding this comment.
Reviewing per-commit, so this may be fixed in a later commit?
You don't need (and we don't want) to embed the whole error string here. It just makes tests brittle. E.g. I want to fix that validation to return multiple errors, one for "begins with" and another for "contains". If I do that, I have to also fix your tests.
The check for WithOrigin("format=k8s-short-name") is sufficiently precise that I have confidence that I won't accidentally fail for a different reason but satisfy the test (which really happens in a TON of manual tests). Similarly, you don't need the value to be duplicated. So this can become:
field.Invalid(field.NewPath("spec.podGroups[0].name", nil, "").WithOrigin("format=k8s-short-name")
And it is just as precise a match, and WAY more understandable. And less brittle :)
Almost every Invalid() error can follow this pattern.
| }, | ||
| "duplicate podGroup names": { | ||
| input: mkValidWorkload(func(obj *scheduling.Workload) { | ||
| obj.Spec.PodGroups = append(obj.Spec.PodGroups, scheduling.PodGroup{ |
Member
There was a problem hiding this comment.
Consider encapsulating this into a function like:
func addPodGroup(name string) func(obj *scheduling.Workload) {
return func(obj *scheduling.Workload) {
obj.Spec.PodGroups = append(obj.Spec.PodGroups, scheduling.PodGroup{
//... use `name`
})
}
}
So this test-case becomes:
"duplicate podGroup names": {
input: mkValidWorkload(addPodGroup("main")),
expectedErrs: field.ErrorList{
field.Duplicate(field.NewPath("spec.podGroups[1]"), nil))
}
Note that I elided the value argument because it doesn't actually add clarity (and is not actually considered in the error matcher anyway!!).
Less LOCs, way more readable.
| // Declarative validation treats 0 as "missing" and returns Required error | ||
| // instead of checking minimum constraint and returning Invalid error. | ||
| "gang minCount zero": { | ||
| input: mkValidWorkload(func(obj *scheduling.Workload) { |
Member
There was a problem hiding this comment.
could add a tweak func setPodGroupMinCount(pgIdx int, min int) so this becomes mkValidWorkload(setPodGroupMinCount(0, 0))
| }, | ||
| "valid with controllerRef": { | ||
| input: mkValidWorkload(func(obj *scheduling.Workload) { | ||
| obj.Spec.ControllerRef = &scheduling.TypedLocalObjectReference{ |
Member
There was a problem hiding this comment.
could make it a tweak func. I won't repeat this any more, I think you see the pattern. Using a literal func as a tweak should be reserved for very special cases. Prefer instead to build up a library of tweaks.
Member
Author
There was a problem hiding this comment.
Yes. Added tweak funcs for the repeatatives patterns.
| allErrs = append(allErrs, field.Required(podGroupsPath, "must have at least one item").MarkCoveredByDeclarative()) | ||
| } else if len(spec.PodGroups) > scheduling.WorkloadMaxPodGroups { | ||
| allErrs = append(allErrs, field.TooMany(fldPath.Child("podGroups"), len(spec.PodGroups), scheduling.WorkloadMaxPodGroups)) | ||
| allErrs = append(allErrs, field.TooMany(podGroupsPath, len(spec.PodGroups), scheduling.WorkloadMaxPodGroups).WithOrigin("maxItems").MarkCoveredByDeclarative()) |
Member
There was a problem hiding this comment.
ideally these would be in the commits that made the change, but it's OK for this one, just to get it done :)
Signed-off-by: helayoty <[email protected]>
This is where all the scrubbed validation helpers are going. Note: This does NOT check for or too-long inputs, and changing it now would be a breaking change. Co-authored-by: Tim Hockin <[email protected]> Signed-off-by: Heba Elayoty <[email protected]>
Signed-off-by: Heba Elayoty <[email protected]>
Member
Author
Merged your commit here. PTAL |
Signed-off-by: Heba Elayoty <[email protected]>
Contributor
|
@helayoty: The following tests failed, say
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. DetailsInstructions 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. |
Signed-off-by: Heba Elayoty <[email protected]>
Member
|
/approve |
lalitc375
reviewed
Jan 14, 2026
Co-Author: Lalit Chauhan <[email protected]> Signed-off-by: Heba Elayoty <[email protected]>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: helayoty, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
LGTM label has been added. DetailsGit tree hash: 96f1c5c58f401e89145a58b3ba65f3d847fee500 |
Contributor
|
/release-note-none |
Member
For a feature PR with KEP, a release note is suggested. @helayoty |
24 tasks
Member
Author
|
/area workload-aware |
Closed
7 tasks
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
13 participants
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.
What type of PR is this?
k8s-path-segment-namevalidation./kind feature
/sig scheduling
What this PR does / why we need it:
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: