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
5 changes: 3 additions & 2 deletions config/core/resources/broker.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ spec:
holding it if left out.'
type: string
delivery:
description: 'Delivery is the delivery specification for Events within
the Broker mesh. This includes things like retries, DLQ, etc.'
description: 'Delivery contains the delivery spec for each trigger
to this Broker. Each trigger delivery spec, if any, overrides this
global delivery spec.'
type: object
properties:
backoffDelay:
Expand Down
53 changes: 53 additions & 0 deletions config/core/resources/trigger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,59 @@ spec:
uri:
type: string
description: 'the target URI or, if ref is provided, a relative URI reference that will be combined with ref to produce a target URI.'
delivery:
description: 'Delivery contains the delivery spec for this specific trigger.'
type: object
properties:
backoffDelay:
description: 'BackoffDelay is the delay before retrying. More
information on Duration format: - https://www.iso.org/iso-8601-date-and-time-format.html
- https://en.wikipedia.org/wiki/ISO_8601 For linear policy,
backoff delay is backoffDelay*<numberOfRetries>. For
exponential policy, backoff delay is backoffDelay*2^<numberOfRetries>.'
type: string
backoffPolicy:
description: ' BackoffPolicy is the retry backoff policy (linear,
exponential).'
type: string
deadLetterSink:
description: 'DeadLetterSink is the sink receiving event that
could not be sent to a destination.'
type: object
properties:
ref:
description: 'Ref points to an Addressable.'
type: object
properties:
apiVersion:
description: 'API version of the referent.'
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/
This is optional field, it gets defaulted
to the object holding it if left out.'
type: string
uri:
description: 'URI can be an absolute URL(non-empty
scheme and non-empty host) pointing to the target
or a relative URI. Relative URIs will be resolved
using the base URI retrieved from Ref.'
type: string
retry:
description: 'Retry is the minimum number of retries the sender
should attempt when sending an event before moving it
to the dead letter sink.'
type: integer
format: int32
status:
type: object
x-kubernetes-preserve-unknown-fields: true
Expand Down
12 changes: 12 additions & 0 deletions docs/spec/broker.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ The attributes filter specifying a list of key-value pairs MUST be supported by
Trigger. Events that pass the attributes filter MUST include context or
extension attributes that match all key-value pairs exactly.

### Delivery Spec

Both BrokerSpec and TriggerSpec have a `Delivery` field of type [`duck.DeliverySpec`](./spec.md#eventingduckv1deliveryspec).
This field, among the other features, allows the user to define the dead letter sink and retries.
The `BrokerSpec.Delivery` field is global across all the Triggers registered with that particular
Broker, while the `TriggerSpec.Delivery`, if configured, fully overrides `BrokerSpec.Delivery` for
that particular Trigger, hence:

* When `BrokerSpec.Delivery` and `TriggerSpec.Delivery` are both not configured, no delivery spec MUST be used.
* When `BrokerSpec.Delivery` is configured, but not the specific `TriggerSpec.Delivery`, then the `BrokerSpec.Delivery` MUST be used.
* When `TriggerSpec.Delivery` is configured, then `TriggerSpec.Delivery` MUST be used.

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 just want to understand how this is supposed to work with the wording about defaulting fields. It's more of an implementation question but has implications on when the defaulting happens and if it even should happen. The PR description seems to say that we default the triggerspec.delivery field, so if that's how we're going to implement it then it matters when the defaulting applies. If however you implement it by not actually updating the triggerspec.delivery but look at the brokerspec.delivery if triggerspec.delivery is missing then the behaviour is different.

In particular, it would be good to clarify that, for example, what happens in the following scenarios:

  1. Create Broker, no delivery

  2. Create Trigger, no delivery

  3. Update Broker, configure delivery, what happens here?

  4. Create Broker, with delivery

  5. Create Trigger, no delivery -> defaulting happens

  6. Update Broker, delete/update broker delivery -> what happens to trigger above

  7. Create Trigger2, no delivery, will it match what Trigger from 2 has?

Just want to understand that behaviour little better about what "defaulting" really means and what the expected behaviour is.
As I said, depending on how this is implemented and if we actually modify the triggerspec.delivery vs. treating missing triggerspec.delivery as 'use brokerspec.delivery' has different semantics potentially and certainly makes the implementation more tricky depending on how it's implemented.

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.

From the PR description, this is what I'm trying to understand with the description on the spec.

Now Broker.Delivery defaults the Trigger.Delivery field. Note: this is a breaking change for mtbroker users that are using the Broker.Delivery field, because this field, even if it remains the same, will change meaning for them (in a followup I'll perform the actual change)

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.

Defaulting basically means that the trigger delivery spec, if any, wins on the broker delivery spec, exactly as it works today with subscription and channel delivery fields. So the result of the above cases:

  1. defaults to Broker.Delivery -> no delivery

  2. defaults to Broker.Delivery -> delivery from Broker.Delivery

  3. defaults to Broker.Delivery -> delivery from Broker.Delivery

  4. defaults to Broker.Delivery -> delivery from Broker.Delivery (no delivery if Broker.Delivery was removed)

  5. defaults to Broker.Delivery -> delivery from Broker.Delivery (no delivery if Broker.Delivery was removed)

Another case:

  1. Create Trigger3, with delivery -> delivery from Trigger3.Delivery

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.

I can provide the implementation for mtbroker to show how that works, but I don't see any particular implementation issues with it

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.

Right, so I think it was (as I tried to say) how it's worded about "defaulting" since typically defaulting means filling in the fields and if that's how you were going to implement it, it could get confusing. If you remove the word defaulting I think I'd be much happier. What I tried to say :) was that spec seems ok, but your PR description confused me wrt. using words defaulting.
If you say:
If triggerspec.delivery is specified it is used. If triggerspec.delivery does not exist brokerspec.delivery is used. Nothing about defaulting :)

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.

How about now? I removed the default word everywhere and added some wording to the spec

## Data Plane

### Ingress
Expand Down
5 changes: 3 additions & 2 deletions pkg/apis/eventing/v1/broker_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ type BrokerSpec struct {
// +optional
Config *duckv1.KReference `json:"config,omitempty"`

// Delivery is the delivery specification for Events within the Broker mesh.
// This includes things like retries, DLQ, etc.
// Delivery contains the delivery spec for each trigger
// to this Broker. Each trigger delivery spec, if any, overrides this
// global delivery spec.
// +optional
Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"`
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/eventing/v1/trigger_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/kmeta"

eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
)

const (
Expand Down Expand Up @@ -84,6 +86,10 @@ type TriggerSpec struct {
// Subscriber is the addressable that receives events from the Broker that pass the Filter. It
// is required.
Subscriber duckv1.Destination `json:"subscriber"`

// Delivery contains the delivery spec for this specific trigger.
// +optional
Delivery *eventingduckv1.DeliverySpec `json:"delivery,omitempty"`
Comment thread
slinkydeveloper marked this conversation as resolved.
}

type TriggerFilter struct {
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/eventing/v1/trigger_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ func (ts *TriggerSpec) Validate(ctx context.Context) *apis.FieldError {
errs = errs.Also(fe.ViaField("subscriber"))
}

if ts.Delivery != nil {
if de := ts.Delivery.Validate(ctx); de != nil {
errs = errs.Also(de.ViaField("delivery"))
}
}

return errs
}

Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/eventing/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 16 additions & 2 deletions pkg/apis/eventing/v1beta1/trigger_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ import (
"context"
"fmt"

duckv1 "knative.dev/eventing/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"
v1 "knative.dev/eventing/pkg/apis/eventing/v1"
"knative.dev/pkg/apis"
)

// ConvertTo implements apis.Convertible
func (source *Trigger) ConvertTo(_ context.Context, to apis.Convertible) error {
func (source *Trigger) ConvertTo(ctx context.Context, to apis.Convertible) error {
switch sink := to.(type) {
case *v1.Trigger:
sink.ObjectMeta = source.ObjectMeta
Expand All @@ -39,6 +41,12 @@ func (source *Trigger) ConvertTo(_ context.Context, to apis.Convertible) error {
sink.Spec.Filter.Attributes[k] = v
}
}
if source.Spec.Delivery != nil {
sink.Spec.Delivery = &duckv1.DeliverySpec{}
if err := source.Spec.Delivery.ConvertTo(ctx, sink.Spec.Delivery); err != nil {
return err
}
}
sink.Status.Status = source.Status.Status
sink.Status.SubscriberURI = source.Status.SubscriberURI
return nil
Expand All @@ -48,7 +56,7 @@ func (source *Trigger) ConvertTo(_ context.Context, to apis.Convertible) error {
}

// ConvertFrom implements apis.Convertible
func (sink *Trigger) ConvertFrom(_ context.Context, from apis.Convertible) error {
func (sink *Trigger) ConvertFrom(ctx context.Context, from apis.Convertible) error {
switch source := from.(type) {
case *v1.Trigger:
sink.ObjectMeta = source.ObjectMeta
Expand All @@ -63,6 +71,12 @@ func (sink *Trigger) ConvertFrom(_ context.Context, from apis.Convertible) error
Attributes: attributes,
}
}
if source.Spec.Delivery != nil {
sink.Spec.Delivery = &duckv1beta1.DeliverySpec{}
if err := sink.Spec.Delivery.ConvertFrom(ctx, source.Spec.Delivery); err != nil {
return err
}
}
sink.Status.Status = source.Status.Status
sink.Status.SubscriberURI = source.Status.SubscriberURI
return nil
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/eventing/v1beta1/trigger_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/kmeta"

eventingduckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"
)

const (
Expand Down Expand Up @@ -87,6 +89,10 @@ type TriggerSpec struct {
// Subscriber is the addressable that receives events from the Broker that pass the Filter. It
// is required.
Subscriber duckv1.Destination `json:"subscriber"`

// Delivery contains the delivery spec for this specific trigger.
// +optional
Delivery *eventingduckv1beta1.DeliverySpec `json:"delivery,omitempty"`
}

type TriggerFilter struct {
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/eventing/v1beta1/trigger_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ func (ts *TriggerSpec) Validate(ctx context.Context) *apis.FieldError {
errs = errs.Also(fe.ViaField("subscriber"))
}

if ts.Delivery != nil {
if de := ts.Delivery.Validate(ctx); de != nil {
errs = errs.Also(de.ViaField("delivery"))
}
}

return errs
}

Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/eventing/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.