Ensure server requirements checks are done against ALL known se… (#7491)

Co-authored-by: Paul Banks <banks@banksco.de>

agent/consul/acl_server.go

@@ -1,15 +1,12 @@
 package consul
 
 import (
-	"fmt"
 	"sync/atomic"
 	"time"
 
 	"github.com/hashicorp/consul/acl"
-	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/lib"
-	"github.com/hashicorp/serf/serf"
 )
 
 var serverACLCacheConfig *structs.ACLCachesConfig = &structs.ACLCachesConfig{
@@ -105,96 +102,33 @@ func (s *Server) updateACLAdvertisement() {
 	s.updateSerfTags("acls", string(structs.ACLModeEnabled))
 }
 
-type serversACLMode struct {
-	// leader is the address of the leader
-	leader string
-
-	// mode indicates the overall ACL mode of the servers
-	mode structs.ACLMode
-
-	// leaderMode is the ACL mode of the leader server
-	leaderMode structs.ACLMode
-
-	// indicates that at least one server was processed
-	found bool
-
-	//
-	server *Server
-}
-
-func (s *serversACLMode) init(leader string) {
-	s.leader = leader
-	s.mode = structs.ACLModeEnabled
-	s.leaderMode = structs.ACLModeUnknown
-	s.found = false
-}
-
-func (s *serversACLMode) update(srv *metadata.Server) bool {
-	fmt.Printf("Processing server acl mode for: %s - %s\n", srv.Name, srv.ACLs)
-	if srv.Status != serf.StatusAlive && srv.Status != serf.StatusFailed {
-		// they are left or something so regardless we treat these servers as meeting
-		// the version requirement
-		return true
-	}
-
-	// mark that we processed at least one server
-	s.found = true
-
-	if srvAddr := srv.Addr.String(); srvAddr == s.leader {
-		s.leaderMode = srv.ACLs
-	}
-
-	switch srv.ACLs {
-	case structs.ACLModeDisabled:
-		// anything disabled means we cant enable ACLs
-		s.mode = structs.ACLModeDisabled
-	case structs.ACLModeEnabled:
-		// do nothing
-	case structs.ACLModeLegacy:
-		// This covers legacy mode and older server versions that don't advertise ACL support
-		if s.mode != structs.ACLModeDisabled && s.mode != structs.ACLModeUnknown {
-			s.mode = structs.ACLModeLegacy
-		}
-	default:
-		if s.mode != structs.ACLModeDisabled {
-			s.mode = structs.ACLModeUnknown
-		}
-	}
-
-	return true
-}
-
 func (s *Server) canUpgradeToNewACLs(isLeader bool) bool {
 	if atomic.LoadInt32(&s.useNewACLs) != 0 {
 		// can't upgrade because we are already upgraded
 		return false
 	}
 
-	var state serversACLMode
 	if !s.InACLDatacenter() {
-		state.init("")
+		foundServers, mode, _ := ServersGetACLMode(s, "", s.config.ACLDatacenter)
-		// use the router to check server information for non-local datacenters
+		if mode != structs.ACLModeEnabled || !foundServers {
-		s.router.CheckServers(s.config.ACLDatacenter, state.update)
+			s.logger.Info("Cannot upgrade to new ACLs, servers in acl datacenter are not yet upgraded", "ACLDatacenter", s.config.ACLDatacenter, "mode", mode, "found", foundServers)
-		if state.mode != structs.ACLModeEnabled || !state.found {
-			s.logger.Info("Cannot upgrade to new ACLs, servers in acl datacenter are not yet upgraded", "ACLDatacenter", s.config.ACLDatacenter, "mode", state.mode, "found", state.found)
 			return false
 		}
 	}
 
-	state.init(string(s.raft.Leader()))
+	leaderAddr := string(s.raft.Leader())
-	// this uses the serverLookup instead of the router as its for the local datacenter
+	foundServers, mode, leaderMode := ServersGetACLMode(s, leaderAddr, s.config.Datacenter)
-	s.serverLookup.CheckServers(state.update)
 	if isLeader {
-		if state.mode == structs.ACLModeLegacy {
+		if mode == structs.ACLModeLegacy {
 			return true
 		}
 	} else {
-		if state.leaderMode == structs.ACLModeEnabled {
+		if leaderMode == structs.ACLModeEnabled {
 			return true
 		}
 	}
 
-	s.logger.Info("Cannot upgrade to new ACLs", "leaderMode", state.leaderMode, "mode", state.mode, "found", state.found, "leader", state.leader)
+	s.logger.Info("Cannot upgrade to new ACLs", "leaderMode", leaderMode, "mode", mode, "found", foundServers, "leader", leaderAddr)
 	return false
 }
 

agent/consul/leader.go

@@ -444,7 +444,7 @@ func (s *Server) initializeLegacyACL() error {
 	// of state in the state store that will cause problems with older
 	// servers consuming snapshots, so we have to wait to create it.
 	var minVersion = version.Must(version.NewVersion("0.9.1"))
-	if ServersMeetMinimumVersion(s.LANMembers(), minVersion) {
+	if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, minVersion); ok {
 		canBootstrap, _, err := state.CanBootstrapACLToken()
 		if err != nil {
 			return fmt.Errorf("failed looking for ACL bootstrap info: %v", err)
@@ -978,7 +978,7 @@ func (s *Server) getOrCreateAutopilotConfig() *autopilot.Config {
 		return config
 	}
 
-	if !ServersMeetMinimumVersion(s.LANMembers(), minAutopilotVersion) {
+	if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, minAutopilotVersion); !ok {
 		logger.Warn("can't initialize until all servers are >= " + minAutopilotVersion.String())
 		return nil
 	}
@@ -1004,7 +1004,7 @@ func (s *Server) bootstrapConfigEntries(entries []structs.ConfigEntry) error {
 		return nil
 	}
 
-	if !ServersMeetMinimumVersion(s.LANMembers(), minCentralizedConfigVersion) {
+	if ok, _ := ServersInDCMeetMinimumVersion(s, s.config.Datacenter, minCentralizedConfigVersion); !ok {
 		s.loggers.
 			Named(logging.CentralConfig).
 			Warn("config: can't initialize until all servers >=" + minCentralizedConfigVersion.String())

agent/consul/leader_connect.go

@@ -193,7 +193,7 @@ func (s *Server) initializeCA() error {
 
 	// If this isn't the primary DC, run the secondary DC routine if the primary has already been upgraded to at least 1.6.0
 	if s.config.PrimaryDatacenter != s.config.Datacenter {
-		versionOk, foundPrimary := ServersInDCMeetMinimumVersion(s.WANMembers(), s.config.PrimaryDatacenter, minMultiDCConnectVersion)
+		versionOk, foundPrimary := ServersInDCMeetMinimumVersion(s, s.config.PrimaryDatacenter, minMultiDCConnectVersion)
 		if !foundPrimary {
 			connectLogger.Warn("primary datacenter is configured but unreachable - deferring initialization of the secondary datacenter CA")
 			// return nil because we will initialize the secondary CA later
@@ -738,7 +738,7 @@ func (s *Server) secondaryCARootWatch(ctx context.Context) error {
 			return nil
 		}
 		if !s.configuredSecondaryCA() {
-			versionOk, primaryFound := ServersInDCMeetMinimumVersion(s.WANMembers(), s.config.PrimaryDatacenter, minMultiDCConnectVersion)
+			versionOk, primaryFound := ServersInDCMeetMinimumVersion(s, s.config.PrimaryDatacenter, minMultiDCConnectVersion)
 			if !primaryFound {
 				return fmt.Errorf("Primary datacenter is unreachable - deferring secondary CA initialization")
 			}

agent/consul/util.go

@@ -273,89 +273,163 @@ func runtimeStats() map[string]string {
 	}
 }
 
-// ServersMeetMinimumVersion returns whether the given alive servers are at least on the
+// checkServersProvider exists so that we can unit tests the requirements checking functions
-// given Consul version
+// without having to spin up a whole agent/server.
-func ServersMeetMinimumVersion(members []serf.Member, minVersion *version.Version) bool {
+type checkServersProvider interface {
-	return ServersMeetRequirements(members, func(srv *metadata.Server) bool {
+	CheckServers(datacenter string, fn func(*metadata.Server) bool)
-		return srv.Status != serf.StatusAlive || !srv.Build.LessThan(minVersion)
-	})
 }
 
-// ServersMeetMinimumVersion returns whether the given alive servers from a particular
+// serverRequirementsFn should inspect the given metadata.Server struct
-// datacenter are at least on the given Consul version. This requires at least 1 alive server in the DC
+// and return two booleans. The first indicates whether the given requirements
-func ServersInDCMeetMinimumVersion(members []serf.Member, datacenter string, minVersion *version.Version) (bool, bool) {
+// are met. The second indicates whether this server should be considered filtered.
-	found := false
+//
-	ok := ServersMeetRequirements(members, func(srv *metadata.Server) bool {
+// The reason for the two booleans is so that a requirement function could "filter"
-		if srv.Status != serf.StatusAlive || srv.Datacenter != datacenter {
+// out the left server members if we only want to consider things which are still
-			return true
+// around or likely to come back (failed state).
+type serverRequirementFn func(*metadata.Server) (ok bool, filtered bool)
+
+type serversMeetRequirementsState struct {
+	// meetsRequirements is the callback to actual check for some specific requirement
+	meetsRequirements serverRequirementFn
+
+	// ok indicates whether all unfiltered servers meet the desired requirements
+	ok bool
+
+	// found is a boolean indicating that the meetsRequirement function accepted at
+	// least one unfiltered server.
+	found bool
+}
+
+func (s *serversMeetRequirementsState) update(srv *metadata.Server) bool {
+	ok, filtered := s.meetsRequirements(srv)
+
+	if filtered {
+		// keep going but don't update any of the internal state as this server
+		// was filtered by the requirements function
+		return true
+	}
+
+	// mark that at least one server processed was not filtered
+	s.found = true
+
+	if !ok {
+		// mark that at least one server does not meet the requirements
+		s.ok = false
+
+		// prevent continuing server evaluation
+		return false
+	}
+
+	// this should already be set but this will prevent accidentally reusing
+	// the state object from causing false-negatives.
+	s.ok = true
+
+	// continue evaluating servers
+	return true
+}
+
+// ServersInDCMeetRequirements returns whether the given server members meet the requirements as defined by the
+// callback function and whether at least one server remains unfiltered by the requirements function.
+func ServersInDCMeetRequirements(provider checkServersProvider, datacenter string, meetsRequirements serverRequirementFn) (ok bool, found bool) {
+	state := serversMeetRequirementsState{meetsRequirements: meetsRequirements, found: false, ok: true}
+
+	provider.CheckServers(datacenter, state.update)
+
+	return state.ok, state.found
+}
+
+// ServersInDCMeetMinimumVersion returns whether the given alive servers from a particular
+// datacenter are at least on the given Consul version. This also returns whether any
+// alive or failed servers are known in that datacenter (ignoring left and leaving ones)
+func ServersInDCMeetMinimumVersion(provider checkServersProvider, datacenter string, minVersion *version.Version) (ok bool, found bool) {
+	return ServersInDCMeetRequirements(provider, datacenter, func(srv *metadata.Server) (bool, bool) {
+		if srv.Status != serf.StatusAlive && srv.Status != serf.StatusFailed {
+			// filter out the left servers as those should not be factored into our requirements
+			return true, true
 		}
 
-		found = true
+		return !srv.Build.LessThan(minVersion), false
-		return !srv.Build.LessThan(minVersion)
 	})
-
-	return ok, found
 }
 
-// ServersMeetRequirements returns whether the given server members meet the requirements as defined by the
+// CheckServers implements the checkServersProvider interface for the Server
-// callback function
+func (s *Server) CheckServers(datacenter string, fn func(*metadata.Server) bool) {
-func ServersMeetRequirements(members []serf.Member, meetsRequirements func(*metadata.Server) bool) bool {
+	if datacenter == s.config.Datacenter {
-	for _, member := range members {
+		// use the ServerLookup type for the local DC
-		if valid, parts := metadata.IsConsulServer(member); valid {
+		s.serverLookup.CheckServers(fn)
-			if !meetsRequirements(parts) {
+	} else {
-				return false
+		// use the router for all non-local DCs
-			}
+		s.router.CheckServers(datacenter, fn)
+	}
+}
+
+type serversACLMode struct {
+	// leader is the address of the leader
+	leader string
+
+	// mode indicates the overall ACL mode of the servers
+	mode structs.ACLMode
+
+	// leaderMode is the ACL mode of the leader server
+	leaderMode structs.ACLMode
+
+	// indicates that at least one server was processed
+	found bool
+}
+
+func (s *serversACLMode) init(leader string) {
+	s.leader = leader
+	s.mode = structs.ACLModeEnabled
+	s.leaderMode = structs.ACLModeUnknown
+	s.found = false
+}
+
+func (s *serversACLMode) update(srv *metadata.Server) bool {
+	if srv.Status != serf.StatusAlive && srv.Status != serf.StatusFailed {
+		// they are left or something so regardless we treat these servers as meeting
+		// the version requirement
+		return true
+	}
+
+	// mark that we processed at least one server
+	s.found = true
+
+	if srvAddr := srv.Addr.String(); srvAddr == s.leader {
+		s.leaderMode = srv.ACLs
+	}
+
+	switch srv.ACLs {
+	case structs.ACLModeDisabled:
+		// anything disabled means we cant enable ACLs
+		s.mode = structs.ACLModeDisabled
+	case structs.ACLModeEnabled:
+		// do nothing
+	case structs.ACLModeLegacy:
+		// This covers legacy mode and older server versions that don't advertise ACL support
+		if s.mode != structs.ACLModeDisabled && s.mode != structs.ACLModeUnknown {
+			s.mode = structs.ACLModeLegacy
+		}
+	default:
+		if s.mode != structs.ACLModeDisabled {
+			s.mode = structs.ACLModeUnknown
 		}
 	}
 
 	return true
 }
 
-func ServersGetACLMode(members []serf.Member, leader string, datacenter string) (numServers int, mode structs.ACLMode, leaderMode structs.ACLMode) {
+// ServersGetACLMode checks all the servers in a particular datacenter and determines
-	numServers = 0
+// what the minimum ACL mode amongst them is and what the leaders ACL mode is.
-	mode = structs.ACLModeEnabled
+// The "found" return value indicates whether there were any servers considered in
-	leaderMode = structs.ACLModeUnknown
+// this datacenter. If that is false then the other mode return values are meaningless
-	for _, member := range members {
+// as they will be ACLModeEnabled and ACLModeUnkown respectively.
-		if valid, parts := metadata.IsConsulServer(member); valid {
+func ServersGetACLMode(provider checkServersProvider, leaderAddr string, datacenter string) (found bool, mode structs.ACLMode, leaderMode structs.ACLMode) {
+	var state serversACLMode
+	state.init(leaderAddr)
 
-			if datacenter != "" && parts.Datacenter != datacenter {
+	provider.CheckServers(datacenter, state.update)
-				continue
-			}
 
-			if parts.Status != serf.StatusAlive && parts.Status != serf.StatusFailed {
+	return state.found, state.mode, state.leaderMode
-				// ignore any server that isn't alive or failed. We are considering failed
-				// because in this state there is a reasonable expectation that they could
-				// become stable again. Also autopilot should remove dead servers if they
-				// are truly gone.
-				continue
-			}
-
-			numServers += 1
-
-			if memberAddr := (&net.TCPAddr{IP: member.Addr, Port: parts.Port}).String(); memberAddr == leader {
-				leaderMode = parts.ACLs
-			}
-
-			switch parts.ACLs {
-			case structs.ACLModeDisabled:
-				// anything disabled means we cant enable ACLs
-				mode = structs.ACLModeDisabled
-			case structs.ACLModeEnabled:
-				// do nothing
-			case structs.ACLModeLegacy:
-				// This covers legacy mode and older server versions that don't advertise ACL support
-				if mode != structs.ACLModeDisabled && mode != structs.ACLModeUnknown {
-					mode = structs.ACLModeLegacy
-				}
-			default:
-				if mode != structs.ACLModeDisabled {
-					mode = structs.ACLModeUnknown
-				}
-			}
-		}
-	}
-
-	return
 }
 
 // InterpolateHIL processes the string as if it were HIL and interpolates only

agent/consul/util_test.go

@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"testing"
 
+	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/go-version"
 	"github.com/hashicorp/serf/serf"
@@ -336,108 +337,53 @@ func TestGetPublicIPv6(t *testing.T) {
 	}
 }
 
-func TestServersMeetMinimumVersion(t *testing.T) {
+type testServersProvider []metadata.Server
-	t.Parallel()
+
-	makeMember := func(version string) serf.Member {
+func (p testServersProvider) CheckServers(datacenter string, fn func(*metadata.Server) bool) {
-		return serf.Member{
+	for _, srv := range p {
-			Name: "foo",
+		// filter these out - now I don't have to modify the tests. Originally the dc filtering
-			Addr: net.IP([]byte{127, 0, 0, 1}),
+		// happened in the ServersInDCMeetMinimumVersion, now we get a list of servers to check
-			Tags: map[string]string{
+		// through the routing infrastructure or server lookup which will map a datacenter to a
-				"role":          "consul",
+		// list of metadata.Server structs that are all in that datacenter.
-				"id":            "asdf",
+		if srv.Datacenter != datacenter {
-				"dc":            "east-aws",
+			continue
-				"port":          "10000",
-				"build":         version,
-				"wan_join_port": "1234",
-				"vsn":           "1",
-				"expect":        "3",
-				"raft_vsn":      "3",
-			},
-			Status: serf.StatusAlive,
 		}
-	}
 
-	cases := []struct {
+		if !fn(&srv) {
-		members  []serf.Member
+			return
-		ver      *version.Version
-		expected bool
-	}{
-		// One server, meets reqs
-		{
-			members: []serf.Member{
-				makeMember("0.7.5"),
-			},
-			ver:      version.Must(version.NewVersion("0.7.5")),
-			expected: true,
-		},
-		// One server, doesn't meet reqs
-		{
-			members: []serf.Member{
-				makeMember("0.7.5"),
-			},
-			ver:      version.Must(version.NewVersion("0.8.0")),
-			expected: false,
-		},
-		// Multiple servers, meets req version
-		{
-			members: []serf.Member{
-				makeMember("0.7.5"),
-				makeMember("0.8.0"),
-			},
-			ver:      version.Must(version.NewVersion("0.7.5")),
-			expected: true,
-		},
-		// Multiple servers, doesn't meet req version
-		{
-			members: []serf.Member{
-				makeMember("0.7.5"),
-				makeMember("0.8.0"),
-			},
-			ver:      version.Must(version.NewVersion("0.8.0")),
-			expected: false,
-		},
-	}
-
-	for _, tc := range cases {
-		result := ServersMeetMinimumVersion(tc.members, tc.ver)
-		if result != tc.expected {
-			t.Fatalf("bad: %v, %v, %v", result, tc.ver.String(), tc)
 		}
 	}
 }
 
 func TestServersInDCMeetMinimumVersion(t *testing.T) {
 	t.Parallel()
-	makeMember := func(version string, datacenter string) serf.Member {
+	makeServer := func(versionStr string, datacenter string) metadata.Server {
-		return serf.Member{
+		return metadata.Server{
-			Name: "foo",
+			Name:        "foo",
-			Addr: net.IP([]byte{127, 0, 0, 1}),
+			ShortName:   "foo",
-			Tags: map[string]string{
+			ID:          "asdf",
-				"role":          "consul",
+			Port:        10000,
-				"id":            "asdf",
+			Expect:      3,
-				"dc":            datacenter,
+			RaftVersion: 3,
-				"port":          "10000",
+			Status:      serf.StatusAlive,
-				"build":         version,
+			WanJoinPort: 1234,
-				"wan_join_port": "1234",
+			Version:     1,
-				"vsn":           "1",
+			Build:       *version.Must(version.NewVersion(versionStr)),
-				"expect":        "3",
+			Datacenter:  datacenter,
-				"raft_vsn":      "3",
-			},
-			Status: serf.StatusAlive,
 		}
 	}
 
 	cases := []struct {
-		members       []serf.Member
+		servers       testServersProvider
 		ver           *version.Version
 		expected      bool
 		expectedFound bool
 	}{
 		// One server, meets reqs
 		{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("0.7.5", "primary"),
+				makeServer("0.7.5", "primary"),
-				makeMember("0.7.3", "secondary"),
+				makeServer("0.7.3", "secondary"),
 			},
 			ver:           version.Must(version.NewVersion("0.7.5")),
 			expected:      true,
@@ -445,9 +391,9 @@ func TestServersInDCMeetMinimumVersion(t *testing.T) {
 		},
 		// One server, doesn't meet reqs
 		{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("0.7.5", "primary"),
+				makeServer("0.7.5", "primary"),
-				makeMember("0.8.1", "secondary"),
+				makeServer("0.8.1", "secondary"),
 			},
 			ver:           version.Must(version.NewVersion("0.8.0")),
 			expected:      false,
@@ -455,10 +401,10 @@ func TestServersInDCMeetMinimumVersion(t *testing.T) {
 		},
 		// Multiple servers, meets req version
 		{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("0.7.5", "primary"),
+				makeServer("0.7.5", "primary"),
-				makeMember("0.8.0", "primary"),
+				makeServer("0.8.0", "primary"),
-				makeMember("0.7.0", "secondary"),
+				makeServer("0.7.0", "secondary"),
 			},
 			ver:           version.Must(version.NewVersion("0.7.5")),
 			expected:      true,
@@ -466,20 +412,20 @@ func TestServersInDCMeetMinimumVersion(t *testing.T) {
 		},
 		// Multiple servers, doesn't meet req version
 		{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("0.7.5", "primary"),
+				makeServer("0.7.5", "primary"),
-				makeMember("0.8.0", "primary"),
+				makeServer("0.8.0", "primary"),
-				makeMember("0.9.1", "secondary"),
+				makeServer("0.9.1", "secondary"),
 			},
 			ver:           version.Must(version.NewVersion("0.8.0")),
 			expected:      false,
 			expectedFound: true,
 		},
 		{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("0.7.5", "secondary"),
+				makeServer("0.7.5", "secondary"),
-				makeMember("0.8.0", "secondary"),
+				makeServer("0.8.0", "secondary"),
-				makeMember("0.9.1", "secondary"),
+				makeServer("0.9.1", "secondary"),
 			},
 			ver:           version.Must(version.NewVersion("0.7.0")),
 			expected:      true,
@@ -488,7 +434,7 @@ func TestServersInDCMeetMinimumVersion(t *testing.T) {
 	}
 
 	for _, tc := range cases {
-		result, found := ServersInDCMeetMinimumVersion(tc.members, "primary", tc.ver)
+		result, found := ServersInDCMeetMinimumVersion(tc.servers, "primary", tc.ver)
 		require.Equal(t, tc.expected, result)
 		require.Equal(t, tc.expectedFound, found)
 	}
@@ -626,114 +572,111 @@ func TestInterpolateHIL(t *testing.T) {
 
 func TestServersGetACLMode(t *testing.T) {
 	t.Parallel()
-	makeMember := func(role string, datacenter string, acls structs.ACLMode, status serf.MemberStatus, addr net.IP) serf.Member {
+	makeServer := func(datacenter string, acls structs.ACLMode, status serf.MemberStatus, addr net.IP) metadata.Server {
-		return serf.Member{
+		return metadata.Server{
-			Name: "foo",
+			Name:        "foo",
-			Addr: addr,
+			ShortName:   "foo",
-			Tags: map[string]string{
+			ID:          "asdf",
-				"role":          role,
+			Port:        10000,
-				"id":            "asdf",
+			Expect:      3,
-				"dc":            datacenter,
+			RaftVersion: 3,
-				"port":          "10000",
+			Status:      status,
-				"build":         "1.6.0",
+			WanJoinPort: 1234,
-				"wan_join_port": "1234",
+			Version:     1,
-				"vsn":           "1",
+			Addr:        &net.TCPAddr{IP: addr, Port: 10000},
-				"expect":        "3",
+			// shouldn't matter for these tests
-				"raft_vsn":      "3",
+			Build:      *version.Must(version.NewVersion("1.7.0")),
-				"acls":          string(acls),
+			Datacenter: datacenter,
-			},
+			ACLs:       acls,
-			Status: status,
 		}
 	}
 
 	type tcase struct {
-		members    []serf.Member
+		servers      testServersProvider
-		leaderAddr string
+		leaderAddr   string
-		datacenter string
+		datacenter   string
-		numServers int
+		foundServers bool
-		minMode    structs.ACLMode
+		minMode      structs.ACLMode
-		leaderMode structs.ACLMode
+		leaderMode   structs.ACLMode
 	}
 
 	cases := map[string]tcase{
 		"filter-members": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusFailed, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusFailed, net.IP([]byte{127, 0, 0, 2})),
-				// filter non-server
-				makeMember("client", "primary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 3})),
 				// filtered datacenter
-				makeMember("consul", "secondary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 4})),
+				makeServer("secondary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 4})),
 				// filtered status
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusLeaving, net.IP([]byte{127, 0, 0, 5})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusLeaving, net.IP([]byte{127, 0, 0, 5})),
 				// filtered status
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusLeft, net.IP([]byte{127, 0, 0, 6})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusLeft, net.IP([]byte{127, 0, 0, 6})),
 				// filtered status
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusNone, net.IP([]byte{127, 0, 0, 7})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusNone, net.IP([]byte{127, 0, 0, 7})),
 			},
-			numServers: 2,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "primary",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeLegacy,
+			minMode:      structs.ACLModeLegacy,
-			leaderMode: structs.ACLModeLegacy,
+			leaderMode:   structs.ACLModeLegacy,
 		},
 		"disabled": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
-				makeMember("consul", "primary", structs.ACLModeDisabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 3})),
+				makeServer("primary", structs.ACLModeDisabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 3})),
 			},
-			numServers: 3,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeDisabled,
+			minMode:      structs.ACLModeDisabled,
-			leaderMode: structs.ACLModeLegacy,
+			leaderMode:   structs.ACLModeLegacy,
 		},
 		"unknown": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
 			},
-			numServers: 2,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeUnknown,
+			minMode:      structs.ACLModeUnknown,
-			leaderMode: structs.ACLModeLegacy,
+			leaderMode:   structs.ACLModeLegacy,
 		},
 		"legacy": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
 			},
-			numServers: 2,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeLegacy,
+			minMode:      structs.ACLModeLegacy,
-			leaderMode: structs.ACLModeEnabled,
+			leaderMode:   structs.ACLModeEnabled,
 		},
 		"enabled": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 2})),
-				makeMember("consul", "primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 3})),
+				makeServer("primary", structs.ACLModeEnabled, serf.StatusAlive, net.IP([]byte{127, 0, 0, 3})),
 			},
-			numServers: 3,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeEnabled,
+			minMode:      structs.ACLModeEnabled,
-			leaderMode: structs.ACLModeEnabled,
+			leaderMode:   structs.ACLModeEnabled,
 		},
 		"failed-members": tcase{
-			members: []serf.Member{
+			servers: testServersProvider{
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusAlive, net.IP([]byte{127, 0, 0, 1})),
-				makeMember("consul", "primary", structs.ACLModeUnknown, serf.StatusFailed, net.IP([]byte{127, 0, 0, 2})),
+				makeServer("primary", structs.ACLModeUnknown, serf.StatusFailed, net.IP([]byte{127, 0, 0, 2})),
-				makeMember("consul", "primary", structs.ACLModeLegacy, serf.StatusFailed, net.IP([]byte{127, 0, 0, 3})),
+				makeServer("primary", structs.ACLModeLegacy, serf.StatusFailed, net.IP([]byte{127, 0, 0, 3})),
 			},
-			numServers: 3,
+			foundServers: true,
-			leaderAddr: "127.0.0.1:10000",
+			leaderAddr:   "127.0.0.1:10000",
-			datacenter: "",
+			datacenter:   "primary",
-			minMode:    structs.ACLModeUnknown,
+			minMode:      structs.ACLModeUnknown,
-			leaderMode: structs.ACLModeLegacy,
+			leaderMode:   structs.ACLModeLegacy,
 		},
 	}
 
@@ -741,11 +684,11 @@ func TestServersGetACLMode(t *testing.T) {
 		name := name
 		tc := tc
 		t.Run(name, func(t *testing.T) {
-			actualServers, actualMinMode, actualLeaderMode := ServersGetACLMode(tc.members, tc.leaderAddr, tc.datacenter)
+			actualServers, actualMinMode, actualLeaderMode := ServersGetACLMode(tc.servers, tc.leaderAddr, tc.datacenter)
 
-			require.Equal(t, tc.numServers, actualServers)
 			require.Equal(t, tc.minMode, actualMinMode)
 			require.Equal(t, tc.leaderMode, actualLeaderMode)
+			require.Equal(t, tc.foundServers, actualServers)
 		})
 	}
 }