Merge delivery spec instead of using non nil specs

[pierDipi]

In some cases, we're using non-nil values of the delivery spec to understand if the user is setting this value or not. For example, when there isn't a delivery spec for a Trigger we're using the entire delivery spec of the Broker.

However, if there is a DLS on a Broker and an associated Trigger overrides some parameters like backoffDelay the DLS of the Broker isn't used and no dead letter sink will be used.

I think, merging the delivery specs of 2 (or more) objects is reasonable and provides much stroger guarantees out of box.

Signed-off-by: Pierangelo Di Pilato [email protected]

Release Note

The delivery configs of the Broker are now merged with the one of the Trigger.
Trigger's delivery configs, if defined, have precedence over the individual Broker's delivery configs.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [pierDipi]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[knative-metrics-robot]

The following is the coverage report on the affected files. Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/duck/v1/delivery_types.go	96.3%	98.0%	1.7
pkg/reconciler/broker/trigger/trigger.go	89.0%	88.8%	-0.2

[codecov[bot]]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 :tada:

Comparison is base (2233333) 82.18% compared to head (80e5c58) 82.24%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6277      +/-   ##
==========================================
+ Coverage   82.18%   82.24%   +0.06%     
==========================================
  Files         231      231              
  Lines        7786     7814      +28     
==========================================
+ Hits         6399     6427      +28     
  Misses        937      937              
  Partials      450      450              

Impacted Files	Coverage Δ
pkg/apis/duck/v1/delivery_types.go	96.72% <100.00%> (+3.17%)	:arrow_up:
pkg/reconciler/broker/trigger/trigger.go	84.10% <100.00%> (-0.21%)	:arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[pierDipi]

/hold to gather consensus

/cc @lionelvillard @devguyio @matzew @aliok @gabo1208

[knative-prow-robot]

@pierDipi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-knative-eventing-reconciler-tests	80e5c5840fa09525e0b0ef964a989a9ba1b517cc	link	false	/test pull-knative-eventing-reconciler-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[matzew]

/assign @lionelvillard please tke a look on this

[lionelvillard]

I think, merging the delivery specs of 2 (or more) objects is reasonable and provides much stroger guarantees out of box.

The drawback of such solution is the delivery options are now spread across multiple objects and consequently make the UX more complicated. If the Trigger's status section includes the actual delivery options then this is not an issue anymore.

[pierDipi]

I think, merging the delivery specs of 2 (or more) objects is reasonable and provides much stroger guarantees out of box.

The drawback of such solution is the delivery options are now spread across multiple objects and consequently make the UX more complicated. If the Trigger's status section includes the actual delivery options then this is not an issue anymore.

That's a good point and I like this idea! I'll update the PR (or send a different PR for this aspect).

[odacremolbap]

I think, merging the delivery specs of 2 (or more) objects is reasonable and provides much stroger guarantees out of box.

sorry for commenting late, I think I have a concern on this.

Most probably if a user sets a delivery options they expect it to override all settings (that is at least my intuitive thinking), I would be ok if we default to merging, document it and show the effective setting at the status as Lionel suggested.

The 2 concerns are:

• What if a user wants to completely override settings from a broker that configured a DLS and want to set no DLS at a trigger. Would there be a way of overriding to a nil DLS?
• Merging setting would be non backwards compatible.

For my first concern, could we add an element that defines inheritance that can be set to merge or override? For the second, if we add that inheritance element, we could default to override and we would be backwards compatible.

WDYT?

[github-actions[bot]]

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

[pierDipi]

Still valid

/remove-lifecycle stale

[pierDipi]

What if a user wants to completely override settings from a broker that configured a DLS and want to set no DLS at a trigger.

Is this a reasonable use case? :thinking: I can't think of a valid reason to do that.

For my first concern, could we add an element that defines inheritance that can be set to merge or override? For the second, if we add that inheritance element, we could default to override and we would be backward compatible.

what about a feature flag, off by default, so that we're backward compatible?

[github-actions[bot]]

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

[knative-prow-robot]

This issue or pull request is stale because it has been open for 90 days with no activity.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close

/lifecycle stale

[github-actions[bot]]

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

[matzew]

what about a feature flag, off by default, so that we're backward compatible?

That sounds like a good idea, I am +1 on this, @pierDipi

[knative-prow-robot]

PR needs rebase.

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.