Fix service entry merge (#50711)

* Fix service entry merge

* Add test

* add release notes

* better test

* working except stable ordering

* sort

* Refactor

* fix tests

* fix tests and notes

pilot/pkg/model/endpointshards.go

@@ -94,15 +94,26 @@ func (es *EndpointShards) Keys() []ShardKey {
 
 // CopyEndpoints takes a snapshot of all endpoints. As input, it takes a map of port name to number, to allow it to group
 // the results by service port number. This is a bit weird, but lets us efficiently construct the format the caller needs.
-func (es *EndpointShards) CopyEndpoints(portMap map[string]int) map[int][]*IstioEndpoint {
+func (es *EndpointShards) CopyEndpoints(portMap map[string]int, ports sets.Set[int]) map[int][]*IstioEndpoint {
 	es.RLock()
 	defer es.RUnlock()
 	res := map[int][]*IstioEndpoint{}
 	for _, v := range es.Shards {
 		for _, ep := range v {
-			portNum, f := portMap[ep.ServicePortName]
-			if !f {
-				continue
+			// use the port name as the key, unless LegacyClusterPortKey is set and takes precedence
+			// In EDS we match on port *name*. But for historical reasons, we match on port number for CDS.
+			var portNum int
+			if ep.LegacyClusterPortKey != 0 {
+				if !ports.Contains(ep.LegacyClusterPortKey) {
+					continue
+				}
+				portNum = ep.LegacyClusterPortKey
+			} else {
+				pn, f := portMap[ep.ServicePortName]
+				if !f {
+					continue
+				}
+				portNum = pn
 			}
 			res[portNum] = append(res[portNum], ep)
 		}

pilot/pkg/model/push_context.go

@@ -1445,8 +1445,10 @@ func (ps *PushContext) initServiceRegistry(env *Environment, configsUpdate sets.
 
 	for _, s := range allServices {
 		portMap := map[string]int{}
+		ports := sets.New[int]()
 		for _, port := range s.Ports {
 			portMap[port.Name] = port.Port
+			ports.Insert(port.Port)
 		}
 
 		svcKey := s.Key()
@@ -1455,7 +1457,7 @@ func (ps *PushContext) initServiceRegistry(env *Environment, configsUpdate sets.
 		}
 		shards, ok := env.EndpointIndex.ShardsForService(string(s.Hostname), s.Attributes.Namespace)
 		if ok {
-			instancesByPort := shards.CopyEndpoints(portMap)
+			instancesByPort := shards.CopyEndpoints(portMap, ports)
 			// Iterate over the instances and add them to the service index to avoid overiding the existing port instances.
 			for port, instances := range instancesByPort {
 				ps.ServiceIndex.instancesByPort[svcKey][port] = instances

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 int
 
 	// ServiceAccount holds the associated service account.
 	ServiceAccount string

pilot/pkg/serviceregistry/serviceentry/conversion.go

@@ -284,7 +284,9 @@ func (s *Controller) convertEndpoint(service *model.Service, servicePort *networ
 			Address:         addr,
 			EndpointPort:    instancePort,
 			ServicePortName: servicePort.Name,
-			Network:         network.ID(wle.Network),
+
+			LegacyClusterPortKey: int(servicePort.Number),
+			Network:              network.ID(wle.Network),
 			Locality: model.Locality{
 				Label:     locality,
 				ClusterID: clusterID,
@@ -341,11 +343,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: int(serviceEntryPort.Number),
+						Labels:               nil,
+						TLSMode:              model.DisabledTLSModeLabel,
 					},
 					Service:     service,
 					ServicePort: convertPort(serviceEntryPort),
@@ -395,6 +398,8 @@ func convertWorkloadInstanceToServiceInstance(workloadInstance *model.WorkloadIn
 			}
 			ep := workloadInstance.Endpoint.ShallowCopy()
 			ep.ServicePortName = serviceEntryPort.Name
+			ep.LegacyClusterPortKey = int(serviceEntryPort.Number)
+
 			ep.EndpointPort = targetPort
 			out = append(out, &model.ServiceInstance{
 				Endpoint:    ep,

pilot/pkg/serviceregistry/serviceentry/conversion_test.go

@@ -617,11 +617,12 @@ func makeInstance(cfg *config.Config, address string, port int,
 	return &model.ServiceInstance{
 		Service: svc,
 		Endpoint: &model.IstioEndpoint{
-			Address:         address,
-			EndpointPort:    uint32(port),
-			ServicePortName: svcPort.Name,
-			Labels:          svcLabels,
-			TLSMode:         tlsMode,
+			Address:              address,
+			EndpointPort:         uint32(port),
+			ServicePortName:      svcPort.Name,
+			LegacyClusterPortKey: int(svcPort.Number),
+			Labels:               svcLabels,
+			TLSMode:              tlsMode,
 		},
 		ServicePort: &model.Port{
 			Name:     svcPort.Name,

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: traffic-management
+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.