Channel go definitions: ¯\_(ツ)_/¯

[evankanderson]

Describe the bug
Hey, it looks like there might be an API bug in Channel:
ChannelableStatus contains a DeadLetterChannel of type *duckv1.KReference, documented as:

DeadLetterChannel is a KReference and is set by the channel when it supports native error handling via a channel
but ChannelableSpec contains a DeliverySpec which has a DeadLetterSink of type duckv1.Destination, which could be a URL.
This means that either spec.delivery.deadLetterSink.ref is unresolved in status, or that the status.deadLetterChannel refers to something different (or both).

Expected behavior
The status of the Channel to include a resolved URL of the spec.delivery.deadLetterSink, or that the spec.delivery.deadLetterSink was only permitted to use the ref form to point to a channel.

To Reproduce
(See the code)

Knative release version
main / release-0.22

Additional context
It's not clear to me what the original intent of these fields was; https://github.com/evankanderson/specs/blob/main/specs/eventing/spec.md#kind-channel references status.deadLetterChannel but does not define spec.delivery; https://github.com/knative/specs/blob/main/specs/eventing/channel.md#spec-requirements indicates the requirement for spec.delivery and hints at dead letter behavior, but does not mention status at all with respect to dead letter behavior.

[evankanderson]

/meow

Please poke me to update the content in https://github.com/knative/specs/tree/main/specs/eventing once this is resolved.

[knative-prow-robot]

@evankanderson:

Details

In response to this:

/meow

Please poke me to update the content in https://github.com/knative/specs/tree/main/specs/eventing once this is resolved.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[evankanderson]

Interesting comment on the original definition: #1949 (comment)

Also ref the original doc: https://docs.google.com/document/d/1qRrzGoHJQO-oc5p-yRK8IRfugd-FM_PXyM7lN5kcqks/edit#heading=h.k1s530888nl1

[vaikas]

The original intent I think was that for some native implementations, they could expose a channel (in the status) that you could expose a a Rref (not URL, since it's a Ref to Channel) that you could then subscribe to using a Subscription if you wanted to receive the dead lettered events there. So, the Spec.Delivery says I'd like you to send dead lettered events here, but the Status would say, I already handled that for you and I will deliver them here and if you're interested, go subscribe to them.

[evankanderson]

Two questions:

1. Do we have any existing channels using the status.deadLetterChannel?
2. Do we want to resolve deadLetterSinkUri in the status and add a field for it like other destinations?

[vaikas]

1. I'm not aware of them, @slinkydeveloper @pierDipi does Kafka for example do this? I can't recall which channel was thought that this would be desireable.
2. That seems totally reasonable to me and consistency is good.

[slinkydeveloper]

Seems like we're not using it. Looking at code, the channel.spec.deadlettersink is resolved directly by the subscriptions (duck) reconciler when there is a deadlettersink in the channel, but not in the subscription. I can't find any usage of the field inside the status.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[evankanderson]

/reopen