From 90f8e8e3727399f3137de22c38304b158290c40f Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Thu, 31 Oct 2024 16:45:54 -0500 Subject: [PATCH] also prevent taking a trip through raft if we know the list of ips has not changed --- agent/consul/internal_endpoint.go | 32 +++++++++++++++--- agent/consul/internal_endpoint_test.go | 47 +++++++++++++++++++------- 2 files changed, 63 insertions(+), 16 deletions(-) diff --git a/agent/consul/internal_endpoint.go b/agent/consul/internal_endpoint.go index af27842d20..074f7ffe09 100644 --- a/agent/consul/internal_endpoint.go +++ b/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{}{} + } + + 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 e4b9a14b70..c5a8a76fb0 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" @@ -3716,21 +3716,34 @@ 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 } - 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) + } } - require.Equal(t, tc.expect, resp) } + tcs := []testcase{ { name: "successful manual ip assignment", @@ -3738,7 +3751,8 @@ func TestInternal_AssignManualServiceVIPs(t *testing.T) { Service: "web", ManualVIPs: []string{"1.1.1.1", "2.2.2.2"}, }, - expect: structs.AssignServiceManualVIPsResponse{Found: true}, + 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 + }) + } }) } }