Check what ports gateways are listening on and filter based on that (#58131)

* Check what ports gateways are listening on and filter based on that

TestTraffic tests are failing when we run them in ambient multi-network
environment. Looking through the failure details it seems that we
overlooked interoperability between ambient and sidecar.

Specifically, when in the same namespace we have both pods/services using
ambient dataplane and pods/services using sidecar at the same time. In
multi-network scenario, what can happen is that sidecars will see
ambient E/W gateways, because pilot generates EDS endpoints for those,
but at the moment sidecars cannot talke to ambient E/W gateways because
those expect double-HBONE.

We want to ultimately allow sidecars to talk to ambient E/W gateways
using double-HBONE (or add support for mTLS to ambient E/W gateways),
but it's a bigger project.

So for now I'm trying to filter gateways that expect double-HBONE from
sidecar endpoints. There are a few options to achieve that that I
considered:

1. [not exactly a fix] Use istio.test.ambient.everywhere flag for
   tests - it avoids the issue, but it also defeats one of the reasons
   for running those tests in the first place
2. [non invasive] check if gw has a non-zero HBONE port and if so
   exclude it from gws used by sidecar - this solves the problem and is
   probably the least risky change
3. [proper, I think] When generating gateways check what ports they
   are actually listening on and when generating EDS endpoints account
   for that (similarly to how we would take into account HBONE port in
   the option 2 above).

I ultimately decided to go for the last option. Option 1 is not exactly
a solution, so I discarded it for that reason. Option 2 works, but it
introduces an assumption that a GW is either ambient and listens on
HBONE port or a mTLS, but not both - this is true now (and probably long
into the future), but it does not have to be. Another thing to mention
is that it creates a bit of assymetry between how HBONE port and mTLS
port are treated in the code - I personally don't like that.

And the last item in favor of the option 3 is that there is a TODO in
the code that suggests that we should at least consider checking what
ports the gw service is actually listening on. Going for option 3,
assuming we don't find any downsides, is that we can close that TODO
as well.

So this change adds a check to the code that generates gateways from
services to confirm that the service actually listens on the port we
intend to use. If the service does not listen on the port, I mark it as
0, so upstream code can check for that.

Then in the EDS endpoint generation code I check if we are generating
for a sidecar and if so, I ignore gateways that don't listen on mTLS
port (by checking if it's 0 or not).

Signed-off-by: Mikhail Krinkin <[email protected]>

* Fix the relevant tests

Signed-off-by: Mikhail Krinkin <[email protected]>

* Fix formatting

Signed-off-by: Mikhail Krinkin <[email protected]>

* Add a release note for the fix

Signed-off-by: Mikhail Krinkin <[email protected]>

---------

Signed-off-by: Mikhail Krinkin <[email protected]>

Author: krinkinmu

pilot/pkg/serviceregistry/kube/controller/network.go

@@ -312,28 +312,43 @@ func (n *networkManager) extractGatewaysInner(svc *model.Service) bool {
 
 // getGatewayDetails returns gateways without the address populated, only the network and (unmapped) port for a given service.
 func (n *networkManager) getGatewayDetails(svc *model.Service) []model.NetworkGateway {
-	// TODO should we start checking if svc's Ports contain the gateway port?
-
 	// We have different types of E/W gateways - those that use mTLS (those are used in sidecar mode when talking cross networks)
 	// and those that use double-HBONE (those are used in ambient mode when talking cross cluster). A gateway service may or may
 	// not listen on the mTLS (15443, by default) or HBONE (15008) ports, depending on the mode of operation used by the mesh
 	// in the remote cluster. We should not use gateways that don't really listen on the right port.
-	hbonePort := uint32(DefaultNetworkGatewayHBONEPort)
-	if _, exists := svc.Ports.GetByPort(DefaultNetworkGatewayHBONEPort); !exists {
-		hbonePort = 0
-	}
 
 	// label based gateways
 	// TODO label based gateways could support being the gateway for multiple networks
 	if nw := svc.Attributes.Labels[label.TopologyNetwork.Name]; nw != "" {
+		hbonePort := DefaultNetworkGatewayHBONEPort
+		gwPort := DefaultNetworkGatewayPort
+
 		if gwPortStr := svc.Attributes.Labels[label.NetworkingGatewayPort.Name]; gwPortStr != "" {
-			if gwPort, err := strconv.Atoi(gwPortStr); err == nil {
-				return []model.NetworkGateway{{Port: uint32(gwPort), HBONEPort: hbonePort, Network: network.ID(nw)}}
+			port, err := strconv.Atoi(gwPortStr)
+			if err != nil {
+				log.Warnf("could not parse %q for %s on %s/%s; defaulting to %d",
+					gwPortStr, label.NetworkingGatewayPort.Name, svc.Attributes.Namespace, svc.Attributes.Name, DefaultNetworkGatewayPort)
+			} else {
+				gwPort = port
 			}
-			log.Warnf("could not parse %q for %s on %s/%s; defaulting to %d",
-				gwPortStr, label.NetworkingGatewayPort.Name, svc.Attributes.Namespace, svc.Attributes.Name, DefaultNetworkGatewayPort)
 		}
-		return []model.NetworkGateway{{Port: DefaultNetworkGatewayPort, HBONEPort: hbonePort, Network: network.ID(nw)}}
+
+		_, acceptMTLS := svc.Ports.GetByPort(gwPort)
+		_, acceptHBONE := svc.Ports.GetByPort(hbonePort)
+
+		if !acceptMTLS && !acceptHBONE {
+			log.Warnf("service %s/%s is labeled as gateway, but does not listen neither on port %d nor on port %d",
+				svc.Attributes.Namespace, svc.Attributes.Name, gwPort, hbonePort)
+			return nil
+		}
+
+		if !acceptMTLS {
+			gwPort = 0
+		}
+		if !acceptHBONE {
+			hbonePort = 0
+		}
+		return []model.NetworkGateway{{Port: uint32(gwPort), HBONEPort: uint32(hbonePort), Network: network.ID(nw)}}
 	}
 
 	// meshNetworks registryServiceName+fromRegistry

pilot/pkg/xds/endpoints/endpoint_builder.go

@@ -927,3 +927,7 @@ func isWaypointProxy(node *model.Proxy) bool {
 
 	return isManagedGateway && controller == constants.ManagedGatewayMeshControllerLabel
 }
+
+func isSidecarProxy(node *model.Proxy) bool {
+	return node != nil && node.Type == model.SidecarProxy
+}

pilot/pkg/xds/endpoints/ep_filters.go

@@ -288,6 +288,20 @@ func (b *EndpointBuilder) selectNetworkGateways(nw network.ID, c cluster.ID) []m
 		return ambientGws
 	}
 
+	// Sidecar proxies cannot talk to ambient E/W gateway for now, so when we see an ambient
+	// E/W gateway (e.g., a gateway that listens on hbone port, but does not have an mTLS port
+	// we filter it out.
+	if isSidecarProxy(b.proxy) {
+		var sidecarGws []model.NetworkGateway
+		for _, gw := range gws {
+			if gw.Port == 0 {
+				continue
+			}
+			sidecarGws = append(sidecarGws, gw)
+		}
+		return sidecarGws
+	}
+
 	return gws
 }
 

pilot/pkg/xds/mesh_network_test.go

@@ -191,6 +191,7 @@ func TestMeshNetworking(t *testing.T) {
 				Spec: corev1.ServiceSpec{
 					Type:        corev1.ServiceTypeClusterIP,
 					ExternalIPs: []string{"3.3.3.3"},
+					Ports:       []corev1.ServicePort{{Port: 15443}},
 				},
 			}},
 		},
@@ -574,7 +575,10 @@ func gatewaySvc(name, ip, network string) *corev1.Service {
 			Namespace: "istio-system",
 			Labels:    map[string]string{label.TopologyNetwork.Name: network},
 		},
-		Spec: corev1.ServiceSpec{Type: corev1.ServiceTypeLoadBalancer},
+		Spec: corev1.ServiceSpec{
+			Type:  corev1.ServiceTypeLoadBalancer,
+			Ports: []corev1.ServicePort{{Port: 15443}},
+		},
 		Status: corev1.ServiceStatus{
 			LoadBalancer: corev1.LoadBalancerStatus{Ingress: []corev1.LoadBalancerIngress{{IP: ip}}},
 		},

releasenotes/notes/57878.yaml

@@ -0,0 +1,8 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: traffic-management
+issue:
+- 57878
+releaseNotes:
+- |
+  **Fixed** the issue when sidecars try to route requests to ambient E/W gateways incorrectly.