Don't override ports for KafkaChannel's service #3045

pierDipi · 2023-04-17T12:22:06Z

By not having the port defined on the KafkaChannel's external
name service, istio (actually Envoy) can't find a matching route
for requests to the channel's service without any port, see [1]

[1] https://www.envoyproxy.io/docs/envoy/latest/faq/debugging/why_is_my_route_not_found

Proposed Changes

Add ports to KafkaChannel's external name service

Release Note

Add ports to KafkaChannel's externalName services

Docs

N/A

By not having the port defined on the KafkaChannel's external name service, istio (actually Envoy) can't find a matching route for requests to the channel's service without any port, see [1] [1] https://www.envoyproxy.io/docs/envoy/latest/faq/debugging/why_is_my_route_not_found Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-04-17T12:27:30Z

control-plane/pkg/reconciler/channel/resources/service.go

@@ -53,11 +53,8 @@ func MakeChannelServiceName(name string) string {
 // pointing to the specified service in a namespace.
 func ExternalService(namespace, service string) ServiceOption {
 	return func(svc *corev1.Service) error {
-		// TODO this overrides the current serviceSpec. Is this correct?


It was already identified as wrong but then left there as TODO

matzew

the TODO was just copied over from the "eventing-kafka" repo, but looks like it was there for a reason 😄

/lgtm
/approve

knative-prow · 2023-04-17T12:28:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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~~ [matzew,pierDipi]

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

codecov · 2023-04-17T12:31:27Z

Codecov Report

Merging #3045 (b5df7e9) into main (ec3ad88) will increase coverage by 16.89%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main    #3045       +/-   ##
=============================================
+ Coverage     64.21%   81.10%   +16.89%     
  Complexity      754      754               
=============================================
  Files           156       77       -79     
  Lines         11034     2625     -8409     
  Branches        232      232               
=============================================
- Hits           7085     2129     -4956     
+ Misses         3437      361     -3076     
+ Partials        512      135      -377

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

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

see 79 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-04-17T14:23:19Z

/test channel-integration-tests-sasl-ssl

* Include Channel’s ExternalName services in Istio mesh Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Run hack/update-codegen.sh Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Format Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Test and release setup Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add IMC and KafkaChannel tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Format and fix tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add access to configmaps Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add config-features for testing and install script for dev Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Debug logging for istio enabled Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix controller Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Include knative-extensions/eventing-kafka-broker#3045 Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Use core zap Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix unit tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add unit tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies and artifacts Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix host name Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade deps and artifacts Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add full config-features Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix boilerplate check Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Improve log message Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * More boilerplate check fixes Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

…ions#16) * Include Channel’s ExternalName services in Istio mesh Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Run hack/update-codegen.sh Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Format Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Test and release setup Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add IMC and KafkaChannel tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Format and fix tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add access to configmaps Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add config-features for testing and install script for dev Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Debug logging for istio enabled Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix controller Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Include knative-extensions/eventing-kafka-broker#3045 Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Use core zap Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix unit tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add unit tests Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies and artifacts Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix host name Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade dependencies Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Upgrade deps and artifacts Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add full config-features Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix boilerplate check Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Improve log message Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * More boilerplate check fixes Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Update codegen Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 17, 2023

pierDipi mentioned this pull request Apr 17, 2023

Include Channel’s ExternalName services in Istio mesh knative-extensions/eventing-istio#16

Merged

pierDipi commented Apr 17, 2023

View reviewed changes

matzew approved these changes Apr 17, 2023

View reviewed changes

knative-prow bot assigned matzew Apr 17, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2023

pierDipi added a commit to pierDipi/eventing-istio that referenced this pull request Apr 17, 2023

Include knative-extensions/eventing-kafka-broker#3045

52ea104

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow bot merged commit 26676d7 into knative-extensions:main Apr 17, 2023
25 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't override ports for KafkaChannel's service #3045

Don't override ports for KafkaChannel's service #3045

pierDipi commented Apr 17, 2023

pierDipi Apr 17, 2023

matzew left a comment

knative-prow bot commented Apr 17, 2023

codecov bot commented Apr 17, 2023 •

edited

Loading

pierDipi commented Apr 17, 2023

Don't override ports for KafkaChannel's service #3045

Don't override ports for KafkaChannel's service #3045

Conversation

pierDipi commented Apr 17, 2023

Proposed Changes

pierDipi Apr 17, 2023

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Apr 17, 2023

codecov bot commented Apr 17, 2023 • edited Loading

Codecov Report

pierDipi commented Apr 17, 2023

codecov bot commented Apr 17, 2023 •

edited

Loading