also prevent taking a trip through raft if we know the list of ips has not changed

agent/consul/internal_endpoint.go

@@ -7,15 +7,17 @@ import (
 	"fmt"
 	"net"
 
+	hashstructure_v2 "github.com/mitchellh/hashstructure/v2"
+
 	"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
@@ -770,17 +772,39 @@ 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())
 		}
+		if _, ok := vipMap[ip]; ok {
+			return fmt.Errorf("duplicate manual ip found: %q", ip)
+		}
+		vipMap[ip] = struct{}{}
 	}
 
-	req := state.ServiceVirtualIP{
-		Service: structs.PeeredServiceName{
+	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:   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"
@@ -3719,18 +3719,31 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) {
 		name        string
 		req         structs.AssignServiceManualVIPsRequest
 		expect      structs.AssignServiceManualVIPsResponse
+		expectAgain structs.AssignServiceManualVIPsResponse
 		expectErr   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)
 			}
+		}
+	}
+
 	tcs := []testcase{
 		{
 			name: "successful manual ip assignment",
@@ -3739,6 +3752,7 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) {
 				ManualVIPs: []string{"1.1.1.1", "2.2.2.2"},
 			},
 			expect:      structs.AssignServiceManualVIPsResponse{Found: true},
+			expectAgain: structs.AssignServiceManualVIPsResponse{Found: true},
 		},
 		{
 			name: "reassign existing ip",
@@ -3754,6 +3768,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",
@@ -3767,7 +3783,14 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) {
 	}
 	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
+				})
+			}
 		})
 	}
 }