Merge pull request #132823 from yongruilin/master_vg_enum

feat(validation-gen): add k8s:enum validators

Kubernetes-commit: bb415b6933d31355276efac2edb71d70c0314036

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/davecgh/go-spew v1.1.1
 	github.com/fxamacker/cbor/v2 v2.8.0
 	github.com/gogo/protobuf v1.3.2
-	github.com/google/gnostic-models v0.6.9
+	github.com/google/gnostic-models v0.7.0
 	github.com/google/go-cmp v0.7.0
 	github.com/google/uuid v1.6.0
 	github.com/moby/spdystream v0.5.0
@@ -24,12 +24,12 @@ require (
 	gopkg.in/evanphx/json-patch.v4 v4.12.0
 	gopkg.in/inf.v0 v0.9.1
 	k8s.io/klog/v2 v2.130.1
-	k8s.io/kube-openapi v0.0.0-20250610211856-8b98d1ed966a
+	k8s.io/kube-openapi v0.0.0-20250628140032-d90c4fd18f59
 	k8s.io/utils v0.0.0-20250604170112-4c0f3b243397
-	sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3
+	sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8
 	sigs.k8s.io/randfill v1.0.0
 	sigs.k8s.io/structured-merge-diff/v4 v4.7.0
-	sigs.k8s.io/yaml v1.4.0
+	sigs.k8s.io/yaml v1.5.0
 )
 
 require (
@@ -47,6 +47,8 @@ require (
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/rogpeppe/go-internal v1.13.1 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
+	go.yaml.in/yaml/v2 v2.4.2 // indirect
+	go.yaml.in/yaml/v3 v3.0.4 // indirect
 	golang.org/x/text v0.23.0 // indirect
 	google.golang.org/protobuf v1.36.5 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect

go.sum

@@ -20,8 +20,8 @@ github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1v
 github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8=
 github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
 github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q=
-github.com/google/gnostic-models v0.6.9 h1:MU/8wDLif2qCXZmzncUQ/BOfxWfthHi63KqpoNbWqVw=
-github.com/google/gnostic-models v0.6.9/go.mod h1:CiWsm0s6BSQd1hRn8/QmxqB6BesYcbSZxsz9b0KuDBw=
+github.com/google/gnostic-models v0.7.0 h1:qwTtogB15McXDaNqTZdzPJRHvaVJlAl+HVQnLmJEJxo=
+github.com/google/gnostic-models v0.7.0/go.mod h1:whL5G0m6dmc5cPxKc5bdKdEN3UjI7OUGxBlw57miDrQ=
 github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
 github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
@@ -80,6 +80,10 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
 github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
+go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI=
+go.yaml.in/yaml/v2 v2.4.2/go.mod h1:081UH+NErpNdqlCXm3TtEran0rJZGxAYx9hb/ELlsPU=
+go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc=
+go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
@@ -129,16 +133,17 @@ gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
 k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
-k8s.io/kube-openapi v0.0.0-20250610211856-8b98d1ed966a h1:ZV3Zr+/7s7aVbjNGICQt+ppKWsF1tehxggNfbM7XnG8=
-k8s.io/kube-openapi v0.0.0-20250610211856-8b98d1ed966a/go.mod h1:5jIi+8yX4RIb8wk3XwBo5Pq2ccx4FP10ohkbSKCZoK8=
+k8s.io/kube-openapi v0.0.0-20250628140032-d90c4fd18f59 h1:Jc4GiFTK2HHOpfQFoQEGXTBTs2pETwHukmoD4yoTqwo=
+k8s.io/kube-openapi v0.0.0-20250628140032-d90c4fd18f59/go.mod h1:GLOk5B+hDbRROvt0X2+hqX64v/zO3vXN7J78OUmBSKw=
 k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 h1:hwvWFiBzdWw1FhfY1FooPn3kzWuJ8tmbZBHi4zVsl1Y=
 k8s.io/utils v0.0.0-20250604170112-4c0f3b243397/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
-sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 h1:/Rv+M11QRah1itp8VhT6HoVx1Ray9eB4DBr+K+/sCJ8=
-sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3/go.mod h1:18nIHnGi6636UCz6m8i4DhaJ65T6EruyzmoQqI2BVDo=
+sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 h1:gBQPwqORJ8d8/YNZWEjoZs7npUVDpVXUUOFfW6CgAqE=
+sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8/go.mod h1:mdzfpAEoE6DHQEN0uh9ZbOCuHbLK5wOm7dK4ctXE9Tg=
 sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=
 sigs.k8s.io/randfill v1.0.0 h1:JfjMILfT8A6RbawdsK2JXGBR5AQVfd+9TbzrlneTyrU=
 sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=
 sigs.k8s.io/structured-merge-diff/v4 v4.7.0 h1:qPeWmscJcXP0snki5IYF79Z8xrl8ETFxgMd7wez1XkI=
 sigs.k8s.io/structured-merge-diff/v4 v4.7.0/go.mod h1:dDy58f92j70zLsuZVuUX5Wp9vtxXpaZnkPGWeqDfCps=
-sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=
 sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=
+sigs.k8s.io/yaml v1.5.0 h1:M10b2U7aEUY6hRtU870n2VTPgR5RZiL/I6Lcc2F4NUQ=
+sigs.k8s.io/yaml v1.5.0/go.mod h1:wZs27Rbxoai4C0f8/9urLZtZtF3avA3gKvGyPdDqTO4=

pkg/api/validate/enum.go

@@ -0,0 +1,40 @@
+/*
+Copyright 2024 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 validate
+
+import (
+	"context"
+	"slices"
+
+	"k8s.io/apimachinery/pkg/api/operation"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+// Enum verifies that the specified value is one of the valid symbols.
+// This is for string enums only.
+func Enum[T ~string](_ context.Context, op operation.Operation, fldPath *field.Path, value, _ *T, symbols sets.Set[T]) field.ErrorList {
+	if value == nil {
+		return nil
+	}
+	if !symbols.Has(*value) {
+		symbolList := symbols.UnsortedList()
+		slices.Sort(symbolList)
+		return field.ErrorList{field.NotSupported[T](fldPath, *value, symbolList)}
+	}
+	return nil
+}

pkg/api/validate/enum_test.go

@@ -0,0 +1,107 @@
+/*
+Copyright 2024 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 validate
+
+import (
+	"context"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/operation"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func TestEnum(t *testing.T) {
+	cases := []struct {
+		value string
+		valid sets.Set[string]
+		err   bool
+	}{{
+		value: "a",
+		valid: sets.New("a", "b", "c"),
+		err:   false,
+	}, {
+		value: "x",
+		valid: sets.New("c", "a", "b"),
+		err:   true,
+	}}
+
+	for i, tc := range cases {
+		result := Enum(context.Background(), operation.Operation{}, field.NewPath("fldpath"), &tc.value, nil, tc.valid)
+		if len(result) > 0 && !tc.err {
+			t.Errorf("case %d: unexpected failure: %v", i, fmtErrs(result))
+			continue
+		}
+		if len(result) == 0 && tc.err {
+			t.Errorf("case %d: unexpected success", i)
+			continue
+		}
+		if len(result) > 0 {
+			if len(result) > 1 {
+				t.Errorf("case %d: unexepected multi-error: %v", i, fmtErrs(result))
+				continue
+			}
+			if want, got := `supported values: "a", "b", "c"`, result[0].Detail; got != want {
+				t.Errorf("case %d: wrong error, expected: %q, got: %q", i, want, got)
+			}
+		}
+	}
+}
+
+func TestEnumTypedef(t *testing.T) {
+	type StringType string
+	const (
+		NotStringFoo StringType = "foo"
+		NotStringBar StringType = "bar"
+		NotStringQux StringType = "qux"
+	)
+
+	cases := []struct {
+		value StringType
+		valid sets.Set[StringType]
+		err   bool
+	}{{
+		value: "foo",
+		valid: sets.New(NotStringFoo, NotStringBar, NotStringQux),
+		err:   false,
+	}, {
+		value: "x",
+		valid: sets.New(NotStringFoo, NotStringBar, NotStringQux),
+		err:   true,
+	}}
+
+	for i, tc := range cases {
+		result := Enum(context.Background(), operation.Operation{}, field.NewPath("fldpath"), &tc.value, nil, tc.valid)
+		if len(result) > 0 && !tc.err {
+			t.Errorf("case %d: unexpected failure: %v", i, fmtErrs(result))
+			continue
+		}
+		if len(result) == 0 && tc.err {
+			t.Errorf("case %d: unexpected success", i)
+			continue
+		}
+		if len(result) > 0 {
+			if len(result) > 1 {
+				t.Errorf("case %d: unexepected multi-error: %v", i, fmtErrs(result))
+				continue
+			}
+			if want, got := `supported values: "bar", "foo", "qux"`, result[0].Detail; got != want {
+				t.Errorf("case %d: wrong error, expected: %q, got: %q", i, want, got)
+			}
+		}
+	}
+}

pkg/api/validation/objectmeta_test.go

@@ -328,19 +328,19 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) {
 			Old:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1"},
 			New:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
 			ExpectedNew:  metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
-			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:16:40 +0000 UTC: field is immutable"},
+			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"1970-01-01T00:16:40Z\": field is immutable"},
 		},
 		"invalid clear deletionTimestamp": {
 			Old:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
 			New:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1"},
 			ExpectedNew:  metav1.ObjectMeta{Name: "test", ResourceVersion: "1"},
-			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"null\": field is immutable"},
+			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: null: field is immutable"},
 		},
 		"invalid change deletionTimestamp": {
 			Old:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
 			New:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later},
 			ExpectedNew:  metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later},
-			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:33:20 +0000 UTC: field is immutable"},
+			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"1970-01-01T00:33:20Z\": field is immutable"},
 		},
 
 		"invalid set deletionGracePeriodSeconds": {
@@ -353,7 +353,7 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) {
 			Old:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
 			New:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1"},
 			ExpectedNew:  metav1.ObjectMeta{Name: "test", ResourceVersion: "1"},
-			ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: \"null\": field is immutable"},
+			ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: null: field is immutable"},
 		},
 		"invalid change deletionGracePeriodSeconds": {
 			Old:          metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
@@ -373,7 +373,7 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) {
 		}
 		for i := range errs {
 			if errs[i].Error() != tc.ExpectedErrs[i] {
-				t.Errorf("%s: error #%d: expected %q, got %q", k, i, tc.ExpectedErrs[i], errs[i].Error())
+				t.Errorf("%s: error #%d:\n  expected: %q\n       got: %q", k, i, tc.ExpectedErrs[i], errs[i].Error())
 			}
 		}
 		if !reflect.DeepEqual(tc.New, tc.ExpectedNew) {
@@ -419,7 +419,7 @@ func TestObjectMetaGenerationUpdate(t *testing.T) {
 		}
 		for i := range errList {
 			if errList[i] != tc.ExpectedErrs[i] {
-				t.Errorf("%s: error #%d: expected %q, got %q", k, i, tc.ExpectedErrs[i], errList[i])
+				t.Errorf("%s: error #%d:\n  expected: %q\n       got: %q", k, i, tc.ExpectedErrs[i], errs[i].Error())
 			}
 		}
 	}

pkg/apis/meta/v1/validation/validation.go

@@ -328,11 +328,11 @@ func ValidateCondition(condition metav1.Condition, fldPath *field.Path) field.Er
 	}
 
 	if condition.LastTransitionTime.IsZero() {
-		allErrs = append(allErrs, field.Required(fldPath.Child("lastTransitionTime"), "must be set"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("lastTransitionTime"), ""))
 	}
 
 	if len(condition.Reason) == 0 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("reason"), "must be set"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("reason"), ""))
 	} else {
 		for _, currErr := range isValidConditionReason(condition.Reason) {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("reason"), condition.Reason, currErr))

pkg/apis/meta/v1/validation/validation_test.go

@@ -132,10 +132,6 @@ func TestInvalidDryRun(t *testing.T) {
 
 }
 
-func boolPtr(b bool) *bool {
-	return &b
-}
-
 func TestValidateDeleteOptionsWithIgnoreStoreReadError(t *testing.T) {
 	fieldPath := field.NewPath("ignoreStoreReadErrorWithClusterBreakingPotential")
 	tests := []struct {
@@ -232,7 +228,7 @@ func TestValidPatchOptions(t *testing.T) {
 		patchType types.PatchType
 	}{{
 		opts: metav1.PatchOptions{
-			Force:        boolPtr(true),
+			Force:        ptr.To(true),
 			FieldManager: "kubectl",
 		},
 		patchType: types.ApplyYAMLPatchType,
@@ -243,7 +239,7 @@ func TestValidPatchOptions(t *testing.T) {
 		patchType: types.ApplyYAMLPatchType,
 	}, {
 		opts: metav1.PatchOptions{
-			Force:        boolPtr(true),
+			Force:        ptr.To(true),
 			FieldManager: "kubectl",
 		},
 		patchType: types.ApplyCBORPatchType,
@@ -290,7 +286,7 @@ func TestInvalidPatchOptions(t *testing.T) {
 		// force on non-apply
 		{
 			opts: metav1.PatchOptions{
-				Force: boolPtr(true),
+				Force: ptr.To(true),
 			},
 			patchType: types.MergePatchType,
 		},
@@ -298,7 +294,7 @@ func TestInvalidPatchOptions(t *testing.T) {
 		{
 			opts: metav1.PatchOptions{
 				FieldManager: "kubectl",
-				Force:        boolPtr(false),
+				Force:        ptr.To(false),
 			},
 			patchType: types.MergePatchType,
 		},
@@ -442,7 +438,7 @@ func TestValidateConditions(t *testing.T) {
 			if !hasError(errs, needle) {
 				t.Errorf("missing %q in\n%v", needle, errorsAsString(errs))
 			}
-			needle = `status.conditions[0].lastTransitionTime: Required value: must be set`
+			needle = `status.conditions[0].lastTransitionTime: Required value`
 			if !hasError(errs, needle) {
 				t.Errorf("missing %q in\n%v", needle, errorsAsString(errs))
 			}

pkg/runtime/scheme_test.go

@@ -33,7 +33,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	runtimetesting "k8s.io/apimachinery/pkg/runtime/testing"
-	runtimetestingv1 "k8s.io/apimachinery/pkg/runtime/testing/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -1125,12 +1124,6 @@ func TestToOpenAPIDefinitionName(t *testing.T) {
 		out         string
 		wantErr     error
 	}{
-		{
-			name:        "built-registerObj type",
-			registerObj: &runtimetestingv1.ExternalSimple{},
-			gvk:         schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Simple"},
-			out:         "io.k8s.apimachinery.pkg.runtime.testing.v1.ExternalSimple",
-		},
 		{
 			name:        "unstructured type",
 			registerObj: &unstructured.Unstructured{},

pkg/runtime/testing/validation.go

@@ -52,6 +52,10 @@ func runValidation(t *testing.T, scheme *runtime.Scheme, options []string, unver
 		t.Fatal(err)
 	}
 	for _, unversionedGVK := range unversionedGVKs {
+		// skip if passed in unversioned object is not internal.
+		if unversionedGVK.Version != runtime.APIVersionInternal {
+			continue
+		}
 		gvs := scheme.VersionsForGroupKind(unversionedGVK.GroupKind())
 		for _, gv := range gvs {
 			gvk := gv.WithKind(unversionedGVK.Kind)
@@ -78,6 +82,10 @@ func runUpdateValidation(t *testing.T, scheme *runtime.Scheme, options []string,
 		t.Fatal(err)
 	}
 	for _, unversionedGVK := range unversionedGVKs {
+		// skip if passed in unversioned object is not internal.
+		if unversionedGVK.Version != runtime.APIVersionInternal {
+			continue
+		}
 		gvs := scheme.VersionsForGroupKind(unversionedGVK.GroupKind())
 		for _, gv := range gvs {
 			gvk := gv.WithKind(unversionedGVK.Kind)

pkg/util/validation/field/errors.go

@@ -17,8 +17,8 @@ limitations under the License.
 package field
 
 import (
+	"encoding/json"
 	"fmt"
-	"reflect"
 	"strconv"
 	"strings"
 
@@ -72,45 +72,43 @@ var omitValue = OmitValueType{}
 // for building nice-looking higher-level error reporting.
 func (e *Error) ErrorBody() string {
 	var s string
-	switch {
-	case e.Type == ErrorTypeRequired:
+	switch e.Type {
+	case ErrorTypeRequired, ErrorTypeForbidden, ErrorTypeTooLong, ErrorTypeInternal:
 		s = e.Type.String()
-	case e.Type == ErrorTypeForbidden:
-		s = e.Type.String()
-	case e.Type == ErrorTypeTooLong:
-		s = e.Type.String()
-	case e.Type == ErrorTypeInternal:
-		s = e.Type.String()
-	case e.BadValue == omitValue:
-		s = e.Type.String()
-	default:
-		value := e.BadValue
-		valueType := reflect.TypeOf(value)
-		if value == nil || valueType == nil {
-			value = "null"
-		} else if valueType.Kind() == reflect.Pointer {
-			if reflectValue := reflect.ValueOf(value); reflectValue.IsNil() {
-				value = "null"
-			} else {
-				value = reflectValue.Elem().Interface()
-			}
+	case ErrorTypeInvalid, ErrorTypeTypeInvalid, ErrorTypeNotSupported,
+		ErrorTypeNotFound, ErrorTypeDuplicate, ErrorTypeTooMany:
+		if e.BadValue == omitValue {
+			s = e.Type.String()
+			break
 		}
-		switch t := value.(type) {
+		switch t := e.BadValue.(type) {
 		case int64, int32, float64, float32, bool:
 			// use simple printer for simple types
-			s = fmt.Sprintf("%s: %v", e.Type, value)
+			s = fmt.Sprintf("%s: %v", e.Type, t)
 		case string:
 			s = fmt.Sprintf("%s: %q", e.Type, t)
-		case fmt.Stringer:
-			// anything that defines String() is better than raw struct
-			s = fmt.Sprintf("%s: %s", e.Type, t.String())
 		default:
-			// fallback to raw struct
-			// TODO: internal types have panic guards against json.Marshalling to prevent
-			// accidental use of internal types in external serialized form.  For now, use
-			// %#v, although it would be better to show a more expressive output in the future
-			s = fmt.Sprintf("%s: %#v", e.Type, value)
+			// use more complex techniques to render more complex types
+			valstr := ""
+			jb, err := json.Marshal(e.BadValue)
+			if err == nil {
+				// best case
+				valstr = string(jb)
+			} else if stringer, ok := e.BadValue.(fmt.Stringer); ok {
+				// anything that defines String() is better than raw struct
+				valstr = stringer.String()
+			} else {
+				// worst case - fallback to raw struct
+				// TODO: internal types have panic guards against json.Marshalling to prevent
+				// accidental use of internal types in external serialized form.  For now, use
+				// %#v, although it would be better to show a more expressive output in the future
+				valstr = fmt.Sprintf("%#v", e.BadValue)
+			}
+			s = fmt.Sprintf("%s: %s", e.Type, valstr)
 		}
+	default:
+		internal := InternalError(nil, fmt.Errorf("unhandled error code: %s: please report this", e.Type))
+		s = internal.ErrorBody()
 	}
 	if len(e.Detail) != 0 {
 		s += fmt.Sprintf(": %s", e.Detail)
@@ -197,7 +195,7 @@ func (t ErrorType) String() string {
 	case ErrorTypeTypeInvalid:
 		return "Invalid value"
 	default:
-		panic(fmt.Sprintf("unrecognized validation error: %q", string(t)))
+		return fmt.Sprintf("<unknown error %q>", string(t))
 	}
 }
 
@@ -258,10 +256,14 @@ func Forbidden(field *Path, detail string) *Error {
 // the given value is too long.  This is similar to Invalid, but the returned
 // error will not include the too-long value. If maxLength is negative, it will
 // be included in the message.  The value argument is not used.
-func TooLong(field *Path, value interface{}, maxLength int) *Error {
+func TooLong(field *Path, _ interface{}, maxLength int) *Error {
 	var msg string
 	if maxLength >= 0 {
-		msg = fmt.Sprintf("may not be more than %d bytes", maxLength)
+		bs := "bytes"
+		if maxLength == 1 {
+			bs = "byte"
+		}
+		msg = fmt.Sprintf("may not be more than %d %s", maxLength, bs)
 	} else {
 		msg = "value is too long"
 	}
@@ -281,7 +283,11 @@ func TooMany(field *Path, actualQuantity, maxQuantity int) *Error {
 	var msg string
 
 	if maxQuantity >= 0 {
-		msg = fmt.Sprintf("must have at most %d items", maxQuantity)
+		is := "items"
+		if maxQuantity == 1 {
+			is = "item"
+		}
+		msg = fmt.Sprintf("must have at most %d %s", maxQuantity, is)
 	} else {
 		msg = "has too many items"
 	}

pkg/util/validation/field/errors_test.go

@@ -19,8 +19,10 @@ package field
 import (
 	"fmt"
 	"reflect"
-	"strings"
 	"testing"
+	"time"
+
+	"k8s.io/utils/ptr"
 )
 
 func TestMakeFuncs(t *testing.T) {
@@ -62,52 +64,6 @@ func TestMakeFuncs(t *testing.T) {
 	}
 }
 
-func TestErrorUsefulMessage(t *testing.T) {
-	{
-		s := Invalid(nil, nil, "").Error()
-		t.Logf("message: %v", s)
-		if !strings.Contains(s, "null") {
-			t.Errorf("error message did not contain 'null': %s", s)
-		}
-	}
-
-	s := Invalid(NewPath("foo"), "bar", "deet").Error()
-	t.Logf("message: %v", s)
-	for _, part := range []string{"foo", "bar", "deet", ErrorTypeInvalid.String()} {
-		if !strings.Contains(s, part) {
-			t.Errorf("error message did not contain expected part '%v'", part)
-		}
-	}
-
-	type complicated struct {
-		Baz   int
-		Qux   string
-		Inner interface{}
-		KV    map[string]int
-	}
-	s = Invalid(
-		NewPath("foo"),
-		&complicated{
-			Baz:   1,
-			Qux:   "aoeu",
-			Inner: &complicated{Qux: "asdf"},
-			KV:    map[string]int{"Billy": 2},
-		},
-		"detail",
-	).Error()
-	t.Logf("message: %v", s)
-	for _, part := range []string{
-		"foo", ErrorTypeInvalid.String(),
-		"Baz", "Qux", "Inner", "KV", "detail",
-		"1", "aoeu", "Billy", "2",
-		// "asdf", TODO: re-enable once we have a better nested printer
-	} {
-		if !strings.Contains(s, part) {
-			t.Errorf("error message did not contain expected part '%v'", part)
-		}
-	}
-}
-
 func TestToAggregate(t *testing.T) {
 	testCases := struct {
 		ErrList         []ErrorList
@@ -167,14 +123,6 @@ func TestErrListFilter(t *testing.T) {
 	}
 }
 
-func TestNotSupported(t *testing.T) {
-	notSupported := NotSupported(NewPath("f"), "v", []string{"a", "b", "c"})
-	expected := `Unsupported value: "v": supported values: "a", "b", "c"`
-	if notSupported.ErrorBody() != expected {
-		t.Errorf("Expected: %s\n, but got: %s\n", expected, notSupported.ErrorBody())
-	}
-}
-
 func TestErrorOrigin(t *testing.T) {
 	err := Invalid(NewPath("field"), "value", "detail")
 
@@ -301,3 +249,438 @@ func TestErrorListRemoveCoveredByDeclarative(t *testing.T) {
 		}
 	}
 }
+
+func TestErrorFormatting(t *testing.T) {
+	cases := []struct {
+		name   string
+		input  *Error
+		expect string
+	}{{
+		name: "required",
+		input: &Error{
+			Type:                 ErrorTypeRequired,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Required value: the details`,
+	}, {
+		name:   "required func",
+		input:  Required(NewPath("path.to.field"), "the details"),
+		expect: `path.to.field: Required value: the details`,
+	}, {
+		name: "forbidden",
+		input: &Error{
+			Type:                 ErrorTypeForbidden,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Forbidden: the details`,
+	}, {
+		name:   "forbidden func",
+		input:  Forbidden(NewPath("path.to.field"), "the details"),
+		expect: `path.to.field: Forbidden: the details`,
+	}, {
+		name: "too long",
+		input: &Error{
+			Type:                 ErrorTypeTooLong,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Too long: the details`,
+	}, {
+		name:   "too long func(1)",
+		input:  TooLong(NewPath("path.to.field"), "the value", 1),
+		expect: `path.to.field: Too long: may not be more than 1 byte`,
+	}, {
+		name:   "too long func(2)",
+		input:  TooLong(NewPath("path.to.field"), "the value", 2),
+		expect: `path.to.field: Too long: may not be more than 2 bytes`,
+	}, {
+		name:   "too long func(-1)",
+		input:  TooLong(NewPath("path.to.field"), "the value", -1),
+		expect: `path.to.field: Too long: value is too long`,
+	}, {
+		name: "too many",
+		input: &Error{
+			Type:                 ErrorTypeTooMany,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Too many: "the value": the details`,
+	}, {
+		name:   "too many func(2, 1)",
+		input:  TooMany(NewPath("path.to.field"), 2, 1),
+		expect: `path.to.field: Too many: 2: must have at most 1 item`,
+	}, {
+		name:   "too many func(3, 2)",
+		input:  TooMany(NewPath("path.to.field"), 3, 2),
+		expect: `path.to.field: Too many: 3: must have at most 2 items`,
+	}, {
+		name:   "too many func(2, -1)",
+		input:  TooMany(NewPath("path.to.field"), 2, -1),
+		expect: `path.to.field: Too many: 2: has too many items`,
+	}, {
+		name:   "too many func(-1, 1)",
+		input:  TooMany(NewPath("path.to.field"), -1, 1),
+		expect: `path.to.field: Too many: must have at most 1 item`,
+	}, {
+		name: "internal error",
+		input: &Error{
+			Type:                 ErrorTypeInternal,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Internal error: the details`,
+	}, {
+		name:   "internal error func",
+		input:  InternalError(NewPath("path.to.field"), fmt.Errorf("the error")),
+		expect: `path.to.field: Internal error: the error`,
+	}, {
+		name: "invalid string",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "invalid string type",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             StringType("the value"),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "invalid int",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             -42,
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: -42: the details`,
+	}, {
+		name: "invalid bool",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             true,
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: true: the details`,
+	}, {
+		name: "invalid struct",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             mkTinyStruct(),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: {"stringField":"stringval","intField":9376,"boolField":true}: the details`,
+	}, {
+		name: "invalid list",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             []string{"one", "two", "three"},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: ["one","two","three"]: the details`,
+	}, {
+		name: "invalid map",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             map[string]int{"one": 1, "two": 2, "three": 3},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: {"one":1,"three":3,"two":2}: the details`,
+	}, {
+		name: "invalid time",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             time.Time{},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "0001-01-01T00:00:00Z": the details`,
+	}, {
+		name: "invalid omitValue",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             omitValue,
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: the details`,
+	}, {
+		name: "invalid untyped nil",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             nil,
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: null: the details`,
+	}, {
+		name: "invalid typed nil",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             (*string)(nil),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: null: the details`,
+	}, {
+		name: "invalid string ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To("the value"),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "invalid string type ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To(StringType("the value")),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "invalid int ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To(-42),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: -42: the details`,
+	}, {
+		name: "invalid bool ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To(true),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: true: the details`,
+	}, {
+		name: "invalid struct ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To(mkTinyStruct()),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: {"stringField":"stringval","intField":9376,"boolField":true}: the details`,
+	}, {
+		name: "invalid list ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To([]string{"one", "two", "three"}),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: ["one","two","three"]: the details`,
+	}, {
+		name: "invalid map ptr",
+		input: &Error{
+			Type:                 ErrorTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             ptr.To(map[string]int{"one": 1, "two": 2, "three": 3}),
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: {"one":1,"three":3,"two":2}: the details`,
+	}, {
+		name:   "invalid func",
+		input:  Invalid(NewPath("path.to.field"), "the value", "the details"),
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "not found",
+		input: &Error{
+			Type:                 ErrorTypeNotFound,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Not found: "the value": the details`,
+	}, {
+		name: "not supported",
+		input: &Error{
+			Type:                 ErrorTypeNotSupported,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Unsupported value: "the value": the details`,
+	}, {
+		name:   "not supported func",
+		input:  NotSupported(NewPath("path.to.field"), "the value", []string{"val1", "val2"}),
+		expect: `path.to.field: Unsupported value: "the value": supported values: "val1", "val2"`,
+	}, {
+		name: "duplicate",
+		input: &Error{
+			Type:                 ErrorTypeDuplicate,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Duplicate value: "the value": the details`,
+	}, {
+		name:   "duplicate func",
+		input:  Duplicate(NewPath("path.to.field"), "the value"),
+		expect: `path.to.field: Duplicate value: "the value"`,
+	}, {
+		name: "type invalid",
+		input: &Error{
+			Type:                 ErrorTypeTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             "the value",
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name:   "type invalid func",
+		input:  TypeInvalid(NewPath("path.to.field"), "the value", "the details"),
+		expect: `path.to.field: Invalid value: "the value": the details`,
+	}, {
+		name: "failed marshal stringer",
+		input: &Error{
+			Type:                 ErrorTypeTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             SelfMarshalerStringer{"invisible"},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: magic: the details`,
+	}, {
+		name: "failed marshal non-stringer",
+		input: &Error{
+			Type:                 ErrorTypeTypeInvalid,
+			Field:                "path.to.field",
+			BadValue:             SelfMarshalerNonStringer{"visible"},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Invalid value: field.SelfMarshalerNonStringer{S:"visible"}: the details`,
+	}, {
+		name: "unknown error type",
+		input: &Error{
+			Type:                 "not real",
+			Field:                "path.to.field",
+			BadValue:             SelfMarshalerNonStringer{"visible"},
+			Detail:               "the details",
+			Origin:               "theOrigin",
+			CoveredByDeclarative: true,
+		},
+		expect: `path.to.field: Internal error: unhandled error code: <unknown error "not real">: please report this: the details`,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := tc.input.Error()
+			if want := tc.expect; result != want {
+				t.Errorf("wrong error string:\n  expected: %q\n       got: %q", want, result)
+			}
+		})
+	}
+}
+
+type StringType string
+
+type TinyStruct struct {
+	StringField string `json:"stringField"`
+	IntField    int    `json:"intField"`
+	BoolField   bool   `json:"boolField"`
+}
+
+func mkTinyStruct() TinyStruct {
+	return TinyStruct{
+		StringField: "stringval",
+		IntField:    9376,
+		BoolField:   true,
+	}
+}
+
+type SelfMarshalerStringer struct{ S string }
+
+func (SelfMarshalerStringer) MarshalJSON() ([]byte, error) {
+	return nil, fmt.Errorf("this always fails")
+}
+
+func (SelfMarshalerStringer) String() string {
+	return "magic"
+}
+
+type SelfMarshalerNonStringer struct{ S string }
+
+func (SelfMarshalerNonStringer) MarshalJSON() ([]byte, error) {
+	return nil, fmt.Errorf("this always fails")
+}

pkg/version/helpers_test.go

@@ -61,7 +61,7 @@ func Test_parseKubeVersion(t *testing.T) {
 		wantOk           bool
 	}{
 		{
-			name:             "invaild version for ga",
+			name:             "invalid version for ga",
 			v:                "v1.1",
 			wantMajorVersion: 0,
 			wantVType:        0,
@@ -69,7 +69,7 @@ func Test_parseKubeVersion(t *testing.T) {
 			wantOk:           false,
 		},
 		{
-			name:             "invaild version for alpha",
+			name:             "invalid version for alpha",
 			v:                "v1alpha1.1",
 			wantMajorVersion: 0,
 			wantVType:        0,