Make server side error messages from vault more clearer #3968

preetapan · 2018-03-12T21:58:38Z

new code

Recent Events:
Time                       Type             Description
2018-03-13T17:05:21-05:00  Alloc Unhealthy  Unhealthy because of failed task
2018-03-13T17:05:21-05:00  Killing          vault: server error in deriving vault token: Nomad Server failed to establish connections to Vault: failed to lookup Vault periodic token: Error making API request.

URL: POST http://127.0.0.1:8200/v1/auth/token/looku
Code: 503. Errors:

* Vault is sealed
2018-03-13T17:05:21-05:00  Task Setup       Building Task Directory

master

Recent Events:
Time                       Type             Description
2018-03-12T16:50:04-05:00  Alloc Unhealthy  Unhealthy because of failed task
2018-03-12T16:50:04-05:00  Killing          Killing task: vault: failed to derive token: failed to create token for task "redis" on alloc "259d25ad-f17d-03bf-691a-95ebb9050479": Nomad Server failed to establish connections to Vault: failed to lookup Vault periodic token: Error making API request.

URL: POST http://127.0.0.1:8200/v1/auth/token/lookup
Code: 503. Errors:

* Vault is sealed
2018-03-12T16:50:04-05:00  Task Setup       Building Task Directory
..

preetapan · 2018-03-12T22:02:06Z

@dadgar @schmichael open to better ways to make this even nicer. Even now there are multiple wrapping errors in the description in recent events, but the first thing in the message is that its a server error so that should make it clear that it needs operator intervention on nomad servers to fix.

schmichael

Need to fix the IsRecoverable issue but otherwise looks good!

schmichael · 2018-03-13T00:31:55Z

nomad/structs/structs.go

+
+	return &RecoverableError{
+		Err:           e.Error(),
+		ServerSideErr: false,


Shouldn't this be true?

schmichael · 2018-03-13T00:39:06Z

nomad/structs/structs.go

 }

 // IsRecoverable returns true if error is a RecoverableError with
 // Recoverable=true. Otherwise false is returned.
 func IsRecoverable(e error) bool {
-	if re, ok := e.(Recoverable); ok {
+	if re, ok := e.(ErrorMeta); ok {


getter.GetError also implements IsRecoverable so we shouldn't change this assertion. Maybe keep the Recoverable interface as is and add:

type ErrorMeta interface { Recoverable IsServerSideError }

...or we could implement IsServerSideError on getter.GetError ... the meaning there seems a bit ambiguous though. Would a 400 fetching an artifact be a server-side error? I think no because Server refers to a Nomad Server, but we'd still want to document the interface's exact behavior thoroughly.

schmichael · 2018-03-13T00:44:38Z

nomad/vault.go

@@ -429,7 +429,7 @@ OUTER:
 				retryTimer.Reset(v.config.ConnectionRetryIntv)
 				v.l.Lock()
 				v.connEstablished = true
-				v.connEstablishedErr = fmt.Errorf("Nomad Server failed to establish connections to Vault: %v", err)
+				v.connEstablishedErr = structs.NewServerSideError(fmt.Errorf("Nomad Server failed to establish connections to Vault: %v", err))


It'd be really nice if Vault didn't reformat API errors into one giant string in the api package :(

nomad/vendor/github.com/hashicorp/vault/api/response.go

Lines 27 to 67 in 89845b3

func (r *Response) Error() error {

// 200 to 399 are okay status codes. 429 is the code for health status of

// standby nodes.

if (r.StatusCode >= 200 && r.StatusCode < 400) || r.StatusCode == 429 {

return nil

}

// We have an error. Let's copy the body into our own buffer first,

// so that if we can't decode JSON, we can at least copy it raw.

var bodyBuf bytes.Buffer

if _, err := io.Copy(&bodyBuf, r.Body); err != nil {

return err

}

// Decode the error response if we can. Note that we wrap the bodyBuf

// in a bytes.Reader here so that the JSON decoder doesn't move the

// read pointer for the original buffer.

var resp ErrorResponse

if err := jsonutil.DecodeJSON(bodyBuf.Bytes(), &resp); err != nil {

// Ignore the decoding error and just drop the raw response

return fmt.Errorf(

"Error making API request.\n\n"+

"URL: %s %s\n"+

"Code: %d. Raw Message:\n\n%s",

r.Request.Method, r.Request.URL.String(),

r.StatusCode, bodyBuf.String())

}

var errBody bytes.Buffer

errBody.WriteString(fmt.Sprintf(

"Error making API request.\n\n"+

"URL: %s %s\n"+

"Code: %d. Errors:\n\n",

r.Request.Method, r.Request.URL.String(),

r.StatusCode))

for _, err := range resp.Errors {

errBody.WriteString(fmt.Sprintf("* %s", err))

}

return fmt.Errorf(errBody.String())

}

If that just returned fmt.Errorf(bodyBuf.String()) our error messages would be much nicer! No embedded newlines and useless HTTP metadata (url+status).

Ideally Vault would return a custom error type that allowed us to pluck out the bits of metadata we cared about.

No need to block this PR on that, but do you think we should open an issue with them?

…earer.

schmichael

Approving with one small change

schmichael · 2018-03-13T22:16:13Z

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)


Assert to *structs.RecoverableError here and skip the second assertion below. One less assertion and there are multiple types that implement structs.Recoverable (even if right now only one of them is used here).

dadgar · 2018-03-13T23:01:27Z

nomad/structs/structs.go

@@ -6118,22 +6133,34 @@ func (r *RecoverableError) IsRecoverable() bool {
 	return r.Recoverable


Fix comment

dadgar · 2018-03-13T23:06:18Z

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)


This should return the err not the resp error

Also should it be marked as a server error? It is likely there is no valid server which is where the error is coming from since all errors other than network are returned via the resp.Error

You are right, this should not be marked as such, will fix

dadgar · 2018-03-13T23:07:37Z

client/task_runner.go

+		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)


remove in

github-actions · 2023-03-11T02:13:35Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

preetapan requested review from schmichael and dadgar March 12, 2018 21:58

schmichael requested changes Mar 13, 2018

View reviewed changes

Remove error wrapping and make vault connection server side errors cl…

71e4061

…earer.

preetapan force-pushed the f-nicer-vault-error branch from 37172c7 to 71e4061 Compare March 13, 2018 22:11

schmichael approved these changes Mar 13, 2018

View reviewed changes

Code review comment

25b338b

dadgar approved these changes Mar 13, 2018

View reviewed changes

preetapan added 4 commits March 13, 2018 18:10

Return the err from server correctly

f4a4685

Address some code review comments

d83ad72

Fix incorrect comment

a6ec63d

Fix lint warning

034db7a

preetapan merged commit c4389cd into master Mar 14, 2018

preetapan deleted the f-nicer-vault-error branch March 14, 2018 01:49

github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make server side error messages from vault more clearer #3968

Make server side error messages from vault more clearer #3968

preetapan commented Mar 12, 2018 •

edited

Loading

preetapan commented Mar 12, 2018

schmichael left a comment

schmichael Mar 13, 2018

schmichael Mar 13, 2018

schmichael Mar 13, 2018

schmichael left a comment

schmichael Mar 13, 2018

dadgar Mar 13, 2018

dadgar Mar 13, 2018

dadgar Mar 13, 2018

preetapan Mar 13, 2018

dadgar Mar 13, 2018

github-actions bot commented Mar 11, 2023

	func (r *Response) Error() error {
	// 200 to 399 are okay status codes. 429 is the code for health status of
	// standby nodes.
	if (r.StatusCode >= 200 && r.StatusCode < 400) \|\| r.StatusCode == 429 {
	return nil
	}

	// We have an error. Let's copy the body into our own buffer first,
	// so that if we can't decode JSON, we can at least copy it raw.
	var bodyBuf bytes.Buffer
	if _, err := io.Copy(&bodyBuf, r.Body); err != nil {
	return err
	}

	// Decode the error response if we can. Note that we wrap the bodyBuf
	// in a bytes.Reader here so that the JSON decoder doesn't move the
	// read pointer for the original buffer.
	var resp ErrorResponse
	if err := jsonutil.DecodeJSON(bodyBuf.Bytes(), &resp); err != nil {
	// Ignore the decoding error and just drop the raw response
	return fmt.Errorf(
	"Error making API request.\n\n"+
	"URL: %s %s\n"+
	"Code: %d. Raw Message:\n\n%s",
	r.Request.Method, r.Request.URL.String(),
	r.StatusCode, bodyBuf.String())
	}

	var errBody bytes.Buffer
	errBody.WriteString(fmt.Sprintf(
	"Error making API request.\n\n"+
	"URL: %s %s\n"+
	"Code: %d. Errors:\n\n",
	r.Request.Method, r.Request.URL.String(),
	r.StatusCode))
	for _, err := range resp.Errors {
	errBody.WriteString(fmt.Sprintf("* %s", err))
	}

	return fmt.Errorf(errBody.String())
	}

		@@ -6118,22 +6133,34 @@ func (r *RecoverableError) IsRecoverable() bool {
		return r.Recoverable

Make server side error messages from vault more clearer #3968

Make server side error messages from vault more clearer #3968

Conversation

preetapan commented Mar 12, 2018 • edited Loading

preetapan commented Mar 12, 2018

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2023

preetapan commented Mar 12, 2018 •

edited

Loading