Merge pull request #506 from karlkfi/karl-field-errors

fix: Handle more validation errors as field errors
This commit is contained in:
Kubernetes Prow Robot 2022-01-07 17:45:05 -08:00 committed by GitHub
commit 476197d25b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 300 additions and 111 deletions

View File

@ -17,7 +17,7 @@ import (
"k8s.io/kubectl/pkg/scheme"
"sigs.k8s.io/cli-utils/pkg/apply/cache"
ktestutil "sigs.k8s.io/cli-utils/pkg/kstatus/polling/testutil"
"sigs.k8s.io/cli-utils/pkg/testutil"
"sigs.k8s.io/cli-utils/pkg/object"
// Using gopkg.in/yaml.v3 instead of sigs.k8s.io/yaml on purpose.
// yaml.v3 correctly parses ints:
@ -597,7 +597,7 @@ func TestMutate(t *testing.T) {
require.Equal(t, tc.reason, reason, "unexpected mutated reason")
for _, efv := range tc.expected {
received, found, err := testutil.NestedField(tc.target.Object, efv.Field...)
received, found, err := object.NestedField(tc.target.Object, efv.Field...)
require.NoError(t, err)
require.True(t, found, "target field not found")
require.Equal(t, efv.Value, received, "unexpected target field value")

108
pkg/object/field.go Normal file
View File

@ -0,0 +1,108 @@
// Copyright 2021 The Kubernetes Authors.
// SPDX-License-Identifier: Apache-2.0
package object
import (
"fmt"
"regexp"
"k8s.io/apimachinery/pkg/util/validation/field"
)
// NestedField gets a value from a KRM map, if it exists, otherwise nil.
// Fields can be string (map key) or int (array index).
func NestedField(obj map[string]interface{}, fields ...interface{}) (interface{}, bool, error) {
var val interface{} = obj
for i, field := range fields {
if val == nil {
return nil, false, nil
}
switch typedField := field.(type) {
case string:
if m, ok := val.(map[string]interface{}); ok {
val, ok = m[typedField]
if !ok {
// not in map
return nil, false, nil
}
} else {
return nil, false, InvalidType(fields[:i+1], val, "map[string]interface{}")
}
case int:
if s, ok := val.([]interface{}); ok {
if typedField >= len(s) {
// index out of range
return nil, false, nil
}
val = s[typedField]
} else {
return nil, false, InvalidType(fields[:i+1], val, "[]interface{}")
}
default:
return nil, false, InvalidType(fields[:i+1], val, "string or int")
}
}
return val, true, nil
}
// InvalidType returns a *Error indicating "invalid value type". This is used
// to report malformed values (e.g. found int, expected string).
func InvalidType(fieldPath []interface{}, value interface{}, validTypes string) *field.Error {
return Invalid(fieldPath, value,
fmt.Sprintf("found type %T, expected %s", value, validTypes))
}
// Invalid returns a *Error indicating "invalid value". This is used
// to report malformed values (e.g. failed regex match, too long, out of bounds).
func Invalid(fieldPath []interface{}, value interface{}, detail string) *field.Error {
return &field.Error{
Type: field.ErrorTypeInvalid,
Field: FieldPath(fieldPath),
BadValue: value,
Detail: detail,
}
}
// NotFound returns a *Error indicating "value not found". This is
// used to report failure to find a requested value (e.g. looking up an ID).
func NotFound(fieldPath []interface{}, value interface{}) *field.Error {
return &field.Error{
Type: field.ErrorTypeNotFound,
Field: FieldPath(fieldPath),
BadValue: value,
Detail: "",
}
}
// Simplistic jsonpath formatter, just for NestedField errors.
func FieldPath(fieldPath []interface{}) string {
path := ""
for _, field := range fieldPath {
switch typedField := field.(type) {
case string:
if isSimpleString(typedField) {
path += fmt.Sprintf(".%s", typedField)
} else {
path += fmt.Sprintf("[%q]", typedField)
}
case int:
path += fmt.Sprintf("[%d]", typedField)
default:
// invalid. try anyway...
path += fmt.Sprintf(".%v", typedField)
}
}
return path
}
var simpleStringRegex = regexp.MustCompile(`^[a-zA-Z]([a-zA-Z0-9_-]*[a-zA-Z0-9])?$`)
// isSimpleString returns true if the input follows the following rules:
// - contains only alphanumeric characters, '_' or '-'
// - starts with an alphabetic character
// - ends with an alphanumeric character
func isSimpleString(s string) bool {
return simpleStringRegex.FindString(s) != ""
}

89
pkg/object/field_test.go Normal file
View File

@ -0,0 +1,89 @@
// Copyright 2020 The Kubernetes Authors.
// SPDX-License-Identifier: Apache-2.0
package object
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestFieldPath(t *testing.T) {
tests := map[string]struct {
fieldPath []interface{}
expected string
}{
"empty path": {
fieldPath: []interface{}{},
expected: "",
},
"kind": {
fieldPath: []interface{}{"kind"},
expected: ".kind",
},
"metadata.name": {
fieldPath: []interface{}{"metadata", "name"},
expected: ".metadata.name",
},
"spec.versions[1].name": {
fieldPath: []interface{}{"spec", "versions", 1, "name"},
expected: ".spec.versions[1].name",
},
"numeric": {
fieldPath: []interface{}{"spec", "123"},
expected: `.spec["123"]`,
},
"alphanumeric, ends with number": {
fieldPath: []interface{}{"spec", "abc123"},
expected: `.spec.abc123`,
},
"alphanumeric, ends with hyphen": {
fieldPath: []interface{}{"spec", "abc123-"},
expected: `.spec["abc123-"]`,
},
"alphanumeric, ends with underscore": {
fieldPath: []interface{}{"spec", "abc123_"},
expected: `.spec["abc123_"]`,
},
"alphanumeric, starts with hyphen": {
fieldPath: []interface{}{"spec", "-abc123"},
expected: `.spec["-abc123"]`,
},
"alphanumeric, starts with underscore": {
fieldPath: []interface{}{"spec", "_abc123"},
expected: `.spec["_abc123"]`,
},
"alphanumeric, starts with number": {
fieldPath: []interface{}{"spec", "_abc123"},
expected: `.spec["_abc123"]`,
},
"alphanumeric, intrnal hyphen": {
fieldPath: []interface{}{"spec", "abc-123"},
expected: `.spec.abc-123`,
},
"alphanumeric, intrnal underscore": {
fieldPath: []interface{}{"spec", "abc_123"},
expected: `.spec.abc_123`,
},
"space": {
fieldPath: []interface{}{"spec", "abc 123"},
expected: `.spec["abc 123"]`,
},
"tab": {
fieldPath: []interface{}{"spec", "abc\t123"},
expected: `.spec["abc\t123"]`,
},
"linebreak": {
fieldPath: []interface{}{"spec", "abc\n123"},
expected: `.spec["abc\n123"]`,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
result := FieldPath(tc.fieldPath)
assert.Equal(t, tc.expected, result)
})
}
}

View File

@ -5,7 +5,6 @@
package object
import (
"errors"
"fmt"
"k8s.io/apimachinery/pkg/api/meta"
@ -142,19 +141,19 @@ func LookupResourceScope(u *unstructured.Unstructured, crds []*unstructured.Unst
// If we couldn't find the type in the cluster, check if we find a
// match in any of the provided CRDs.
for _, crd := range crds {
group, found, err := unstructured.NestedString(crd.Object, "spec", "group")
group, found, err := NestedField(crd.Object, "spec", "group")
if err != nil {
return nil, fmt.Errorf("spec.group: %v", err)
return nil, err
}
if !found || group == "" {
return nil, errors.New("spec.group not found")
return nil, NotFound([]interface{}{"spec", "group"}, group)
}
kind, found, err := unstructured.NestedString(crd.Object, "spec", "names", "kind")
kind, found, err := NestedField(crd.Object, "spec", "names", "kind")
if err != nil {
return nil, fmt.Errorf("spec.kind: %v", err)
return nil, err
}
if !found || kind == "" {
return nil, errors.New("spec.kind not found")
return nil, NotFound([]interface{}{"spec", "kind"}, group)
}
if gvk.Kind != kind || gvk.Group != group {
continue
@ -168,9 +167,9 @@ func LookupResourceScope(u *unstructured.Unstructured, crds []*unstructured.Unst
GroupVersionKind: gvk,
}
}
scopeName, _, err := unstructured.NestedString(crd.Object, "spec", "scope")
scopeName, _, err := NestedField(crd.Object, "spec", "scope")
if err != nil {
return nil, fmt.Errorf("spec.scope: %v", err)
return nil, err
}
switch scopeName {
case "Namespaced":
@ -178,7 +177,8 @@ func LookupResourceScope(u *unstructured.Unstructured, crds []*unstructured.Unst
case "Cluster":
return meta.RESTScopeRoot, nil
default:
return nil, fmt.Errorf("unknown scope %q", scopeName)
return nil, Invalid([]interface{}{"spec", "scope"}, scopeName,
"expected Namespaced or Cluster")
}
}
return nil, &UnknownTypeError{
@ -187,24 +187,27 @@ func LookupResourceScope(u *unstructured.Unstructured, crds []*unstructured.Unst
}
func crdDefinesVersion(crd *unstructured.Unstructured, version string) (bool, error) {
versionsSlice, found, err := unstructured.NestedSlice(crd.Object, "spec", "versions")
versions, found, err := NestedField(crd.Object, "spec", "versions")
if err != nil {
return false, fmt.Errorf("spec.versions: %v", err)
return false, err
}
if !found || len(versionsSlice) == 0 {
return false, errors.New("spec.versions not found")
if !found {
return false, NotFound([]interface{}{"spec", "versions"}, versions)
}
for i, ver := range versionsSlice {
verObj, ok := ver.(map[string]interface{})
if !ok {
return false, fmt.Errorf("spec.versions[%d]: expecting map, got: %T", i, ver)
}
name, found, err := unstructured.NestedString(verObj, "name")
versionsSlice, ok := versions.([]interface{})
if !ok {
return false, InvalidType([]interface{}{"spec", "versions"}, versions, "[]interface{}")
}
if len(versionsSlice) == 0 {
return false, Invalid([]interface{}{"spec", "versions"}, versionsSlice, "must not be empty")
}
for i := range versionsSlice {
name, found, err := NestedField(crd.Object, "spec", "versions", i, "name")
if err != nil {
return false, fmt.Errorf("spec.versions[%d].name: %w", i, err)
return false, err
}
if !found {
return false, fmt.Errorf("spec.versions[%d].name not found", i)
return false, NotFound([]interface{}{"spec", "versions", i, "name"}, name)
}
if name == version {
return true, nil

View File

@ -58,6 +58,13 @@ func (v *Validator) Validate(resources []*unstructured.Unstructured) error {
var errs []*ValidationError
for _, r := range resources {
var errList field.ErrorList
if err := v.validateKind(r); err != nil {
if fieldErr, ok := isFieldError(err); ok {
errList = append(errList, fieldErr)
} else {
return err
}
}
if err := v.validateName(r); err != nil {
if fieldErr, ok := isFieldError(err); ok {
errList = append(errList, fieldErr)
@ -112,6 +119,14 @@ func findCRDs(us []*unstructured.Unstructured) []*unstructured.Unstructured {
return crds
}
// validateKind validates the value of the kind field of the resource.
func (v *Validator) validateKind(u *unstructured.Unstructured) error {
if u.GetKind() == "" {
return field.Required(field.NewPath("kind"), "kind is required")
}
return nil
}
// validateName validates the value of the name field of the resource.
func (v *Validator) validateName(u *unstructured.Unstructured) error {
if u.GetName() == "" {
@ -122,6 +137,10 @@ func (v *Validator) validateName(u *unstructured.Unstructured) error {
// validateNamespace validates the value of the namespace field of the resource.
func (v *Validator) validateNamespace(u *unstructured.Unstructured, crds []*unstructured.Unstructured) error {
// skip namespace validation if kind is missing (avoid redundant error)
if u.GetKind() == "" {
return nil
}
scope, err := LookupResourceScope(u, crds, v.Mapper)
if err != nil {
return err

View File

@ -22,6 +22,40 @@ func TestValidate(t *testing.T) {
resources []*unstructured.Unstructured
expectedError error
}{
"missing kind": {
resources: []*unstructured.Unstructured{
{
Object: map[string]interface{}{
"apiVersion": "apps/v1",
"metadata": map[string]interface{}{
"name": "foo",
"namespace": "default",
},
},
},
},
expectedError: &object.MultiValidationError{
Errors: []*object.ValidationError{
{
GroupVersionKind: schema.GroupVersionKind{
Group: "apps",
Version: "v1",
Kind: "",
},
Name: "foo",
Namespace: "default",
FieldErrors: []*field.Error{
{
Type: field.ErrorTypeRequired,
Field: "kind",
BadValue: "",
Detail: "kind is required",
},
},
},
},
},
},
"errors are reported for resources": {
resources: []*unstructured.Unstructured{
testutil.Unstructured(t, `
@ -241,17 +275,15 @@ metadata:
crdGV.WithResource("customresourcedefinition"), meta.RESTScopeRoot)
mapper = meta.MultiRESTMapper([]meta.RESTMapper{mapper, crdMapper})
err = (&object.Validator{
validator := &object.Validator{
Mapper: mapper,
}).Validate(tc.resources)
}
err = validator.Validate(tc.resources)
if tc.expectedError == nil {
assert.NoError(t, err)
return
}
require.Error(t, err)
assert.Equal(t, tc.expectedError, err)
require.EqualError(t, err, tc.expectedError.Error())
})
}
}

View File

@ -1,62 +0,0 @@
// Copyright 2021 The Kubernetes Authors.
// SPDX-License-Identifier: Apache-2.0
package testutil
import (
"fmt"
)
// NestedField gets a value from a KRM map, if it exists, otherwise nil.
// Fields can be string (map key) or int (array index).
func NestedField(obj map[string]interface{}, fields ...interface{}) (interface{}, bool, error) {
var val interface{} = obj
for i, field := range fields {
if val == nil {
return nil, false, nil
}
switch typedField := field.(type) {
case string:
if m, ok := val.(map[string]interface{}); ok {
val, ok = m[typedField]
if !ok {
// not in map
return nil, false, nil
}
} else {
return nil, false, fmt.Errorf("%v accessor error: %v is of the type %T, expected map[string]interface{}", jsonPath(fields[:i+1]), val, val)
}
case int:
if s, ok := val.([]interface{}); ok {
if typedField >= len(s) {
// index out of range
return nil, false, nil
}
val = s[typedField]
} else {
return nil, false, fmt.Errorf("%v accessor error: %v is of the type %T, expected []interface{}", jsonPath(fields[:i+1]), val, val)
}
default:
return nil, false, fmt.Errorf("%v accessor error: field %v is of the type %T, expected string or int", jsonPath(fields[:i+1]), field, typedField)
}
}
return val, true, nil
}
// Simplistic jsonpath formatter, just for NestedField errors.
func jsonPath(fields []interface{}) string {
path := ""
for _, field := range fields {
switch typedField := field.(type) {
case string:
path += fmt.Sprintf(".%s", typedField)
case int:
path += fmt.Sprintf("[%d]", typedField)
default:
// invalid. try anyway...
path += fmt.Sprintf(".%v", typedField)
}
}
return path
}

View File

@ -279,21 +279,21 @@ func dependsOnTest(ctx context.Context, c client.Client, invConfig InventoryConf
By("verify pod1 created and ready")
result := assertUnstructuredExists(ctx, c, pod1Obj)
podIP, found, err := testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err := object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness
By("verify pod2 created and ready")
result = assertUnstructuredExists(ctx, c, pod2Obj)
podIP, found, err = testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err = object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness
By("verify pod3 created and ready")
result = assertUnstructuredExists(ctx, c, pod3Obj)
podIP, found, err = testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err = object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness

View File

@ -179,7 +179,7 @@ func inventoryPolicyMustMatchTest(ctx context.Context, c client.Client, invConfi
By("Verify resource wasn't updated")
result := assertUnstructuredExists(ctx, c, deployment1Obj)
replicas, found, err := testutil.NestedField(result.Object, "spec", "replicas")
replicas, found, err := object.NestedField(result.Object, "spec", "replicas")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(replicas).To(Equal(int64(4)))
@ -346,12 +346,12 @@ func inventoryPolicyAdoptIfNoInventoryTest(ctx context.Context, c client.Client,
By("Verify resource was updated and added to inventory")
result := assertUnstructuredExists(ctx, c, deployment1Obj)
replicas, found, err := testutil.NestedField(result.Object, "spec", "replicas")
replicas, found, err := object.NestedField(result.Object, "spec", "replicas")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(replicas).To(Equal(int64(6)))
value, found, err := testutil.NestedField(result.Object, "metadata", "annotations", "config.k8s.io/owning-inventory")
value, found, err := object.NestedField(result.Object, "metadata", "annotations", "config.k8s.io/owning-inventory")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(value).To(Equal(invName))
@ -528,12 +528,12 @@ func inventoryPolicyAdoptAllTest(ctx context.Context, c client.Client, invConfig
By("Verify resource was updated and added to inventory")
result := assertUnstructuredExists(ctx, c, deployment1Obj)
replicas, found, err := testutil.NestedField(result.Object, "spec", "replicas")
replicas, found, err := object.NestedField(result.Object, "spec", "replicas")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(replicas).To(Equal(int64(6)))
value, found, err := testutil.NestedField(result.Object, "metadata", "annotations", "config.k8s.io/owning-inventory")
value, found, err := object.NestedField(result.Object, "metadata", "annotations", "config.k8s.io/owning-inventory")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(value).To(Equal(secondInvName))

View File

@ -228,12 +228,12 @@ func mutationTest(ctx context.Context, c client.Client, invConfig InventoryConfi
By("verify podB is created and ready")
result := assertUnstructuredExists(ctx, c, podBObj)
podIP, found, err := testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err := object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness
containerPort, found, err := testutil.NestedField(result.Object, "spec", "containers", 0, "ports", 0, "containerPort")
containerPort, found, err := object.NestedField(result.Object, "spec", "containers", 0, "ports", 0, "containerPort")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(containerPort).To(Equal(int64(80)))
@ -243,12 +243,12 @@ func mutationTest(ctx context.Context, c client.Client, invConfig InventoryConfi
By("verify podA is mutated, created, and ready")
result = assertUnstructuredExists(ctx, c, podAObj)
podIP, found, err = testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err = object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness
envValue, found, err := testutil.NestedField(result.Object, "spec", "containers", 0, "env", 0, "value")
envValue, found, err := object.NestedField(result.Object, "spec", "containers", 0, "env", 0, "value")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(envValue).To(Equal(host))

View File

@ -146,7 +146,7 @@ func pruneRetrieveErrorTest(ctx context.Context, c client.Client, invConfig Inve
By("Verify pod1 created and ready")
result := assertUnstructuredExists(ctx, c, pod1Obj)
podIP, found, err := testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err := object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness
@ -281,7 +281,7 @@ func pruneRetrieveErrorTest(ctx context.Context, c client.Client, invConfig Inve
By("Verify pod2 created and ready")
result = assertUnstructuredExists(ctx, c, pod2Obj)
podIP, found, err = testutil.NestedField(result.Object, "status", "podIP")
podIP, found, err = object.NestedField(result.Object, "status", "podIP")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(podIP).NotTo(BeEmpty()) // use podIP as proxy for readiness

View File

@ -13,7 +13,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"sigs.k8s.io/cli-utils/pkg/apply"
"sigs.k8s.io/cli-utils/pkg/common"
"sigs.k8s.io/cli-utils/pkg/testutil"
"sigs.k8s.io/cli-utils/pkg/object"
"sigs.k8s.io/controller-runtime/pkg/client"
)
@ -41,11 +41,11 @@ func serversideApplyTest(ctx context.Context, c client.Client, invConfig Invento
result := assertUnstructuredExists(ctx, c, withNamespace(manifestToUnstructured(deployment1), namespaceName))
// LastAppliedConfigAnnotation annotation is only set for client-side apply and we've server-side applied here.
_, found, err := testutil.NestedField(result.Object, "metadata", "annotations", v1.LastAppliedConfigAnnotation)
_, found, err := object.NestedField(result.Object, "metadata", "annotations", v1.LastAppliedConfigAnnotation)
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeFalse())
manager, found, err := testutil.NestedField(result.Object, "metadata", "managedFields", 0, "manager")
manager, found, err := object.NestedField(result.Object, "metadata", "managedFields", 0, "manager")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(manager).To(Equal("test"))
@ -54,11 +54,11 @@ func serversideApplyTest(ctx context.Context, c client.Client, invConfig Invento
result = assertUnstructuredExists(ctx, c, manifestToUnstructured(apiservice1))
// LastAppliedConfigAnnotation annotation is only set for client-side apply and we've server-side applied here.
_, found, err = testutil.NestedField(result.Object, "metadata", "annotations", v1.LastAppliedConfigAnnotation)
_, found, err = object.NestedField(result.Object, "metadata", "annotations", v1.LastAppliedConfigAnnotation)
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeFalse())
manager, found, err = testutil.NestedField(result.Object, "metadata", "managedFields", 0, "manager")
manager, found, err = object.NestedField(result.Object, "metadata", "managedFields", 0, "manager")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())
Expect(manager).To(Equal("test"))