Cleans up version 8 ACLs in the agent and the docs.

agent/agent.go

@@ -309,16 +309,6 @@ func (a *Agent) Start() error {
 
 		a.delegate = server
 		a.state.delegate = server
-
-		// Automatically register the "consul" service on server nodes
-		consulService := structs.NodeService{
-			Service: consul.ConsulServiceName,
-			ID:      consul.ConsulServiceID,
-			Port:    c.Ports.Server,
-			Tags:    []string{},
-		}
-
-		a.state.AddService(&consulService, c.GetTokenForAgent())
 	} else {
 		client, err := consul.NewClientLogger(consulCfg, a.logger)
 		if err != nil {
@@ -1309,17 +1299,17 @@ func (a *Agent) sendCoordinate() {
 			members := a.LANMembers()
 			grok, err := consul.CanServersUnderstandProtocol(members, 3)
 			if err != nil {
-				a.logger.Printf("[ERR] agent: failed to check servers: %s", err)
+				a.logger.Printf("[ERR] agent: Failed to check servers: %s", err)
 				continue
 			}
 			if !grok {
-				a.logger.Printf("[DEBUG] agent: skipping coordinate updates until servers are upgraded")
+				a.logger.Printf("[DEBUG] agent: Skipping coordinate updates until servers are upgraded")
 				continue
 			}
 
 			c, err := a.GetLANCoordinate()
 			if err != nil {
-				a.logger.Printf("[ERR] agent: failed to get coordinate: %s", err)
+				a.logger.Printf("[ERR] agent: Failed to get coordinate: %s", err)
 				continue
 			}
 
@@ -1331,7 +1321,11 @@ func (a *Agent) sendCoordinate() {
 			}
 			var reply struct{}
 			if err := a.RPC("Coordinate.Update", &req, &reply); err != nil {
-				a.logger.Printf("[ERR] agent: coordinate update error: %s", err)
+				if strings.Contains(err.Error(), permissionDenied) {
+					a.logger.Printf("[WARN] agent: Coordinate update blocked by ACLs")
+				} else {
+					a.logger.Printf("[ERR] agent: Coordinate update error: %v", err)
+				}
 				continue
 			}
 		case <-a.shutdownCh:
@@ -1561,13 +1555,6 @@ func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.Che
 // RemoveService is used to remove a service entry.
 // The agent will make a best effort to ensure it is deregistered
 func (a *Agent) RemoveService(serviceID string, persist bool) error {
-	// Protect "consul" service from deletion by a user
-	if _, ok := a.delegate.(*consul.Server); ok && serviceID == consul.ConsulServiceID {
-		return fmt.Errorf(
-			"Deregistering the %s service is not allowed",
-			consul.ConsulServiceID)
-	}
-
 	// Validate ServiceID
 	if serviceID == "" {
 		return fmt.Errorf("ServiceID missing")
@@ -2069,9 +2056,6 @@ func (a *Agent) loadServices(conf *Config) error {
 // known to the local agent.
 func (a *Agent) unloadServices() error {
 	for _, service := range a.state.Services() {
-		if service.ID == consul.ConsulServiceID {
-			continue
-		}
 		if err := a.RemoveService(service.ID, false); err != nil {
 			return fmt.Errorf("Failed deregistering service '%s': %v", service.ID, err)
 		}

agent/agent_endpoint_test.go

@@ -57,7 +57,7 @@ func TestAgent_Services(t *testing.T) {
 		t.Fatalf("Err: %v", err)
 	}
 	val := obj.(map[string]*structs.NodeService)
-	if len(val) != 2 {
+	if len(val) != 1 {
 		t.Fatalf("bad services: %v", obj)
 	}
 	if val["mysql"].Port != 5000 {
@@ -70,6 +70,14 @@ func TestAgent_Services_ACLFilter(t *testing.T) {
 	a := NewTestAgent(t.Name(), TestACLConfig())
 	defer a.Shutdown()
 
+	srv1 := &structs.NodeService{
+		ID:      "mysql",
+		Service: "mysql",
+		Tags:    []string{"master"},
+		Port:    5000,
+	}
+	a.state.AddService(srv1, "")
+
 	t.Run("no token", func(t *testing.T) {
 		req, _ := http.NewRequest("GET", "/v1/agent/services", nil)
 		obj, err := a.srv.AgentServices(nil, req)

agent/agent_test.go

@@ -492,11 +492,6 @@ func TestAgent_RemoveService(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	// Remove the consul service
-	if err := a.RemoveService("consul", false); err == nil {
-		t.Fatalf("should have errored")
-	}
-
 	// Remove without an ID
 	if err := a.RemoveService("", false); err == nil {
 		t.Fatalf("should have errored")
@@ -882,34 +877,6 @@ func TestAgent_updateTTLCheck(t *testing.T) {
 	}
 }
 
-func TestAgent_ConsulService(t *testing.T) {
-	t.Parallel()
-	a := NewTestAgent(t.Name(), nil)
-	defer a.Shutdown()
-
-	// Consul service is registered
-	services := a.state.Services()
-	if _, ok := services[consul.ConsulServiceID]; !ok {
-		t.Fatalf("%s service should be registered", consul.ConsulServiceID)
-	}
-
-	// todo(fs): data race
-	func() {
-		a.state.Lock()
-		defer a.state.Unlock()
-
-		// Perform anti-entropy on consul service
-		if err := a.state.syncService(consul.ConsulServiceID); err != nil {
-			t.Fatalf("err: %s", err)
-		}
-	}()
-
-	// Consul service should be in sync
-	if !a.state.serviceStatus[consul.ConsulServiceID].inSync {
-		t.Fatalf("%s service should be in sync", consul.ConsulServiceID)
-	}
-}
-
 func TestAgent_PersistService(t *testing.T) {
 	t.Parallel()
 	cfg := TestConfig()
@@ -1432,19 +1399,8 @@ func TestAgent_unloadServices(t *testing.T) {
 	if err := a.unloadServices(); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-
-	// Make sure it was unloaded and the consul service remains
-	found = false
-	for id := range a.state.Services() {
-		if id == svc.ID {
-			t.Fatalf("should have unloaded services")
-		}
-		if id == consul.ConsulServiceID {
-			found = true
-		}
-	}
-	if !found {
-		t.Fatalf("consul service should not be removed")
+	if len(a.state.Services()) != 0 {
+		t.Fatalf("should have unloaded services")
 	}
 }
 

agent/consul/acl.go

@@ -341,7 +341,7 @@ func (f *aclFilter) allowService(service string) bool {
 		return true
 	}
 
-	if !f.enforceVersion8 && service == ConsulServiceID {
+	if !f.enforceVersion8 && service == structs.ConsulServiceID {
 		return true
 	}
 

agent/consul/catalog_endpoint.go

@@ -64,7 +64,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error
 		// is going away after version 0.8). We check this same policy
 		// later if version 0.8 is enabled, so we can eventually just
 		// delete this and do all the ACL checks down there.
-		if args.Service.Service != ConsulServiceName {
+		if args.Service.Service != structs.ConsulServiceName {
 			if acl != nil && !acl.ServiceWrite(args.Service.Service) {
 				return errPermissionDenied
 			}

agent/consul/health_endpoint_test.go

@@ -54,7 +54,7 @@ func TestHealth_ChecksInState(t *testing.T) {
 	if checks[0].Name != "memory utilization" {
 		t.Fatalf("Bad: %v", checks[0])
 	}
-	if checks[1].CheckID != SerfCheckID {
+	if checks[1].CheckID != structs.SerfCheckID {
 		t.Fatalf("Bad: %v", checks[1])
 	}
 }

agent/consul/leader.go

@@ -18,14 +18,8 @@ import (
 )
 
 const (
-	SerfCheckID           types.CheckID = "serfHealth"
-	SerfCheckName                       = "Serf Health Status"
-	SerfCheckAliveOutput                = "Agent alive and reachable"
-	SerfCheckFailedOutput               = "Agent not live or unreachable"
-	ConsulServiceID                     = "consul"
-	ConsulServiceName                   = "consul"
-	newLeaderEvent                      = "consul:new-leader"
-	barrierWriteTimeout                 = 2 * time.Minute
+	newLeaderEvent      = "consul:new-leader"
+	barrierWriteTimeout = 2 * time.Minute
 )
 
 // monitorLeadership is used to monitor if we acquire or lose our role
@@ -334,7 +328,7 @@ func (s *Server) reconcileReaped(known map[string]struct{}) error {
 	}
 	for _, check := range checks {
 		// Ignore any non serf checks
-		if check.CheckID != SerfCheckID {
+		if check.CheckID != structs.SerfCheckID {
 			continue
 		}
 
@@ -359,7 +353,7 @@ func (s *Server) reconcileReaped(known map[string]struct{}) error {
 		}
 		serverPort := 0
 		for _, service := range services.Services {
-			if service.ID == ConsulServiceID {
+			if service.ID == structs.ConsulServiceID {
 				serverPort = service.Port
 				break
 			}
@@ -430,8 +424,8 @@ func (s *Server) handleAliveMember(member serf.Member) error {
 	var service *structs.NodeService
 	if valid, parts := agent.IsConsulServer(member); valid {
 		service = &structs.NodeService{
-			ID:      ConsulServiceID,
-			Service: ConsulServiceName,
+			ID:      structs.ConsulServiceID,
+			Service: structs.ConsulServiceName,
 			Port:    parts.Port,
 		}
 
@@ -473,7 +467,7 @@ func (s *Server) handleAliveMember(member serf.Member) error {
 			return err
 		}
 		for _, check := range checks {
-			if check.CheckID == SerfCheckID && check.Status == api.HealthPassing {
+			if check.CheckID == structs.SerfCheckID && check.Status == api.HealthPassing {
 				return nil
 			}
 		}
@@ -490,10 +484,10 @@ AFTER_CHECK:
 		Service:    service,
 		Check: &structs.HealthCheck{
 			Node:    member.Name,
-			CheckID: SerfCheckID,
-			Name:    SerfCheckName,
+			CheckID: structs.SerfCheckID,
+			Name:    structs.SerfCheckName,
 			Status:  api.HealthPassing,
-			Output:  SerfCheckAliveOutput,
+			Output:  structs.SerfCheckAliveOutput,
 		},
 
 		// If there's existing information about the node, do not
@@ -520,7 +514,7 @@ func (s *Server) handleFailedMember(member serf.Member) error {
 			return err
 		}
 		for _, check := range checks {
-			if check.CheckID == SerfCheckID && check.Status == api.HealthCritical {
+			if check.CheckID == structs.SerfCheckID && check.Status == api.HealthCritical {
 				return nil
 			}
 		}
@@ -535,10 +529,10 @@ func (s *Server) handleFailedMember(member serf.Member) error {
 		Address:    member.Addr.String(),
 		Check: &structs.HealthCheck{
 			Node:    member.Name,
-			CheckID: SerfCheckID,
-			Name:    SerfCheckName,
+			CheckID: structs.SerfCheckID,
+			Name:    structs.SerfCheckName,
 			Status:  api.HealthCritical,
-			Output:  SerfCheckFailedOutput,
+			Output:  structs.SerfCheckFailedOutput,
 		},
 
 		// If there's existing information about the node, do not

agent/consul/leader_test.go

@@ -53,10 +53,10 @@ func TestLeader_RegisterMember(t *testing.T) {
 	if len(checks) != 1 {
 		t.Fatalf("client missing check")
 	}
-	if checks[0].CheckID != SerfCheckID {
+	if checks[0].CheckID != structs.SerfCheckID {
 		t.Fatalf("bad check: %v", checks[0])
 	}
-	if checks[0].Name != SerfCheckName {
+	if checks[0].Name != structs.SerfCheckName {
 		t.Fatalf("bad check: %v", checks[0])
 	}
 	if checks[0].Status != api.HealthPassing {
@@ -125,10 +125,10 @@ func TestLeader_FailedMember(t *testing.T) {
 	if len(checks) != 1 {
 		t.Fatalf("client missing check")
 	}
-	if checks[0].CheckID != SerfCheckID {
+	if checks[0].CheckID != structs.SerfCheckID {
 		t.Fatalf("bad check: %v", checks[0])
 	}
-	if checks[0].Name != SerfCheckName {
+	if checks[0].Name != structs.SerfCheckName {
 		t.Fatalf("bad check: %v", checks[0])
 	}
 
@@ -270,8 +270,8 @@ func TestLeader_Reconcile_ReapMember(t *testing.T) {
 		Address:    "127.1.1.1",
 		Check: &structs.HealthCheck{
 			Node:    "no-longer-around",
-			CheckID: SerfCheckID,
-			Name:    SerfCheckName,
+			CheckID: structs.SerfCheckID,
+			Name:    structs.SerfCheckName,
 			Status:  api.HealthCritical,
 		},
 		WriteRequest: structs.WriteRequest{
@@ -378,8 +378,8 @@ func TestLeader_Reconcile_Races(t *testing.T) {
 		NodeMeta:   map[string]string{"hello": "world"},
 		Check: &structs.HealthCheck{
 			Node:    c1.config.NodeName,
-			CheckID: SerfCheckID,
-			Name:    SerfCheckName,
+			CheckID: structs.SerfCheckID,
+			Name:    structs.SerfCheckName,
 			Status:  api.HealthCritical,
 			Output:  "",
 		},

agent/consul/structs/catalog.go

@@ -0,0 +1,21 @@
+package structs
+
+import (
+	"github.com/hashicorp/consul/types"
+)
+
+// These are used to manage the built-in "serfHealth" check that's attached
+// to every node in the catalog.
+const (
+	SerfCheckID           types.CheckID = "serfHealth"
+	SerfCheckName                       = "Serf Health Status"
+	SerfCheckAliveOutput                = "Agent alive and reachable"
+	SerfCheckFailedOutput               = "Agent not live or unreachable"
+)
+
+// These are used to manage the "consul" service that's attached to every Consul
+// server node in the catalog.
+const (
+	ConsulServiceID   = "consul"
+	ConsulServiceName = "consul"
+)

agent/local.go

@@ -9,7 +9,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/hashicorp/consul/agent/consul"
 	"github.com/hashicorp/consul/agent/consul/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/lib"
@@ -483,6 +482,11 @@ func (l *localState) setSyncState() error {
 		// If we don't have the service locally, deregister it
 		existing, ok := l.services[id]
 		if !ok {
+			// The consul service is created automatically, and does
+			// not need to be deregistered.
+			if id == structs.ConsulServiceID {
+				continue
+			}
 			l.serviceStatus[id] = syncStatus{inSync: false}
 			continue
 		}
@@ -517,8 +521,8 @@ func (l *localState) setSyncState() error {
 		existing, ok := l.checks[id]
 		if !ok {
 			// The Serf check is created automatically, and does not
-			// need to be registered
-			if id == consul.SerfCheckID {
+			// need to be deregistered.
+			if id == structs.SerfCheckID {
 				continue
 			}
 			l.checkStatus[id] = syncStatus{inSync: false}