Default dead letter sink namespace to the object namespace #5748

pierDipi · 2021-09-21T18:00:30Z

Dead letter sink namespace is not defaulted to the object namespace for:

Broker
Trigger
Channel
Subscription

Forcing every implementation to define the logic:

"If spec.delivery.deadLetterSink.ref.namespace is empty, use metadata.namespace"

Fixes #5747 #5639

Proposed Changes

Default dead letter sink namespace to the object namespace

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

The field `spec.delivery.deadLetterSink.ref.namespace` for Broker, Trigger, Channel and Subscription if not specified is defaulted to `metadata.namespace`.

knative-prow-robot · 2021-09-21T18:00:38Z

[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

codecov · 2021-09-21T18:07:16Z

Codecov Report

Merging #5748 (7aed22b) into main (e05ccfc) will increase coverage by 0.00%.
The diff coverage is 86.66%.

@@           Coverage Diff           @@
##             main    #5748   +/-   ##
=======================================
  Coverage   82.51%   82.52%           
=======================================
  Files         202      203    +1     
  Lines        6360     6374   +14     
=======================================
+ Hits         5248     5260   +12     
- Misses        767      768    +1     
- Partials      345      346    +1

Impacted Files	Coverage Δ
pkg/apis/messaging/v1/subscription_defaults.go	`71.42% <66.66%> (-28.58%)`	⬇️
pkg/apis/duck/v1/delivery_defaults.go	`100.00% <100.00%> (ø)`
pkg/apis/eventing/v1/broker_defaults.go	`100.00% <100.00%> (ø)`
pkg/apis/eventing/v1/trigger_defaults.go	`100.00% <100.00%> (ø)`
pkg/apis/messaging/v1/channel_defaults.go	`100.00% <100.00%> (ø)`
...kg/apis/messaging/v1/in_memory_channel_defaults.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e05ccfc...7aed22b. Read the comment docs.

pierDipi · 2021-09-22T06:14:47Z

/hold waiting for @duglin to comment

lionelvillard · 2021-09-22T13:07:48Z

this breaks eventing-rabbitmq

/cc @benmoss

duglin · 2021-09-22T13:27:56Z

can't comment on the code change, but I'm ok with the concept!

With upcoming changes to eventing DLQs will be defaulted to the namespace of their parent object if the namespace isn't specified in the ref. For some reason this causes a panic from the fake dynamic client that doesn't occur if the namespace isn't specified. It happens because ksvcs have been registered with the fake dynamic client as a type, but I'm not really sure why they only panic when a namespace is present. xref knative/eventing#5748 for the namespace defaulting change xref knative/eventing#5185 for more discussion of the changed behavior of client-go fake dynamic clients.

benmoss · 2021-09-23T14:59:38Z

@pierDipi @lionelvillard downstream rabbit should be good now with knative-extensions/eventing-rabbitmq#433

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

pierDipi · 2021-09-23T17:52:40Z

Thanks! Let's see how this run goes!

knative-metrics-robot · 2021-09-23T17:55:25Z

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_defaults.go	Do not exist	100.0%
pkg/apis/messaging/v1/subscription_defaults.go	100.0%	85.7%	-14.3

pierDipi · 2021-09-23T18:29:29Z

It failed again https://github.com/knative/eventing/runs/3691016364?check_suite_focus=true 🤔

benmoss · 2021-09-23T19:22:45Z

Oh of course, we have the same test copy and pasted into two different packages, how silly of me to forget to replicate the changes 😄

benmoss · 2021-09-23T20:05:37Z

@pierDipi try again 😄

pierDipi · 2021-09-23T20:49:39Z

Thanks Ben, it works!

/unhold

matzew · 2021-09-24T05:41:09Z

pkg/apis/messaging/v1/in_memory_channel_defaults.go

 	imc.Spec.SetDefaults(ctx)
 }

 func (imcs *InMemoryChannelSpec) SetDefaults(ctx context.Context) {
-	// TODO: Nothing to default here...
+	imcs.Delivery.SetDefaults(ctx)


@pierDipi should we do that on other downstream channels too?

Original issue came up in here: knative-extensions/eventing-kafka-broker#1254

Not sure if it is done in other places too

Sure, I'll do it.

matzew · 2021-09-24T11:24:21Z

/lgtm

pierDipi · 2021-09-24T12:03:13Z

/retest

pierDipi · 2021-11-15T10:25:02Z

/cherry-pick release-0.26

pierDipi · 2021-11-15T10:25:21Z

/cherry-pick release-0.25

knative-prow-robot · 2021-11-15T10:25:50Z

@pierDipi: new pull request created: #5904

In response to this:

/cherry-pick release-0.26

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.

knative-prow-robot · 2021-11-15T10:25:58Z

@pierDipi: new pull request created: #5905

In response to this:

/cherry-pick release-0.25

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.

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 21, 2021

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 21, 2021

pierDipi force-pushed the KNATIVE-5747_Default-dls-namespace branch from 0a628c9 to 1b448d3 Compare September 21, 2021 18:02

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2021

knative-prow-robot requested a review from benmoss September 22, 2021 13:07

benmoss mentioned this pull request Sep 23, 2021

Switch broker DLQ test to use a k8s Service knative-extensions/eventing-rabbitmq#433

Merged

Default dead letter sink namespace to the object namespace

7aed22b

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

pierDipi force-pushed the KNATIVE-5747_Default-dls-namespace branch from 1b448d3 to 7aed22b Compare September 23, 2021 17:49

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2021

matzew reviewed Sep 24, 2021

View reviewed changes

knative-prow-robot assigned matzew Sep 24, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021

knative-prow-robot merged commit 384523f into knative:main Sep 24, 2021

pierDipi deleted the KNATIVE-5747_Default-dls-namespace branch September 24, 2021 16:00

pierDipi mentioned this pull request Sep 27, 2021

[Channel] Set defaults of spec.delivery knative-extensions/eventing-kafka#895

Merged

benmoss mentioned this pull request Oct 5, 2021

Fixed trigger reconciler details on code #5783

Closed

gabo1208 mentioned this pull request Oct 5, 2021

Trigger deadLetterUri does not conform to specification, should be deadLetterSinkUri #5639

Closed

pierDipi mentioned this pull request Oct 19, 2021

Default KafkaSource sink ref namespace to object namespace knative-extensions/eventing-kafka#943

Merged

knative-prow-robot mentioned this pull request Nov 15, 2021

[release-0.26] Default dead letter sink namespace to the object namespace #5904

Closed

knative-prow-robot mentioned this pull request Nov 15, 2021

[release-0.25] Default dead letter sink namespace to the object namespace #5905

Closed

This was referenced Nov 15, 2021

[release-0.25] Default dead letter sink namespace to the object namespace #5906

Merged

[release-0.26] Default dead letter sink namespace to the object namespace #5907

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default dead letter sink namespace to the object namespace #5748

Default dead letter sink namespace to the object namespace #5748

pierDipi commented Sep 21, 2021 •

edited by devguyio

Loading

knative-prow-robot commented Sep 21, 2021

codecov bot commented Sep 21, 2021 •

edited

Loading

pierDipi commented Sep 22, 2021

lionelvillard commented Sep 22, 2021

duglin commented Sep 22, 2021

benmoss commented Sep 23, 2021

pierDipi commented Sep 23, 2021

knative-metrics-robot commented Sep 23, 2021

pierDipi commented Sep 23, 2021

benmoss commented Sep 23, 2021

benmoss commented Sep 23, 2021

pierDipi commented Sep 23, 2021

matzew Sep 24, 2021

aliok Sep 24, 2021

pierDipi Sep 24, 2021

matzew commented Sep 24, 2021

pierDipi commented Sep 24, 2021

pierDipi commented Nov 15, 2021

pierDipi commented Nov 15, 2021

knative-prow-robot commented Nov 15, 2021

knative-prow-robot commented Nov 15, 2021

Default dead letter sink namespace to the object namespace #5748

Default dead letter sink namespace to the object namespace #5748

Conversation

pierDipi commented Sep 21, 2021 • edited by devguyio Loading

Proposed Changes

Pre-review Checklist

knative-prow-robot commented Sep 21, 2021

codecov bot commented Sep 21, 2021 • edited Loading

Codecov Report

pierDipi commented Sep 22, 2021

lionelvillard commented Sep 22, 2021

duglin commented Sep 22, 2021

benmoss commented Sep 23, 2021

pierDipi commented Sep 23, 2021

knative-metrics-robot commented Sep 23, 2021

pierDipi commented Sep 23, 2021

benmoss commented Sep 23, 2021

benmoss commented Sep 23, 2021

pierDipi commented Sep 23, 2021

matzew Sep 24, 2021

Choose a reason for hiding this comment

aliok Sep 24, 2021

Choose a reason for hiding this comment

pierDipi Sep 24, 2021

Choose a reason for hiding this comment

matzew commented Sep 24, 2021

pierDipi commented Sep 24, 2021

pierDipi commented Nov 15, 2021

pierDipi commented Nov 15, 2021

knative-prow-robot commented Nov 15, 2021

knative-prow-robot commented Nov 15, 2021

pierDipi commented Sep 21, 2021 •

edited by devguyio

Loading

codecov bot commented Sep 21, 2021 •

edited

Loading