From 9f924400e0b7ccb97f9a716e6f19709fab2efea4 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Mon, 11 May 2020 12:38:04 -0500 Subject: [PATCH 1/2] 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 --- agent/consul/state/catalog.go | 33 ++++--- agent/consul/state/catalog_test.go | 150 ++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 28 deletions(-) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index 19c8e7af7..36912dfbf 100644 --- a/agent/consul/state/catalog.go +++ b/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 err error + var ( + 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,21 @@ 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) { +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 +2557,24 @@ 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) { +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 +2592,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 diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 9a108541f..0067911c5 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -4530,6 +4530,49 @@ 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)) + + ws = memdb.NewWatchSet() + 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", @@ -5160,21 +5203,32 @@ func TestStateStore_GatewayServices_Ingress(t *testing.T) { require.Len(t, results, 2) }) - // TODO(ingress): This test case fails right now because of a - // bug in DeleteService where we delete are entries associated - // to a service, not just an entry created by a wildcard. - // t.Run("check ingress2 gateway services again", func(t *testing.T) { - // ws = memdb.NewWatchSet() - // idx, results, err := s.GatewayServices(ws, "ingress2", nil) - // require.NoError(err) - // require.Equal(uint64(18), idx) - // require.Len(results, 1) - // require.Equal("ingress2", results[0].Gateway.ID) - // require.Equal("service1", results[0].Service.ID) - // require.Equal(3333, results[0].Port) - // }) + t.Run("check ingress2 gateway services again", func(t *testing.T) { + expected := structs.GatewayServices{ + { + Gateway: structs.NewServiceID("ingress2", nil), + Service: structs.NewServiceID("service1", nil), + GatewayKind: structs.ServiceKindIngressGateway, + Port: 3333, + Protocol: "http", + RaftIndex: structs.RaftIndex{ + CreateIndex: 14, + ModifyIndex: 14, + }, + }, + } + 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)) @@ -5185,12 +5239,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)) From 107c7a9ca765b6ed0867be55a5ef44d3af070e9b Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Mon, 11 May 2020 14:04:59 -0500 Subject: [PATCH 2/2] PR comment and better formatting --- agent/consul/state/catalog.go | 21 +++++++++++++++++++-- agent/consul/state/catalog_test.go | 1 - 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index 36912dfbf..9b8dc3975 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -2524,7 +2524,15 @@ 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) (bool, 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 false, nil, fmt.Errorf("unexpected config entry type: %T", conf) @@ -2560,7 +2568,16 @@ func (s *Store) ingressConfigGatewayServices(tx *memdb.Txn, gateway structs.Serv return false, gatewayServices, nil } -func (s *Store) terminatingConfigGatewayServices(tx *memdb.Txn, gateway structs.ServiceID, conf structs.ConfigEntry, entMeta *structs.EnterpriseMeta) (bool, 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 false, nil, fmt.Errorf("unexpected config entry type: %T", conf) diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 0067911c5..c5e14d1f7 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -4545,7 +4545,6 @@ func TestStateStore_GatewayServices_Terminating(t *testing.T) { }, nil)) assert.False(t, watchFired(ws)) - ws = memdb.NewWatchSet() idx, out, err = s.GatewayServices(ws, "gateway", nil) assert.Nil(t, err) assert.Equal(t, idx, uint64(21))