diff --git a/CHANGELOG.md b/CHANGELOG.md index 497a99f0a1a..c41c729c058 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ BUG FIXES: * api: Return a 404 if endpoint not found instead of redirecting to /ui/ [[GH-6658](https://github.com/hashicorp/nomad/issues/6658)] * api: Decompress web socket response body if gzipped on error responses [[GH-6650](https://github.com/hashicorp/nomad/issues/6650)] * api: Fixed a bug where some FS/Allocation API endpoints didn't return error messages [[GH-6427](https://github.com/hashicorp/nomad/issues/6427)] + * api: Return 40X status code for failing ACL requests, rather than 500 [[GH-6421](https://github.com/hashicorp/nomad/issues/6421)] * cli: Make scoring column orders consistent `nomad alloc status` [[GH-6609](https://github.com/hashicorp/nomad/issues/6609)] * cli: Fixed a bug where a cli user may fail to query FS/Allocation API endpoints if they lack `node:read` capability [[GH-6423](https://github.com/hashicorp/nomad/issues/6423)] * client: Fixed a bug where a client may not restart dead internal processes upon client's restart on Windows [[GH-6426](https://github.com/hashicorp/nomad/issues/6426)] diff --git a/command/agent/http.go b/command/agent/http.go index cb18c7f31e5..01caac0cd32 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -304,6 +304,9 @@ func (s *HTTPServer) wrap(handler func(resp http.ResponseWriter, req *http.Reque errMsg := err.Error() if http, ok := err.(HTTPCodedError); ok { code = http.Code() + } else if ecode, emsg, ok := structs.CodeFromRPCCodedErr(err); ok { + code = ecode + errMsg = emsg } else { // RPC errors get wrapped, so manually unwrap by only looking at their suffix if strings.HasSuffix(errMsg, structs.ErrPermissionDenied.Error()) { diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go index 7c382e4dcb9..f45a46497f5 100644 --- a/nomad/acl_endpoint.go +++ b/nomad/acl_endpoint.go @@ -18,7 +18,7 @@ import ( var ( // aclDisabled is returned when an ACL endpoint is hit but ACLs are not enabled - aclDisabled = fmt.Errorf("ACL support disabled") + aclDisabled = structs.NewErrRPCCoded(400, "ACL support disabled") ) const ( @@ -55,13 +55,13 @@ func (a *ACL) UpsertPolicies(args *structs.ACLPolicyUpsertRequest, reply *struct // Validate non-zero set of policies if len(args.Policies) == 0 { - return fmt.Errorf("must specify as least one policy") + return structs.NewErrRPCCoded(400, "must specify as least one policy") } // Validate each policy, compute hash for idx, policy := range args.Policies { if err := policy.Validate(); err != nil { - return fmt.Errorf("policy %d invalid: %v", idx, err) + return structs.NewErrRPCCodedf(404, "policy %d invalid: %v", idx, err) } policy.SetHash() } @@ -99,7 +99,7 @@ func (a *ACL) DeletePolicies(args *structs.ACLPolicyDeleteRequest, reply *struct // Validate non-zero set of policies if len(args.Names) == 0 { - return fmt.Errorf("must specify as least one policy") + return structs.NewErrRPCCoded(400, "must specify as least one policy") } // Update via Raft @@ -370,9 +370,9 @@ func (a *ACL) Bootstrap(args *structs.ACLTokenBootstrapRequest, reply *structs.A // Check if there is a reset index specified specifiedIndex := a.fileBootstrapResetIndex() if specifiedIndex == 0 { - return fmt.Errorf("ACL bootstrap already done (reset index: %d)", resetIdx) + return structs.NewErrRPCCodedf(400, "ACL bootstrap already done (reset index: %d)", resetIdx) } else if specifiedIndex != resetIdx { - return fmt.Errorf("Invalid bootstrap reset index (specified %d, reset index: %d)", specifiedIndex, resetIdx) + return structs.NewErrRPCCodedf(400, "Invalid bootstrap reset index (specified %d, reset index: %d)", specifiedIndex, resetIdx) } // Setup the reset index to allow bootstrapping again @@ -404,7 +404,7 @@ func (a *ACL) Bootstrap(args *structs.ACLTokenBootstrapRequest, reply *structs.A } out, err := state.ACLTokenByAccessorID(nil, args.Token.AccessorID) if err != nil { - return fmt.Errorf("token lookup failed: %v", err) + return structs.NewErrRPCCodedf(400, "token lookup failed: %v", err) } reply.Tokens = append(reply.Tokens, out) @@ -448,7 +448,7 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A // Validate non-zero set of tokens if len(args.Tokens) == 0 { - return fmt.Errorf("must specify as least one token") + return structs.NewErrRPCCoded(400, "must specify as least one token") } // Force the request to the authoritative region if we are creating global tokens @@ -466,7 +466,7 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A // the entire request as a single batch. if hasGlobal { if !allGlobal { - return fmt.Errorf("cannot upsert mixed global and non-global tokens") + return structs.NewErrRPCCoded(400, "cannot upsert mixed global and non-global tokens") } // Force the request to the authoritative region if it has global @@ -494,7 +494,7 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A // Validate each token for idx, token := range args.Tokens { if err := token.Validate(); err != nil { - return fmt.Errorf("token %d invalid: %v", idx, err) + return structs.NewErrRPCCodedf(400, "token %d invalid: %v", idx, err) } // Generate an accessor and secret ID if new @@ -507,15 +507,15 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A // Verify the token exists out, err := state.ACLTokenByAccessorID(nil, token.AccessorID) if err != nil { - return fmt.Errorf("token lookup failed: %v", err) + return structs.NewErrRPCCodedf(400, "token lookup failed: %v", err) } if out == nil { - return fmt.Errorf("cannot find token %s", token.AccessorID) + return structs.NewErrRPCCodedf(404, "cannot find token %s", token.AccessorID) } // Cannot toggle the "Global" mode if token.Global != out.Global { - return fmt.Errorf("cannot toggle global mode of %s", token.AccessorID) + return structs.NewErrRPCCodedf(400, "cannot toggle global mode of %s", token.AccessorID) } } @@ -538,7 +538,7 @@ func (a *ACL) UpsertTokens(args *structs.ACLTokenUpsertRequest, reply *structs.A for _, token := range args.Tokens { out, err := state.ACLTokenByAccessorID(nil, token.AccessorID) if err != nil { - return fmt.Errorf("token lookup failed: %v", err) + return structs.NewErrRPCCodedf(400, "token lookup failed: %v", err) } reply.Tokens = append(reply.Tokens, out) } @@ -557,7 +557,7 @@ func (a *ACL) DeleteTokens(args *structs.ACLTokenDeleteRequest, reply *structs.G // Validate non-zero set of tokens if len(args.AccessorIDs) == 0 { - return fmt.Errorf("must specify as least one token") + return structs.NewErrRPCCoded(400, "must specify as least one token") } if done, err := a.srv.forward("ACL.DeleteTokens", args, args, reply); done { @@ -585,7 +585,7 @@ func (a *ACL) DeleteTokens(args *structs.ACLTokenDeleteRequest, reply *structs.G for _, accessor := range args.AccessorIDs { token, err := state.ACLTokenByAccessorID(nil, accessor) if err != nil { - return fmt.Errorf("token lookup failed: %v", err) + return structs.NewErrRPCCodedf(400, "token lookup failed: %v", err) } if token == nil { nonexistentTokens = append(nonexistentTokens, accessor) @@ -599,14 +599,14 @@ func (a *ACL) DeleteTokens(args *structs.ACLTokenDeleteRequest, reply *structs.G } if len(nonexistentTokens) != 0 { - return fmt.Errorf("Cannot delete nonexistent tokens: %v", strings.Join(nonexistentTokens, ", ")) + return structs.NewErrRPCCodedf(400, "Cannot delete nonexistent tokens: %v", strings.Join(nonexistentTokens, ", ")) } // Disallow mixed requests with global and non-global tokens since we forward // the entire request as a single batch. if hasGlobal { if !allGlobal { - return fmt.Errorf("cannot delete mixed global and non-global tokens") + return structs.NewErrRPCCoded(400, "cannot delete mixed global and non-global tokens") } // Force the request to the authoritative region if it has global diff --git a/nomad/acl_endpoint_test.go b/nomad/acl_endpoint_test.go index 4f55a7e1dbe..58601e12a17 100644 --- a/nomad/acl_endpoint_test.go +++ b/nomad/acl_endpoint_test.go @@ -931,7 +931,7 @@ func TestACLEndpoint_DeleteTokens_WithNonexistentToken(t *testing.T) { assert.NotNil(err) expectedError := fmt.Sprintf("Cannot delete nonexistent tokens: %s", nonexistentToken.AccessorID) - assert.Contains(expectedError, err.Error()) + assert.Contains(err.Error(), expectedError) } func TestACLEndpoint_Bootstrap(t *testing.T) { diff --git a/nomad/structs/errors.go b/nomad/structs/errors.go index 9ec28976242..6bb0ce0cbfa 100644 --- a/nomad/structs/errors.go +++ b/nomad/structs/errors.go @@ -3,6 +3,7 @@ package structs import ( "errors" "fmt" + "strconv" "strings" ) @@ -25,6 +26,8 @@ const ( ErrUnknownJobPrefix = "Unknown job" ErrUnknownEvaluationPrefix = "Unknown evaluation" ErrUnknownDeploymentPrefix = "Unknown deployment" + + errRPCCodedErrorPrefix = "RPC Error:: " ) var ( @@ -144,3 +147,38 @@ func IsErrUnknownNomadVersion(err error) bool { func IsErrNodeLacksRpc(err error) bool { return err != nil && strings.Contains(err.Error(), errNodeLacksRpc) } + +// NewErrRPCCoded wraps an RPC error with a code to be converted to HTTP status +// code +func NewErrRPCCoded(code int, msg string) error { + return fmt.Errorf("%s%d,%s", errRPCCodedErrorPrefix, code, msg) +} + +// NewErrRPCCoded wraps an RPC error with a code to be converted to HTTP status +// code +func NewErrRPCCodedf(code int, format string, args ...interface{}) error { + msg := fmt.Sprintf(format, args...) + return fmt.Errorf("%s%d,%s", errRPCCodedErrorPrefix, code, msg) +} + +// CodeFromRPCCodedErr returns the code and message of error if it's an RPC error +// created through NewErrRPCCoded function. Returns `ok` false if error is not +// an rpc error +func CodeFromRPCCodedErr(err error) (code int, msg string, ok bool) { + if err == nil || !strings.HasPrefix(err.Error(), errRPCCodedErrorPrefix) { + return 0, "", false + } + + headerLen := len(errRPCCodedErrorPrefix) + parts := strings.SplitN(err.Error()[headerLen:], ",", 2) + if len(parts) != 2 { + return 0, "", false + } + + code, err = strconv.Atoi(parts[0]) + if err != nil { + return 0, "", false + } + + return code, parts[1], true +} diff --git a/nomad/structs/errors_test.go b/nomad/structs/errors_test.go new file mode 100644 index 00000000000..08e5fb71662 --- /dev/null +++ b/nomad/structs/errors_test.go @@ -0,0 +1,49 @@ +package structs + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRPCCodedErrors(t *testing.T) { + cases := []struct { + err error + code int + message string + }{ + { + NewErrRPCCoded(400, "a test message,here"), + 400, + "a test message,here", + }, + { + NewErrRPCCodedf(500, "a test message,here %s %s", "and,here%s", "second"), + 500, + "a test message,here and,here%s second", + }, + } + + for _, c := range cases { + t.Run(c.err.Error(), func(t *testing.T) { + code, msg, ok := CodeFromRPCCodedErr(c.err) + assert.True(t, ok) + assert.Equal(t, c.code, code) + assert.Equal(t, c.message, msg) + }) + } + + negativeCases := []string{ + "random error", + errRPCCodedErrorPrefix, + errRPCCodedErrorPrefix + "123", + errRPCCodedErrorPrefix + "qwer,asdf", + } + for _, c := range negativeCases { + t.Run(c, func(t *testing.T) { + _, _, ok := CodeFromRPCCodedErr(errors.New(c)) + assert.False(t, ok) + }) + } +}