Skip to content

Commit

Permalink
Subscription.Spec.Channel now uses KReference (#5412)
Browse files Browse the repository at this point in the history 
* Subscription.Spec.Channel now uses KReference

Signed-off-by: Francesco Guardiani <[email protected]>

* Fix test and gofmt

Signed-off-by: Francesco Guardiani <[email protected]>

* Fix another test

Signed-off-by: Francesco Guardiani <[email protected]>

* Fix comments

Signed-off-by: Francesco Guardiani <[email protected]>
  • Loading branch information
@slinkydeveloper
slinkydeveloper authored May 19, 2021
1 parent 0653ac4 commit 453e4c0
Show file tree
Hide file tree
Showing 13 changed files with 73 additions and 196 deletions.
14 changes: 1 addition & 13 deletions config/core/resources/subscription.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,18 @@ spec:
type: object
properties:
channel:
description: 'Reference to a channel that will be used to create the subscription You can specify only the following fields of the ObjectReference: - Kind - APIVersion - Name The resource pointed by this ObjectReference must meet the contract to the ChannelableSpec duck type. If the resource does not meet this contract it will be reflected in the Subscription''s status. This field is immutable. We have no good answer on what happens to the events that are currently in the channel being consumed from and what the semantics there should be. For now, you can always delete the Subscription and recreate it to point to a different channel, giving the user more control over what semantics should be used (drain the channel first, possibly have events dropped, etc.)'
description: 'Reference to a channel that will be used to create the subscription. You can specify only the following fields of the KReference: kind, apiVersion and name. The resource pointed by this KReference must meet the contract to the ChannelableSpec duck type. If the resource does not meet this contract it will be reflected in the Subscription''s status. This field is immutable. We have no good answer on what happens to the events that are currently in the channel being consumed from and what the semantics there should be. For now, you can always delete the Subscription and recreate it to point to a different channel, giving the user more control over what semantics should be used (drain the channel first, possibly have events dropped, etc.)'
type: object
properties:
apiVersion:
description: API version of the referent.
type: string
fieldPath:
description: 'If referring to a piece of an object instead of an entire object, this string should contain a valid JSON/Go field access statement, such as desiredState.manifest.containers[2]. For example, if the object reference is to a container within a pod, this would take on a value like: "spec.containers{name}" (where "name" refers to the name of the container that triggered the event) or if no container name is specified "spec.containers[2]" (container with index 2 in this pod). This syntax is chosen only to have some well-defined way of referencing a part of an object.'
type: string
kind:
description: 'Kind of the referent. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
name:
description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names'
type: string
namespace:
description: 'Namespace of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/'
type: string
resourceVersion:
description: 'Specific resourceVersion to which this reference is made, if any. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency'
type: string
uid:
description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
type: string
delivery:
description: Delivery configuration
type: object
Expand Down
64 changes: 11 additions & 53 deletions pkg/apis/messaging/v1/subscribable_channelable_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,69 +17,27 @@ limitations under the License.
package v1

import (
"reflect"
"context"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)

func isChannelEmpty(f corev1.ObjectReference) bool {
return equality.Semantic.DeepEqual(f, corev1.ObjectReference{})
func isChannelEmpty(f duckv1.KReference) bool {
return equality.Semantic.DeepEqual(f, duckv1.KReference{})
}

// Valid if it is a valid object reference.
func isValidChannel(f corev1.ObjectReference) *apis.FieldError {
return IsValidObjectReference(f)
}

func IsValidObjectReference(f corev1.ObjectReference) *apis.FieldError {
return checkRequiredObjectReferenceFields(f).
Also(checkDisallowedObjectReferenceFields(f))
}
func isValidChannel(ctx context.Context, f duckv1.KReference) *apis.FieldError {
errs := f.Validate(ctx)

// Check the corev1.ObjectReference to make sure it has the required fields. They
// are not checked for anything more except that they are set.
func checkRequiredObjectReferenceFields(f corev1.ObjectReference) *apis.FieldError {
var errs *apis.FieldError
if f.Name == "" {
errs = errs.Also(apis.ErrMissingField("name"))
}
if f.APIVersion == "" {
errs = errs.Also(apis.ErrMissingField("apiVersion"))
}
if f.Kind == "" {
errs = errs.Also(apis.ErrMissingField("kind"))
}
return errs
}

// Check the corev1.ObjectReference to make sure it only has the following fields set:
// Name, Kind, APIVersion
// If any other fields are set and is not the Zero value, returns an apis.FieldError
// with the fieldpaths for all those fields.
func checkDisallowedObjectReferenceFields(f corev1.ObjectReference) *apis.FieldError {
disallowedFields := []string{}
// See if there are any fields that have been set that should not be.
// TODO: Hoist this kind of stuff into pkg repository.
s := reflect.ValueOf(f)
typeOf := s.Type()
for i := 0; i < s.NumField(); i++ {
field := s.Field(i)
fieldName := typeOf.Field(i).Name
if fieldName == "Name" || fieldName == "Kind" || fieldName == "APIVersion" {
continue
}
if !cmp.Equal(field.Interface(), reflect.Zero(field.Type()).Interface()) {
disallowedFields = append(disallowedFields, fieldName)
}
}
if len(disallowedFields) > 0 {
fe := apis.ErrDisallowedFields(disallowedFields...)
// Namespace field is disallowed
if f.Namespace != "" {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields"
return fe
errs = errs.Also(fe)
}
return nil

return errs
}
100 changes: 24 additions & 76 deletions pkg/apis/messaging/v1/subscribable_channelable_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,42 +17,57 @@ limitations under the License.
package v1

import (
"context"
"testing"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)

var validationTests = []struct {
name string
ref corev1.ObjectReference
ref duckv1.KReference
want *apis.FieldError
}{
{
name: "valid object ref",
ref: corev1.ObjectReference{
name: "valid kref",
ref: duckv1.KReference{
Name: "boaty-mcboatface",
APIVersion: "messaging.knative.dev/v1",
Kind: "MyChannel",
},
want: nil,
},
{
name: "invalid object ref",
ref: corev1.ObjectReference{
name: "missing kind",
ref: duckv1.KReference{
Name: "boaty-mcboatface",
APIVersion: "messaging.knative.dev/v1",
Kind: "",
},
want: apis.ErrMissingField("kind"),
},
{
name: "contains namespace",
ref: duckv1.KReference{
Name: "boaty-mcboatface",
APIVersion: "messaging.knative.dev/v1",
Kind: "MyChannel",
Namespace: "my-namespace",
},
want: func() *apis.FieldError {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields"
return fe
}(),
},
}

func TestIsChannelEmpty(t *testing.T) {
name := "non empty"
t.Run(name, func(t *testing.T) {
r := corev1.ObjectReference{
r := duckv1.KReference{
Name: "boaty-mcboatface",
APIVersion: "messaging.knative.dev/v1",
Kind: "Channel",
Expand All @@ -64,7 +79,7 @@ func TestIsChannelEmpty(t *testing.T) {

name = "empty"
t.Run(name, func(t *testing.T) {
r := corev1.ObjectReference{}
r := duckv1.KReference{}
if !isChannelEmpty(r) {
t.Errorf("%s: isChannelEmpty(%s) should be true", name, r)
}
Expand All @@ -74,77 +89,10 @@ func TestIsChannelEmpty(t *testing.T) {
func TestIsValidChannel(t *testing.T) {
for _, test := range validationTests {
t.Run(test.name, func(t *testing.T) {
got := isValidChannel(test.ref)
got := isValidChannel(context.TODO(), test.ref)
if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
t.Errorf("%s: validation (-want, +got) = %v", test.name, diff)
}
})
}
}

func TestIsValidObjectReference(t *testing.T) {
tests := []struct {
name string
ref corev1.ObjectReference
want []*apis.FieldError
}{
{
name: "missing api version and kind",
ref: corev1.ObjectReference{
Name: "boaty-mcboatface",
APIVersion: "",
Kind: "",
},
want: []*apis.FieldError{
apis.ErrMissingField("apiVersion"),
apis.ErrMissingField("kind"),
},
},
{
name: "missing name",
ref: corev1.ObjectReference{
Name: "",
APIVersion: "eventing.knative.dev/v1",
Kind: "Strait",
},
want: []*apis.FieldError{
apis.ErrMissingField("name"),
},
},
{
name: "missing all",
ref: corev1.ObjectReference{
Name: "",
APIVersion: "",
Kind: "",
},
want: []*apis.FieldError{
apis.ErrMissingField("name"),
apis.ErrMissingField("apiVersion"),
apis.ErrMissingField("kind"),
},
},
{
name: "missing none",
ref: corev1.ObjectReference{
Name: "kind",
APIVersion: "messaging.knative.dev/v1",
Kind: "Channel",
},
want: []*apis.FieldError{},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
allWanted := &apis.FieldError{}
for _, fe := range test.want {
allWanted = allWanted.Also(fe)
}
got := IsValidObjectReference(test.ref)
if diff := cmp.Diff(allWanted.Error(), got.Error()); diff != "" {
t.Errorf("%s: validation (-want, +got) = %v", test.name, diff)
}
})
}
}
7 changes: 3 additions & 4 deletions pkg/apis/messaging/v1/subscription_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package v1

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -74,11 +73,11 @@ var (
// channel --> reply
type SubscriptionSpec struct {
// Reference to a channel that will be used to create the subscription
// You can specify only the following fields of the ObjectReference:
// You can specify only the following fields of the KReference:
// - Kind
// - APIVersion
// - Name
// The resource pointed by this ObjectReference must meet the
// The resource pointed by this KReference must meet the
// contract to the ChannelableSpec duck type. If the resource does not
// meet this contract it will be reflected in the Subscription's status.
//
Expand All @@ -89,7 +88,7 @@ type SubscriptionSpec struct {
// channel, giving the user more control over what semantics should
// be used (drain the channel first, possibly have events dropped,
// etc.)
Channel corev1.ObjectReference `json:"channel"`
Channel duckv1.KReference `json:"channel"`

// Subscriber is reference to (optional) function for processing events.
// Events from the Channel will be delivered here and replies are
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/messaging/v1/subscription_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (ss *SubscriptionSpec) Validate(ctx context.Context) *apis.FieldError {
fe := apis.ErrMissingField("channel")
fe.Details = "the Subscription must reference a channel"
return fe
} else if fe := isValidChannel(ss.Channel); fe != nil {
} else if fe := isValidChannel(ctx, ss.Channel); fe != nil {
errs = errs.Also(fe.ViaField("channel"))
}

Expand Down
Loading

0 comments on commit 453e4c0

Please sign in to comment.