Add namespace to referenced addressable before resolving destination

[pierDipi]

The resolver uses ref.namespace to lookup the destination
addressable.
ref.namespace for the deadLetterSink is not defaulted by the
webhook like for trigger.subscriber.ref, so we might receive a
destination with an empty string for
deadLetterSink.ref.namespace which leads to not finding the
referenced addressable service.

Fixes #1011

Proposed Changes

• Add namespace to referenced addressable before resolving destination

Release Note

The fields `Broker.spec.delivery.deadLetterSink.ref.namespace` and `Trigger.spec.delivery.deadLetterSink.ref.namespace` are optional.
By default the controller uses the object namespace. 

[knative-metrics-robot]

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

File	Old Coverage	New Coverage	Delta
control-plane/pkg/core/config/utils.go	91.1%	89.8%	-1.2

[codecov]

Codecov Report

Merging #1254 (3bbff49) into main (2a22d01) will increase coverage by 0.18%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##               main    #1254      +/-   ##
============================================
+ Coverage     75.51%   75.70%   +0.18%     
  Complexity      460      460              
============================================
  Files            87       87              
  Lines          3059     3058       -1     
  Branches        136      136              
============================================
+ Hits           2310     2315       +5     
+ Misses          582      579       -3     
+ Partials        167      164       -3     

Flag	Coverage Δ
java-unittests	80.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
control-plane/pkg/core/config/utils.go	79.48% <33.33%> (-1.85%)	⬇️
control-plane/pkg/security/config.go	68.96% <0.00%> (+20.48%)	⬆️

---

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 2a22d01...3bbff49. Read the comment docs.

[aliok]

@pierDipi
Have you checked if the namespace is optional in the spec?
If so, is it documented that it defaults to parent object namespace?

[pierDipi]

The spec doesn't explicitly talk about this field being optional or the default value (I'll open an issue).

The namespace for a KReference is (implementation-wise) optional and we historically defaulted it to the object namespace, for example in Trigger subscriber [1] (there are many examples).

That is also the logically expected behavior of our users as you can see in the linked issue. (#1011)

I opened an issue in eventing to add this behavior knative/eventing#5747 (and a PR).

1. https://github.com/knative/eventing/blob/f031ba23b23d20ed8d13491000feb0abcf50d562/pkg/apis/eventing/v1/trigger_defaults.go#L40-L41

---

Regarding this PR, I'd keep, regardless of the outcome of knative/eventing#5747, the logic of this PR to make sure we don't pass invalid data to the resolver that requires a namespace to be set.

[aliok]

/lgtm
/approve

Thanks @pierDipi for fixing this issue and also taking care of the same issue in other places

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, 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

[pierDipi]

No tests were recorded. 🤔 ❓ ❓
/retest

[pierDipi]

/retest

[pierDipi]

/retest

[pierDipi]

/cherry-pick release-0.25

[pierDipi]

/cherry-pick release-0.25

[pierDipi]

/cherry-pick release-0.24

[pierDipi]

/cherry-pick release-0.23

[knative-prow-robot]

@pierDipi: new pull request created: #1262

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.

[knative-prow-robot]

@pierDipi: #1254 failed to apply on top of branch "release-0.24":

Applying: Add namespace to referenced addressable before resolving destination
Using index info to reconstruct a base tree...
M	control-plane/pkg/core/config/utils.go
M	control-plane/pkg/reconciler/broker/broker_test.go
M	control-plane/pkg/reconciler/testing/objects_common.go
Falling back to patching base and 3-way merge...
Auto-merging control-plane/pkg/reconciler/testing/objects_common.go
CONFLICT (content): Merge conflict in control-plane/pkg/reconciler/testing/objects_common.go
Auto-merging control-plane/pkg/reconciler/broker/broker_test.go
Auto-merging control-plane/pkg/core/config/utils.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add namespace to referenced addressable before resolving destination
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.24

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]

@pierDipi: #1254 failed to apply on top of branch "release-0.23":

Applying: Add namespace to referenced addressable before resolving destination
Using index info to reconstruct a base tree...
M	control-plane/pkg/core/config/utils.go
M	control-plane/pkg/reconciler/broker/broker_test.go
M	control-plane/pkg/reconciler/testing/objects_common.go
Falling back to patching base and 3-way merge...
Auto-merging control-plane/pkg/reconciler/testing/objects_common.go
CONFLICT (content): Merge conflict in control-plane/pkg/reconciler/testing/objects_common.go
Auto-merging control-plane/pkg/reconciler/broker/broker_test.go
Auto-merging control-plane/pkg/core/config/utils.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add namespace to referenced addressable before resolving destination
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.23

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]

@pierDipi: new pull request could not be created: failed to create pull request against knative-sandbox/eventing-kafka-broker#release-0.25 from head knative-prow-robot:cherry-pick-1254-to-release-0.25: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for knative-prow-robot:cherry-pick-1254-to-release-0.25."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

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.