Update ingress/terminating wildcard logic and handle destinations

.changelog/13958.txt

@@ -0,0 +1,4 @@
+```release-note:bug
+connect: Ingress gateways with a wildcard service entry should no longer pick up non-connect services as upstreams.
+connect: Terminating gateways with a wildcard service entry should no longer pick up connect services as upstreams.
+```

agent/consul/state/catalog.go

@@ -890,6 +890,15 @@ func ensureServiceTxn(tx WriteTxn, idx uint64, node string, preserveIndexes bool
 			return fmt.Errorf("failed updating upstream/downstream association")
 		}
 
+		service := svc.Service
+		if svc.Kind == structs.ServiceKindConnectProxy {
+			service = svc.Proxy.DestinationServiceName
+		}
+		sn := structs.ServiceName{Name: service, EnterpriseMeta: svc.EnterpriseMeta}
+		if err = checkGatewayWildcardsAndUpdate(tx, idx, &sn, svc, structs.GatewayServiceKindService); err != nil {
+			return fmt.Errorf("failed updating gateway mapping: %s", err)
+		}
+
 		supported, err := virtualIPsSupported(tx, nil)
 		if err != nil {
 			return err
@@ -897,12 +906,6 @@ func ensureServiceTxn(tx WriteTxn, idx uint64, node string, preserveIndexes bool
 
 		// Update the virtual IP for the service
 		if supported {
-			service := svc.Service
-			if svc.Kind == structs.ServiceKindConnectProxy {
-				service = svc.Proxy.DestinationServiceName
-			}
-
-			sn := structs.ServiceName{Name: service, EnterpriseMeta: svc.EnterpriseMeta}
 			psn := structs.PeeredServiceName{Peer: svc.PeerName, ServiceName: sn}
 			vip, err := assignServiceVirtualIP(tx, idx, psn)
 			if err != nil {
@@ -3653,7 +3656,7 @@ func updateGatewayNamespace(tx WriteTxn, idx uint64, service *structs.GatewaySer
 			continue
 		}
 
-		supportsIngress, supportsTerminating, err := serviceConnectInstances(tx, sn.ServiceName, entMeta)
+		supportsIngress, supportsTerminating, err := serviceHasConnectInstances(tx, sn.ServiceName, entMeta)
 		if err != nil {
 			return err
 		}
@@ -3730,15 +3733,15 @@ func updateGatewayNamespace(tx WriteTxn, idx uint64, service *structs.GatewaySer
 	return nil
 }
 
-// serviceConnectInstances returns whether the service has at least one connect instance,
+// serviceHasConnectInstances returns whether the service has at least one connect instance,
 // and at least one non-connect instance.
-func serviceConnectInstances(tx WriteTxn, serviceName string, entMeta *acl.EnterpriseMeta) (bool, bool, error) {
+func serviceHasConnectInstances(tx WriteTxn, serviceName string, entMeta *acl.EnterpriseMeta) (bool, bool, error) {
 	hasConnectInstance := false
-	connectQuery := Query{
+	query := Query{
 		Value:          serviceName,
 		EnterpriseMeta: *entMeta,
 	}
-	svc, err := tx.First(tableServices, indexConnect, connectQuery)
+	svc, err := tx.First(tableServices, indexConnect, query)
 	if err != nil {
 		return false, false, fmt.Errorf("failed service lookup: %s", err)
 	}
@@ -3747,11 +3750,7 @@ func serviceConnectInstances(tx WriteTxn, serviceName string, entMeta *acl.Enter
 	}
 
 	hasNonConnectInstance := false
-	nonConnectQuery := Query{
+	iter, err := tx.Get(tableServices, indexService, query)
-		Value:          serviceName,
-		EnterpriseMeta: *entMeta,
-	}
-	iter, err := tx.Get(tableServices, indexService, nonConnectQuery)
 	if err != nil {
 		return false, false, fmt.Errorf("failed service lookup: %s", err)
 	}
@@ -3810,22 +3809,26 @@ func checkGatewayWildcardsAndUpdate(tx WriteTxn, idx uint64, svc *structs.Servic
 		return fmt.Errorf("failed gateway lookup for %q: %s", svc.Name, err)
 	}
 
-	supportsIngress, supportsTerminating, err := serviceConnectInstances(tx, svc.Name, &svc.EnterpriseMeta)
+	hasConnectInstance, hasNonConnectInstance, err := serviceHasConnectInstances(tx, svc.Name, &svc.EnterpriseMeta)
 	if err != nil {
 		return err
 	}
-	if ns != nil && ns.Connect.Native {
+	// If we were passed a NodeService, this might be the first registered instance of the service
-		supportsIngress = true
+	// so we need to count it as either a connect or non-connect instance.
-	} else {
+	if ns != nil {
-		supportsTerminating = true
+		if ns.Connect.Native || ns.Kind == structs.ServiceKindConnectProxy {
+			hasConnectInstance = true
+		} else {
+			hasNonConnectInstance = true
+		}
 	}
 
 	for service := svcGateways.Next(); service != nil; service = svcGateways.Next() {
 		if wildcardSvc, ok := service.(*structs.GatewayService); ok && wildcardSvc != nil {
-			if wildcardSvc.GatewayKind == structs.ServiceKindIngressGateway && !supportsIngress {
+			if wildcardSvc.GatewayKind == structs.ServiceKindIngressGateway && !hasConnectInstance {
 				continue
 			}
-			if wildcardSvc.GatewayKind == structs.ServiceKindTerminatingGateway && !supportsTerminating {
+			if wildcardSvc.GatewayKind == structs.ServiceKindTerminatingGateway && !hasNonConnectInstance && kind != structs.GatewayServiceKindDestination {
 				continue
 			}
 
@@ -3888,7 +3891,7 @@ func cleanupGatewayWildcards(tx WriteTxn, idx uint64, svc *structs.ServiceNode)
 	// If there are no connect instances left, ingress gateways with a wildcard entry can remove
 	// their association with it (same with terminating gateways if there are no non-connect
 	// instances left).
-	hasConnectInstance, hasNonConnectInstance, err := serviceConnectInstances(tx, svc.ServiceName, &svc.EnterpriseMeta)
+	hasConnectInstance, hasNonConnectInstance, err := serviceHasConnectInstances(tx, svc.ServiceName, &svc.EnterpriseMeta)
 	if err != nil {
 		return err
 	}

agent/consul/state/catalog_test.go

@@ -6320,23 +6320,80 @@ func TestStateStore_GatewayServices_WildcardAssociation(t *testing.T) {
 	})
 
 	t.Run("do not associate connect-proxy services with gateway", func(t *testing.T) {
+		// Should only associate web (the destination service of the proxy), not the
+		// sidecar service name itself.
 		testRegisterSidecarProxy(t, s, 19, "node1", "web")
-		require.False(t, watchFired(ws))
+		expected := structs.GatewayServices{
+			{
+				Gateway:      structs.NewServiceName("wildcardIngress", nil),
+				Service:      structs.NewServiceName("service1", nil),
+				GatewayKind:  structs.ServiceKindIngressGateway,
+				Port:         4444,
+				Protocol:     "http",
+				FromWildcard: true,
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 12,
+					ModifyIndex: 12,
+				},
+			},
+			{
+				Gateway:      structs.NewServiceName("wildcardIngress", nil),
+				Service:      structs.NewServiceName("service2", nil),
+				GatewayKind:  structs.ServiceKindIngressGateway,
+				Port:         4444,
+				Protocol:     "http",
+				FromWildcard: true,
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 12,
+					ModifyIndex: 12,
+				},
+			},
+			{
+				Gateway:      structs.NewServiceName("wildcardIngress", nil),
+				Service:      structs.NewServiceName("service3", nil),
+				GatewayKind:  structs.ServiceKindIngressGateway,
+				Port:         4444,
+				Protocol:     "http",
+				FromWildcard: true,
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 12,
+					ModifyIndex: 12,
+				},
+			},
+			{
+				Gateway:      structs.NewServiceName("wildcardIngress", nil),
+				Service:      structs.NewServiceName("web", nil),
+				ServiceKind:  structs.GatewayServiceKindService,
+				GatewayKind:  structs.ServiceKindIngressGateway,
+				Port:         4444,
+				Protocol:     "http",
+				FromWildcard: true,
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 19,
+					ModifyIndex: 19,
+				},
+			},
+		}
+
 		idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil)
 		require.NoError(t, err)
-		require.Equal(t, uint64(16), idx)
+		require.Equal(t, uint64(19), idx)
-		require.Len(t, results, 3)
+		require.ElementsMatch(t, results, expected)
 	})
 
 	t.Run("do not associate consul services with gateway", func(t *testing.T) {
+		ws := memdb.NewWatchSet()
+		_, _, err := s.GatewayServices(ws, "wildcardIngress", nil)
+		require.NoError(t, err)
+
 		require.Nil(t, s.EnsureService(20, "node1",
 			&structs.NodeService{ID: "consul", Service: "consul", Tags: nil},
 		))
 		require.False(t, watchFired(ws))
 		idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil)
 		require.NoError(t, err)
-		require.Equal(t, uint64(16), idx)
+		require.Equal(t, uint64(19), idx)
-		require.Len(t, results, 3)
+		require.Len(t, results, 4)
 	})
 }
 
@@ -6525,7 +6582,7 @@ func setupIngressState(t *testing.T, s *Store) memdb.WatchSet {
 	testRegisterNode(t, s, 0, "node1")
 	testRegisterNode(t, s, 1, "node2")
 
-	// Register some connect and non-connect services against the nodes.
+	// Register some connect services against the nodes.
 	testRegisterIngressService(t, s, 3, "node1", "wildcardIngress")
 	testRegisterIngressService(t, s, 4, "node1", "ingress1")
 	testRegisterIngressService(t, s, 5, "node1", "ingress2")
@@ -6533,7 +6590,15 @@ func setupIngressState(t *testing.T, s *Store) memdb.WatchSet {
 	testRegisterIngressService(t, s, 7, "node1", "nothingIngress")
 	testRegisterConnectService(t, s, 8, "node1", "service1")
 	testRegisterConnectService(t, s, 9, "node2", "service2")
-	testRegisterConnectService(t, s, 10, "node2", "service3")
+	testRegisterService(t, s, 10, "node2", "service3")
+	testRegisterServiceWithChangeOpts(t, s, 11, "node2", "service3-proxy", false, func(service *structs.NodeService) {
+		service.Kind = structs.ServiceKindConnectProxy
+		service.Proxy = structs.ConnectProxyConfig{
+			DestinationServiceName: "service3",
+		}
+	})
+
+	// Register some non-connect services - these shouldn't be picked up by a wildcard.
 	testRegisterService(t, s, 17, "node1", "service4")
 	testRegisterService(t, s, 18, "node2", "service5")