Add Validate() methods to our types. (#1520)

* Add Validate() methods to our types.

This change introduces a new structured error type `FieldError` for propagating field validation errors.  The intent is to enable context-insensitive validation to still propagate fully-qualified field names.  This is done via:

```go
func (f *Foo) Validate() *FieldError {
  // Return the context-insensitive validation error.
  return &FieldError{
    Message: "always grumpy",
    Paths: []string{"list", "of", "direct", "fields"},
  },
}

func (b *Bar) Validate() *FieldError {
  if err := b.Foo.Validate(); err != nil {
    // Now propagate the validation error giving it context.
    return err.ViaField("foo")
  }
  // ViaField also handles nil, so you can write:
  return b.AnotherFoo.Validate().ViaField("anotherFoo")
}
```

Armed with these, we add `Validate() *FieldError` methods to each of our core resources, with the aim of moving the `pkg/webhook` validation to leverage them.

* Migrate `pkg/webhook` to our `Validate()` methods.

This clears out the inline validation in `pkg/webhook` and adopts the version from `pkg/apis/...`

Related: knative/serving#1518

* Fix issues found self-reviewing.

Replace an instance of `""` with `currentField` for clarity

Disable deepcopy-gen for FieldError

* Fix several typos Jon found.

* Fix the webhook integration test.

pkg/apis/serving/v1alpha1/configuration_types.go

@@ -51,6 +51,9 @@ type Configuration struct {
 	Status ConfigurationStatus `json:"status,omitempty"`
 }
 
+// Check that Configuration may be validated.
+var _ Validatable = (*Configuration)(nil)
+
 // ConfigurationSpec holds the desired state of the Configuration (from the client).
 type ConfigurationSpec struct {
 	// TODO: Generation does not work correctly with CRD. They are scrubbed

pkg/apis/serving/v1alpha1/configuration_validation.go

@@ -0,0 +1,36 @@
+/*
+Copyright 2017 The Knative 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 v1alpha1
+
+import (
+	"k8s.io/apimachinery/pkg/api/equality"
+)
+
+func (c *Configuration) Validate() *FieldError {
+	return c.Spec.Validate().ViaField("spec")
+}
+
+func (cs *ConfigurationSpec) Validate() *FieldError {
+	if equality.Semantic.DeepEqual(cs, &ConfigurationSpec{}) {
+		return errMissingField(currentField)
+	}
+	// In the context of Configuration, serving state may not be specified at all.
+	// TODO(mattmoor): Check ObjectMeta for Name/Namespace/GenerateName
+	if cs.RevisionTemplate.Spec.ServingState != "" {
+		return errDisallowedFields("revisionTemplate.spec.servingState")
+	}
+	return cs.RevisionTemplate.Validate().ViaField("revisionTemplate")
+}

pkg/apis/serving/v1alpha1/configuration_validation_test.go

@@ -0,0 +1,129 @@
+/*
+Copyright 2017 The Knative 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 v1alpha1
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestConfigurationSpecValidation(t *testing.T) {
+	tests := []struct {
+		name string
+		c    *ConfigurationSpec
+		want *FieldError
+	}{{
+		name: "valid",
+		c: &ConfigurationSpec{
+			RevisionTemplate: RevisionTemplateSpec{
+				Spec: RevisionSpec{
+					Container: corev1.Container{
+						Image: "hellworld",
+					},
+				},
+			},
+		},
+		want: nil,
+	}, {
+		// This is a Configuration specific addition to the basic Revision validation.
+		name: "specifies serving state",
+		c: &ConfigurationSpec{
+			RevisionTemplate: RevisionTemplateSpec{
+				Spec: RevisionSpec{
+					ServingState: "Active",
+					Container: corev1.Container{
+						Image: "hellworld",
+					},
+				},
+			},
+		},
+		want: errDisallowedFields("revisionTemplate.spec.servingState"),
+	}, {
+		name: "propagate revision failures",
+		c: &ConfigurationSpec{
+			RevisionTemplate: RevisionTemplateSpec{
+				Spec: RevisionSpec{
+					Container: corev1.Container{
+						Name:  "stuart",
+						Image: "hellworld",
+					},
+				},
+			},
+		},
+		want: errDisallowedFields("revisionTemplate.spec.container.name"),
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := test.c.Validate()
+			if diff := cmp.Diff(test.want, got); diff != "" {
+				t.Errorf("validateContainer (-want, +got) = %v", diff)
+			}
+		})
+	}
+}
+
+func TestConfigurationValidation(t *testing.T) {
+	tests := []struct {
+		name string
+		c    *Configuration
+		want *FieldError
+	}{{
+		name: "valid",
+		c: &Configuration{
+			Spec: ConfigurationSpec{
+				RevisionTemplate: RevisionTemplateSpec{
+					Spec: RevisionSpec{
+						Container: corev1.Container{
+							Image: "hellworld",
+						},
+					},
+				},
+			},
+		},
+		want: nil,
+	}, {
+		name: "propagate revision failures",
+		c: &Configuration{
+			Spec: ConfigurationSpec{
+				RevisionTemplate: RevisionTemplateSpec{
+					Spec: RevisionSpec{
+						Container: corev1.Container{
+							Name:  "stuart",
+							Image: "hellworld",
+						},
+					},
+				},
+			},
+		},
+		want: errDisallowedFields("spec.revisionTemplate.spec.container.name"),
+	}, {
+		name: "empty spec",
+		c:    &Configuration{},
+		want: errMissingField("spec"),
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := test.c.Validate()
+			if diff := cmp.Diff(test.want, got); diff != "" {
+				t.Errorf("validateContainer (-want, +got) = %v", diff)
+			}
+		})
+	}
+}

pkg/apis/serving/v1alpha1/field_error.go

@@ -0,0 +1,93 @@
+/*
+Copyright 2017 The Knative 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 v1alpha1
+
+import (
+	"fmt"
+	"strings"
+)
+
+// currentField is a constant to supply as a fieldPath for when there is
+// a problem with the current field itself.
+const currentField = ""
+
+// FieldError is used to propagate the context of errors pertaining to
+// specific fields in a manner suitable for use in a recursive walk, so
+// that errors contain the appropriate field context.
+// +k8s:deepcopy-gen=false
+type FieldError struct {
+	Message string
+	Paths   []string
+}
+
+// FieldError implements error
+var _ error = (*FieldError)(nil)
+
+// Validatable indicates that a particular type may have its fields validated.
+type Validatable interface {
+	// Validate checks the validity of this types fields.
+	Validate() *FieldError
+}
+
+// ViaField is used to propagate a validation error along a field access.
+// For example, if a type recursively validates its "spec" via:
+//   if err := foo.Spec.Validate(); err != nil {
+//     // Augment any field paths with the context that they were accessed
+//     // via "spec".
+//     return err.ViaField("spec")
+//   }
+func (fe *FieldError) ViaField(prefix ...string) *FieldError {
+	if fe == nil {
+		return nil
+	}
+	var newPaths []string
+	for _, oldPath := range fe.Paths {
+		if oldPath == currentField {
+			newPaths = append(newPaths, strings.Join(prefix, "."))
+		} else {
+			newPaths = append(newPaths,
+				strings.Join(append(prefix, oldPath), "."))
+		}
+	}
+	fe.Paths = newPaths
+	return fe
+}
+
+// Error implements error
+func (fe *FieldError) Error() string {
+	return fmt.Sprintf("%v: %v", fe.Message, strings.Join(fe.Paths, ", "))
+}
+
+func errMissingField(fieldPaths ...string) *FieldError {
+	return &FieldError{
+		Message: "missing field(s)",
+		Paths:   fieldPaths,
+	}
+}
+
+func errDisallowedFields(fieldPaths ...string) *FieldError {
+	return &FieldError{
+		Message: "must not set the field(s)",
+		Paths:   fieldPaths,
+	}
+}
+
+func errInvalidValue(value string, fieldPath string) *FieldError {
+	return &FieldError{
+		Message: fmt.Sprintf("invalid value %q", value),
+		Paths:   []string{fieldPath},
+	}
+}

pkg/apis/serving/v1alpha1/field_error_test.go

@@ -0,0 +1,91 @@
+/*
+Copyright 2017 The Knative 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 v1alpha1
+
+import (
+	"testing"
+)
+
+func TestFieldError(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      *FieldError
+		prefixes [][]string
+		want     string
+	}{{
+		name: "simple single no propagation",
+		err: &FieldError{
+			Message: "hear me roar",
+			Paths:   []string{"foo.bar"},
+		},
+		want: "hear me roar: foo.bar",
+	}, {
+		name: "simple single propagation",
+		err: &FieldError{
+			Message: `invalid value "blah"`,
+			Paths:   []string{"foo"},
+		},
+		prefixes: [][]string{{"bar"}, {"baz", "ugh"}, {"hoola"}},
+		want:     `invalid value "blah": hoola.baz.ugh.bar.foo`,
+	}, {
+		name: "simple multiple propagation",
+		err: &FieldError{
+			Message: "invalid field(s)",
+			Paths:   []string{"foo", "bar"},
+		},
+		prefixes: [][]string{{"baz", "ugh"}},
+		want:     "invalid field(s): baz.ugh.foo, baz.ugh.bar",
+	}, {
+		name: "single propagation, empty start",
+		err: &FieldError{
+			Message: "invalid field(s)",
+			// We might see this validating a scalar leaf.
+			Paths: []string{currentField},
+		},
+		prefixes: [][]string{{"baz", "ugh"}},
+		want:     "invalid field(s): baz.ugh",
+	}, {
+		name: "single propagation, no paths",
+		err: &FieldError{
+			Message: "invalid field(s)",
+			Paths:   nil,
+		},
+		prefixes: [][]string{{"baz", "ugh"}},
+		want:     "invalid field(s): ",
+	}, {
+		name:     "nil propagation",
+		err:      nil,
+		prefixes: [][]string{{"baz", "ugh"}},
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fe := test.err
+			// Simulate propagation up a call stack.
+			for _, prefix := range test.prefixes {
+				fe = fe.ViaField(prefix...)
+			}
+			if test.want != "" {
+				got := fe.Error()
+				if got != test.want {
+					t.Errorf("Error() = %v, wanted %v", got, test.want)
+				}
+			} else if fe != nil {
+				t.Errorf("ViaField() = %v, wanted nil", fe)
+			}
+		})
+	}
+}

pkg/apis/serving/v1alpha1/revision_types.go

@@ -46,6 +46,9 @@ type Revision struct {
 	Status RevisionStatus `json:"status,omitempty"`
 }
 
+// Check that Revision may be validated.
+var _ Validatable = (*Revision)(nil)
+
 // RevisionTemplateSpec describes the data a revision should have when created from a template.
 // Based on: https://github.com/kubernetes/api/blob/e771f807/core/v1/types.go#L3179-L3190
 type RevisionTemplateSpec struct {