Repository: knative-sandbox/eventing-kafka-broker
Pull request #3045 (2023-04-17 12:22:06)

Don't override ports for KafkaChannel's service
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

<!-- Please include the 'why' behind your changes if no issue exists -->

## Proposed Changes

- Add ports to KafkaChannel's external name service

<!--
If this change has user-visible impact, follow the instructions below.
Examples include:

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic

Otherwise delete the rest of this template.
-->

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->

```release-note
Add ports to KafkaChannel's externalName services
```

**Docs**

N/A


knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/3045#" title="Author self-approved">pierDipi</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Feventing-kafka-broker).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/eventing-kafka-broker/blob/main/OWNERS)~~ [pierDipi]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/3045#pullrequestreview-1387924358" title="Approved">matzew</a>*, *<a href="https://github.com/knative-sandbox/eventing-kafka-broker/pull/3045#" title="Author self-approved">pierDipi</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Feventing-kafka-broker).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/eventing-kafka-broker/blob/main/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
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: ## [Codecov](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/3045?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#3045](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/3045?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (b5df7e9) into [main](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/commit/ec3ad8828b2e15a92267c62d279c0d153793c0e5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (ec3ad88) will **increase** coverage by `16.89%`.
> The diff coverage is `n/a`.

```diff
@@              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](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#carryforward-flags-in-the-pull-request-comment) to find out more.

[see 79 files with indirect coverage changes](https://codecov.io/gh/knative-sandbox/eventing-kafka-broker/pull/3045/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)


Patch:
diff --git a/control-plane/pkg/reconciler/channel/resources/service.go b/control-plane/pkg/reconciler/channel/resources/service.go
index 750957c8f..6b9d1410e 100644
--- a/control-plane/pkg/reconciler/channel/resources/service.go
+++ b/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?
-		svc.Spec = corev1.ServiceSpec{
-			Type:         corev1.ServiceTypeExternalName,
-			ExternalName: network.GetServiceHostname(service, namespace),
-		}
+		svc.Spec.Type = corev1.ServiceTypeExternalName
+		svc.Spec.ExternalName = network.GetServiceHostname(service, namespace)
 		return nil
 	}
 }


Test patch:
diff --git a/control-plane/pkg/reconciler/testing/objects_channel.go b/control-plane/pkg/reconciler/testing/objects_channel.go
index 5fd18ca8b..2a7b6127b 100644
--- a/control-plane/pkg/reconciler/testing/objects_channel.go
+++ b/control-plane/pkg/reconciler/testing/objects_channel.go
@@ -26,14 +26,15 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	clientgotesting "k8s.io/client-go/testing"
 	"k8s.io/utils/pointer"
-	messagingv1beta1 "knative.dev/eventing-kafka-broker/control-plane/pkg/apis/messaging/v1beta1"
-	"knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel/resources"
 	eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
 	v1 "knative.dev/eventing/pkg/apis/messaging/v1"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 	"knative.dev/pkg/network"
 
+	messagingv1beta1 "knative.dev/eventing-kafka-broker/control-plane/pkg/apis/messaging/v1beta1"
+	"knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel/resources"
+
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/config"
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/contract"
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/kafka"
@@ -352,6 +353,13 @@ func NewPerChannelService(env *config.Env) *corev1.Service {
 		Spec: corev1.ServiceSpec{
 			Type:         corev1.ServiceTypeExternalName,
 			ExternalName: network.GetServiceHostname(env.IngressName, env.SystemNamespace),
+			Ports: []corev1.ServicePort{
+				{
+					Name:     "http",
+					Protocol: corev1.ProtocolTCP,
+					Port:     80,
+				},
+			},
 		},
 	}