state: ensure that identical manual virtual IP updates result in not bumping the modify indexes (#21909)

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

@@ -0,0 +1,3 @@
+```release-note:bug
+state: ensure that identical manual virtual IP updates result in not bumping the modify indexes
+```

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)

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,41 @@ 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
+		idx1 := s1.raft.CommitIndex()
 		err := msgpackrpc.CallWithCodec(codec, "Internal.AssignManualServiceVIPs", tc.req, &resp)
+		idx2 := s1.raft.CommitIndex()
 		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)
+				require.Equal(t, idx1, idx2, "no raft operations occurred")
+			} 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 +3927,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 +3947,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 +3956,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 +3965,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
+				})
+			}
 		})
 	}
 }

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)
-	}
-	if err := updateVirtualIPMaxIndexes(tx, idx, psn.ServiceName.PartitionOrDefault(), psn.Peer); err != nil {
-		return false, nil, 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 = 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

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)
+	newPSN := func(name, peer string) structs.PeeredServiceName {
+		return structs.PeeredServiceName{
+			ServiceName: structs.ServiceName{
+				Name:           name,
+				EnterpriseMeta: *acl.DefaultEnterpriseMeta(),
+			},
+			Peer: peer,
+		}
+	}
 
-	// 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,
+	checkMaxIndexes := func(t *testing.T, expect, expectImported uint64) {
+		t.Helper()
+		tx := s.db.Txn(false)
+		defer tx.Abort()
+
+		idx := maxIndexWatchTxn(tx, nil, tableServiceVirtualIPs)
+		require.Equal(t, expect, idx)
+
+		entMeta := acl.DefaultEnterpriseMeta()
+
+		importedIdx := maxIndexTxn(tx, partitionedIndexEntryName(tableServiceVirtualIPs+".imported", entMeta.PartitionOrDefault()))
+		require.Equal(t, expectImported, importedIdx)
 	}
 
-	// Service successfully registers into the state store.
-	testRegisterNode(t, s, 0, "node1")
-	require.NoError(t, s.EnsureService(1, "node1", ns1))
+	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)
+		}
+	}
 
-	// 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)
+	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)
+	}
 
-	// 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)
+	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)
+	})
+
+	// 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.
-	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"})
+	var assignIndex1 uint64
+	testutil.RunStep(t, "assign to existent service does something", func(t *testing.T) {
+		useIdx := nextIndex()
+		assignIndex1 = useIdx
 
-	// Register another service via config entry.
-	s.EnsureConfigEntry(3, &structs.ServiceResolverConfigEntry{
-		Kind: structs.ServiceResolver,
-		Name: "bar",
+		// 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 := structs.PeeredServiceName{ServiceName: structs.ServiceName{Name: "bar"}}
-	vip, err = s.VirtualIPForService(psn2)
-	require.NoError(t, err)
-	assert.Equal(t, "240.0.0.2", vip)
+	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) {

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
+}

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)
+	}
+}