diff --git a/.changelog/13958.txt b/.changelog/13958.txt new file mode 100644 index 000000000..a64a0026b --- /dev/null +++ b/.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. +``` diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index a280ba9bf..578d97e5f 100644 --- a/agent/consul/state/catalog.go +++ b/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{ - Value: serviceName, - EnterpriseMeta: *entMeta, - } - iter, err := tx.Get(tableServices, indexService, nonConnectQuery) + iter, err := tx.Get(tableServices, indexService, query) 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 { - supportsIngress = true - } else { - supportsTerminating = true + // If we were passed a NodeService, this might be the first registered instance of the service + // so we need to count it as either a connect or non-connect instance. + if ns != nil { + 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 } diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 357844dc0..501a7e0d3 100644 --- a/agent/consul/state/catalog_test.go +++ b/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.Len(t, results, 3) + require.Equal(t, uint64(19), idx) + 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.Len(t, results, 3) + require.Equal(t, uint64(19), idx) + 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")