Return HTTP status codes other than 200 and 500 from node and debug endpoints #8937

rkapka · 2021-05-25T11:22:53Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Implements returning HTTP status codes other than 200 and 500 from https://ethereum.github.io/eth2.0-APIs/#/Debug and https://ethereum.github.io/eth2.0-APIs/#/Node. This PR includes the necessary middleware code to support this feature.

Which issues(s) does this PR fix?

Part of API Middleware feature

Other notes for review

N/A

rkapka · 2021-05-25T11:26:26Z

beacon-chain/rpc/statefetcher/fetcher.go

+var ErrInvalidStateId = errors.New("invalid state ID")
+
+// StateNotFoundError represents an error scenario where a state could not be found.
+type StateNotFoundError struct {


I created a new type to be able to compare errors. This error has a formatted string, so using var errFoo = errors.New("foo") would not work.

rkapka · 2021-05-25T11:27:15Z

beacon-chain/rpc/statefetcher/fetcher.go

@@ -73,7 +93,7 @@ func (p *StateProvider) State(ctx context.Context, stateId []byte) (iface.Beacon
 			slotNumber, parseErr := strconv.ParseUint(stateIdString, 10, 64)
 			if parseErr != nil {
 				// ID format does not match any valid options.
-				return nil, errors.New("invalid state ID: " + stateIdString)


I removed the appended ID because it is known to the requestor anyway, and it makes it easier to compare errors without it.

prestonvanloon · 2021-05-25T11:56:04Z

beacon-chain/rpc/debugv1/debug.go

+		stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError)
+		if ok {


Suggested change

stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError)

if ok {

if stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError); ok {

prestonvanloon · 2021-05-25T11:58:07Z

beacon-chain/rpc/nodev1/node.go

+		_ = grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))
 		return &emptypb.Empty{}, nil


Please handle error. I think static analysis will yell at you otherwise

It could be combined to a single line, although may not be as readable.

Suggested change

_ = grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))

return &emptypb.Empty{}, nil

return &emptypb.Empty{}, grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))

I decided to return a positive result here. Added a comment explaining why.

We return a positive result because failing to set a non-gRPC related header should not cause the gRPC call to fail.

prestonvanloon · 2021-05-25T11:59:37Z

beacon-chain/rpc/statefetcher/fetcher.go

+// NewStateNotFoundError creates a new error instance.
+func NewStateNotFoundError(stateRootsSize int) StateNotFoundError {
+	return StateNotFoundError{
+		message: fmt.Sprintf("state not found in the last %d state roots in head state", stateRootsSize),


Is this error specifically for a "head state"? Or could it be for any state?

Suggested change

message: fmt.Sprintf("state not found in the last %d state roots in head state", stateRootsSize),

message: fmt.Sprintf("state not found in the last %d state roots", stateRootsSize),

It can be any state, thanks for catching this.

# Conflicts: # beacon-chain/rpc/nodev1/BUILD.bazel # beacon-chain/rpc/nodev1/node.go

codecov · 2021-05-26T09:06:14Z

Codecov Report

Merging #8937 (3d37ac9) into feature/api-middleware (8696ee9) will decrease coverage by 0.07%.
The diff coverage is 38.23%.

@@                    Coverage Diff                     @@
##           feature/api-middleware    #8937      +/-   ##
==========================================================
- Coverage                   60.40%   60.33%   -0.08%     
==========================================================
  Files                         531      531              
  Lines                       37799    37826      +27     
==========================================================
- Hits                        22832    22821      -11     
- Misses                      11690    11735      +45     
+ Partials                     3277     3270       -7

…ndpoints (#8937) * error codes for node endpoints * error codes for debug endpoints * better comment about headers * gzl * review comments * comment on return value * update fakeChecker used for fuzz tests * fix failing tests

* HTTP proxy server for Eth2 APIs (#8904) * Implement API HTTP proxy server * cleanup + more comments * gateway will no longer be dependent on beaconv1 * handle error during ErrorJson type assertion * simplify handling of endpoint data * fix mux v1 route * use URL encoding for all requests * comment fieldProcessor * fix failing test * change proxy port to not interfere with e2e * gzl * simplify conditional expression * Move appending custom error header to grpcutils package * add api-middleware-port flag * fix documentation for processField * modify e2e port * change field processing error message * better error message for field processing * simplify base64ToHexProcessor * fix json structs * Run several new endpoints through API middleware (#8922) * Implement API HTTP proxy server * cleanup + more comments * gateway will no longer be dependent on beaconv1 * handle error during ErrorJson type assertion * simplify handling of endpoint data * fix mux v1 route * use URL encoding for all requests * comment fieldProcessor * fix failing test * change proxy port to not interfere with e2e * gzl * simplify conditional expression * Move appending custom error header to grpcutils package * add api-middleware-port flag * fix documentation for processField * modify e2e port * change field processing error message * better error message for field processing * simplify base64ToHexProcessor * fix json structs * /eth/v1/beacon/states/{state_id}/validators * /eth/v1/beacon/states/{state_id}/validators/{validator_id} * /eth/v1/beacon/states/{state_id}/validator_balances * /eth/v1/beacon/states/{state_id}/committees * allow skipping base64-encoding for query params * /eth/v1/beacon/pool/attestations * replace break with continue * Remove unused functions (#8924) Co-authored-by: terence tsao <terence@prysmaticlabs.com> * Process SSZ-serialized beacon state through API middleware (#8925) * update field names * Process SSZ-serialized beacon state through API middleware * revert changes to go.mod and go.sum * Revert "Merge branch '__develop' into feature/api-middleware" This reverts commit 7c739a8, reversing changes made to 2d0f8e0. * update ethereumapis * update validator field name * update deps.bzl * update json tags (#8942) * Run `/node/syncing` through API Middleware (#8944) * add IsSyncing field to grpc response * run /node/syncing through the middleware Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Return HTTP status codes other than 200 and 500 from node and debug endpoints (#8937) * error codes for node endpoints * error codes for debug endpoints * better comment about headers * gzl * review comments * comment on return value * update fakeChecker used for fuzz tests * fix failing tests * Allow to pass URL params literally, without encoding to base64 (#8938) * Allow to pass URL params literally, without encoding to base64 * fix compile error Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Process SSZ-serialized beacon state through API middleware (#8925) * update field names * Process SSZ-serialized beacon state through API middleware * revert changes to go.mod and go.sum * Revert "Merge branch '__develop' into feature/api-middleware" This reverts commit 7c739a8, reversing changes made to 2d0f8e0. * update ethereumapis * update validator field name * update deps.bzl * update json tags (#8942) * Run `/node/syncing` through API Middleware (#8944) * add IsSyncing field to grpc response * run /node/syncing through the middleware Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Return HTTP status codes other than 200 and 500 from node and debug endpoints (#8937) * error codes for node endpoints * error codes for debug endpoints * better comment about headers * gzl * review comments * comment on return value * update fakeChecker used for fuzz tests * fix failing tests * Allow to pass URL params literally, without encoding to base64 (#8938) * Allow to pass URL params literally, without encoding to base64 * fix compile error Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * unused import * Return correct status codes from beacon endpoints (#8960) * Various API Middleware fixes (#8963) * Return correct status codes from `/states` endpoints * better error messages in debug and node * better error messages in state * returning correct error codes from validator endpoints * correct error codes for getting a block header * gzl * fix err variable name * fix nil block comparison * test fixes * make status enum test better * fix ineffectual assignment * make PR unstuck * return proper status codes * return uppercase keys from /config/spec * return lowercase validator status * convert requested enum values to uppercase * validator fixes * Implement `/beacon/headers` endpoint (#8966) * Refactor API Middleware into more manageable code (#8984) * move endpoint registration out of shared package * divide main function into smaller components * return early on error * implement hooks * implement custom handlers and add documentation * fix test compile error * restrict package visibility * remove redundant error checking * rename file * API Middleware unit tests (#8998) * move endpoint registration out of shared package * divide main function into smaller components * return early on error * implement hooks * implement custom handlers and add documentation * fix test compile error * restrict package visibility * remove redundant error checking * rename file * api_middleware_processing * endpoints * gzl * remove gazelle:ignore * merge * Implement SSZ version of `/blocks/{block_id}` (#8970) * Implement SSZ version of `/blocks/{block_id}` * add dependencies back * fix indentation in deps.bzl * parameterize ssz functions * get block ssz * update ethereumapis dependency * gzl * Do not reuse `Endpoint` structs between API calls (#9007) * code refactor * implement endpoint factory * fix test * fmt * include pbs * gaz * test naming fixes * remove unused code * radek comments * revert endpoint test * bring back bytes test case * move `signedBeaconBlock` to `migration` package * change `fmt.Errorf` to `errors.Wrap` * capitalize SSZ * capitalize URL * more review feedback * rename `handleGetBlockSSZ` to `handleGetBeaconBlockSSZ` * rename `IndexOutOfRangeError` to `ValidatorIndexOutOfRangeError` * simplify parameter names * test header * more corrections * properly allocate array capacity Co-authored-by: terence tsao <terence@prysmaticlabs.com> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: Nishant Das <nishdas93@gmail.com>

rkapka added 2 commits May 25, 2021 13:09

error codes for node endpoints

929e304

error codes for debug endpoints

de5ee4f

rkapka requested a review from a team as a code owner May 25, 2021 11:22

rkapka requested review from kasey, jmozah and 0xKiwi May 25, 2021 11:22

rkapka added the API Api related tasks label May 25, 2021

rkapka commented May 25, 2021

View reviewed changes

rkapka added 2 commits May 25, 2021 13:29

better comment about headers

bfadfdf

gzl

d93d803

prestonvanloon reviewed May 25, 2021

View reviewed changes

rkapka added 6 commits May 25, 2021 17:10

review comments

5e79f24

Merge branch 'feature/api-middleware' into api-error-codes

2a841fd

# Conflicts: # beacon-chain/rpc/nodev1/BUILD.bazel # beacon-chain/rpc/nodev1/node.go

comment on return value

f615b55

update fakeChecker used for fuzz tests

1154ce4

Merge branch 'feature/api-middleware' into api-error-codes

fb121a3

fix failing tests

062a6ab

prestonvanloon approved these changes May 26, 2021

View reviewed changes

rkapka added the OK to merge label May 26, 2021

Merge branch 'feature/api-middleware' into api-error-codes

3d37ac9

prylabs-bulldozer bot merged commit a2f82b2 into feature/api-middleware May 26, 2021

delete-merged-branch bot deleted the api-error-codes branch May 26, 2021 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return HTTP status codes other than 200 and 500 from node and debug endpoints #8937

Return HTTP status codes other than 200 and 500 from node and debug endpoints #8937

rkapka commented May 25, 2021 •

edited

rkapka May 25, 2021

rkapka May 25, 2021

prestonvanloon May 25, 2021

prestonvanloon May 25, 2021

rkapka May 25, 2021

prestonvanloon May 25, 2021

rkapka May 25, 2021

codecov bot commented May 26, 2021 •

edited

		stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError)
		if ok {

	stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError)
	if ok {
	if stateNotFoundErr, ok := err.(*statefetcher.StateNotFoundError); ok {

		_ = grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))
		return &emptypb.Empty{}, nil

	_ = grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))
	return &emptypb.Empty{}, nil
	return &emptypb.Empty{}, grpc.SetHeader(ctx, metadata.Pairs(grpcutils.HttpCodeMetadataKey, strconv.Itoa(http.StatusPartialContent)))

	message: fmt.Sprintf("state not found in the last %d state roots in head state", stateRootsSize),
	message: fmt.Sprintf("state not found in the last %d state roots", stateRootsSize),

Return HTTP status codes other than 200 and 500 from node and debug endpoints #8937

Return HTTP status codes other than 200 and 500 from node and debug endpoints #8937

Conversation

rkapka commented May 25, 2021 • edited

rkapka May 25, 2021

Choose a reason for hiding this comment

rkapka May 25, 2021

Choose a reason for hiding this comment

prestonvanloon May 25, 2021

Choose a reason for hiding this comment

prestonvanloon May 25, 2021

Choose a reason for hiding this comment

rkapka May 25, 2021

Choose a reason for hiding this comment

prestonvanloon May 25, 2021

Choose a reason for hiding this comment

rkapka May 25, 2021

Choose a reason for hiding this comment

codecov bot commented May 26, 2021 • edited

Codecov Report

rkapka commented May 25, 2021 •

edited

codecov bot commented May 26, 2021 •

edited