From d24d4707751b03eec40021bdba37a8aea806fa47 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Tue, 7 Jan 2020 11:58:29 -0600 Subject: [PATCH] comments: cleanup some leftover debug comments and such --- client/allocrunner/taskrunner/envoybootstrap_hook.go | 1 - client/allocrunner/taskrunner/task_runner_test.go | 2 +- client/allocrunner/taskrunner/tasklet.go | 2 +- client/allocrunner/taskrunner/volume_hook.go | 2 +- client/consul/identities_test.go | 4 ++-- command/agent/consul/acl_testing.go | 7 +++---- nomad/consul.go | 4 ++-- nomad/consul_policy.go | 10 ++++++++-- nomad/node_endpoint.go | 7 +++++-- nomad/structs/config/consul.go | 2 -- 10 files changed, 23 insertions(+), 18 deletions(-) diff --git a/client/allocrunner/taskrunner/envoybootstrap_hook.go b/client/allocrunner/taskrunner/envoybootstrap_hook.go index 29e53fb49..93e725be0 100644 --- a/client/allocrunner/taskrunner/envoybootstrap_hook.go +++ b/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-- { diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 44530a4f5..b36172302 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/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) diff --git a/client/allocrunner/taskrunner/tasklet.go b/client/allocrunner/taskrunner/tasklet.go index 63bf56c09..0f6d2e578 100644 --- a/client/allocrunner/taskrunner/tasklet.go +++ b/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: } diff --git a/client/allocrunner/taskrunner/volume_hook.go b/client/allocrunner/taskrunner/volume_hook.go index fd9a5a1d1..1e0935aea 100644 --- a/client/allocrunner/taskrunner/volume_hook.go +++ b/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" diff --git a/client/consul/identities_test.go b/client/consul/identities_test.go index e56000d4a..0ac7ac275 100644 --- a/client/consul/identities_test.go +++ b/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") diff --git a/command/agent/consul/acl_testing.go b/command/agent/consul/acl_testing.go index 00979543f..0d4c2af07 100644 --- a/command/agent/consul/acl_testing.go +++ b/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 - ServiceIdentities: nil, // todo - ModifyIndex: 0, + Policies: nil, // todo add more if needed + ServiceIdentities: nil, // todo add more if needed }, nil, nil default: return nil, nil, nil diff --git a/nomad/consul.go b/nomad/consul.go index 8c7846f24..9d1f86289 100644 --- a/nomad/consul.go +++ b/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 } diff --git a/nomad/consul_policy.go b/nomad/consul_policy.go index 4f8a62379..51cb2bc32 100644 --- a/nomad/consul_policy.go +++ b/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, diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index d9ee28ef6..671e5778c 100644 --- a/nomad/node_endpoint.go +++ b/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: diff --git a/nomad/structs/config/consul.go b/nomad/structs/config/consul.go index 9c324dde8..a0326ae13 100644 --- a/nomad/structs/config/consul.go +++ b/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 }