luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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
This commit is contained in:
Chris Piraino 2020-05-11 14:46:48 -05:00 committed by GitHub
parent 1a7c99cf31 107c7a9ca7
commit 1173b31949
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 171 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
agent/consul/state/catalog.go
View File

@ -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,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

149
agent/consul/state/catalog_test.go
View File

@ -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
// 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))
@ -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))