comments: cleanup some leftover debug comments and such

client/allocrunner/taskrunner/envoybootstrap_hook.go

@@ -121,7 +121,6 @@ func (h *envoyBootstrapHook) Prestart(ctx context.Context, req *interfaces.TaskP
 		siToken:        siToken,
 	}.args()
 
-	// put old stuff in here
 	// Since Consul services are registered asynchronously with this task
 	// hook running, retry a small number of times with backoff.
 	for tries := 3; ; tries-- {

client/allocrunner/taskrunner/task_runner_test.go

@@ -1830,7 +1830,7 @@ func TestTaskRunner_RestartSignalTask_NotRunning(t *testing.T) {
 		require.Fail(t, "timed out waiting for task to complete")
 	}
 
-	// Assert the task unblocked and never restarted
+	// Assert the task ran and never restarted
 	state := tr.TaskState()
 	require.Equal(t, structs.TaskStateDead, state.State)
 	require.False(t, state.Failed)

client/allocrunner/taskrunner/tasklet.go

@@ -148,7 +148,7 @@ func (t *tasklet) run() *taskletHandle {
 
 			select {
 			case <-t.shutdownCh:
-				// We've been told to exit and just unblocked so exit
+				// We've been told to exit and just ran so exit
 				return
 			default:
 			}

client/allocrunner/taskrunner/volume_hook.go

@@ -96,7 +96,7 @@ func (h *volumeHook) Prestart(ctx context.Context, req *interfaces.TaskPrestartR
 		return err
 	}
 
-	// Because this hook is also unblocked on restores, we only add mounts that do not
+	// Because this hook is also ran on restores, we only add mounts that do not
 	// already exist. Although this loop is somewhat expensive, there are only
 	// a small number of mounts that exist within most individual tasks. We may
 	// want to revisit this using a `hookdata` param to be "mount only once"

client/consul/identities_test.go

@@ -9,7 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestCSI_DeriveTokens(t *testing.T) {
+func TestSI_DeriveTokens(t *testing.T) {
 	logger := testlog.HCLogger(t)
 	dFunc := func(alloc *structs.Allocation, taskNames []string) (map[string]string, error) {
 		return map[string]string{"a": "b"}, nil
@@ -20,7 +20,7 @@ func TestCSI_DeriveTokens(t *testing.T) {
 	require.Equal(t, map[string]string{"a": "b"}, tokens)
 }
 
-func TestCSI_DeriveTokens_error(t *testing.T) {
+func TestSI_DeriveTokens_error(t *testing.T) {
 	logger := testlog.HCLogger(t)
 	dFunc := func(alloc *structs.Allocation, taskNames []string) (map[string]string, error) {
 		return nil, errors.New("some failure")

command/agent/consul/acl_testing.go

@@ -88,7 +88,7 @@ func (m *MockACLsAPI) RoleRead(roleID string, _ *api.QueryOptions) (*api.ACLRole
 				ID:   ExamplePolicyID1,
 				Name: "example-policy-1",
 			}},
-			ServiceIdentities: nil, // would it ever make sense ?
+			ServiceIdentities: nil,
 		}, nil, nil
 	case ExampleRoleID2:
 		return &api.ACLRole{
@@ -104,9 +104,8 @@ func (m *MockACLsAPI) RoleRead(roleID string, _ *api.QueryOptions) (*api.ACLRole
 		return &api.ACLRole{
 			ID:                ExampleRoleID3,
 			Name:              "example-role-3",
-			Policies:          nil, // todo
+			Policies:          nil, // todo add more if needed
-			ServiceIdentities: nil, // todo
+			ServiceIdentities: nil, // todo add more if needed
-			ModifyIndex:       0,
 		}, nil, nil
 	default:
 		return nil, nil, nil

nomad/consul.go

@@ -378,14 +378,14 @@ func (c *consulACLsAPI) bgRetryRevoke() {
 	copy(toPurge, c.bgRetryRevocation)
 
 	if err := c.parallelRevoke(context.Background(), toPurge); err != nil {
-		c.logger.Warn("background token revocation failed", "error", err)
+		c.logger.Warn("background SI token revocation failed", "error", err)
 		return
 	}
 
 	// Call the revocation function
 	if err := c.purgeFunc(toPurge); err != nil {
 		// Just try again later (revocation is idempotent)
-		c.logger.Error("token revocation failed", "error", err)
+		c.logger.Error("background SI token purge failed", "error", err)
 		return
 	}
 

nomad/consul_policy.go

@@ -8,17 +8,21 @@ import (
 	"github.com/pkg/errors"
 )
 
-// ConsulServiceRule represents a policy for a service
+// ConsulServiceRule represents a policy for a service.
 type ConsulServiceRule struct {
 	Name   string `hcl:",key"`
 	Policy string
 }
 
+// ConsulPolicy represents the parts of a ConsulServiceRule Policy that are
+// relevant to Service Identity authorizations.
 type ConsulPolicy struct {
 	Services        []*ConsulServiceRule `hcl:"service,expand"`
 	ServicePrefixes []*ConsulServiceRule `hcl:"service_prefix,expand"`
 }
 
+// IsEmpty returns true if there are no Services or ServicePrefixes defined for
+// the ConsulPolicy.
 func (cp *ConsulPolicy) IsEmpty() bool {
 	if cp == nil {
 		return true
@@ -26,6 +30,9 @@ func (cp *ConsulPolicy) IsEmpty() bool {
 	return len(cp.Services) == 0 && len(cp.ServicePrefixes) == 0
 }
 
+// ParseConsulPolicy parses raw string s into a ConsulPolicy. An error is
+// returned if decoding the policy fails, or if the decoded policy has no
+// Services or ServicePrefixes defined.
 func ParseConsulPolicy(s string) (*ConsulPolicy, error) {
 	cp := new(ConsulPolicy)
 	if err := hcl.Decode(cp, s); err != nil {
@@ -71,7 +78,6 @@ func (c *consulACLsAPI) hasSufficientPolicy(task string, token *api.ACLToken) (b
 	return false, nil
 }
 
-// policyAllowsServiceWrite
 func (c *consulACLsAPI) policyAllowsServiceWrite(task string, policyID string) (bool, error) {
 	policy, _, err := c.aclClient.PolicyRead(policyID, &api.QueryOptions{
 		AllowStale: false,

nomad/node_endpoint.go

@@ -350,6 +350,9 @@ func (n *Node) deregister(args *structs.NodeBatchDeregisterRequest,
 			return err
 		} else if l := len(accessors); l > 0 {
 			n.logger.Debug("revoking si accessors on node due to deregister", "num_accessors", l, "node_id", nodeID)
+			// Unlike with the Vault integration, there's no error returned here, since
+			// bootstrapping the Consul client is elsewhere. Errors in revocation trigger
+			// background retry attempts rather than inline error handling.
 			_ = n.srv.consulACLs.RevokeTokens(context.Background(), accessors, true)
 		}
 
@@ -465,10 +468,10 @@ func (n *Node) UpdateStatus(args *structs.NodeUpdateStatusRequest, reply *struct
 
 		// Determine if there are any SI token accessors on the node to cleanup
 		if accessors, err := n.srv.State().SITokenAccessorsByNode(ws, args.NodeID); err != nil {
-			n.logger.Error("looking up si accessors for node failed", "node_id", args.NodeID, "error", err)
+			n.logger.Error("looking up SI accessors for node failed", "node_id", args.NodeID, "error", err)
 			return err
 		} else if l := len(accessors); l > 0 {
-			n.logger.Debug("revoking si accessors on node due to down state", "num_accessors", l, "node_id", args.NodeID)
+			n.logger.Debug("revoking SI accessors on node due to down state", "num_accessors", l, "node_id", args.NodeID)
 			_ = n.srv.consulACLs.RevokeTokens(context.Background(), accessors, true)
 		}
 	default:

nomad/structs/config/consul.go

@@ -146,8 +146,6 @@ func DefaultConsulConfig() *ConsulConfig {
 //
 // If allow_unauthenticated is false, the operator must provide a token on
 // job submission (i.e. -consul-token or $CONSUL_TOKEN).
-//
-// todo: seems like we should be using this somewhere...
 func (c *ConsulConfig) AllowsUnauthenticated() bool {
 	return c.AllowUnauthenticated != nil && *c.AllowUnauthenticated
 }