Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create new Subscription in pkg/apis/eventing as per the new spec. #421

Merged
merged 15 commits into from
Sep 14, 2018
Merged

Create new Subscription in pkg/apis/eventing as per the new spec. #421

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
313b88b
Rename Subscription fields to: from,to,processor
Sep 8, 2018
758ae8c
make things refs
Sep 11, 2018
dc67d24
address pr feedback, remove left over cruft
Sep 11, 2018
a20d789
add the contracts we expect each object reference to fulfill
Sep 11, 2018
7e2d059
use Sinkable contract for the To ref
Sep 12, 2018
28be0a6
update tests to refs
Sep 12, 2018
0c3fcd7
more tests
Sep 12, 2018
785ddbc
moar tests
Sep 12, 2018
973d77c
add test for Subscription object
Sep 12, 2018
b8a8e49
processor -> call
Sep 12, 2018
38983f0
Revert changes to pkg/channels, move everything over to pkg/eventing
Sep 12, 2018
c9fc3b7
add tests
Sep 13, 2018
add1e88
address pr feedback
Sep 13, 2018
6a6778a
rename meta_v1 to metav1
Sep 13, 2018
9237900
address PR comments
Sep 14, 2018
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
66 changes: 56 additions & 10 deletions pkg/apis/channels/v1alpha1/subscription_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/knative/pkg/apis"
"github.com/knative/pkg/webhook"
"k8s.io/api/core/v1"
corev1 "k8s.io/api/core/v1"
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)
Expand All @@ -47,8 +48,14 @@ var _ apis.Immutable = (*Subscription)(nil)
var _ runtime.Object = (*Subscription)(nil)
var _ webhook.GenericCRD = (*ClusterBus)(nil)

// SubscriptionSpec specifies the Channel and Subscriber and the configuration
// arguments for the Subscription.
// SubscriptionSpec specifies the Channel for incoming events, a handler and the Channel
// for outgoing messages.
// from --[transform]--> to
// Note that the following are valid configurations also:
// Sink, no outgoing events:
// from -- transform
// no-op function (identity transformation):
// from --> to
type SubscriptionSpec struct {
// TODO: Generation does not work correctly with CRD. They are scrubbed
// by the APIserver (https://github.com/kubernetes/kubernetes/issues/58778)
Expand All @@ -57,14 +64,53 @@ type SubscriptionSpec struct {
// +optional
Generation int64 `json:"generation,omitempty"`

// Channel is the name of the channel to subscribe to.
Channel string `json:"channel"`

// Subscriber is the name of the subscriber service DNS name.
Subscriber string `json:"subscriber"`

// Target service DNS name for replies returned by the subscriber.
ReplyTo string `json:"replyTo,omitempty"`
// Reference to an object that will be used to create the subscription
// for receiving events. The object must have spec.subscriptions
// list which will then be modified accordingly.
//
// This object must fulfill the Subscribable contract.
//
// You can specify only the following fields of the ObjectReference:
// - Kind
// - APIVersion
// - Name
From *corev1.ObjectReference `json:"from,omitempty"`

// Processor is reference to (optional) function for processing events.
// Events from the From channel will be delivered here and replies
// are sent to To channel.
//
// This object must fulfill the Targetable contract.
//
// Reference to an object that will be used to deliver events for
// (optional) processing before sending them to To for further
// if specified for additional Subscriptions to then subscribe
// to these events for further processing.
//
// For example, this could be a reference to a Route resource
// or a Service resource.
// TODO: Specify the required fields the target object must
// have in the status.
// You can specify only the following fields of the ObjectReference:
// - Kind
// - APIVersion
// - Name
// +optional
Processor *corev1.ObjectReference `json:"processor,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Is the idea that you would need to create a k8s Service to be able to call something which is not on the k8s cluster (e.g. a VM)? Previously we had the idea that a targetable was either a DNS name or an ObjectReference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so, this is a bit interesting in a sense, that I'd expect that if you're hitting something that's outside the cluster, just a DNS entry is probably not going to cut it. I'd expect things like credentials to be involved on that, so as not to just be wide open to everything. I could hoist this into a higher level object to give us some future proofing for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to allow the reference for a targetable to also include a k8s Service. It doesn't implement the targetable interface, but we know how to shoehorn it into what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scothis when we talked about this wrt Targetable, thinking was that a service was not enough, that it would actually have to be a URI. So, I'm hoisting that up.


// To is the (optional) resolved channel where (optionally) processed
// events get sent.
//
// This object must fulfill the Sinkable contract.
//
// TODO: Specify the required fields the target object must
// have in the status.
// You can specify only the following fields of the ObjectReference:
// - Kind
// - APIVersion
// - Name
Copy link
Member

Choose a reason for hiding this comment

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

What about UID?

I'm assuming that Namespace is specifically excluded?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about UID?

Is there a good way to lookup a resource by UID?

I'm assuming that Namespace is specifically excluded?

Keeping subscriptions within a single namespace is a good idea until we understand fully understand the tenancy consequences of allowing them to cross namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's start with names (no UIDs), and a single namespace until we have more understanding on what cases this does not handel?

// +optional
To *corev1.ObjectReference `json:"to,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on other parts of this CRD, so leaving the comment here:

  1. Can we remove Arguments until we need them, and prefer to have Subscription-specific arguments rather than Channel-specific Arguments? (See also s/[]Arguments/runtime.RawExtension/.)
  2. What conditions do we need? I think we probably want at least 2 Types: Ready and ReferencesResolved, where Ready is the canonical "happy state" type.
    • Do we need LastUpdateTime and LastTransitionTime? I'd be inclined to leave off that bookkeeping until we have evidence we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove Arguments until we need them

Yes, we haven't come across a use case for argument/params with the current buses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.


// Arguments is a list of configuration arguments for the Subscription. The
// Arguments for a channel must contain values for each of the Parameters
Expand Down
22 changes: 15 additions & 7 deletions pkg/apis/channels/v1alpha1/subscription_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,29 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/knative/pkg/apis"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
)

func (s *Subscription) Validate() *apis.FieldError {
return s.Spec.Validate().ViaField("spec")
}

func (ss *SubscriptionSpec) Validate() *apis.FieldError {
if ss.Channel == "" {
fe := apis.ErrMissingField("channel")
fe.Details = "the Subscription must reference a Channel"
if ss.From == nil || equality.Semantic.DeepEqual(ss.From, &corev1.ObjectReference{}) {
fe := apis.ErrMissingField("from")
fe.Details = "the Subscription must reference a from channel"
Copy link
Member

Choose a reason for hiding this comment

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

This should probably also ensure that the ss.From.Name and ss.From.Kind (at least) are non-empty.

(In general, it feels like there should be a function to validate an ObjectReference in the context of Subscription.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

le sigh :) Yes, there's bunch of more work to be done here :) Thanks for the comments, I just don't want to keep churning these things until we settle on the shape and the names. I'm going to add validation for now that it can only be a Channel for now as discussed in Slack yesterday, and later on we can relax.

return fe
}
if ss.Subscriber == "" {
fe := apis.ErrMissingField("subscriber")
fe.Details = "the Subscription must reference a Subscriber"
if ss.Processor == nil && ss.To == nil {
fe := apis.ErrMissingField("to", "processor")
fe.Details = "the Subscription must reference at least one of (to channel or a processor)"
return fe
}

if equality.Semantic.DeepEqual(ss.Processor, &corev1.ObjectReference{}) && equality.Semantic.DeepEqual(ss.To, &corev1.ObjectReference{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could also cause issues if one is nil and the other is empty

fe := apis.ErrMissingField("to", "processor")
fe.Details = "the Subscription must reference at least one of (to channel or a processor)"
return fe
}
return nil
Expand All @@ -49,7 +57,7 @@ func (current *Subscription) CheckImmutableFields(og apis.Immutable) *apis.Field
return nil
}

ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Subscriber", "Arguments")
ignoreArguments := cmpopts.IgnoreFields(SubscriptionSpec{}, "Processor", "Arguments")
Copy link
Contributor

Choose a reason for hiding this comment

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

To should also allow mutation, only From should be immutable. It was an oversight that ReplyTo is immutable right now.

if diff := cmp.Diff(original.Spec, current.Spec, ignoreArguments); diff != "" {
return &apis.FieldError{
Message: "Immutable fields changed (-old +new)",
Expand Down
Loading