Fix service entry merge

pilot/pkg/model/endpointshards.go

@@ -23,6 +23,7 @@ import (
 	"istio.io/istio/pilot/pkg/serviceregistry/provider"
 	"istio.io/istio/pkg/cluster"
 	"istio.io/istio/pkg/config/schema/kind"
+	"istio.io/istio/pkg/ptr"
 	"istio.io/istio/pkg/util/sets"
 )
 
@@ -100,7 +101,9 @@ func (es *EndpointShards) CopyEndpoints(portMap map[string]int) map[int][]*Istio
 	res := map[int][]*IstioEndpoint{}
 	for _, v := range es.Shards {
 		for _, ep := range v {
-			portNum, f := portMap[ep.ServicePortName]
+			// use the port name as the key, unless LegacyClusterPortKey is set and takes precedence
+			k := ptr.NonEmptyOrDefault(ep.LegacyClusterPortKey, ep.ServicePortName)
+			portNum, f := portMap[k]
 			if !f {
 				continue
 			}

pilot/pkg/model/push_context.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"math"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -1446,7 +1447,8 @@ func (ps *PushContext) initServiceRegistry(env *Environment, configsUpdate sets.
 	for _, s := range allServices {
 		portMap := map[string]int{}
 		for _, port := range s.Ports {
-			portMap[port.Name] = port.Port
+			// In EDS we match on port *name*. But for historical reasons, we match on port number for CDS.
+			portMap[strconv.Itoa(port.Port)] = port.Port
 		}
 
 		svcKey := s.Key()

pilot/pkg/model/service.go

@@ -486,6 +486,10 @@ type IstioEndpoint struct {
 
 	// ServicePortName tracks the name of the port, this is used to select the IstioEndpoint by service port.
 	ServicePortName string
+	// LegacyClusterPortKey provides an alternative key from ServicePortName to support legacy quirks in the API.
+	// Basically, EDS merges by port name, but CDS historically ignored port name and matched on number.
+	// Note that for Kubernetes Service, this is identical - its only ServiceEntry where these checks can differ
+	LegacyClusterPortKey string
 
 	// ServiceAccount holds the associated service account.
 	ServiceAccount string

pilot/pkg/serviceregistry/serviceentry/conversion.go

@@ -16,6 +16,7 @@ package serviceentry
 
 import (
 	"net/netip"
+	"strconv"
 	"strings"
 	"time"
 
@@ -284,7 +285,9 @@ func (s *Controller) convertEndpoint(service *model.Service, servicePort *networ
 			Address:         addr,
 			EndpointPort:    instancePort,
 			ServicePortName: servicePort.Name,
-			Network:         network.ID(wle.Network),
+
+			LegacyClusterPortKey: strconv.Itoa(int(servicePort.Number)),
+			Network:              network.ID(wle.Network),
 			Locality: model.Locality{
 				Label:     locality,
 				ClusterID: clusterID,
@@ -341,11 +344,12 @@ func (s *Controller) convertServiceEntryToInstances(cfg config.Config, services
 				}
 				out = append(out, &model.ServiceInstance{
 					Endpoint: &model.IstioEndpoint{
-						Address:         string(service.Hostname),
-						EndpointPort:    endpointPort,
-						ServicePortName: serviceEntryPort.Name,
-						Labels:          nil,
-						TLSMode:         model.DisabledTLSModeLabel,
+						Address:              string(service.Hostname),
+						EndpointPort:         endpointPort,
+						ServicePortName:      serviceEntryPort.Name,
+						LegacyClusterPortKey: strconv.Itoa(int(serviceEntryPort.Number)),
+						Labels:               nil,
+						TLSMode:              model.DisabledTLSModeLabel,
 					},
 					Service:     service,
 					ServicePort: convertPort(serviceEntryPort),
@@ -395,6 +399,8 @@ func convertWorkloadInstanceToServiceInstance(workloadInstance *model.WorkloadIn
 			}
 			ep := workloadInstance.Endpoint.ShallowCopy()
 			ep.ServicePortName = serviceEntryPort.Name
+			ep.LegacyClusterPortKey = strconv.Itoa(int(serviceEntryPort.Number))
+
 			ep.EndpointPort = targetPort
 			out = append(out, &model.ServiceInstance{
 				Endpoint:    ep,

pilot/pkg/xds/cds_test.go

@@ -31,6 +31,7 @@ import (
 	"istio.io/istio/pkg/config"
 	"istio.io/istio/pkg/config/schema/gvk"
 	"istio.io/istio/pkg/ptr"
+	"istio.io/istio/pkg/slices"
 	"istio.io/istio/pkg/test/util/assert"
 	"istio.io/istio/pkg/util/sets"
 )
@@ -232,3 +233,95 @@ func TestSAN(t *testing.T) {
 		})
 	}
 }
+
+func TestServiceEntryMerge(t *testing.T) {
+	// Regression test for https://github.com/istio/istio/issues/50478
+	s := xds.NewFakeDiscoveryServer(t, xds.FakeOptions{ConfigString: `apiVersion: networking.istio.io/v1beta1
+kind: ServiceEntry
+metadata:
+  name: se1
+spec:
+  hosts:
+  - example.com
+  ports:
+  - name: port1
+    number: 80
+    protocol: HTTP
+  resolution: DNS
+---
+apiVersion: networking.istio.io/v1beta1
+kind: ServiceEntry
+metadata:
+  name: se2
+spec:
+  hosts:
+  - example.com
+  ports:
+  - name: port1
+    number: 8080
+    protocol: HTTP
+  resolution: DNS
+---
+apiVersion: networking.istio.io/v1beta1
+kind: ServiceEntry
+metadata:
+  name: se3
+spec:
+  hosts:
+  - example.com
+  ports:
+  - name: port1
+    number: 80
+    protocol: HTTP
+  resolution: DNS
+---
+apiVersion: networking.istio.io/v1beta1
+kind: ServiceEntry
+metadata:
+  name: se4
+spec:
+  hosts:
+  - example.com
+  ports:
+  - name: port1
+    number: 80
+    targetPort: 1234
+    protocol: HTTP
+  resolution: DNS
+---
+apiVersion: networking.istio.io/v1beta1
+kind: ServiceEntry
+metadata:
+  name: se5
+spec:
+  hosts:
+  - example.com
+  ports:
+  - name: port1
+    number: 80
+    targetPort: 999
+    protocol: HTTP
+  resolution: DNS
+  endpoints:
+  - address: endpoint.example.com
+  - address: endpoint-port-override.example.com
+    ports:
+      port1: 2345
+`})
+
+	res := xdstest.ExtractClusterEndpoints(s.Clusters(s.SetupProxy(nil)))
+	// TODO(https://github.com/istio/istio/issues/50749) order should be deterministic
+	slices.Sort(res["outbound|80||example.com"])
+	assert.Equal(t, res, map[string][]string{
+		"outbound|8080||example.com": {"example.com:8080"},
+		// Kind of weird to have multiple here, but it is what it is...
+		// If we had targetPort, etc, set here this would be required
+		"outbound|80||example.com": {
+			"endpoint-port-override.example.com:2345",
+			"endpoint.example.com:999",
+			"example.com:1234",
+			"example.com:80",
+			"example.com:80",
+		},
+	})
+}

releasenotes/notes/se-conflict.yaml

@@ -0,0 +1,9 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: networking
+issue:
+  - 50478
+releaseNotes:
+  - |
+    **Fixed** a behavioral change in Istio 1.20 that caused merging of ServiceEntries with the same hostname and port names
+    to give unexpected results.