diff --git a/agent/consul/fsm/snapshot_test.go b/agent/consul/fsm/snapshot_test.go index eea7bd63b3..5a7665965e 100644 --- a/agent/consul/fsm/snapshot_test.go +++ b/agent/consul/fsm/snapshot_test.go @@ -518,6 +518,16 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { }, })) + // Add a service-resolver entry to get a virtual IP for service foo + resolverEntry := &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "foo", + } + require.NoError(t, fsm.state.EnsureConfigEntry(34, resolverEntry)) + vip, err = fsm.state.VirtualIPForService(structs.PeeredServiceName{ServiceName: structs.NewServiceName("foo", nil)}) + require.NoError(t, err) + require.Equal(t, vip, "240.0.0.3") + // Snapshot snap, err := fsm.Snapshot() require.NoError(t, err) @@ -621,6 +631,10 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) { vip, err = fsm2.state.VirtualIPForService(psn) require.NoError(t, err) require.Equal(t, vip, "240.0.0.2") + psn = structs.PeeredServiceName{ServiceName: structs.NewServiceName("foo", nil)} + vip, err = fsm2.state.VirtualIPForService(psn) + require.NoError(t, err) + require.Equal(t, vip, "240.0.0.3") // Verify key is set _, d, err := fsm2.state.KVSGet(nil, "/test", nil) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index fdda21d723..3d181346f5 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -2016,6 +2016,33 @@ func freeServiceVirtualIP( return nil } + // Don't deregister the virtual IP if at least one instance of this service still exists. + q := Query{ + Value: psn.ServiceName.Name, + EnterpriseMeta: psn.ServiceName.EnterpriseMeta, + PeerName: psn.Peer, + } + if remainingService, err := tx.First(tableServices, indexService, q); err == nil { + if remainingService != nil { + return nil + } + } else { + return fmt.Errorf("failed service lookup for %q: %s", psn.ServiceName.Name, err) + } + + // Don't deregister the virtual IP if at least one resolver/router/splitter config entry still + // references this service. + configEntryVIPKinds := []string{structs.ServiceResolver, structs.ServiceRouter, structs.ServiceSplitter} + for _, kind := range configEntryVIPKinds { + _, entry, err := configEntryTxn(tx, nil, kind, psn.ServiceName.Name, &psn.ServiceName.EnterpriseMeta) + if err != nil { + return fmt.Errorf("failed config entry lookup for %s/%s: %s", kind, psn.ServiceName.Name, err) + } + if entry != nil { + return nil + } + } + // Don't deregister the virtual IP if at least one terminating gateway still references this service. termGatewaySupported, err := terminatingGatewayVirtualIPsSupported(tx, nil) if err != nil { diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index e4b3665742..e14b8e1ae3 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -455,6 +455,15 @@ func deleteConfigEntryTxn(tx WriteTxn, idx uint64, kind, name string, entMeta *a return fmt.Errorf("failed updating index: %s", err) } + // If this is a resolver/router/splitter, attempt to delete the virtual IP associated + // with this service. + if kind == structs.ServiceResolver || kind == structs.ServiceRouter || kind == structs.ServiceSplitter { + psn := structs.PeeredServiceName{ServiceName: sn} + if err := freeServiceVirtualIP(tx, idx, psn, nil); err != nil { + return fmt.Errorf("failed to clean up virtual IP for %q: %v", psn.String(), err) + } + } + return nil } @@ -465,14 +474,15 @@ func insertConfigEntryWithTxn(tx WriteTxn, idx uint64, conf structs.ConfigEntry) // If the config entry is for a terminating or ingress gateway we update the memdb table // that associates gateways <-> services. - if conf.GetKind() == structs.TerminatingGateway || conf.GetKind() == structs.IngressGateway { + kind := conf.GetKind() + if kind == structs.TerminatingGateway || kind == structs.IngressGateway { err := updateGatewayServices(tx, idx, conf, conf.GetEnterpriseMeta()) if err != nil { return fmt.Errorf("failed to associate services to gateway: %v", err) } } - switch conf.GetKind() { + switch kind { case structs.ServiceDefaults: if conf.(*structs.ServiceConfigEntry).Destination != nil { sn := structs.ServiceName{Name: conf.GetName(), EnterpriseMeta: *conf.GetEnterpriseMeta()} @@ -499,6 +509,15 @@ func insertConfigEntryWithTxn(tx WriteTxn, idx uint64, conf structs.ConfigEntry) if err != nil { return err } + case structs.ServiceResolver: + fallthrough + case structs.ServiceRouter: + fallthrough + case structs.ServiceSplitter: + psn := structs.PeeredServiceName{ServiceName: structs.NewServiceName(conf.GetName(), conf.GetEnterpriseMeta())} + if _, err := assignServiceVirtualIP(tx, idx, psn); err != nil { + return err + } } // Insert the config entry and update the index diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index b6db8d52cb..ea98909239 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -1,6 +1,7 @@ package state import ( + "fmt" "testing" "time" @@ -3032,3 +3033,149 @@ func TestStore_ValidateServiceIntentionsErrorOnIncompatibleProtocols(t *testing. }) } } + +func TestStateStore_ConfigEntry_VirtualIP(t *testing.T) { + createServiceInstance := func(t *testing.T, s *Store, name string) { + ns1 := &structs.NodeService{ + ID: name, + Service: name, + Address: "1.1.1.1", + Port: 1111, + Connect: structs.ServiceConnect{Native: true}, + } + require.NoError(t, s.EnsureService(0, "node1", ns1)) + } + deleteServiceInstance := func(t *testing.T, s *Store, name string) { + require.NoError(t, s.DeleteService(0, "node1", name, nil, "")) + } + createServiceResolver := func(t *testing.T, s *Store, name string) { + require.NoError(t, s.EnsureConfigEntry(0, &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: name, + })) + } + createServiceRouter := func(t *testing.T, s *Store, name string) { + require.NoError(t, s.EnsureConfigEntry(0, &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: name, + })) + } + createServiceSplitter := func(t *testing.T, s *Store, name string) { + require.NoError(t, s.EnsureConfigEntry(0, &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: name, + Splits: []structs.ServiceSplit{ + {Weight: 100}, + }, + })) + } + deleteConfigEntry := func(t *testing.T, s *Store, kind, name string) { + require.NoError(t, s.DeleteConfigEntry(0, kind, name, nil)) + } + ensureVirtualIP := func(t *testing.T, s *Store, service string, value string) { + vip, err := s.VirtualIPForService(structs.PeeredServiceName{ServiceName: structs.ServiceName{Name: service}}) + require.NoError(t, err) + require.Equal(t, value, vip) + } + + testVIPStateStore := func(t *testing.T) *Store { + s := testStateStore(t) + setVirtualIPFlags(t, s) + testRegisterNode(t, s, 0, "node1") + s.EnsureConfigEntry(0, &structs.ProxyConfigEntry{ + Kind: structs.ProxyDefaults, + Name: structs.ProxyConfigGlobal, + Config: map[string]interface{}{ + "protocol": "http", + }, + }) + return s + } + + cases := []struct { + kind string + createFunc func(*testing.T, *Store, string) + }{ + { + kind: structs.ServiceResolver, + createFunc: createServiceResolver, + }, + { + kind: structs.ServiceRouter, + createFunc: createServiceRouter, + }, + { + kind: structs.ServiceSplitter, + createFunc: createServiceSplitter, + }, + } + for _, tc := range cases { + t.Run(fmt.Sprintf("create and delete %s with no service instances", tc.kind), func(t *testing.T) { + s := testVIPStateStore(t) + + // Create unrelated service instance + createServiceInstance(t, s, "unrelated") + + // Create the config entry and make sure a virtual ip is allocated + ensureVirtualIP(t, s, "foo", "") + tc.createFunc(t, s, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.2") + + // Delete the config entry and make sure the virtual ip is freed and reused + ensureVirtualIP(t, s, "bar", "") + deleteConfigEntry(t, s, tc.kind, "foo") + ensureVirtualIP(t, s, "foo", "") + tc.createFunc(t, s, "bar") + ensureVirtualIP(t, s, "bar", "240.0.0.2") + }) + + t.Run(fmt.Sprintf("create and delete %s with service instances", tc.kind), func(t *testing.T) { + s := testVIPStateStore(t) + + // Create a foo service instance and an unrelated service instance + createServiceInstance(t, s, "foo") + + // Creating the config entry should not affect the service virtual IP + ensureVirtualIP(t, s, "foo", "240.0.0.1") + tc.createFunc(t, s, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.1") + + // Deleting should also not affect the service virtual IP because there are still existing + // service instances that need the VIP. + deleteConfigEntry(t, s, tc.kind, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.1") + + // Now delete the service instance, which should free up the virtual IP + deleteServiceInstance(t, s, "foo") + ensureVirtualIP(t, s, "foo", "") + + // Make sure the free address can be reused + tc.createFunc(t, s, "bar") + ensureVirtualIP(t, s, "bar", "240.0.0.1") + }) + + t.Run(fmt.Sprintf("create and delete service instance while %s still exists", tc.kind), func(t *testing.T) { + s := testVIPStateStore(t) + + // Create the config entry to get the virtual IP + tc.createFunc(t, s, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.1") + + // Creating service instance should not affect virtual IP + createServiceInstance(t, s, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.1") + + // Deleting should also not affect the service virtual IP because the config entry still exists. + deleteServiceInstance(t, s, "foo") + ensureVirtualIP(t, s, "foo", "240.0.0.1") + + // Now delete the config entry, which should free up the ip + deleteConfigEntry(t, s, tc.kind, "foo") + ensureVirtualIP(t, s, "foo", "") + + // Make sure the free address can be reused + tc.createFunc(t, s, "bar") + ensureVirtualIP(t, s, "bar", "240.0.0.1") + }) + } +}