Return early from updateGatewayServices if nothing to update (#7838)

* Return early from updateGatewayServices if nothing to update

Previously, we returned an empty slice of gatewayServices, which caused
us to accidentally delete everything in the memdb table

* PR comment and better formatting

agent/consul/state/catalog.go

@@ -2472,20 +2472,23 @@ func checkSessionsTxn(tx *memdb.Txn, hc *structs.HealthCheck) ([]*sessionCheck,
 
 // updateGatewayServices associates services with gateways as specified in a gateway config entry
 func (s *Store) updateGatewayServices(tx *memdb.Txn, idx uint64, conf structs.ConfigEntry, entMeta *structs.EnterpriseMeta) error {
-	var gatewayServices structs.GatewayServices
+	var (
-	var err error
+		noChange        bool
+		gatewayServices structs.GatewayServices
+		err             error
+	)
 
 	gatewayID := structs.NewServiceID(conf.GetName(), entMeta)
 	switch conf.GetKind() {
 	case structs.IngressGateway:
-		gatewayServices, err = s.ingressConfigGatewayServices(tx, gatewayID, conf, entMeta)
+		noChange, gatewayServices, err = s.ingressConfigGatewayServices(tx, gatewayID, conf, entMeta)
 	case structs.TerminatingGateway:
-		gatewayServices, err = s.terminatingConfigGatewayServices(tx, gatewayID, conf, entMeta)
+		noChange, gatewayServices, err = s.terminatingConfigGatewayServices(tx, gatewayID, conf, entMeta)
 	default:
 		return fmt.Errorf("config entry kind %q does not need gateway-services", conf.GetKind())
 	}
 	// Return early if there is an error OR we don't have any services to update
-	if err != nil {
+	if err != nil || noChange {
 		return err
 	}
 
@@ -2521,21 +2524,29 @@ func (s *Store) updateGatewayServices(tx *memdb.Txn, idx uint64, conf structs.Co
 	return nil
 }
 
-func (s *Store) ingressConfigGatewayServices(tx *memdb.Txn, gateway structs.ServiceID, conf structs.ConfigEntry, entMeta *structs.EnterpriseMeta) (structs.GatewayServices, error) {
+// ingressConfigGatewayServices constructs a list of GatewayService structs for
+// insertion into the memdb table, specific to ingress gateways. The boolean
+// returned indicates that there are no changes necessary to the memdb table.
+func (s *Store) ingressConfigGatewayServices(
+	tx *memdb.Txn,
+	gateway structs.ServiceID,
+	conf structs.ConfigEntry,
+	entMeta *structs.EnterpriseMeta,
+) (bool, structs.GatewayServices, error) {
 	entry, ok := conf.(*structs.IngressGatewayConfigEntry)
 	if !ok {
-		return nil, fmt.Errorf("unexpected config entry type: %T", conf)
+		return false, nil, fmt.Errorf("unexpected config entry type: %T", conf)
 	}
 
 	// Check if service list matches the last known list for the config entry, if it does, skip the update
 	_, c, err := s.configEntryTxn(tx, nil, conf.GetKind(), conf.GetName(), entMeta)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get config entry: %v", err)
+		return false, nil, fmt.Errorf("failed to get config entry: %v", err)
 	}
 	if cfg, ok := c.(*structs.IngressGatewayConfigEntry); ok && cfg != nil {
 		if reflect.DeepEqual(cfg.Listeners, entry.Listeners) {
 			// Services are the same, nothing to update
-			return nil, nil
+			return true, nil, nil
 		}
 	}
 
@@ -2554,24 +2565,33 @@ func (s *Store) ingressConfigGatewayServices(tx *memdb.Txn, gateway structs.Serv
 			gatewayServices = append(gatewayServices, mapping)
 		}
 	}
-	return gatewayServices, nil
+	return false, gatewayServices, nil
 }
 
-func (s *Store) terminatingConfigGatewayServices(tx *memdb.Txn, gateway structs.ServiceID, conf structs.ConfigEntry, entMeta *structs.EnterpriseMeta) (structs.GatewayServices, error) {
+// terminatingConfigGatewayServices constructs a list of GatewayService structs
+// for insertion into the memdb table, specific to terminating gateways. The
+// boolean returned indicates that there are no changes necessary to the memdb
+// table.
+func (s *Store) terminatingConfigGatewayServices(
+	tx *memdb.Txn,
+	gateway structs.ServiceID,
+	conf structs.ConfigEntry,
+	entMeta *structs.EnterpriseMeta,
+) (bool, structs.GatewayServices, error) {
 	entry, ok := conf.(*structs.TerminatingGatewayConfigEntry)
 	if !ok {
-		return nil, fmt.Errorf("unexpected config entry type: %T", conf)
+		return false, nil, fmt.Errorf("unexpected config entry type: %T", conf)
 	}
 
 	// Check if service list matches the last known list for the config entry, if it does, skip the update
 	_, c, err := s.configEntryTxn(tx, nil, conf.GetKind(), conf.GetName(), entMeta)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get config entry: %v", err)
+		return false, nil, fmt.Errorf("failed to get config entry: %v", err)
 	}
 	if cfg, ok := c.(*structs.TerminatingGatewayConfigEntry); ok && cfg != nil {
 		if reflect.DeepEqual(cfg.Services, entry.Services) {
 			// Services are the same, nothing to update
-			return nil, nil
+			return true, nil, nil
 		}
 	}
 
@@ -2589,7 +2609,7 @@ func (s *Store) terminatingConfigGatewayServices(tx *memdb.Txn, gateway structs.
 
 		gatewayServices = append(gatewayServices, mapping)
 	}
-	return gatewayServices, nil
+	return false, gatewayServices, nil
 }
 
 // updateGatewayNamespace is used to target all services within a namespace

agent/consul/state/catalog_test.go

@@ -4529,6 +4529,48 @@ func TestStateStore_GatewayServices_Terminating(t *testing.T) {
 	}
 	assert.Equal(t, expect, out)
 
+	// Check that we don't update on same exact config
+	assert.Nil(t, s.EnsureConfigEntry(21, &structs.TerminatingGatewayConfigEntry{
+		Kind: "terminating-gateway",
+		Name: "gateway",
+		Services: []structs.LinkedService{
+			{
+				Name: "db",
+			},
+			{
+				Name: "api",
+			},
+		},
+	}, nil))
+	assert.False(t, watchFired(ws))
+
+	idx, out, err = s.GatewayServices(ws, "gateway", nil)
+	assert.Nil(t, err)
+	assert.Equal(t, idx, uint64(21))
+	assert.Len(t, out, 2)
+
+	expect = structs.GatewayServices{
+		{
+			Service:     structs.NewServiceID("api", nil),
+			Gateway:     structs.NewServiceID("gateway", nil),
+			GatewayKind: structs.ServiceKindTerminatingGateway,
+			RaftIndex: structs.RaftIndex{
+				CreateIndex: 21,
+				ModifyIndex: 21,
+			},
+		},
+		{
+			Service:     structs.NewServiceID("db", nil),
+			Gateway:     structs.NewServiceID("gateway", nil),
+			GatewayKind: structs.ServiceKindTerminatingGateway,
+			RaftIndex: structs.RaftIndex{
+				CreateIndex: 21,
+				ModifyIndex: 21,
+			},
+		},
+	}
+	assert.Equal(t, expect, out)
+
 	// Associate gateway with a wildcard and add TLS config
 	assert.Nil(t, s.EnsureConfigEntry(22, &structs.TerminatingGatewayConfigEntry{
 		Kind: "terminating-gateway",
@@ -5159,21 +5201,32 @@ func TestStateStore_GatewayServices_Ingress(t *testing.T) {
 		require.Len(t, results, 2)
 	})
 
-	// TODO(ingress): This test case fails right now because of a
+	t.Run("check ingress2 gateway services again", func(t *testing.T) {
-	// bug in DeleteService where we delete are entries associated
+		expected := structs.GatewayServices{
-	// to a service, not just an entry created by a wildcard.
+			{
-	// t.Run("check ingress2 gateway services again", func(t *testing.T) {
+				Gateway:     structs.NewServiceID("ingress2", nil),
-	// 	ws = memdb.NewWatchSet()
+				Service:     structs.NewServiceID("service1", nil),
-	// 	idx, results, err := s.GatewayServices(ws, "ingress2", nil)
+				GatewayKind: structs.ServiceKindIngressGateway,
-	// 	require.NoError(err)
+				Port:        3333,
-	// 	require.Equal(uint64(18), idx)
+				Protocol:    "http",
-	// 	require.Len(results, 1)
+				RaftIndex: structs.RaftIndex{
-	// 	require.Equal("ingress2", results[0].Gateway.ID)
+					CreateIndex: 14,
-	// 	require.Equal("service1", results[0].Service.ID)
+					ModifyIndex: 14,
-	// 	require.Equal(3333, results[0].Port)
+				},
-	// })
+			},
+		}
+		ws = memdb.NewWatchSet()
+		idx, results, err := s.GatewayServices(ws, "ingress2", nil)
+		require.NoError(t, err)
+		require.Equal(t, uint64(18), idx)
+		require.ElementsMatch(t, results, expected)
+	})
 
 	t.Run("deleting a wildcard config entry", func(t *testing.T) {
+		ws = memdb.NewWatchSet()
+		_, _, err := s.GatewayServices(ws, "wildcardIngress", nil)
+		require.NoError(t, err)
+
 		require.Nil(t, s.DeleteConfigEntry(19, "ingress-gateway", "wildcardIngress", nil))
 		require.True(t, watchFired(ws))
 
@@ -5184,12 +5237,82 @@ func TestStateStore_GatewayServices_Ingress(t *testing.T) {
 		require.Len(t, results, 0)
 	})
 
+	t.Run("update ingress1 with exact same config entry", func(t *testing.T) {
+		ingress1 := &structs.IngressGatewayConfigEntry{
+			Kind: "ingress-gateway",
+			Name: "ingress1",
+			Listeners: []structs.IngressListener{
+				{
+					Port:     1111,
+					Protocol: "http",
+					Services: []structs.IngressService{
+						{
+							Name:  "service1",
+							Hosts: []string{"test.example.com"},
+						},
+					},
+				},
+				{
+					Port:     2222,
+					Protocol: "http",
+					Services: []structs.IngressService{
+						{
+							Name: "service2",
+						},
+					},
+				},
+			},
+		}
+
+		ws = memdb.NewWatchSet()
+		_, _, err := s.GatewayServices(ws, "ingress1", nil)
+		require.NoError(t, err)
+
+		require.Nil(t, s.EnsureConfigEntry(20, ingress1, nil))
+		require.False(t, watchFired(ws))
+
+		expected := structs.GatewayServices{
+			{
+				Gateway:     structs.NewServiceID("ingress1", nil),
+				Service:     structs.NewServiceID("service1", nil),
+				GatewayKind: structs.ServiceKindIngressGateway,
+				Port:        1111,
+				Protocol:    "http",
+				Hosts:       []string{"test.example.com"},
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 13,
+					ModifyIndex: 13,
+				},
+			},
+			{
+				Gateway:     structs.NewServiceID("ingress1", nil),
+				Service:     structs.NewServiceID("service2", nil),
+				GatewayKind: structs.ServiceKindIngressGateway,
+				Port:        2222,
+				Protocol:    "http",
+				RaftIndex: structs.RaftIndex{
+					CreateIndex: 13,
+					ModifyIndex: 13,
+				},
+			},
+		}
+		idx, results, err := s.GatewayServices(ws, "ingress1", nil)
+		require.NoError(t, err)
+		require.Equal(t, uint64(19), idx)
+		require.ElementsMatch(t, results, expected)
+	})
+
 	t.Run("updating a config entry with zero listeners", func(t *testing.T) {
 		ingress1 := &structs.IngressGatewayConfigEntry{
 			Kind:      "ingress-gateway",
 			Name:      "ingress1",
 			Listeners: []structs.IngressListener{},
 		}
+
+		ws = memdb.NewWatchSet()
+		_, _, err := s.GatewayServices(ws, "ingress1", nil)
+		require.NoError(t, err)
+
 		require.Nil(t, s.EnsureConfigEntry(20, ingress1, nil))
 		require.True(t, watchFired(ws))