From d9d9be5a6aa258178060df740a299d0d74b4c6a0 Mon Sep 17 00:00:00 2001 From: Scott Nichols Date: Mon, 27 Aug 2018 15:36:24 -0700 Subject: [PATCH 1/4] Validate flow as told in the flow spec comments. --- pkg/apis/flows/v1alpha1/flow_defaults.go | 4 +- pkg/apis/flows/v1alpha1/flow_types.go | 3 +- pkg/apis/flows/v1alpha1/flow_validation.go | 43 +++++- .../flows/v1alpha1/flow_validation_test.go | 142 ++++++++++++++++++ 4 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 pkg/apis/flows/v1alpha1/flow_validation_test.go diff --git a/pkg/apis/flows/v1alpha1/flow_defaults.go b/pkg/apis/flows/v1alpha1/flow_defaults.go index 7cf54d7c183..d02436f284d 100644 --- a/pkg/apis/flows/v1alpha1/flow_defaults.go +++ b/pkg/apis/flows/v1alpha1/flow_defaults.go @@ -26,9 +26,9 @@ func (fs *FlowSpec) SetDefaults() { } func (fa *FlowAction) SetDefaults() { - // TODO anything? + // nothing to do } func (et *EventTrigger) SetDefaults() { - // TODO anything? + // // nothing to do } diff --git a/pkg/apis/flows/v1alpha1/flow_types.go b/pkg/apis/flows/v1alpha1/flow_types.go index 59ec3bcb2e8..bbb6882ac18 100644 --- a/pkg/apis/flows/v1alpha1/flow_types.go +++ b/pkg/apis/flows/v1alpha1/flow_types.go @@ -42,10 +42,9 @@ type Flow struct { Status FlowStatus `json:"status"` } -// Check that Flow can be validated, can be defaulted, and has immutable fields. +// Check that Flow can be validated and can be defaulted. var _ apis.Validatable = (*Flow)(nil) var _ apis.Defaultable = (*Flow)(nil) -var _ apis.Immutable = (*Flow)(nil) var _ runtime.Object = (*Flow)(nil) var _ webhook.GenericCRD = (*Flow)(nil) diff --git a/pkg/apis/flows/v1alpha1/flow_validation.go b/pkg/apis/flows/v1alpha1/flow_validation.go index b5a4bbd9d08..eed3d8ce431 100644 --- a/pkg/apis/flows/v1alpha1/flow_validation.go +++ b/pkg/apis/flows/v1alpha1/flow_validation.go @@ -19,15 +19,19 @@ package v1alpha1 import ( "github.com/knative/pkg/apis" "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/util/validation" + "net/url" ) +// Validate validates the Flow resource. func (f *Flow) Validate() *apis.FieldError { return f.Spec.Validate().ViaField("spec") } +// Validate validates the flow spec for a valid action and trigger. func (fs *FlowSpec) Validate() *apis.FieldError { if equality.Semantic.DeepEqual(fs, &FlowSpec{}) { - return apis.ErrMissingField(apis.CurrentField) + return apis.ErrMissingField("trigger", "action") } if err := fs.Trigger.Validate(); err != nil { @@ -39,17 +43,42 @@ func (fs *FlowSpec) Validate() *apis.FieldError { return nil } +// Validate validates event trigger has both eventType and resource set. func (et *EventTrigger) Validate() *apis.FieldError { - // TODO(n3wscott): Implement this. + + if et.EventType == "" { + return apis.ErrMissingField("eventType") + } + + if et.Resource == "" { + return apis.ErrMissingField("resource") + } + return nil } +// Validate validates flow action has a valid formed target or targetURI, but not both. func (fa *FlowAction) Validate() *apis.FieldError { - // TODO(n3wscott): Implement this. - return nil -} + switch { + case fa.Target != nil && fa.TargetURI != nil: + return apis.ErrMultipleOneOf("target", "targetURI") + case fa.Target != nil: + if errs := validation.IsQualifiedName(fa.Target.Name); len(errs) > 0 { + return apis.ErrInvalidValue(fa.Target.Name, "name").ViaField("target") + } + return nil + case fa.TargetURI != nil: + if _, err := url.ParseRequestURI(*fa.TargetURI); err != nil { + return apis.ErrInvalidValue(*fa.TargetURI, "targetURI") + } + return nil + default: + return apis.ErrMissingOneOf("target", "targetURI") + } + + if fa.Target != nil && fa.TargetURI != nil { + + } -func (current *Flow) CheckImmutableFields(og apis.Immutable) *apis.FieldError { - // TODO(n3wscott): Anything Immutable? return nil } diff --git a/pkg/apis/flows/v1alpha1/flow_validation_test.go b/pkg/apis/flows/v1alpha1/flow_validation_test.go new file mode 100644 index 00000000000..7f702a141d1 --- /dev/null +++ b/pkg/apis/flows/v1alpha1/flow_validation_test.go @@ -0,0 +1,142 @@ +/* +Copyright 2018 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 ( + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/apis" + "k8s.io/api/core/v1" + "testing" +) + +func TestFlowSpecValidation(t *testing.T) { + tests := []struct { + name string + f *FlowSpec + want *apis.FieldError + }{{ + name: "valid with URI", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http://example.com/action"), + }, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + }, { + name: "valid with Target", + f: &FlowSpec{ + Action: FlowAction{ + Target: &v1.ObjectReference{ + Name: "foo", + }, + }, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + }, { + name: "invalid with URI", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http//example.com/action"), + }, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: `invalid value "http//example.com/action"`, + Paths: []string{"action.targetURI"}, + }, + }, { + name: "invalid with Target", + f: &FlowSpec{ + Action: FlowAction{ + Target: &v1.ObjectReference{ + Name: "f@o", + }, + }, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: `invalid value "f@o"`, + Paths: []string{"action.target.name"}, + }, + }, { + name: "invalid with both target and targetURI", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http://example.com/action"), + Target: &v1.ObjectReference{ + Name: "foo", + }, + }, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: `expected exactly one, got both`, + Paths: []string{"action.target", "action.targetURI"}, + }, + }, { + name: "invalid, no target or targetURI", + f: &FlowSpec{ + Action: FlowAction{}, + Trigger: EventTrigger{ + EventType: "foo", + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: `expected exactly one, got neither`, + Paths: []string{"action.target", "action.targetURI"}, + }, + }, { + name: "empty", + f: &FlowSpec{}, + want: &apis.FieldError{ + Message: "missing field(s)", + Paths: []string{"trigger", "action"}, + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.f.Validate() + ignoreArguments := cmpopts.IgnoreFields(apis.FieldError{}, "Details") + if diff := cmp.Diff(test.want, got, ignoreArguments); diff != "" { + t.Errorf("validateFeed (-want, +got) = %v", diff) + } + }) + } +} + +// stringPtr returns a pointer to the passed string. +func stringPtr(s string) *string { + return &s +} From 4042b0a89074bb08c5d526ba6e40db416354c9c5 Mon Sep 17 00:00:00 2001 From: Scott Nichols Date: Mon, 27 Aug 2018 15:45:42 -0700 Subject: [PATCH 2/4] Adding missing edge cases. --- pkg/apis/flows/v1alpha1/flow_validation.go | 7 ++- .../flows/v1alpha1/flow_validation_test.go | 43 +++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/pkg/apis/flows/v1alpha1/flow_validation.go b/pkg/apis/flows/v1alpha1/flow_validation.go index eed3d8ce431..ec31825fe2b 100644 --- a/pkg/apis/flows/v1alpha1/flow_validation.go +++ b/pkg/apis/flows/v1alpha1/flow_validation.go @@ -49,6 +49,9 @@ func (et *EventTrigger) Validate() *apis.FieldError { if et.EventType == "" { return apis.ErrMissingField("eventType") } + if errs := validation.IsQualifiedName(et.EventType); len(errs) > 0 { + return apis.ErrInvalidValue(et.EventType, "eventType") + } if et.Resource == "" { return apis.ErrMissingField("resource") @@ -76,9 +79,5 @@ func (fa *FlowAction) Validate() *apis.FieldError { return apis.ErrMissingOneOf("target", "targetURI") } - if fa.Target != nil && fa.TargetURI != nil { - - } - return nil } diff --git a/pkg/apis/flows/v1alpha1/flow_validation_test.go b/pkg/apis/flows/v1alpha1/flow_validation_test.go index 7f702a141d1..dd237ec5cab 100644 --- a/pkg/apis/flows/v1alpha1/flow_validation_test.go +++ b/pkg/apis/flows/v1alpha1/flow_validation_test.go @@ -116,6 +116,49 @@ func TestFlowSpecValidation(t *testing.T) { Message: `expected exactly one, got neither`, Paths: []string{"action.target", "action.targetURI"}, }, + }, { + name: "invalid, missing event type", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http://example.com/action"), + }, + Trigger: EventTrigger{ + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: "missing field(s)", + Paths: []string{"trigger.eventType"}, + }, + }, { + name: "invalid event type", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http://example.com/action"), + }, + Trigger: EventTrigger{ + EventType: "f@o", + Resource: "bar", + }, + }, + want: &apis.FieldError{ + Message: `invalid value "f@o"`, + Paths: []string{"trigger.eventType"}, + }, + }, { + name: "invalid, missing resource", + f: &FlowSpec{ + Action: FlowAction{ + TargetURI: stringPtr("http://example.com/action"), + }, + Trigger: EventTrigger{ + EventType: "foo", + }, + }, + want: &apis.FieldError{ + Message: "missing field(s)", + Paths: []string{"trigger.resource"}, + }, }, { name: "empty", f: &FlowSpec{}, From b50e469444cfd6aa1b608a19f3b4868e35b2325d Mon Sep 17 00:00:00 2001 From: Scott Nichols Date: Mon, 27 Aug 2018 15:48:43 -0700 Subject: [PATCH 3/4] remove unused return. --- pkg/apis/flows/v1alpha1/flow_validation.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/apis/flows/v1alpha1/flow_validation.go b/pkg/apis/flows/v1alpha1/flow_validation.go index ec31825fe2b..e11025aa5e4 100644 --- a/pkg/apis/flows/v1alpha1/flow_validation.go +++ b/pkg/apis/flows/v1alpha1/flow_validation.go @@ -78,6 +78,4 @@ func (fa *FlowAction) Validate() *apis.FieldError { default: return apis.ErrMissingOneOf("target", "targetURI") } - - return nil } From 5a9fc0afd1c9620f68fffcac23ea73545343e823 Mon Sep 17 00:00:00 2001 From: Scott Nichols Date: Tue, 28 Aug 2018 06:55:18 -0700 Subject: [PATCH 4/4] Validate method comment was wrong, fixed. --- pkg/apis/flows/v1alpha1/flow_validation_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/flows/v1alpha1/flow_validation_test.go b/pkg/apis/flows/v1alpha1/flow_validation_test.go index dd237ec5cab..d0b0eb8ff01 100644 --- a/pkg/apis/flows/v1alpha1/flow_validation_test.go +++ b/pkg/apis/flows/v1alpha1/flow_validation_test.go @@ -173,7 +173,7 @@ func TestFlowSpecValidation(t *testing.T) { got := test.f.Validate() ignoreArguments := cmpopts.IgnoreFields(apis.FieldError{}, "Details") if diff := cmp.Diff(test.want, got, ignoreArguments); diff != "" { - t.Errorf("validateFeed (-want, +got) = %v", diff) + t.Errorf("validateFlow (-want, +got) = %v", diff) } }) }