From 6ef8ae9965f14d5c4810ea49c6e6ab63fe725cb5 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Fri, 17 Apr 2020 11:24:34 -0500 Subject: [PATCH] Fix bug where non-typical services are associated with gateways (#7662) On every service registration, we check to see if a service should be assassociated to a wildcard gateway-service. This fixes an issue where we did not correctly check to see if the service being registered was a "typical" service or not. --- agent/consul/state/catalog.go | 44 +++++++++++++++-------- agent/consul/state/catalog_test.go | 56 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index 2cf3b33599..129fd9916a 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -776,21 +776,9 @@ func (s *Store) ensureServiceTxn(tx *memdb.Txn, idx uint64, node string, svc *st } // Check if this service is covered by a gateway's wildcard specifier - svcGateways, err := s.serviceGateways(tx, structs.WildcardSpecifier, &svc.EnterpriseMeta) + err = s.checkGatewayWildcardsAndUpdate(tx, idx, svc) if err != nil { - return fmt.Errorf("failed gateway lookup for %q: %s", svc.Service, err) - } - for service := svcGateways.Next(); service != nil; service = svcGateways.Next() { - if wildcardSvc, ok := service.(*structs.GatewayService); ok && wildcardSvc != nil { - - // Copy the wildcard mapping and modify it - gatewaySvc := wildcardSvc.Clone() - gatewaySvc.Service = structs.NewServiceID(svc.Service, &svc.EnterpriseMeta) - - if err = s.updateGatewayService(tx, idx, gatewaySvc); err != nil { - return fmt.Errorf("Failed to associate service %q with gateway %q", gatewaySvc.Service.String(), gatewaySvc.Gateway.String()) - } - } + return fmt.Errorf("failed updating gateway mapping: %s", err) } // Create the service node entry and populate the indexes. Note that @@ -2606,6 +2594,34 @@ func (s *Store) updateGatewayService(tx *memdb.Txn, idx uint64, mapping *structs return nil } +// checkWildcardForGatewaysAndUpdate checks whether a service matches a +// wildcard definition in gateway config entries and if so adds it the the +// gateway-services table. +func (s *Store) checkGatewayWildcardsAndUpdate(tx *memdb.Txn, idx uint64, svc *structs.NodeService) error { + // Do not associate non-typical services with gateways or consul services + if svc.Kind != structs.ServiceKindTypical || svc.Service == "consul" { + return nil + } + + svcGateways, err := s.serviceGateways(tx, structs.WildcardSpecifier, &svc.EnterpriseMeta) + if err != nil { + return fmt.Errorf("failed gateway lookup for %q: %s", svc.Service, err) + } + for service := svcGateways.Next(); service != nil; service = svcGateways.Next() { + if wildcardSvc, ok := service.(*structs.GatewayService); ok && wildcardSvc != nil { + + // Copy the wildcard mapping and modify it + gatewaySvc := wildcardSvc.Clone() + gatewaySvc.Service = structs.NewServiceID(svc.Service, &svc.EnterpriseMeta) + + if err = s.updateGatewayService(tx, idx, gatewaySvc); err != nil { + return fmt.Errorf("Failed to associate service %q with gateway %q", gatewaySvc.Service.String(), gatewaySvc.Gateway.String()) + } + } + } + return nil +} + // serviceGateways returns all GatewayService entries with the given service name. This effectively looks up // all the gateways mapped to this service. func (s *Store) serviceGateways(tx *memdb.Txn, name string, entMeta *structs.EnterpriseMeta) (memdb.ResultIterator, error) { diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index df66682962..485966bf88 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -4854,6 +4854,62 @@ func TestStateStore_GatewayServices_Ingress(t *testing.T) { }) } +func TestStateStore_GatewayServices_WildcardAssociation(t *testing.T) { + s := testStateStore(t) + setupIngressState(t, s) + require := require.New(t) + ws := memdb.NewWatchSet() + + t.Run("base case for wildcard", func(t *testing.T) { + idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil) + require.NoError(err) + require.Equal(uint64(14), idx) + require.Len(results, 3) + }) + + t.Run("do not associate ingress services with gateway", func(t *testing.T) { + testRegisterIngressService(t, s, 15, "node1", "testIngress") + require.False(watchFired(ws)) + idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil) + require.NoError(err) + require.Equal(uint64(14), idx) + require.Len(results, 3) + }) + + t.Run("do not associate terminating-gateway services with gateway", func(t *testing.T) { + require.Nil(s.EnsureService(16, "node1", + &structs.NodeService{ + Kind: structs.ServiceKindTerminatingGateway, ID: "gateway", Service: "gateway", Port: 443, + }, + )) + require.False(watchFired(ws)) + idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil) + require.NoError(err) + require.Equal(uint64(14), idx) + require.Len(results, 3) + }) + + t.Run("do not associate connect-proxy services with gateway", func(t *testing.T) { + testRegisterSidecarProxy(t, s, 17, "node1", "web") + require.False(watchFired(ws)) + idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil) + require.NoError(err) + require.Equal(uint64(14), idx) + require.Len(results, 3) + }) + + t.Run("do not associate consul services with gateway", func(t *testing.T) { + require.Nil(s.EnsureService(18, "node1", + &structs.NodeService{ID: "consul", Service: "consul", Tags: nil}, + )) + require.False(watchFired(ws)) + idx, results, err := s.GatewayServices(ws, "wildcardIngress", nil) + require.NoError(err) + require.Equal(uint64(14), idx) + require.Len(results, 3) + }) +} + func setupIngressState(t *testing.T, s *Store) memdb.WatchSet { // Querying with no matches gives an empty response ws := memdb.NewWatchSet()