fix port merge of kubernetes services in sidecar #50840

ramaraochavali · 2024-05-03T15:52:07Z

K8s servie merge in sidecar is broken since #48896. When a sidecar is defined with two egress listeners pointing to two ports, the port merge does not happen

  egress:
  - hosts:
    - '*/foo.svc.cluster.local'
    port:
      name: tcp-port1
      number: 7443
      protocol: TCP
  - hosts:
    - '*/foo.svc.cluster.local'
    port:
      name: tcp-port2
      number: 7442
      protocol: TCP

With the above sidecar, the merged service will only have one port and hence only cluster is created.

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2024-05-03T15:53:01Z

pilot/pkg/model/sidecar_test.go

@@ -2060,56 +2076,6 @@ func TestCreateSidecarScope(t *testing.T) {
 				},
 			},
 		},
-		{


This test not relevant. Kubernetes service in the same namespace should be merged. The existing changed test verifies this behaviour

ramaraochavali · 2024-05-03T15:53:34Z

pilot/pkg/model/sidecar_test.go

@@ -2201,7 +2167,7 @@ func TestCreateSidecarScope(t *testing.T) {
 			expectedServices: []*Service{
 				{
 					Hostname: "proxy",
-					Ports:    port7443,
+					Ports:    port744x,


Again here, k8s service ports should be merged while taking precedence

ramaraochavali · 2024-05-03T15:54:12Z

This broke our existing deployments after upgrade.

hzxuzhonghu · 2024-05-06T04:11:53Z

can you describe how you merge svces in the egress listeners. As we can see in xds generator, sometimes we use services in egress listenrs, some times we use services in sidecarScope

ramaraochavali · 2024-05-06T04:15:28Z

See this example

  egress:
  - hosts:
    - '*/foo.svc.cluster.local'
    port:
      name: tcp-port1
      number: 7443
      protocol: TCP
  - hosts:
    - '*/foo.svc.cluster.local'
    port:
      name: tcp-port2
      number: 7442
      protocol: TCP

It generates two listeners but only one cluster with outbound|7443||foo.svc.cluster.local. It does not generate cluster for 7442. This is broken in current release

hzxuzhonghu · 2024-05-06T07:22:42Z

pilot/pkg/model/sidecar.go

-		if existing.Attributes.ServiceRegistry == provider.Kubernetes {
-			log.Debugf("Service %s/%s from registry %s ignored by %s/%s/%s", s.Attributes.Namespace, s.Hostname, s.Attributes.ServiceRegistry,
+		// We do not merge k8s service with any other services from other registries
+		if existing.Attributes.ServiceRegistry == provider.Kubernetes && s.Attributes.ServiceRegistry != provider.Kubernetes {


how could this occur two services in k8s have same fqdn

istio/pilot/pkg/model/sidecar.go

Line 426 in 044be69

sc.appendSidecarServices(servicesAdded, s)

- It can come from here if two egress listener select the same service with different port

Please try with this sidecar #50840 (comment)

I see how this occur, it is the egress listener with generate intermeditae svc with only one port from the original k8s svc.

Yes and we need to merge at that end

hzxuzhonghu · 2024-05-06T07:23:37Z

pilot/pkg/model/sidecar.go

-			log.Debugf("Service %s/%s from registry %s ignored by %s/%s/%s", existing.Attributes.Namespace, existing.Hostname, existing.Attributes.ServiceRegistry,
+		// newly created Services cannot take ownership unexpectedly. However, the Service is from Kubernetes it should
+		// take precedence over ones not. This prevents someone from "domain squatting" on the hostname before a Kubernetes Service is created.
+		if existing.Attributes.ServiceRegistry != provider.Kubernetes && s.Attributes.ServiceRegistry == provider.Kubernetes {


The above comment apply here

hzxuzhonghu

Can you add multiple sidecar egress listeners svc match test

hzxuzhonghu · 2024-05-06T07:36:00Z

pilot/pkg/model/sidecar.go

-		if existing.Attributes.ServiceRegistry == provider.Kubernetes {
-			log.Debugf("Service %s/%s from registry %s ignored by %s/%s/%s", s.Attributes.Namespace, s.Hostname, s.Attributes.ServiceRegistry,
+		// We do not merge k8s service with any other services from other registries
+		if existing.Attributes.ServiceRegistry == provider.Kubernetes && s.Attributes.ServiceRegistry != provider.Kubernetes {


I see how this occur, it is the egress listener with generate intermeditae svc with only one port from the original k8s svc.

ramaraochavali · 2024-05-06T08:18:07Z

Can you add multiple sidecar egress listeners svc match test

We already have that test

istio/pilot/pkg/model/sidecar_test.go

Line 2214 in 044be69

name: "multi-port-merge",

- it did not fail because we do not specify service registry attribute. I fixed it

hzxuzhonghu · 2024-05-06T10:42:21Z

@ramaraochavali The test has service resides in different namespaces

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali · 2024-05-06T10:58:32Z

I do not think that really matters much because first service has all the ports and is in ns1 namespace- even that failed without this. Any way, I have added additional test

hzxuzhonghu

LG now

istio-testing · 2024-05-07T09:41:19Z

In response to a cherrypick label: new pull request created: #50887

* upstream/master: fix release-notes kind (istio#50906) discovery fake: allow kube-client override (istio#50896) change #none to none for use-waypoint (istio#50842) Update iptables image SHA (istio#50898) Update BASE_VERSION to master-2024-05-07T22-48-43 (istio#50897) Add some more useful logging around iptables selection in ambient CNI (istio#50891) Automator: update ztunnel@master in istio/istio@master (istio#50890) stop default for istio.io/waypoint-for in istioctl (istio#50889) Update integration README by adding a missing command-line flag and fixing typos (istio#50866) Automator: update proxy@master in istio/istio@master (istio#50888) fix port merge of kubernetes services in sidecar (istio#50840) bump proxy and ambient test (istio#50882)

ramaraochavali added 3 commits May 3, 2024 21:13

fix k8s service merge in sidecar

75280da

Signed-off-by: Rama Chavali <[email protected]>

add release notes

750e7cb

Signed-off-by: Rama Chavali <[email protected]>

rever unnecessary changes

2396027

Signed-off-by: Rama Chavali <[email protected]>

ramaraochavali requested a review from a team as a code owner May 3, 2024 15:52

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 3, 2024

ramaraochavali commented May 3, 2024

View reviewed changes

hzxuzhonghu reviewed May 6, 2024

View reviewed changes

add one mpore test

2da0df4

Signed-off-by: Rama Chavali <[email protected]>

hzxuzhonghu approved these changes May 7, 2024

View reviewed changes

ramaraochavali added the cherrypick/release-1.22 Set this label on a PR to auto-merge it to the release-1.22 branch label May 7, 2024

istio-testing merged commit e66d1b8 into istio:master May 7, 2024
28 checks passed

istio-testing mentioned this pull request May 7, 2024

[release-1.22] fix port merge of kubernetes services in sidecar #50887

Merged

ramaraochavali deleted the fix/sidecar_port branch May 7, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix port merge of kubernetes services in sidecar #50840

fix port merge of kubernetes services in sidecar #50840

ramaraochavali commented May 3, 2024 •

edited by istio-policy-bot

Loading

ramaraochavali May 3, 2024

ramaraochavali May 3, 2024

ramaraochavali commented May 3, 2024

hzxuzhonghu commented May 6, 2024

ramaraochavali commented May 6, 2024

hzxuzhonghu May 6, 2024

ramaraochavali May 6, 2024

ramaraochavali May 6, 2024

hzxuzhonghu May 6, 2024

ramaraochavali May 6, 2024

hzxuzhonghu May 6, 2024

hzxuzhonghu left a comment

hzxuzhonghu May 6, 2024

ramaraochavali commented May 6, 2024

hzxuzhonghu commented May 6, 2024

ramaraochavali commented May 6, 2024 •

edited

Loading

hzxuzhonghu left a comment

istio-testing commented May 7, 2024

@@ @@ -2060,56 +2076,6 @@ func TestCreateSidecarScope(t *testing.T) { @@
               				},
               			},
               		},
-              		{

fix port merge of kubernetes services in sidecar #50840

fix port merge of kubernetes services in sidecar #50840

Conversation

ramaraochavali commented May 3, 2024 • edited by istio-policy-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented May 3, 2024

hzxuzhonghu commented May 6, 2024

ramaraochavali commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented May 6, 2024

hzxuzhonghu commented May 6, 2024

ramaraochavali commented May 6, 2024 • edited Loading

hzxuzhonghu left a comment

Choose a reason for hiding this comment

istio-testing commented May 7, 2024

ramaraochavali commented May 3, 2024 •

edited by istio-policy-bot

Loading

ramaraochavali commented May 6, 2024 •

edited

Loading