Trigger deadLetterUri does not conform to specification, should be deadLetterSinkUri

[knguyen0125]

Describe the bug
A clear and concise description of what the bug is.

In Trigger Lifecycle specification, it specify the following:

If the Trigger's `spec.delivery.deadLetterSink` field it set, it MUST be resolved to a URI and reported in `status.deadLetterSinkUri` in the same manner as the `spec.subscriber` field before setting the Ready condition to true.

The newly merged new feature #5551 does not conform to this specification.

Additionally, since the CRD for Broker and Trigger does not contain deadLetterSinkUri nor deadLetterUri in status field, this change actually did not do anything as the newly added deadLetterUri is removed anyway.

Expected behavior
A clear and concise description of what you expected to happen.

• Status for resolved dead letter sink URI correctly conforms to specification (use deadLetterSinkUri instead of deadLetterUri)
• CRD for Broker and Trigger contains deadLetterSinkUri in status

To Reproduce
Steps to reproduce the behavior.

1. Install Knative Eventing 0.25, MTChannelBasedBroker 0.25 and InMemoryChannel 0.25
2. Create a trigger with deadLetterSink delivery option
3. Trigger is resolved without deadLetterUri
4. Manually add deadLetterUri to Trigger CRD
5. Trigger is resolved with deadLetterUri

Knative release version

0.25

Additional context
Add any other context about the problem here such as proposed priority

[pierDipi]

Hi @knguyen0125 👋, thanks for reporting!

Since you find the fix (4. Manually add deadLetterUri to Trigger CRD), can you please create a PR to fix this issue?

The Trigger CRD is in https://github.com/knative/eventing/blob/main/config/core/resources/trigger.yaml

/good-first-issue
/area brokers

[knative-prow-robot]

@pierDipi:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Hi @knguyen0125 👋, thanks for reporting!

Since you find the fix (4. Manually add deadLetterUri to Trigger CRD), can you please create a PR to fix this issue?

The Trigger CRD is in https://github.com/knative/eventing/blob/main/config/core/resources/trigger.yaml

/good-first-issue
/area brokers

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.

[benmoss]

/assign

[benmoss]

/reopen

we still need to add a resolved DeadLetterSinkURI to the broker status

[knative-prow-robot]

@benmoss: Reopened this issue.

In response to this:

/reopen

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.

[lionelvillard]

@benmoss what the status on the one?

[benmoss]

Not actively working on it, took myself off as assignee

[gabo1208]

Is this a GA blocker by any means? @lionelvillard @salaboy

[lionelvillard]

yes.

[gabo1208]

/assign

[salaboy]

@gabo1208 good stuff.. can you please keep me posted about this one as well

[gabo1208]

this was fixed by @pierDipi here #5748

[gabo1208]

It is fixed for the Triggers and is unclear for the Brokers so we should discuss this in a separate issue for the brokers i think.

[gabo1208]

Since this issue is specific to Triggers, have been divided into two issues:

• This one for the Triggers that is already fixed and should be closed
• Brokers status deadLetterUri is not been updated #5787

[devguyio]

@gabo1208 that makes sense, I'm closing this issue . Fixed by #5748

/close

[knative-prow-robot]

@devguyio: Closing this issue.

In response to this:

@gabo1208 that makes sense, I'm closing this issue . Fixed by #5748

/close

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.