From 0f1c4359d9e88c7e204c42e0d056b7f01547ef7d Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Sun, 8 Jul 2018 23:15:15 -0700 Subject: [PATCH] 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: https://github.com/knative/serving/issues/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. --- Gopkg.lock | 2 +- .../serving/v1alpha1/configuration_types.go | 3 + .../v1alpha1/configuration_validation.go | 36 ++ .../v1alpha1/configuration_validation_test.go | 129 +++++++ pkg/apis/serving/v1alpha1/field_error.go | 93 +++++ pkg/apis/serving/v1alpha1/field_error_test.go | 91 +++++ pkg/apis/serving/v1alpha1/revision_types.go | 3 + .../serving/v1alpha1/revision_validation.go | 95 +++++ .../v1alpha1/revision_validation_test.go | 327 ++++++++++++++++++ pkg/apis/serving/v1alpha1/route_types.go | 3 + pkg/apis/serving/v1alpha1/route_validation.go | 102 ++++++ .../serving/v1alpha1/route_validation_test.go | 266 ++++++++++++++ pkg/apis/serving/v1alpha1/service_types.go | 3 + .../serving/v1alpha1/service_validation.go | 57 +++ .../v1alpha1/service_validation_test.go | 265 ++++++++++++++ pkg/webhook/configuration.go | 90 +---- pkg/webhook/configuration_test.go | 53 ++- pkg/webhook/revision.go | 4 + pkg/webhook/route.go | 70 +--- pkg/webhook/route_test.go | 76 ++-- pkg/webhook/service.go | 38 +- pkg/webhook/service_test.go | 51 ++- pkg/webhook/webhook_integration_test.go | 5 +- pkg/webhook/webhook_test.go | 5 +- 24 files changed, 1624 insertions(+), 243 deletions(-) create mode 100644 pkg/apis/serving/v1alpha1/configuration_validation.go create mode 100644 pkg/apis/serving/v1alpha1/configuration_validation_test.go create mode 100644 pkg/apis/serving/v1alpha1/field_error.go create mode 100644 pkg/apis/serving/v1alpha1/field_error_test.go create mode 100644 pkg/apis/serving/v1alpha1/revision_validation.go create mode 100644 pkg/apis/serving/v1alpha1/revision_validation_test.go create mode 100644 pkg/apis/serving/v1alpha1/route_validation.go create mode 100644 pkg/apis/serving/v1alpha1/route_validation_test.go create mode 100644 pkg/apis/serving/v1alpha1/service_validation.go create mode 100644 pkg/apis/serving/v1alpha1/service_validation_test.go diff --git a/Gopkg.lock b/Gopkg.lock index 80e6bea5..1c002251 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -966,6 +966,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "1f3ec3b3f2abfd23633833a183e9d5cdfb6c0526a29acd9f6788e200fca631f8" + inputs-digest = "431ad47f0253f68661f750381a990e8e19ee70e6f3fae106cf2445fb43f74999" solver-name = "gps-cdcl" solver-version = 1 diff --git a/pkg/apis/serving/v1alpha1/configuration_types.go b/pkg/apis/serving/v1alpha1/configuration_types.go index 079b530e..5d111032 100644 --- a/pkg/apis/serving/v1alpha1/configuration_types.go +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/configuration_validation.go b/pkg/apis/serving/v1alpha1/configuration_validation.go new file mode 100644 index 00000000..5a28faeb --- /dev/null +++ b/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") +} diff --git a/pkg/apis/serving/v1alpha1/configuration_validation_test.go b/pkg/apis/serving/v1alpha1/configuration_validation_test.go new file mode 100644 index 00000000..0401b6fb --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/apis/serving/v1alpha1/field_error.go b/pkg/apis/serving/v1alpha1/field_error.go new file mode 100644 index 00000000..acb7ad10 --- /dev/null +++ b/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}, + } +} diff --git a/pkg/apis/serving/v1alpha1/field_error_test.go b/pkg/apis/serving/v1alpha1/field_error_test.go new file mode 100644 index 00000000..b3982af0 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 936a4af5..f9efb497 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/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 { diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go new file mode 100644 index 00000000..954197a0 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/revision_validation.go @@ -0,0 +1,95 @@ +/* +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 ( + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" +) + +func (rt *Revision) Validate() *FieldError { + return rt.Spec.Validate().ViaField("spec") +} + +func (rt *RevisionTemplateSpec) Validate() *FieldError { + return rt.Spec.Validate().ViaField("spec") +} + +func (rs *RevisionSpec) Validate() *FieldError { + if equality.Semantic.DeepEqual(rs, &RevisionSpec{}) { + return errMissingField(currentField) + } + if err := rs.ServingState.Validate(); err != nil { + return err.ViaField("servingState") + } + if err := validateContainer(rs.Container); err != nil { + return err.ViaField("container") + } + return rs.ConcurrencyModel.Validate().ViaField("concurrencyModel") +} + +func (ss RevisionServingStateType) Validate() *FieldError { + switch ss { + case RevisionServingStateType(""), + RevisionServingStateRetired, + RevisionServingStateReserve, + RevisionServingStateActive: + return nil + default: + return errInvalidValue(string(ss), currentField) + } +} + +func (cm RevisionRequestConcurrencyModelType) Validate() *FieldError { + switch cm { + case RevisionRequestConcurrencyModelType(""), + RevisionRequestConcurrencyModelMulti, + RevisionRequestConcurrencyModelSingle: + return nil + default: + return errInvalidValue(string(cm), currentField) + } +} + +func validateContainer(container corev1.Container) *FieldError { + if equality.Semantic.DeepEqual(container, corev1.Container{}) { + return errMissingField(currentField) + } + // Some corev1.Container fields are set by Knative Serving controller. We disallow them + // here to avoid silently overwriting these fields and causing confusions for + // the users. See pkg/controller/revision/resources/deploy.go#makePodSpec. + var ignoredFields []string + if container.Name != "" { + ignoredFields = append(ignoredFields, "name") + } + if !equality.Semantic.DeepEqual(container.Resources, corev1.ResourceRequirements{}) { + ignoredFields = append(ignoredFields, "resources") + } + if len(container.Ports) > 0 { + ignoredFields = append(ignoredFields, "ports") + } + if len(container.VolumeMounts) > 0 { + ignoredFields = append(ignoredFields, "volumeMounts") + } + if container.Lifecycle != nil { + ignoredFields = append(ignoredFields, "lifecycle") + } + if len(ignoredFields) > 0 { + // Complain about all ignored fields so that user can remove them all at once. + return errDisallowedFields(ignoredFields...) + } + return nil +} diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go new file mode 100644 index 00000000..1a5684f7 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -0,0 +1,327 @@ +/* +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" + "k8s.io/apimachinery/pkg/api/resource" +) + +func TestContainerValidation(t *testing.T) { + tests := []struct { + name string + c corev1.Container + want *FieldError + }{{ + name: "empty container", + c: corev1.Container{}, + want: errMissingField(currentField), + }, { + name: "valid container", + c: corev1.Container{ + Image: "foo", + }, + want: nil, + }, { + name: "has a name", + c: corev1.Container{ + Name: "foo", + }, + want: errDisallowedFields("name"), + }, { + name: "has resources", + c: corev1.Container{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceName("cpu"): resource.MustParse("25m"), + }, + }, + }, + want: errDisallowedFields("resources"), + }, { + name: "has ports", + c: corev1.Container{ + Ports: []corev1.ContainerPort{{ + Name: "http", + ContainerPort: 8080, + }}, + }, + want: errDisallowedFields("ports"), + }, { + name: "has volumeMounts", + c: corev1.Container{ + VolumeMounts: []corev1.VolumeMount{{ + MountPath: "mount/path", + Name: "name", + }}, + }, + want: errDisallowedFields("volumeMounts"), + }, { + name: "has lifecycle", + c: corev1.Container{ + Lifecycle: &corev1.Lifecycle{}, + }, + want: errDisallowedFields("lifecycle"), + }, { + name: "has numerous problems", + c: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceName("cpu"): resource.MustParse("25m"), + }, + }, + Ports: []corev1.ContainerPort{{ + Name: "http", + ContainerPort: 8080, + }}, + VolumeMounts: []corev1.VolumeMount{{ + MountPath: "mount/path", + Name: "name", + }}, + Lifecycle: &corev1.Lifecycle{}, + }, + want: errDisallowedFields("name", "resources", "ports", "volumeMounts", "lifecycle"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := validateContainer(test.c) + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("validateContainer (-want, +got) = %v", diff) + } + }) + } +} + +func TestConcurrencyModelValidation(t *testing.T) { + tests := []struct { + name string + cm RevisionRequestConcurrencyModelType + want *FieldError + }{{ + name: "single", + cm: RevisionRequestConcurrencyModelSingle, + want: nil, + }, { + name: "multi", + cm: RevisionRequestConcurrencyModelMulti, + want: nil, + }, { + name: "empty", + cm: "", + want: nil, + }, { + name: "bogus", + cm: "bogus", + want: errInvalidValue("bogus", currentField), + }, { + name: "balderdash", + cm: "balderdash", + want: errInvalidValue("balderdash", currentField), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.cm.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestServingStateValidation(t *testing.T) { + tests := []struct { + name string + ss RevisionServingStateType + want *FieldError + }{{ + name: "active", + ss: "Active", + want: nil, + }, { + name: "reserve", + ss: "Reserve", + want: nil, + }, { + name: "retired", + ss: "Retired", + want: nil, + }, { + name: "empty", + ss: "", + want: nil, + }, { + name: "bogus", + ss: "bogus", + want: errInvalidValue("bogus", currentField), + }, { + name: "balderdash", + ss: "balderdash", + want: errInvalidValue("balderdash", currentField), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.ss.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestRevisionSpecValidation(t *testing.T) { + tests := []struct { + name string + rs *RevisionSpec + want *FieldError + }{{ + name: "valid", + rs: &RevisionSpec{ + Container: corev1.Container{ + Image: "helloworld", + }, + ConcurrencyModel: "Multi", + }, + want: nil, + }, { + name: "has bad serving state", + rs: &RevisionSpec{ + ServingState: "blah", + }, + want: errInvalidValue("blah", "servingState"), + }, { + name: "bad concurrency model", + rs: &RevisionSpec{ + Container: corev1.Container{ + Image: "helloworld", + }, + ConcurrencyModel: "bogus", + }, + want: errInvalidValue("bogus", "concurrencyModel"), + }, { + name: "bad container spec", + rs: &RevisionSpec{ + Container: corev1.Container{ + Name: "steve", + Image: "helloworld", + }, + }, + want: errDisallowedFields("container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.rs.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestRevisionTemplateSpecValidation(t *testing.T) { + tests := []struct { + name string + rts *RevisionTemplateSpec + want *FieldError + }{{ + name: "valid", + rts: &RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "helloworld", + }, + ConcurrencyModel: "Multi", + }, + }, + want: nil, + }, { + name: "empty spec", + rts: &RevisionTemplateSpec{}, + want: errMissingField("spec"), + }, { + name: "nested spec error", + rts: &RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "kevin", + Image: "helloworld", + }, + ConcurrencyModel: "Multi", + }, + }, + want: errDisallowedFields("spec.container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.rts.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestRevisionValidation(t *testing.T) { + tests := []struct { + name string + r *Revision + want *FieldError + }{{ + name: "valid", + r: &Revision{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "helloworld", + }, + ConcurrencyModel: "Multi", + }, + }, + want: nil, + }, { + name: "empty spec", + r: &Revision{}, + want: errMissingField("spec"), + }, { + name: "nested spec error", + r: &Revision{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "kevin", + Image: "helloworld", + }, + ConcurrencyModel: "Multi", + }, + }, + want: errDisallowedFields("spec.container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.r.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} diff --git a/pkg/apis/serving/v1alpha1/route_types.go b/pkg/apis/serving/v1alpha1/route_types.go index eafcf64b..352e1790 100644 --- a/pkg/apis/serving/v1alpha1/route_types.go +++ b/pkg/apis/serving/v1alpha1/route_types.go @@ -49,6 +49,9 @@ type Route struct { Status RouteStatus `json:"status,omitempty"` } +// Check that Route may be validated. +var _ Validatable = (*Route)(nil) + // TrafficTarget holds a single entry of the routing table for a Route. type TrafficTarget struct { // Name is optionally used to expose a dedicated hostname for referencing this diff --git a/pkg/apis/serving/v1alpha1/route_validation.go b/pkg/apis/serving/v1alpha1/route_validation.go new file mode 100644 index 00000000..a4356d89 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/route_validation.go @@ -0,0 +1,102 @@ +/* +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" + + "k8s.io/apimachinery/pkg/api/equality" +) + +func (rt *Route) Validate() *FieldError { + return rt.Spec.Validate().ViaField("spec") +} + +func (rs *RouteSpec) Validate() *FieldError { + if equality.Semantic.DeepEqual(rs, &RouteSpec{}) { + return errMissingField(currentField) + } + + // Where a named traffic target points + type namedTarget struct { + r string // revision name + c string // config name + i int // index of first occurrence + } + + // Track the targets of named TrafficTarget entries (to detect duplicates). + trafficMap := make(map[string]namedTarget) + + percentSum := 0 + for i, tt := range rs.Traffic { + if err := tt.Validate(); err != nil { + return err.ViaField(fmt.Sprintf("traffic[%d]", i)) + } + percentSum += tt.Percent + + if tt.Name == "" { + // No Name field, so skip the uniqueness check. + continue + } + nt := namedTarget{ + r: tt.RevisionName, + c: tt.ConfigurationName, + i: i, + } + if ent, ok := trafficMap[tt.Name]; !ok { + // No entry exists, so add ours + trafficMap[tt.Name] = nt + } else if ent.r != nt.r || ent.c != nt.c { + return &FieldError{ + Message: fmt.Sprintf("Multiple definitions for %q", tt.Name), + Paths: []string{ + fmt.Sprintf("traffic[%d].name", ent.i), + fmt.Sprintf("traffic[%d].name", nt.i), + }, + } + } + } + + if percentSum != 100 { + return &FieldError{ + Message: fmt.Sprintf("Traffic targets sum to %d, want 100", percentSum), + Paths: []string{"traffic"}, + } + } + return nil +} + +func (tt *TrafficTarget) Validate() *FieldError { + switch { + case tt.RevisionName != "" && tt.ConfigurationName != "": + return &FieldError{ + Message: "Expected exactly one, got both", + Paths: []string{"revisionName", "configurationName"}, + } + case tt.RevisionName != "": + case tt.ConfigurationName != "": + // These are fine. + default: + return &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"revisionName", "configurationName"}, + } + } + if tt.Percent < 0 || tt.Percent > 100 { + return errInvalidValue(fmt.Sprintf("%d", tt.Percent), "percent") + } + return nil +} diff --git a/pkg/apis/serving/v1alpha1/route_validation_test.go b/pkg/apis/serving/v1alpha1/route_validation_test.go new file mode 100644 index 00000000..4702ae10 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/route_validation_test.go @@ -0,0 +1,266 @@ +/* +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" +) + +func TestRouteValidation(t *testing.T) { + tests := []struct { + name string + r *Route + want *FieldError + }{{ + name: "valid", + r: &Route{ + Spec: RouteSpec{ + Traffic: []TrafficTarget{{ + RevisionName: "foo", + Percent: 100, + }}, + }, + }, + want: nil, + }, { + name: "valid split", + r: &Route{ + Spec: RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "prod", + RevisionName: "foo", + Percent: 90, + }, { + Name: "experiment", + ConfigurationName: "bar", + Percent: 10, + }}, + }, + }, + want: nil, + }, { + name: "invalid traffic entry", + r: &Route{ + Spec: RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "foo", + Percent: 100, + }}, + }, + }, + want: &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{ + "spec.traffic[0].revisionName", + "spec.traffic[0].configurationName", + }, + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.r.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestRouteSpecValidation(t *testing.T) { + tests := []struct { + name string + rs *RouteSpec + want *FieldError + }{{ + name: "valid", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + RevisionName: "foo", + Percent: 100, + }}, + }, + want: nil, + }, { + name: "valid split", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "prod", + RevisionName: "foo", + Percent: 90, + }, { + Name: "experiment", + ConfigurationName: "bar", + Percent: 10, + }}, + }, + want: nil, + }, { + name: "empty spec", + rs: &RouteSpec{}, + want: errMissingField(currentField), + }, { + name: "invalid traffic entry", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "foo", + Percent: 100, + }}, + }, + want: &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"traffic[0].revisionName", "traffic[0].configurationName"}, + }, + }, { + name: "invalid name conflict", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "foo", + RevisionName: "bar", + Percent: 50, + }, { + Name: "foo", + RevisionName: "baz", + Percent: 50, + }}, + }, + want: &FieldError{ + Message: `Multiple definitions for "foo"`, + Paths: []string{"traffic[0].name", "traffic[1].name"}, + }, + }, { + name: "valid name collision (same revision)", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + Name: "foo", + RevisionName: "bar", + Percent: 50, + }, { + Name: "foo", + RevisionName: "bar", + Percent: 50, + }}, + }, + want: nil, + }, { + name: "invalid total percentage", + rs: &RouteSpec{ + Traffic: []TrafficTarget{{ + RevisionName: "bar", + Percent: 99, + }, { + RevisionName: "baz", + Percent: 99, + }}, + }, + want: &FieldError{ + Message: "Traffic targets sum to 198, want 100", + Paths: []string{"traffic"}, + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.rs.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} + +func TestTrafficTargetValidation(t *testing.T) { + tests := []struct { + name string + tt *TrafficTarget + want *FieldError + }{{ + name: "valid with name and revision", + tt: &TrafficTarget{ + Name: "foo", + RevisionName: "bar", + Percent: 12, + }, + want: nil, + }, { + name: "valid with name and configuration", + tt: &TrafficTarget{ + Name: "baz", + ConfigurationName: "blah", + Percent: 37, + }, + want: nil, + }, { + name: "valid with no percent", + tt: &TrafficTarget{ + Name: "ooga", + ConfigurationName: "booga", + }, + want: nil, + }, { + name: "valid with no name", + tt: &TrafficTarget{ + ConfigurationName: "booga", + Percent: 100, + }, + want: nil, + }, { + name: "invalid with both", + tt: &TrafficTarget{ + RevisionName: "foo", + ConfigurationName: "bar", + }, + want: &FieldError{ + Message: "Expected exactly one, got both", + Paths: []string{"revisionName", "configurationName"}, + }, + }, { + name: "invalid with neither", + tt: &TrafficTarget{ + Name: "foo", + Percent: 100, + }, + want: &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"revisionName", "configurationName"}, + }, + }, { + name: "invalid percent too low", + tt: &TrafficTarget{ + RevisionName: "foo", + Percent: -5, + }, + want: errInvalidValue("-5", "percent"), + }, { + name: "invalid percent too high", + tt: &TrafficTarget{ + RevisionName: "foo", + Percent: 101, + }, + want: errInvalidValue("101", "percent"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.tt.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("Validate (-want, +got) = %v", diff) + } + }) + } +} diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index a0caf049..fba91833 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -39,6 +39,9 @@ type Service struct { Status ServiceStatus `json:"status,omitempty"` } +// Check that Service may be validated. +var _ Validatable = (*Service)(nil) + // ServiceSpec represents the configuration for the Service object. // Exactly one of its members (other than Generation) must be specified. type ServiceSpec struct { diff --git a/pkg/apis/serving/v1alpha1/service_validation.go b/pkg/apis/serving/v1alpha1/service_validation.go new file mode 100644 index 00000000..8cc2c9b3 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/service_validation.go @@ -0,0 +1,57 @@ +/* +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 + +func (s *Service) Validate() *FieldError { + return s.Spec.Validate().ViaField("spec") +} + +func (ss *ServiceSpec) Validate() *FieldError { + // We would do this semantic DeepEqual, but the spec is comprised + // entirely of a oneof, the validation for which produces a clearer + // error message. + // if equality.Semantic.DeepEqual(ss, &ServiceSpec{}) { + // return errMissingField(currentField) + // } + + switch { + case ss.RunLatest != nil && ss.Pinned != nil: + return &FieldError{ + Message: "Expected exactly one, got both", + Paths: []string{"runLatest", "pinned"}, + } + case ss.RunLatest != nil: + return ss.RunLatest.Validate().ViaField("runLatest") + case ss.Pinned != nil: + return ss.Pinned.Validate().ViaField("pinned") + default: + return &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"runLatest", "pinned"}, + } + } +} + +func (pt *PinnedType) Validate() *FieldError { + if pt.RevisionName == "" { + return errMissingField("revisionName") + } + return pt.Configuration.Validate().ViaField("configuration") +} + +func (rlt *RunLatestType) Validate() *FieldError { + return rlt.Configuration.Validate().ViaField("configuration") +} diff --git a/pkg/apis/serving/v1alpha1/service_validation_test.go b/pkg/apis/serving/v1alpha1/service_validation_test.go new file mode 100644 index 00000000..12d6a7b3 --- /dev/null +++ b/pkg/apis/serving/v1alpha1/service_validation_test.go @@ -0,0 +1,265 @@ +/* +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 TestServiceValidation(t *testing.T) { + tests := []struct { + name string + s *Service + want *FieldError + }{{ + name: "valid runLatest", + s: &Service{ + Spec: ServiceSpec{ + RunLatest: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + }, + }, + want: nil, + }, { + name: "valid pinned", + s: &Service{ + Spec: ServiceSpec{ + Pinned: &PinnedType{ + RevisionName: "asdf", + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + }, + }, + want: nil, + }, { + name: "invalid both types", + s: &Service{ + Spec: ServiceSpec{ + RunLatest: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + Pinned: &PinnedType{ + RevisionName: "asdf", + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + }, + }, + want: &FieldError{ + Message: "Expected exactly one, got both", + Paths: []string{"spec.runLatest", "spec.pinned"}, + }, + }, { + name: "invalid neither type", + s: &Service{}, + want: &FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"spec.runLatest", "spec.pinned"}, + }, + }, { + name: "invalid runLatest", + s: &Service{ + Spec: ServiceSpec{ + RunLatest: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "foo", + Image: "hellworld", + }, + }, + }, + }, + }, + }, + }, + want: errDisallowedFields("spec.runLatest.configuration.revisionTemplate.spec.container.name"), + }, { + name: "invalid pinned", + s: &Service{ + Spec: ServiceSpec{ + Pinned: &PinnedType{ + RevisionName: "asdf", + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "foo", + Image: "hellworld", + }, + }, + }, + }, + }, + }, + }, + want: errDisallowedFields("spec.pinned.configuration.revisionTemplate.spec.container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.s.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("validateContainer (-want, +got) = %v", diff) + } + }) + } +} + +func TestRunLatestTypeValidation(t *testing.T) { + tests := []struct { + name string + rlt *RunLatestType + want *FieldError + }{{ + name: "valid", + rlt: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + want: nil, + }, { + name: "propagate revision failures", + rlt: &RunLatestType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "stuart", + Image: "hellworld", + }, + }, + }, + }, + }, + want: errDisallowedFields("configuration.revisionTemplate.spec.container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.rlt.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("validateContainer (-want, +got) = %v", diff) + } + }) + } +} + +func TestPinnedTypeValidation(t *testing.T) { + tests := []struct { + name string + pt *PinnedType + want *FieldError + }{{ + name: "valid", + pt: &PinnedType{ + RevisionName: "foo", + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Image: "hellworld", + }, + }, + }, + }, + }, + want: nil, + }, { + name: "missing revision name", + pt: &PinnedType{ + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "stuart", + Image: "hellworld", + }, + }, + }, + }, + }, + want: errMissingField("revisionName"), + }, { + name: "propagate revision failures", + pt: &PinnedType{ + RevisionName: "foo", + Configuration: ConfigurationSpec{ + RevisionTemplate: RevisionTemplateSpec{ + Spec: RevisionSpec{ + Container: corev1.Container{ + Name: "stuart", + Image: "hellworld", + }, + }, + }, + }, + }, + want: errDisallowedFields("configuration.revisionTemplate.spec.container.name"), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.pt.Validate() + if diff := cmp.Diff(test.want, got); diff != "" { + t.Errorf("validateContainer (-want, +got) = %v", diff) + } + }) + } +} diff --git a/pkg/webhook/configuration.go b/pkg/webhook/configuration.go index 70dd15c6..d8d0452e 100644 --- a/pkg/webhook/configuration.go +++ b/pkg/webhook/configuration.go @@ -18,30 +18,15 @@ package webhook import ( "context" "errors" - "fmt" "path" - "reflect" - "strings" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/logging" "github.com/mattbaird/jsonpatch" - corev1 "k8s.io/api/core/v1" ) -func errMissingField(fieldPath string) error { - return fmt.Errorf("Configuration is missing %q", fieldPath) -} - -func errDisallowedFields(fieldPaths string) error { - return fmt.Errorf("The configuration spec must not set the field(s): %s", fieldPaths) -} - var ( - errEmptySpecInConfiguration = errMissingField("spec") - errEmptyRevisionTemplateInSpec = errMissingField("spec.revisionTemplate") - errEmptyContainerInRevisionTemplate = errMissingField("spec.revisionTemplate.spec.container") - errInvalidConfigurationInput = errors.New("failed to convert input into Configuration") + errInvalidConfigurationInput = errors.New("failed to convert input into Configuration") ) // ValidateConfiguration is Configuration resource specific validation and mutation handler @@ -51,82 +36,15 @@ func ValidateConfiguration(ctx context.Context) ResourceCallback { if err != nil { return err } - if err := validateConfiguration(newConfiguration); err != nil { + + // Can't just `return newConfiguration.Validate()` because it doesn't properly nil-check. + if err := newConfiguration.Validate(); err != nil { return err } return nil } } -func validateConfiguration(configuration *v1alpha1.Configuration) error { - if reflect.DeepEqual(configuration.Spec, v1alpha1.ConfigurationSpec{}) { - return errEmptySpecInConfiguration - } - if err := validateConfigurationSpec(&configuration.Spec); err != nil { - return err - } - return validateConcurrencyModel(configuration.Spec.RevisionTemplate.Spec.ConcurrencyModel) -} - -func validateConfigurationSpec(configurationSpec *v1alpha1.ConfigurationSpec) error { - if err := validateTemplate(&configurationSpec.RevisionTemplate); err != nil { - return err - } - return nil -} - -func validateTemplate(template *v1alpha1.RevisionTemplateSpec) error { - if reflect.DeepEqual(*template, v1alpha1.RevisionTemplateSpec{}) { - return errEmptyRevisionTemplateInSpec - } - if template.Spec.ServingState != "" { - return errDisallowedFields("revisionTemplate.spec.servingState") - } - if err := validateContainer(template.Spec.Container); err != nil { - return err - } - return nil -} - -func validateConcurrencyModel(value v1alpha1.RevisionRequestConcurrencyModelType) error { - switch value { - case v1alpha1.RevisionRequestConcurrencyModelType(""), v1alpha1.RevisionRequestConcurrencyModelMulti, v1alpha1.RevisionRequestConcurrencyModelSingle: - return nil - default: - return fmt.Errorf("Unrecognized value for concurrencyModel: %q", value) - } -} - -func validateContainer(container corev1.Container) error { - if reflect.DeepEqual(container, corev1.Container{}) { - return errEmptyContainerInRevisionTemplate - } - // Some corev1.Container fields are set by Knative Serving controller. We disallow them - // here to avoid silently overwriting these fields and causing confusions for - // the users. See pkg/controller/revision/resources/deploy.go#makePodSpec. - var ignoredFields []string - if container.Name != "" { - ignoredFields = append(ignoredFields, "revisionTemplate.spec.container.name") - } - if !reflect.DeepEqual(container.Resources, corev1.ResourceRequirements{}) { - ignoredFields = append(ignoredFields, "revisionTemplate.spec.container.resources") - } - if len(container.Ports) > 0 { - ignoredFields = append(ignoredFields, "revisionTemplate.spec.container.ports") - } - if len(container.VolumeMounts) > 0 { - ignoredFields = append(ignoredFields, "revisionTemplate.spec.container.volumeMounts") - } - if container.Lifecycle != nil { - ignoredFields = append(ignoredFields, "revisionTemplate.spec.container.lifecycle") - } - if len(ignoredFields) > 0 { - // Complain about all ignored fields so that user can remove them all at once. - return errDisallowedFields(strings.Join(ignoredFields, ", ")) - } - return nil -} - // SetConfigurationDefaults set defaults on an configurations. func SetConfigurationDefaults(ctx context.Context) ResourceDefaulter { return func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error { diff --git a/pkg/webhook/configuration_test.go b/pkg/webhook/configuration_test.go index f7dcf2a0..c902f077 100644 --- a/pkg/webhook/configuration_test.go +++ b/pkg/webhook/configuration_test.go @@ -17,7 +17,6 @@ package webhook import ( "fmt" - "strings" "testing" "github.com/knative/serving/pkg/apis/serving/v1alpha1" @@ -50,8 +49,13 @@ func TestEmptySpecInConfigurationNotAllowed(t *testing.T) { Spec: v1alpha1.ConfigurationSpec{}, } - if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err != errEmptySpecInConfiguration { - t.Fatalf("Expected: %s. Failed with %s", errEmptySpecInConfiguration, err) + got := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration) + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateConfiguration() = %v, wanted %v", got, want) } } @@ -67,8 +71,13 @@ func TestEmptyTemplateInSpecNotAllowed(t *testing.T) { }, } - if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err != errEmptyRevisionTemplateInSpec { - t.Fatalf("Expected: %s. Failed with %s", errEmptyRevisionTemplateInSpec, err) + got := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration) + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec.revisionTemplate.spec"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateConfiguration() = %v, wanted %v", got, want) } } @@ -88,8 +97,13 @@ func TestEmptyContainerNotAllowed(t *testing.T) { }, } - if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err != errEmptyContainerInRevisionTemplate { - t.Fatalf("Expected: %v. Failed with %v", errEmptyRevisionTemplateInSpec, err) + got := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration) + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec.revisionTemplate.spec.container"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateConfiguration() = %v, wanted %v", got, want) } } @@ -112,7 +126,7 @@ func TestServingStateNotAllowed(t *testing.T) { }, }, } - expected := fmt.Sprintf("The configuration spec must not set the field(s): revisionTemplate.spec.servingState") + expected := fmt.Sprintf("must not set the field(s): spec.revisionTemplate.spec.servingState") if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err == nil || err.Error() != expected { t.Fatalf("Result of ValidateConfiguration function: %s. Expected: %s.", err, expected) } @@ -150,24 +164,29 @@ func TestUnwantedFieldInContainerNotAllowed(t *testing.T) { }, }, } - unwanted := []string{ - "revisionTemplate.spec.container.name", - "revisionTemplate.spec.container.resources", - "revisionTemplate.spec.container.ports", - "revisionTemplate.spec.container.volumeMounts", - "revisionTemplate.spec.container.lifecycle", + want := &v1alpha1.FieldError{ + Message: "must not set the field(s)", + Paths: []string{ + "spec.revisionTemplate.spec.container.name", + "spec.revisionTemplate.spec.container.resources", + "spec.revisionTemplate.spec.container.ports", + "spec.revisionTemplate.spec.container.volumeMounts", + "spec.revisionTemplate.spec.container.lifecycle", + }, } - expected := fmt.Sprintf("The configuration spec must not set the field(s): %s", strings.Join(unwanted, ", ")) + expected := want.Error() if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err == nil || err.Error() != expected { t.Fatalf("Expected: %s. Failed with %s", expected, err) } configuration.Spec.RevisionTemplate.Spec.Container.Name = "" - expected = fmt.Sprintf("The configuration spec must not set the field(s): %s", strings.Join(unwanted[1:], ", ")) + want.Paths = want.Paths[1:] + expected = want.Error() if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err == nil || err.Error() != expected { t.Fatalf("Expected: %s. Failed with %s", expected, err) } configuration.Spec.RevisionTemplate.Spec.Container.Resources = corev1.ResourceRequirements{} - expected = fmt.Sprintf("The configuration spec must not set the field(s): %s", strings.Join(unwanted[2:], ", ")) + want.Paths = want.Paths[1:] + expected = want.Error() if err := ValidateConfiguration(TestContextWithLogger(t))(nil, &configuration, &configuration); err == nil || err.Error() != expected { t.Fatalf("Expected: %s. Failed with %s", expected, err) } diff --git a/pkg/webhook/revision.go b/pkg/webhook/revision.go index 0b44ca2b..c1a7fa56 100644 --- a/pkg/webhook/revision.go +++ b/pkg/webhook/revision.go @@ -50,6 +50,10 @@ func ValidateRevision(ctx context.Context) ResourceCallback { } } + // Can't just `return newRevision.Validate()` because it doesn't properly nil-check. + if err := n.Validate(); err != nil { + return err + } return nil } } diff --git a/pkg/webhook/route.go b/pkg/webhook/route.go index 2cab4257..773a483c 100644 --- a/pkg/webhook/route.go +++ b/pkg/webhook/route.go @@ -25,11 +25,7 @@ import ( ) var ( - errInvalidRevisions = errors.New("the route must have exactly one of revisionName or configurationName in traffic field") - errInvalidRouteInput = errors.New("failed to convert input into Route") - errInvalidTargetPercentSum = errors.New("the route must have traffic percent sum equal to 100") - errNegativeTargetPercent = errors.New("the route cannot have a negative traffic percent") - errTrafficTargetsNotUnique = errors.New("the traffic targets must be unique") + errInvalidRouteInput = errors.New("failed to convert input into Route") ) // ValidateRoute is Route resource specific validation and mutation handler @@ -39,71 +35,13 @@ func ValidateRoute(ctx context.Context) ResourceCallback { if err != nil { return err } - if err := validateTrafficTarget(newRoute); err != nil { - return err - } - if err := validateUniqueTrafficTarget(newRoute); err != nil { + + // Can't just `return newRoute.Validate()` because it doesn't properly nil-check. + if err := newRoute.Validate(); err != nil { return err } - - return nil - } -} - -func validateTrafficTarget(route *v1alpha1.Route) error { - // A service as a placeholder that's not backed by anything is allowed. - if route.Spec.Traffic == nil { return nil } - - percentSum := 0 - for _, trafficTarget := range route.Spec.Traffic { - revisionLen := len(trafficTarget.RevisionName) - configurationLen := len(trafficTarget.ConfigurationName) - if (revisionLen == 0 && configurationLen == 0) || - (revisionLen != 0 && configurationLen != 0) { - return errInvalidRevisions - } - - if trafficTarget.Percent < 0 { - return errNegativeTargetPercent - } - percentSum += trafficTarget.Percent - } - - if percentSum != 100 { - return errInvalidTargetPercentSum - } - return nil -} - -func validateUniqueTrafficTarget(route *v1alpha1.Route) error { - if route.Spec.Traffic == nil { - return nil - } - - type tt struct { - revision string - configuration string - } - trafficMap := make(map[string]tt) - for _, trafficTarget := range route.Spec.Traffic { - if trafficTarget.Name == "" { - continue - } - - traffic := tt{ - revision: trafficTarget.RevisionName, - configuration: trafficTarget.ConfigurationName, - } - - if _, ok := trafficMap[trafficTarget.Name]; !ok { - trafficMap[trafficTarget.Name] = traffic - } else if trafficMap[trafficTarget.Name] != traffic { - return errTrafficTargetsNotUnique - } - } - return nil } func unmarshalRoutes( diff --git a/pkg/webhook/route_test.go b/pkg/webhook/route_test.go index a11b8077..d7fb3076 100644 --- a/pkg/webhook/route_test.go +++ b/pkg/webhook/route_test.go @@ -50,13 +50,14 @@ func TestValidRouteWithTrafficAllowed(t *testing.T) { } } -func TestEmptyTrafficTargetWithoutTrafficAllowed(t *testing.T) { - route := createRouteWithTraffic(nil) +// TODO(mattmoor): Why would we allow this? +// func TestEmptyTrafficTargetWithoutTrafficAllowed(t *testing.T) { +// route := createRouteWithTraffic(nil) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != nil { - t.Fatalf("Expected allowed, but failed with: %s.", err) - } -} +// if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != nil { +// t.Fatalf("Expected allowed, but failed with: %s.", err) +// } +// } func TestNoneRouteTypeForOldResourceNotAllowed(t *testing.T) { revision := v1alpha1.Revision{ @@ -66,8 +67,10 @@ func TestNoneRouteTypeForOldResourceNotAllowed(t *testing.T) { }, } - if err := ValidateRoute(TestContextWithLogger(t))(nil, &revision, &revision); err != errInvalidRouteInput { - t.Fatalf("Expected: %s. Failed with: %s.", errInvalidRouteInput, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &revision, &revision) + want := errInvalidRouteInput + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -79,8 +82,10 @@ func TestNoneRouteTypeForNewResourceNotAllowed(t *testing.T) { }, } - if err := ValidateRoute(TestContextWithLogger(t))(nil, nil, &revision); err != errInvalidRouteInput { - t.Fatalf("Expected: %s. Failed with: %s.", errInvalidRouteInput, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, nil, &revision) + want := errInvalidRouteInput + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -89,8 +94,16 @@ func TestEmptyRevisionAndConfigurationInOneTargetNotAllowed(t *testing.T) { Percent: 100, }}) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != errInvalidRevisions { - t.Fatalf("Expected: %s. Failed with: %s.", errInvalidRevisions, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route) + want := &v1alpha1.FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{ + "spec.traffic[0].revisionName", + "spec.traffic[0].configurationName", + }, + } + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -101,8 +114,16 @@ func TestBothRevisionAndConfigurationInOneTargetNotAllowed(t *testing.T) { Percent: 100, }}) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != errInvalidRevisions { - t.Fatalf("Expected: %s. Failed with: %s.", errInvalidRevisions, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route) + want := &v1alpha1.FieldError{ + Message: "Expected exactly one, got both", + Paths: []string{ + "spec.traffic[0].revisionName", + "spec.traffic[0].configurationName", + }, + } + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -112,8 +133,13 @@ func TestNegativeTargetPercentNotAllowed(t *testing.T) { Percent: -20, }}) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != errNegativeTargetPercent { - t.Fatalf("Expected: %s. Failed with: %s.", errNegativeTargetPercent, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route) + want := &v1alpha1.FieldError{ + Message: `invalid value "-20"`, + Paths: []string{"spec.traffic[0].percent"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -125,8 +151,13 @@ func TestNotAllowedIfTrafficPercentSumIsNot100(t *testing.T) { Percent: 50, }}) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != errInvalidTargetPercentSum { - t.Fatalf("Expected: %s. Failed with: %s.", errInvalidTargetPercentSum, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route) + want := &v1alpha1.FieldError{ + Message: "Traffic targets sum to 50, want 100", + Paths: []string{"spec.traffic"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } @@ -141,7 +172,12 @@ func TestNotAllowedIfTrafficNamesNotUnique(t *testing.T) { Percent: 50, }}) - if err := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route); err != errTrafficTargetsNotUnique { - t.Fatalf("Expected: %s. Failed with: %s.", errTrafficTargetsNotUnique, err) + got := ValidateRoute(TestContextWithLogger(t))(nil, &route, &route) + want := &v1alpha1.FieldError{ + Message: `Multiple definitions for "test"`, + Paths: []string{"spec.traffic[0].name", "spec.traffic[1].name"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateRoute() = %v, wanted %v", got, want) } } diff --git a/pkg/webhook/service.go b/pkg/webhook/service.go index 08cdd170..34000d08 100644 --- a/pkg/webhook/service.go +++ b/pkg/webhook/service.go @@ -18,8 +18,6 @@ package webhook import ( "context" "errors" - "fmt" - "reflect" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/logging" @@ -27,19 +25,9 @@ import ( ) var ( - errInvalidRollouts = errors.New("the service must have exactly one of runLatest or pinned in spec field") - errMissingRevisionName = errors.New("the PinnedType must have revision specified") errInvalidServiceInput = errors.New("failed to convert input into Service") ) -func errServiceMissingField(fieldPath string) error { - return fmt.Errorf("Service is missing %q", fieldPath) -} - -func errServiceDisallowedFields(fieldPaths string) error { - return fmt.Errorf("The service spec must not set the field(s): %s", fieldPaths) -} - // ValidateService is Service resource specific validation and mutation handler func ValidateService(ctx context.Context) ResourceCallback { return func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error { @@ -49,30 +37,12 @@ func ValidateService(ctx context.Context) ResourceCallback { return err } - return validateSpec(newService) - } -} - -func validateSpec(s *v1alpha1.Service) error { - if s.Spec.RunLatest != nil && s.Spec.Pinned != nil || - s.Spec.RunLatest == nil && s.Spec.Pinned == nil { - return errInvalidRollouts - } - if s.Spec.Pinned != nil { - pinned := s.Spec.Pinned - if len(pinned.RevisionName) == 0 { - return errServiceMissingField("spec.pinned.revisionName") - } - if reflect.DeepEqual(pinned.Configuration, v1alpha1.ConfigurationSpec{}) { - return errServiceMissingField("spec.pinned.configuration") + // Can't just `return newService.Validate()` because it doesn't properly nil-check. + if err := newService.Validate(); err != nil { + return err } - return validateConfigurationSpec(&pinned.Configuration) - } - runLatest := s.Spec.RunLatest - if reflect.DeepEqual(runLatest.Configuration, v1alpha1.ConfigurationSpec{}) { - return errServiceMissingField("spec.runLatest.configuration") + return nil } - return validateConfigurationSpec(&runLatest.Configuration) } func unmarshalServices( diff --git a/pkg/webhook/service_test.go b/pkg/webhook/service_test.go index 7f7b2a21..ba66ecbd 100644 --- a/pkg/webhook/service_test.go +++ b/pkg/webhook/service_test.go @@ -27,12 +27,16 @@ func TestEmptySpec(t *testing.T) { s := v1alpha1.Service{ Spec: v1alpha1.ServiceSpec{}, } - err := ValidateService(TestContextWithLogger(t))(nil, &s, &s) - if err == nil { + got := ValidateService(TestContextWithLogger(t))(nil, &s, &s) + if got == nil { t.Errorf("Expected failure, but succeeded with: %+v", s) } - if e, a := errInvalidRollouts, err; e != a { - t.Errorf("Expected %s got %s", e, a) + want := &v1alpha1.FieldError{ + Message: "Expected exactly one, got neither", + Paths: []string{"spec.runLatest", "spec.pinned"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateService() = %v, wanted %v", got, want) } } @@ -55,13 +59,16 @@ func TestRunLatestWithMissingConfiguration(t *testing.T) { RunLatest: &v1alpha1.RunLatestType{}, }, } - err := ValidateService(TestContextWithLogger(t))(nil, &s, &s) - if err == nil { + got := ValidateService(TestContextWithLogger(t))(nil, &s, &s) + if got == nil { t.Errorf("Expected failure, but succeeded with: %+v", s) } - - if e, a := errServiceMissingField("spec.runLatest.configuration").Error(), err.Error(); e != a { - t.Errorf("Expected %s got %s", e, a) + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec.runLatest.configuration"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateService() = %v, wanted %v", got, want) } } @@ -88,12 +95,17 @@ func TestPinnedFailsWithNoRevisionName(t *testing.T) { }, }, } - err := ValidateService(TestContextWithLogger(t))(nil, &s, &s) - if err == nil { + got := ValidateService(TestContextWithLogger(t))(nil, &s, &s) + if got == nil { t.Errorf("Expected failure, but succeeded with: %+v", s) } - if e, a := errServiceMissingField("spec.pinned.revisionName").Error(), err.Error(); e != a { - t.Errorf("Expected %s got %s", e, a) + + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec.pinned.revisionName"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateService() = %v, wanted %v", got, want) } } @@ -105,12 +117,17 @@ func TestPinnedFailsWithNoConfiguration(t *testing.T) { }, }, } - err := ValidateService(TestContextWithLogger(t))(nil, &s, &s) - if err == nil { + got := ValidateService(TestContextWithLogger(t))(nil, &s, &s) + if got == nil { t.Errorf("Expected failure, but succeeded with: %+v", s) } - if e, a := errServiceMissingField("spec.pinned.configuration").Error(), err.Error(); e != a { - t.Errorf("Expected %s got %s", e, a) + + want := &v1alpha1.FieldError{ + Message: "missing field(s)", + Paths: []string{"spec.pinned.configuration"}, + } + if got.Error() != want.Error() { + t.Errorf("ValidateService() = %v, wanted %v", got, want) } } diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index 7708af3a..6af10b3f 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -319,7 +319,10 @@ func TestInvalidResponseForConfiguration(t *testing.T) { t.Errorf("Response status = %v, wanted %v", got, want) } - if !strings.Contains(reviewResponse.Response.Result.Message, "Unrecognized value for concurrencyModel") { + if !strings.Contains(reviewResponse.Response.Result.Message, "invalid value") { + t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message) + } + if !strings.Contains(reviewResponse.Response.Result.Message, "spec.revisionTemplate.spec.concurrencyModel") { t.Errorf("Received unexpected response status message %s", reviewResponse.Response.Result.Message) } } diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index f052ecb6..7f448fbe 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -406,7 +406,7 @@ func TestInvalidNewServiceNoSpecs(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) svc := createServicePinned(0, testServiceName) svc.Spec.Pinned = nil - expectFailsWith(t, ac.admit(TestContextWithLogger(t), createCreateService(svc)), "exactly one of runLatest or pinned") + expectFailsWith(t, ac.admit(TestContextWithLogger(t), createCreateService(svc)), "Expected exactly one, got neither: spec.runLatest, spec.pinned") } func TestInvalidNewServiceNoRevisionNameInPinned(t *testing.T) { @@ -715,12 +715,14 @@ func createWebhook(ac *AdmissionController, webhook *admissionregistrationv1beta } func expectAllowed(t *testing.T, resp *admissionv1beta1.AdmissionResponse) { + t.Helper() if !resp.Allowed { t.Errorf("Expected allowed, but failed with %+v", resp.Result) } } func expectFailsWith(t *testing.T, resp *admissionv1beta1.AdmissionResponse, contains string) { + t.Helper() if resp.Allowed { t.Errorf("expected denial, got allowed") return @@ -731,6 +733,7 @@ func expectFailsWith(t *testing.T, resp *admissionv1beta1.AdmissionResponse, con } func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) { + t.Helper() var actual []jsonpatch.JsonPatchOperation // Keep track of the patches we've found foundExpected := make([]bool, len(e))