[Observer] Enable REST API on observers

[UlyanaAndrukhiv]

#3138

Context

This pull request adds a new RestServerApi interface for REST service, ServerRequestHandler - structure that implements RestServerApi and represents local requests, RestForwarder- structure that also implements RestServerApi and handles the request forwarding to upstream. According to similar API proxy pattern adds RestRouter for observer node - structure that represents the routing proxy algorithm and splits requests between a local and forward requests which can't be handled locally to an upstream access node.

As part of this PR:

• The protobuf have been extended (added GetExecutionResultByID request/response, see Added GetExecutionResultByID request/response flow#1346).
• Tests for the RestRouter have been added, RestServerApi mock have been generated and added.
• Integration TestObserverRest test have been added.

[peterargue]

thanks for the PR @UlyanaAndrukhiv! Overall, there are a lot of great improvements here and the code is clean and well written.

I have a general thought on the structure of how the forwarding happens. In the GRPC to GRPC forwarding, there's some efficiency gained by using a router approach since we don't have to convert back and forth between protobuf and local models. Instead, the response is simply streamed back to the caller as is.

In the rest case, we have to do the conversions (unless we proxied REST, but I agree that grpc is better). Using a router here means you had to implement effectively the same code twice, once in ServerRequestHandler e.g.

flow-go/engine/access/rest/server_request_handler.go

Lines 34 to 53 in 264f86f

	func (h *ServerRequestHandler) GetTransactionByID(r request.GetTransaction, context context.Context, link models.LinkGenerator, _ flow.Chain) (models.Transaction, error) {
	var response models.Transaction
	tx, err := h.backend.GetTransaction(context, r.ID)
	if err != nil {
	return response, err
	}
	var txr *access.TransactionResult
	// only lookup result if transaction result is to be expanded
	if r.ExpandsResult {
	txr, err = h.backend.GetTransactionResult(context, r.ID, r.BlockID, r.CollectionID)
	if err != nil {
	return response, err
	}
	}
	response.Build(tx, txr, link)
	return response, nil
	}

and once in RestForwarder e.g.

flow-go/engine/access/rest/apiproxy/forwarder.go

Lines 46 to 84 in 264f86f

	func (f *RestForwarder) GetTransactionByID(r request.GetTransaction, context context.Context, link models.LinkGenerator, chain flow.Chain) (models.Transaction, error) {
	var response models.Transaction
	upstream, err := f.FaultTolerantClient()
	if err != nil {
	return response, err
	}
	getTransactionRequest := &accessproto.GetTransactionRequest{
	Id: r.ID[:],
	}
	transactionResponse, err := upstream.GetTransaction(context, getTransactionRequest)
	if err != nil {
	return response, err
	}
	var transactionResultResponse *accessproto.TransactionResultResponse
	// only lookup result if transaction result is to be expanded
	if r.ExpandsResult {
	getTransactionResultRequest := &accessproto.GetTransactionRequest{
	Id: r.ID[:],
	BlockId: r.BlockID[:],
	CollectionId: r.CollectionID[:],
	}
	transactionResultResponse, err = upstream.GetTransactionResult(context, getTransactionResultRequest)
	if err != nil {
	return response, err
	}
	}
	flowTransaction, err := convert.MessageToTransaction(transactionResponse.Transaction, chain)
	if err != nil {
	return response, err
	}
	flowTransactionResult := access.MessageToTransactionResult(transactionResultResponse)
	response.Build(&flowTransaction, flowTransactionResult, link)
	return response, nil
	}

I worry that having the 2 implementations will make the rest server even harder to maintain then it is now.

Another option is to provide a customized backend implementation to the existing rest server. That backend could embed the default backend and override the methods that should be proxied. Then the overrides can simply call the upstream node and do the format conversions to return the appropriate model.

This has a few benefits:

1. There's less duplication of logic and we won't need a second set of grpc converter methods to map to the rest models
2. the REST server can remain mostly unmodified, reducing the amount of change going out at once
3. There's a pattern established for backing the REST server with a grpc implementation. This would allow breaking the REST server out from the node entirely in the future.

What are your thoughts on that approach?

[peterargue]

@UlyanaAndrukhiv thanks for making those changes. Added a few more comments, but it's getting close

[peterargue]

added a couple small comments, and there's one previous testing comment, but otherwise looks good.

Please make sure to manually test out the REST API on observers using localnet. It's possible something was missed in the test coverage.

[peterargue]

Looks like you added MessageToBlockEvents implementations. Can you also move BlockEventsToMessages and blockEventsToMessage to the convert/events.go file?

[durkmurder]

@peterargue I think in such tests it's enough to mock backend, instead of mocking every dependency of backend. I am not asking to change it, since it's a pattern which is used in many tests but could be a thing for future task to refactor access/observer tests for easier maintainability.

[durkmurder]

Looks good on my side. Very good PR, lots of good changes.

[peterargue]

@UlyanaAndrukhiv can you run make tidy and push any updates.

[peterargue]

Looks good. Great work @UlyanaAndrukhiv!

[peterargue]

bors merge

[bors]

Build failed:

• Unit Tests (network)