From 71e4061e0e1860a88246243c57fc6f0f5c7e5332 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 17:09:03 -0500 Subject: [PATCH 1/6] Remove error wrapping and make vault connection server side errors clearer. --- client/client.go | 4 ++-- client/task_runner.go | 7 ++++++ nomad/node_endpoint.go | 12 +++++++--- nomad/structs/structs.go | 43 ++++++++++++++++++++++++++++++++++- nomad/structs/structs_test.go | 2 +- 5 files changed, 61 insertions(+), 7 deletions(-) diff --git a/client/client.go b/client/client.go index 705efb524e7e..8f11cbbaac9b 100644 --- a/client/client.go +++ b/client/client.go @@ -1754,11 +1754,11 @@ func (c *Client) deriveToken(alloc *structs.Allocation, taskNames []string, vcli var resp structs.DeriveVaultTokenResponse if err := c.RPC("Node.DeriveVaultToken", &req, &resp); err != nil { c.logger.Printf("[ERR] client.vault: DeriveVaultToken RPC failed: %v", err) - return nil, fmt.Errorf("DeriveVaultToken RPC failed: %v", err) + return nil, structs.NewWrappedServerError(resp.Error) } if resp.Error != nil { c.logger.Printf("[ERR] client.vault: failed to derive vault tokens: %v", resp.Error) - return nil, resp.Error + return nil, structs.NewWrappedServerError(resp.Error) } if resp.Tasks == nil { c.logger.Printf("[ERR] client.vault: failed to derive vault token: invalid response") diff --git a/client/task_runner.go b/client/task_runner.go index 9d45d91f7f6a..f60064fb2028 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -854,6 +854,13 @@ func (r *TaskRunner) deriveVaultToken() (token string, exit bool) { return tokens[r.task.Name], false } + // Check if this is a server side error + if structs.IsServerSide(err) { + r.logger.Printf("[ERR] client: failed to derive Vault token for task %v on alloc %q: %v", + r.task.Name, r.alloc.ID, err) + r.Kill("vault", fmt.Sprintf("server error in deriving vault token: %v", err), true) + return "", true + } // Check if we can't recover from the error if !structs.IsRecoverable(err) { r.logger.Printf("[ERR] client: failed to derive Vault token for task %v on alloc %q: %v", diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 75e0cf9ffe07..722dce496149 100644 --- a/nomad/node_endpoint.go +++ b/nomad/node_endpoint.go @@ -1143,7 +1143,14 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest, if e == nil { return } - reply.Error = structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError) + re, ok := e.(structs.Recoverable) + if ok { + // No need to wrap if error already implements Recoverable + reply.Error = re.(*structs.RecoverableError) + } else { + reply.Error = structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError) + } + n.srv.logger.Printf("[ERR] nomad.client: DeriveVaultToken failed (recoverable %v): %v", recoverable, e) } @@ -1269,8 +1276,7 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest, secret, err := n.srv.vault.CreateToken(ctx, alloc, task) if err != nil { - wrapped := fmt.Sprintf("failed to create token for task %q on alloc %q: %v", task, alloc.ID, err) - return structs.WrapRecoverable(wrapped, err) + return err } results[task] = secret diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 7f3e2e3c3a4c..c498e61604e7 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4189,7 +4189,7 @@ func (event *TaskEvent) PopulateEventDisplayMessage() { } case TaskKilling: if event.KillReason != "" { - desc = fmt.Sprintf("Killing task: %v", event.KillReason) + desc = event.KillReason } else if event.KillTimeout != 0 { desc = fmt.Sprintf("Sent interrupt. Waiting %v before force killing", event.KillTimeout) } else { @@ -6134,6 +6134,47 @@ func IsRecoverable(e error) bool { return false } +// WrappedServerError wraps an error and satisfies +// both the Recoverable and the ServerSideError interfaces +type WrappedServerError struct { + Err error +} + +// NewWrappedServerError is used to create a wrapped server side error +func NewWrappedServerError(e error) error { + return &WrappedServerError{ + Err: e, + } +} + +func (r *WrappedServerError) IsRecoverable() bool { + return IsRecoverable(r.Err) +} + +func (r *WrappedServerError) Error() string { + return r.Err.Error() +} + +func (r *WrappedServerError) IsServerSide() bool { + return true +} + +// Recoverable is an interface for errors to implement to indicate whether or +// not they are fatal or recoverable. +type ServerSideError interface { + error + IsServerSide() bool +} + +// IsServerSide returns true if error is a wrapped +// server side error +func IsServerSide(e error) bool { + if se, ok := e.(ServerSideError); ok { + return se.IsServerSide() + } + return false +} + // ACLPolicy is used to represent an ACL policy type ACLPolicy struct { Name string // Unique name diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index fa1b132d3b6c..34c36c97236f 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -3077,7 +3077,7 @@ func TestTaskEventPopulate(t *testing.T) { {NewTaskEvent(TaskRestarting).SetRestartDelay(2 * time.Second).SetRestartReason(ReasonWithinPolicy), "Task restarting in 2s"}, {NewTaskEvent(TaskRestarting).SetRestartReason("Chaos Monkey did it"), "Chaos Monkey did it - Task restarting in 0s"}, {NewTaskEvent(TaskKilling), "Sent interrupt"}, - {NewTaskEvent(TaskKilling).SetKillReason("Its time for you to die"), "Killing task: Its time for you to die"}, + {NewTaskEvent(TaskKilling).SetKillReason("Its time for you to die"), "Its time for you to die"}, {NewTaskEvent(TaskKilling).SetKillTimeout(1 * time.Second), "Sent interrupt. Waiting 1s before force killing"}, {NewTaskEvent(TaskTerminated).SetExitCode(-1).SetSignal(3), "Exit Code: -1, Signal: 3"}, {NewTaskEvent(TaskTerminated).SetMessage("Goodbye"), "Exit Code: 0, Exit Message: \"Goodbye\""}, From 25b338b2a8d76e5e5c8bdf0e63ddda48363a8658 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 17:52:33 -0500 Subject: [PATCH 2/6] Code review comment --- nomad/node_endpoint.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nomad/node_endpoint.go b/nomad/node_endpoint.go index 722dce496149..5bc30ded9195 100644 --- a/nomad/node_endpoint.go +++ b/nomad/node_endpoint.go @@ -1143,10 +1143,10 @@ func (n *Node) DeriveVaultToken(args *structs.DeriveVaultTokenRequest, if e == nil { return } - re, ok := e.(structs.Recoverable) + re, ok := e.(*structs.RecoverableError) if ok { - // No need to wrap if error already implements Recoverable - reply.Error = re.(*structs.RecoverableError) + // No need to wrap if error is already a RecoverableError + reply.Error = re } else { reply.Error = structs.NewRecoverableError(e, recoverable).(*structs.RecoverableError) } From f4a4685d84aaa0cc46e050bb74502be102722e1e Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 18:10:14 -0500 Subject: [PATCH 3/6] Return the err from server correctly --- client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/client.go b/client/client.go index 8f11cbbaac9b..adf12496f81b 100644 --- a/client/client.go +++ b/client/client.go @@ -1754,7 +1754,7 @@ func (c *Client) deriveToken(alloc *structs.Allocation, taskNames []string, vcli var resp structs.DeriveVaultTokenResponse if err := c.RPC("Node.DeriveVaultToken", &req, &resp); err != nil { c.logger.Printf("[ERR] client.vault: DeriveVaultToken RPC failed: %v", err) - return nil, structs.NewWrappedServerError(resp.Error) + return nil, structs.NewWrappedServerError(err) } if resp.Error != nil { c.logger.Printf("[ERR] client.vault: failed to derive vault tokens: %v", resp.Error) From d83ad728db82d7a1611d514fc090ceadc51b9b5b Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 18:19:16 -0500 Subject: [PATCH 4/6] Address some code review comments --- client/client.go | 2 +- client/task_runner.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client/client.go b/client/client.go index adf12496f81b..501f9c312c47 100644 --- a/client/client.go +++ b/client/client.go @@ -1754,7 +1754,7 @@ func (c *Client) deriveToken(alloc *structs.Allocation, taskNames []string, vcli var resp structs.DeriveVaultTokenResponse if err := c.RPC("Node.DeriveVaultToken", &req, &resp); err != nil { c.logger.Printf("[ERR] client.vault: DeriveVaultToken RPC failed: %v", err) - return nil, structs.NewWrappedServerError(err) + return nil, fmt.Errorf("DeriveVaultToken RPC failed: %v", err) } if resp.Error != nil { c.logger.Printf("[ERR] client.vault: failed to derive vault tokens: %v", resp.Error) diff --git a/client/task_runner.go b/client/task_runner.go index f60064fb2028..aff63f5c2694 100644 --- a/client/task_runner.go +++ b/client/task_runner.go @@ -858,7 +858,7 @@ func (r *TaskRunner) deriveVaultToken() (token string, exit bool) { if structs.IsServerSide(err) { r.logger.Printf("[ERR] client: failed to derive Vault token for task %v on alloc %q: %v", r.task.Name, r.alloc.ID, err) - r.Kill("vault", fmt.Sprintf("server error in deriving vault token: %v", err), true) + r.Kill("vault", fmt.Sprintf("server error deriving vault token: %v", err), true) return "", true } // Check if we can't recover from the error From a6ec63d35fccfec4c0126315d45e7abe3ba9cf79 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 18:25:41 -0500 Subject: [PATCH 5/6] Fix incorrect comment --- nomad/structs/structs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index c498e61604e7..5b555c3b97e8 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6159,8 +6159,8 @@ func (r *WrappedServerError) IsServerSide() bool { return true } -// Recoverable is an interface for errors to implement to indicate whether or -// not they are fatal or recoverable. +// ServerSideError is an interface for errors to implement to indicate +// errors occuring after the request makes it to a server type ServerSideError interface { error IsServerSide() bool From 034db7a3f028c8fe9451e1f165ba40e452cdbddb Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 13 Mar 2018 20:49:01 -0500 Subject: [PATCH 6/6] Fix lint warning --- nomad/structs/structs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 5b555c3b97e8..21b914469112 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6137,7 +6137,7 @@ func IsRecoverable(e error) bool { // WrappedServerError wraps an error and satisfies // both the Recoverable and the ServerSideError interfaces type WrappedServerError struct { - Err error + Err error } // NewWrappedServerError is used to create a wrapped server side error @@ -6160,7 +6160,7 @@ func (r *WrappedServerError) IsServerSide() bool { } // ServerSideError is an interface for errors to implement to indicate -// errors occuring after the request makes it to a server +// errors occurring after the request makes it to a server type ServerSideError interface { error IsServerSide() bool