From d335aa371e68555f3343e910614043ecc97bcef5 Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Mon, 25 Nov 2024 11:18:22 -0500 Subject: [PATCH] Backport of state: ensure that identical manual virtual IP updates result in not bumping the modify indexes into release/1.20.x (#21969) The consul-k8s endpoints controller issues catalog register and manual virtual ip updates without first checking to see if the updates would be effectively not changing anything. This is supposed to be reasonable because the state store functions do the check for a no-op update and should discard repeat updates so that downstream blocking queries watching one of the resources don't fire pointlessly (and CPU wastefully). While this is true for the check/service/node catalog updates, it is not true for the "manual virtual ip" updates triggered by the PUT /v1/internal/service-virtual-ip. Forcing the connect injector pod to recycle while watching some lightly modified FSM code can show that a lot of updates are of the update list of ips from [A] to [A]. Immediately following this stray update you can see a lot of activity in proxycfg and xds packages waking up due to blocking queries triggered by this. This PR skips updates that change nothing both: - at the RPC layer before passing it to raft (ideally) - if the write does make it through raft and get applied to the FSM (failsafe) --- .changelog/21909.txt | 3 + agent/consul/internal_endpoint.go | 32 ++- agent/consul/internal_endpoint_test.go | 64 ++++- agent/consul/state/catalog.go | 54 +++- agent/consul/state/catalog_test.go | 340 ++++++++++++++++++++----- lib/stringslice/stringslice.go | 14 + lib/stringslice/stringslice_test.go | 25 ++ 7 files changed, 439 insertions(+), 93 deletions(-) create mode 100644 .changelog/21909.txt diff --git a/.changelog/21909.txt b/.changelog/21909.txt new file mode 100644 index 0000000000..b49562f137 --- /dev/null +++ b/.changelog/21909.txt @@ -0,0 +1,3 @@ +```release-note:bug +state: ensure that identical manual virtual IP updates result in not bumping the modify indexes +``` diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index cab2195845..6afb8405f3 100644 --- a/agent/consul/internal_endpoint.go +++ b/agent/consul/internal_endpoint.go @@ -7,15 +7,18 @@ import ( "fmt" "net" + hashstructure_v2 "github.com/mitchellh/hashstructure/v2" + "golang.org/x/exp/maps" + "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" "github.com/hashicorp/serf/serf" - hashstructure_v2 "github.com/mitchellh/hashstructure/v2" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/lib/stringslice" ) const MaximumManualVIPsPerService = 8 @@ -782,17 +785,38 @@ func (m *Internal) AssignManualServiceVIPs(args *structs.AssignServiceManualVIPs return fmt.Errorf("cannot associate more than %d manual virtual IPs with the same service", MaximumManualVIPsPerService) } + vipMap := make(map[string]struct{}) for _, ip := range args.ManualVIPs { parsedIP := net.ParseIP(ip) if parsedIP == nil || parsedIP.To4() == nil { return fmt.Errorf("%q is not a valid IPv4 address", parsedIP.String()) } + vipMap[ip] = struct{}{} + } + // Silently ignore duplicates. + args.ManualVIPs = maps.Keys(vipMap) + + psn := structs.PeeredServiceName{ + ServiceName: structs.NewServiceName(args.Service, &args.EnterpriseMeta), + } + + // Check to see if we can skip the raft apply entirely. + { + existingIPs, err := m.srv.fsm.State().ServiceManualVIPs(psn) + if err != nil { + return fmt.Errorf("error checking for existing manual ips for service: %w", err) + } + if existingIPs != nil && stringslice.EqualMapKeys(existingIPs.ManualIPs, vipMap) { + *reply = structs.AssignServiceManualVIPsResponse{ + Found: true, + UnassignedFrom: nil, + } + return nil + } } req := state.ServiceVirtualIP{ - Service: structs.PeeredServiceName{ - ServiceName: structs.NewServiceName(args.Service, &args.EnterpriseMeta), - }, + Service: psn, ManualIPs: args.ManualVIPs, } resp, err := m.srv.raftApplyMsgpack(structs.UpdateVirtualIPRequestType, req) diff --git a/agent/consul/internal_endpoint_test.go b/agent/consul/internal_endpoint_test.go index 91f46cb760..3c73f31c17 100644 --- a/agent/consul/internal_endpoint_test.go +++ b/agent/consul/internal_endpoint_test.go @@ -12,11 +12,11 @@ import ( "testing" "time" - "github.com/hashicorp/consul-net-rpc/net/rpc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" msgpackrpc "github.com/hashicorp/consul-net-rpc/net-rpc-msgpackrpc" + "github.com/hashicorp/consul-net-rpc/net/rpc" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" @@ -3885,21 +3885,38 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { require.NoError(t, msgpackrpc.CallWithCodec(codec, "Internal.AssignManualServiceVIPs", req, &resp)) type testcase struct { - name string - req structs.AssignServiceManualVIPsRequest - expect structs.AssignServiceManualVIPsResponse - expectErr string + name string + req structs.AssignServiceManualVIPsRequest + expect structs.AssignServiceManualVIPsResponse + expectAgain structs.AssignServiceManualVIPsResponse + expectErr string + expectIPs []string } - run := func(t *testing.T, tc testcase) { + + run := func(t *testing.T, tc testcase, again bool) { + if tc.expectErr != "" && again { + return // we don't retest known errors + } + var resp structs.AssignServiceManualVIPsResponse err := msgpackrpc.CallWithCodec(codec, "Internal.AssignManualServiceVIPs", tc.req, &resp) if tc.expectErr != "" { - require.Error(t, err) - require.Contains(t, err.Error(), tc.expectErr) - return + testutil.RequireErrorContains(t, err, tc.expectErr) + } else { + if again { + require.Equal(t, tc.expectAgain, resp) + } else { + require.Equal(t, tc.expect, resp) + } + + psn := structs.PeeredServiceName{ServiceName: structs.NewServiceName(tc.req.Service, nil)} + got, err := s1.fsm.State().ServiceManualVIPs(psn) + require.NoError(t, err) + require.NotNil(t, got) + require.Equal(t, tc.expectIPs, got.ManualIPs) } - require.Equal(t, tc.expect, resp) } + tcs := []testcase{ { name: "successful manual ip assignment", @@ -3907,7 +3924,19 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { Service: "web", ManualVIPs: []string{"1.1.1.1", "2.2.2.2"}, }, - expect: structs.AssignServiceManualVIPsResponse{Found: true}, + expectIPs: []string{"1.1.1.1", "2.2.2.2"}, + expect: structs.AssignServiceManualVIPsResponse{Found: true}, + expectAgain: structs.AssignServiceManualVIPsResponse{Found: true}, + }, + { + name: "successfully ignoring duplicates", + req: structs.AssignServiceManualVIPsRequest{ + Service: "web", + ManualVIPs: []string{"1.2.3.4", "5.6.7.8", "1.2.3.4", "5.6.7.8"}, + }, + expectIPs: []string{"1.2.3.4", "5.6.7.8"}, + expect: structs.AssignServiceManualVIPsResponse{Found: true}, + expectAgain: structs.AssignServiceManualVIPsResponse{Found: true}, }, { name: "reassign existing ip", @@ -3915,6 +3944,7 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { Service: "web", ManualVIPs: []string{"8.8.8.8"}, }, + expectIPs: []string{"8.8.8.8"}, expect: structs.AssignServiceManualVIPsResponse{ Found: true, UnassignedFrom: []structs.PeeredServiceName{ @@ -3923,6 +3953,8 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { }, }, }, + // When we repeat this operation the second time it's a no-op. + expectAgain: structs.AssignServiceManualVIPsResponse{Found: true}, }, { name: "invalid ip", @@ -3930,13 +3962,19 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { Service: "web", ManualVIPs: []string{"3.3.3.3", "invalid"}, }, - expect: structs.AssignServiceManualVIPsResponse{}, expectErr: "not a valid", }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - run(t, tc) + t.Run("initial", func(t *testing.T) { + run(t, tc, false) + }) + if tc.expectErr == "" { + t.Run("repeat", func(t *testing.T) { + run(t, tc, true) // only repeat a write if it isn't an known error + }) + } }) } } diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index dcfe4ec91f..b8588f17cc 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -8,6 +8,8 @@ import ( "fmt" "net" "reflect" + "slices" + "sort" "strings" "github.com/hashicorp/go-memdb" @@ -18,6 +20,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/lib/maps" + "github.com/hashicorp/consul/lib/stringslice" "github.com/hashicorp/consul/types" ) @@ -1106,6 +1109,9 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam for _, ip := range ips { assignedIPs[ip] = struct{}{} } + + txnNeedsCommit := false + modifiedEntries := make(map[structs.PeeredServiceName]struct{}) for ip := range assignedIPs { entry, err := tx.First(tableServiceVirtualIPs, indexManualVIPs, psn.ServiceName.PartitionOrDefault(), ip) @@ -1118,7 +1124,13 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam } newEntry := entry.(ServiceVirtualIP) - if newEntry.Service.ServiceName.Matches(psn.ServiceName) { + + var ( + thisServiceName = newEntry.Service.ServiceName + thisPeer = newEntry.Service.Peer + ) + + if thisServiceName.Matches(psn.ServiceName) && thisPeer == psn.Peer { continue } @@ -1130,6 +1142,7 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam filteredIPs = append(filteredIPs, existingIP) } } + sort.Strings(filteredIPs) newEntry.ManualIPs = filteredIPs newEntry.ModifyIndex = idx @@ -1137,6 +1150,12 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err) } modifiedEntries[newEntry.Service] = struct{}{} + + if err := updateVirtualIPMaxIndexes(tx, idx, thisServiceName.PartitionOrDefault(), thisPeer); err != nil { + return false, nil, err + } + + txnNeedsCommit = true } entry, err := tx.First(tableServiceVirtualIPs, indexID, psn) @@ -1149,23 +1168,37 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam } newEntry := entry.(ServiceVirtualIP) - newEntry.ManualIPs = ips - newEntry.ModifyIndex = idx - if err := tx.Insert(tableServiceVirtualIPs, newEntry); err != nil { - return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err) + // Check to see if the slice already contains the same ips. + if !stringslice.EqualMapKeys(newEntry.ManualIPs, assignedIPs) { + newEntry.ManualIPs = slices.Clone(ips) + newEntry.ModifyIndex = idx + + sort.Strings(newEntry.ManualIPs) + + if err := tx.Insert(tableServiceVirtualIPs, newEntry); err != nil { + return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err) + } + if err := updateVirtualIPMaxIndexes(tx, idx, psn.ServiceName.PartitionOrDefault(), psn.Peer); err != nil { + return false, nil, err + } + txnNeedsCommit = true } - if err := updateVirtualIPMaxIndexes(tx, idx, psn.ServiceName.PartitionOrDefault(), psn.Peer); err != nil { - return false, nil, err - } - if err = tx.Commit(); err != nil { - return false, nil, err + + if txnNeedsCommit { + if err = tx.Commit(); err != nil { + return false, nil, err + } } return true, maps.SliceOfKeys(modifiedEntries), nil } func updateVirtualIPMaxIndexes(txn WriteTxn, idx uint64, partition, peerName string) error { + // update global max index (for snapshots) + if err := indexUpdateMaxTxn(txn, idx, tableServiceVirtualIPs); err != nil { + return fmt.Errorf("failed while updating index: %w", err) + } // update per-partition max index if err := indexUpdateMaxTxn(txn, idx, partitionedIndexEntryName(tableServiceVirtualIPs, partition)); err != nil { return fmt.Errorf("failed while updating partitioned index: %w", err) @@ -3086,6 +3119,7 @@ func servicesVirtualIPsTxn(tx ReadTxn, ws memdb.WatchSet) (uint64, []ServiceVirt vips = append(vips, vip) } + // Pull from the global one idx := maxIndexWatchTxn(tx, nil, tableServiceVirtualIPs) return idx, vips, nil diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index cef608bc1c..8445acf987 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -13,15 +13,15 @@ import ( "testing" "time" - "github.com/hashicorp/consul/acl" - "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/hashicorp/go-memdb" - "github.com/hashicorp/go-uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/hashicorp/go-memdb" + "github.com/hashicorp/go-uuid" + + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib/stringslice" @@ -1963,81 +1963,289 @@ func TestStateStore_AssignManualVirtualIPs(t *testing.T) { s := testStateStore(t) setVirtualIPFlags(t, s) - // Attempt to assign manual virtual IPs to a service that doesn't exist - should be a no-op. - psn := structs.PeeredServiceName{ServiceName: structs.ServiceName{Name: "foo", EnterpriseMeta: *acl.DefaultEnterpriseMeta()}} - found, svcs, err := s.AssignManualServiceVIPs(0, psn, []string{"7.7.7.7", "8.8.8.8"}) - require.NoError(t, err) - require.False(t, found) - require.Empty(t, svcs) - serviceVIP, err := s.ServiceManualVIPs(psn) - require.NoError(t, err) - require.Nil(t, serviceVIP) - - // Create the service registration. - entMeta := structs.DefaultEnterpriseMetaInDefaultPartition() - ns1 := &structs.NodeService{ - ID: "foo", - Service: "foo", - Address: "1.1.1.1", - Port: 1111, - Connect: structs.ServiceConnect{Native: true}, - EnterpriseMeta: *entMeta, + newPSN := func(name, peer string) structs.PeeredServiceName { + return structs.PeeredServiceName{ + ServiceName: structs.ServiceName{ + Name: name, + EnterpriseMeta: *acl.DefaultEnterpriseMeta(), + }, + Peer: peer, + } } - // Service successfully registers into the state store. - testRegisterNode(t, s, 0, "node1") - require.NoError(t, s.EnsureService(1, "node1", ns1)) + checkMaxIndexes := func(t *testing.T, expect, expectImported uint64) { + t.Helper() + tx := s.db.Txn(false) + defer tx.Abort() - // Make sure there's a virtual IP for the foo service. - vip, err := s.VirtualIPForService(psn) - require.NoError(t, err) - assert.Equal(t, "240.0.0.1", vip) + idx := maxIndexWatchTxn(tx, nil, tableServiceVirtualIPs) + require.Equal(t, expect, idx) - // No manual IP should be set yet. - serviceVIP, err = s.ServiceManualVIPs(psn) - require.NoError(t, err) - require.Equal(t, "0.0.0.1", serviceVIP.IP.String()) - require.Empty(t, serviceVIP.ManualIPs) + entMeta := acl.DefaultEnterpriseMeta() - // Attempt to assign manual virtual IPs again. - found, svcs, err = s.AssignManualServiceVIPs(2, psn, []string{"7.7.7.7", "8.8.8.8"}) - require.NoError(t, err) - require.True(t, found) - require.Empty(t, svcs) - serviceVIP, err = s.ServiceManualVIPs(psn) - require.NoError(t, err) - require.Equal(t, "0.0.0.1", serviceVIP.IP.String()) - require.Equal(t, serviceVIP.ManualIPs, []string{"7.7.7.7", "8.8.8.8"}) + importedIdx := maxIndexTxn(tx, partitionedIndexEntryName(tableServiceVirtualIPs+".imported", entMeta.PartitionOrDefault())) + require.Equal(t, expectImported, importedIdx) + } - // Register another service via config entry. - s.EnsureConfigEntry(3, &structs.ServiceResolverConfigEntry{ - Kind: structs.ServiceResolver, - Name: "bar", + assignManual := func( + t *testing.T, + idx uint64, + psn structs.PeeredServiceName, + ips []string, + modified ...structs.PeeredServiceName, + ) { + t.Helper() + found, svcs, err := s.AssignManualServiceVIPs(idx, psn, ips) + require.NoError(t, err) + require.True(t, found) + if len(modified) == 0 { + require.Empty(t, svcs) + } else { + require.ElementsMatch(t, modified, svcs) + } + } + + checkVIP := func( + t *testing.T, + psn structs.PeeredServiceName, + expectVIP string, + ) { + t.Helper() + // Make sure there's a virtual IP for the foo service. + vip, err := s.VirtualIPForService(psn) + require.NoError(t, err) + assert.Equal(t, expectVIP, vip) + } + + checkManualVIP := func( + t *testing.T, + psn structs.PeeredServiceName, + expectIP string, + expectManual []string, + expectIndex uint64, + ) { + t.Helper() + serviceVIP, err := s.ServiceManualVIPs(psn) + require.NoError(t, err) + require.Equal(t, expectIP, serviceVIP.IP.String()) + if len(expectManual) == 0 { + require.Empty(t, serviceVIP.ManualIPs) + } else { + require.Equal(t, expectManual, serviceVIP.ManualIPs) + } + require.Equal(t, expectIndex, serviceVIP.ModifyIndex) + } + + psn := newPSN("foo", "") + + lastIndex := uint64(0) + nextIndex := func() uint64 { + lastIndex++ + return lastIndex + } + + testutil.RunStep(t, "assign to nonexistent service is noop", func(t *testing.T) { + useIdx := nextIndex() + + // Attempt to assign manual virtual IPs to a service that doesn't exist - should be a no-op. + found, svcs, err := s.AssignManualServiceVIPs(useIdx, psn, []string{"7.7.7.7", "8.8.8.8"}) + require.NoError(t, err) + require.False(t, found) + require.Empty(t, svcs) + + serviceVIP, err := s.ServiceManualVIPs(psn) + require.NoError(t, err) + require.Nil(t, serviceVIP) + + checkMaxIndexes(t, 0, 0) }) - psn2 := structs.PeeredServiceName{ServiceName: structs.ServiceName{Name: "bar"}} - vip, err = s.VirtualIPForService(psn2) - require.NoError(t, err) - assert.Equal(t, "240.0.0.2", vip) + // Create the service registration. + var regIndex1 uint64 + testutil.RunStep(t, "create service 1", func(t *testing.T) { + useIdx := nextIndex() + regIndex1 = useIdx + + entMeta := acl.DefaultEnterpriseMeta() + ns1 := &structs.NodeService{ + ID: "foo", + Service: "foo", + Address: "1.1.1.1", + Port: 1111, + Connect: structs.ServiceConnect{Native: true}, + EnterpriseMeta: *entMeta, + } + + // Service successfully registers into the state store. + testRegisterNode(t, s, useIdx, "node1") + require.NoError(t, s.EnsureService(useIdx, "node1", ns1)) + + // Make sure there's a virtual IP for the foo service. + checkVIP(t, psn, "240.0.0.1") + + // No manual IP should be set yet. + checkManualVIP(t, psn, "0.0.0.1", []string{}, regIndex1) + + checkMaxIndexes(t, regIndex1, 0) + }) + + // Attempt to assign manual virtual IPs again. + var assignIndex1 uint64 + testutil.RunStep(t, "assign to existent service does something", func(t *testing.T) { + useIdx := nextIndex() + assignIndex1 = useIdx + + // inserting in the wrong order to test the string sort + assignManual(t, useIdx, psn, []string{"7.7.7.7", "8.8.8.8", "6.6.6.6"}) + + checkManualVIP(t, psn, "0.0.0.1", []string{ + "6.6.6.6", "7.7.7.7", "8.8.8.8", + }, assignIndex1) + + checkMaxIndexes(t, assignIndex1, 0) + }) + + psn2 := newPSN("bar", "") + + var regIndex2 uint64 + testutil.RunStep(t, "create service 2", func(t *testing.T) { + useIdx := nextIndex() + regIndex2 = useIdx + + // Register another service via config entry. + s.EnsureConfigEntry(useIdx, &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "bar", + }) + + checkVIP(t, psn2, "240.0.0.2") + + // No manual IP should be set yet. + checkManualVIP(t, psn2, "0.0.0.2", []string{}, regIndex2) + + checkMaxIndexes(t, regIndex2, 0) + }) // Attempt to assign manual virtual IPs for bar, with one IP overlapping with foo. // This should cause the ip to be removed from foo's list of manual IPs. - found, svcs, err = s.AssignManualServiceVIPs(4, psn2, []string{"7.7.7.7", "9.9.9.9"}) - require.NoError(t, err) - require.True(t, found) - require.ElementsMatch(t, svcs, []structs.PeeredServiceName{psn}) + var assignIndex2 uint64 + testutil.RunStep(t, "assign to existent service and ip is removed from another", func(t *testing.T) { + useIdx := nextIndex() + assignIndex2 = useIdx - serviceVIP, err = s.ServiceManualVIPs(psn) - require.NoError(t, err) - require.Equal(t, "0.0.0.1", serviceVIP.IP.String()) - require.Equal(t, []string{"8.8.8.8"}, serviceVIP.ManualIPs) - require.Equal(t, uint64(4), serviceVIP.ModifyIndex) + assignManual(t, useIdx, psn2, []string{"7.7.7.7", "9.9.9.9"}, psn) - serviceVIP, err = s.ServiceManualVIPs(psn2) - require.NoError(t, err) - require.Equal(t, "0.0.0.2", serviceVIP.IP.String()) - require.Equal(t, []string{"7.7.7.7", "9.9.9.9"}, serviceVIP.ManualIPs) - require.Equal(t, uint64(4), serviceVIP.ModifyIndex) + checkManualVIP(t, psn, "0.0.0.1", []string{ + "6.6.6.6", "8.8.8.8", // 7.7.7.7 was stolen by psn2 + }, assignIndex2) + checkManualVIP(t, psn2, "0.0.0.2", []string{ + "7.7.7.7", "9.9.9.9", + }, assignIndex2) + + checkMaxIndexes(t, assignIndex2, 0) + }) + + psn3 := newPSN("gir", "peer1") + + var regIndex3 uint64 + testutil.RunStep(t, "create peered service 1", func(t *testing.T) { + useIdx := nextIndex() + regIndex3 = useIdx + + // Create the service registration. + entMetaPeer := acl.DefaultEnterpriseMeta() + nsPeer1 := &structs.NodeService{ + ID: "gir", + Service: "gir", + Address: "9.9.9.9", + Port: 2222, + PeerName: "peer1", + Connect: structs.ServiceConnect{Native: true}, + EnterpriseMeta: *entMetaPeer, + } + + // Service successfully registers into the state store. + testRegisterPeering(t, s, useIdx, "peer1") + testRegisterNodeOpts(t, s, useIdx, "node9", func(n *structs.Node) error { + n.PeerName = "peer1" + return nil + }) + require.NoError(t, s.EnsureService(useIdx, "node9", nsPeer1)) + + checkVIP(t, psn3, "240.0.0.3") + + // No manual IP should be set yet. + checkManualVIP(t, psn3, "0.0.0.3", []string{}, regIndex3) + + checkMaxIndexes(t, regIndex3, regIndex3) + }) + + // Assign manual virtual IPs to peered service. + var assignIndex3 uint64 + testutil.RunStep(t, "assign to peered service and steal from non-peered", func(t *testing.T) { + useIdx := nextIndex() + assignIndex3 = useIdx + + // 5.5.5.5 is stolen from psn + assignManual(t, useIdx, psn3, []string{"5.5.5.5", "6.6.6.6"}, psn) + + checkManualVIP(t, psn, "0.0.0.1", []string{ + "8.8.8.8", // 5.5.5.5 was stolen by psn3 + }, assignIndex3) + checkManualVIP(t, psn2, "0.0.0.2", []string{ + "7.7.7.7", "9.9.9.9", + }, assignIndex2) + checkManualVIP(t, psn3, "0.0.0.3", []string{ + "5.5.5.5", "6.6.6.6", + }, assignIndex3) + + checkMaxIndexes(t, assignIndex3, assignIndex3) + }) + + var assignIndex4 uint64 + testutil.RunStep(t, "assign to non-peered service and steal from peered", func(t *testing.T) { + useIdx := nextIndex() + assignIndex4 = useIdx + + // 6.6.6.6 is stolen from psn3 + assignManual(t, useIdx, psn2, []string{ + "7.7.7.7", "9.9.9.9", "6.6.6.6", + }, psn3) + + checkManualVIP(t, psn, "0.0.0.1", []string{ + "8.8.8.8", // 5.5.5.5 was stolen by psn3 + }, assignIndex3) + checkManualVIP(t, psn2, "0.0.0.2", []string{ + "6.6.6.6", "7.7.7.7", "9.9.9.9", + }, assignIndex4) + checkManualVIP(t, psn3, "0.0.0.3", []string{ + "5.5.5.5", + }, assignIndex4) + + checkMaxIndexes(t, assignIndex4, assignIndex4) + }) + + testutil.RunStep(t, "repeat the last write and no indexes should be bumped", func(t *testing.T) { + useIdx := nextIndex() + + assignManual(t, useIdx, psn2, []string{ + "7.7.7.7", "9.9.9.9", "6.6.6.6", + }) // no modified this time + + // no changes + checkManualVIP(t, psn, "0.0.0.1", []string{ + "8.8.8.8", + }, assignIndex3) + checkManualVIP(t, psn2, "0.0.0.2", []string{ + "6.6.6.6", "7.7.7.7", "9.9.9.9", + }, assignIndex4) + checkManualVIP(t, psn3, "0.0.0.3", []string{ + "5.5.5.5", + }, assignIndex4) + + // no change + checkMaxIndexes(t, assignIndex4, assignIndex4) + }) } func TestStateStore_EnsureService_ReassignFreedVIPs(t *testing.T) { diff --git a/lib/stringslice/stringslice.go b/lib/stringslice/stringslice.go index 7c32864b94..71e90dba2a 100644 --- a/lib/stringslice/stringslice.go +++ b/lib/stringslice/stringslice.go @@ -80,3 +80,17 @@ func CloneStringSlice(s []string) []string { copy(out, s) return out } + +// EqualMapKeys returns true if the slice equals the keys of +// the map ignoring any ordering. +func EqualMapKeys[V any](a []string, b map[string]V) bool { + if len(a) != len(b) { + return false + } + for _, ip := range a { + if _, ok := b[ip]; !ok { + return false + } + } + return true +} diff --git a/lib/stringslice/stringslice_test.go b/lib/stringslice/stringslice_test.go index dd25071757..b861d9c38a 100644 --- a/lib/stringslice/stringslice_test.go +++ b/lib/stringslice/stringslice_test.go @@ -63,3 +63,28 @@ func TestMergeSorted(t *testing.T) { }) } } + +func TestEqualMapKeys(t *testing.T) { + for _, tc := range []struct { + a []string + b map[string]int + same bool + }{ + // same + {nil, nil, true}, + {[]string{}, nil, true}, + {nil, map[string]int{}, true}, + {[]string{}, map[string]int{}, true}, + {[]string{"a"}, map[string]int{"a": 1}, true}, + {[]string{"b", "a"}, map[string]int{"a": 1, "b": 1}, true}, + // different + {[]string{"a"}, map[string]int{}, false}, + {[]string{}, map[string]int{"a": 1}, false}, + {[]string{"b", "a"}, map[string]int{"c": 1, "a": 1, "b": 1}, false}, + {[]string{"b", "a"}, map[string]int{"c": 1, "a": 1, "b": 1}, false}, + {[]string{"b", "a", "c"}, map[string]int{"a": 1, "b": 1}, false}, + } { + got := EqualMapKeys(tc.a, tc.b) + require.Equal(t, tc.same, got) + } +}