Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/apis/flows/v1alpha1/flow_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
3 changes: 1 addition & 2 deletions pkg/apis/flows/v1alpha1/flow_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
44 changes: 35 additions & 9 deletions pkg/apis/flows/v1alpha1/flow_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -39,17 +43,39 @@ 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.
return nil
}

func (fa *FlowAction) Validate() *apis.FieldError {
// TODO(n3wscott): Implement this.
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")
}

return nil
}

func (current *Flow) CheckImmutableFields(og apis.Immutable) *apis.FieldError {
// TODO(n3wscott): Anything Immutable?
return nil
// Validate validates flow action has a valid formed target or targetURI, but not both.
func (fa *FlowAction) Validate() *apis.FieldError {
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")
}
}
185 changes: 185 additions & 0 deletions pkg/apis/flows/v1alpha1/flow_validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/*
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{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets recreated a few times. Maybe pull it into a variable?

validEventTrigger := ...

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"},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still catching up with the changes for validation code while I was gone. I like what's being done with them, I'm just little confused on what the eventual returned to the user and how the paths get stitched together. But that's not really part of this PR, just need to go read some more code :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that the field context is prefixed as you come back up the call stack, so you can traverse a object graph and call validate on sub-objects. If those objects throw errors, you add the context only you know about in the context the object is used. IE:

err = obj.AnotherObj.Validate()
if err != nil {
  return err.ViaField("AnotherObj")
}

AnotherObj's type was able to have a validate function that is only aware of the objects requirements. Only obj knew the context in which the user knows how they used AnotherObj.

Check out knative/pkg/apis

},
}, {
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: "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{},
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("validateFlow (-want, +got) = %v", diff)
}
})
}
}

// stringPtr returns a pointer to the passed string.
func stringPtr(s string) *string {
return &s
}