grpc, xds: recovery middleware to return and log error in case of panic

[gmichelo]

1. xds and grpc servers:
   1.1. to use recovery middleware with callback that prints stack trace to log
   1.2. callback turn the panic into a core.Internal error
2. added unit test for grpc server

Partially fixes 2nd and 3rd tasks in list: #10715.

[gmichelo]

@dnephin, I am not sure why check-go-mod check fails. I committed the new go.mod and go.sum. Is there any issue with that?

[hashicorp-cla]

All committers have signed the CLA.

[dhiaayachi]

Hi @BigMikes,
Thank you for working on this PR. We had a discussion with the team about this and the following came out of it:

• We agree on the principal of having a way to recover from a panic
• We are no sure that adding an extra dependency to consul middleware is justified for this.

I will spend some time to check what options are acceptable to solve this and add it in here. So you can adapt the PR to it, if you are still interested in working on it.

Thank you again and sorry for the long delay.

[gmichelo]

Hi @dhiaayachi,

thanks for letting me know. No worries, let me know once you have the new instructions and I'll fix my PR.

[dnephin]

Thank you for working on this! I think this is looking good. I left a couple suggestions below for the test and sharing implementation with the two servers.

We also noticed that v2 of this middleware library is still a pre-release (rc2), so may not be production ready yet. I think we should either use v1, or write our own interceptor. I think we'd be fine with either.

[dnephin]

I guess we could also expose a function from agent/grpc that returns these two options, and use that here? The handler in agent/grpc could use the same PanicHandlerMiddleware function to add them to opts, that way its clear that both of these gRPC servers are using the same middleware.

opts :=  []grpc.ServerOption{grpc.MaxConcurrentStreams(2048)}
opts = append(opts, agentgrpc.PanicHandlerMiddleware(s.Logger)...)

What do you think?

[gmichelo]

The problem is that agent/grpc/handler.go also requires another interceptor, which is not used in xds' server:

middleware.WithStreamServerChain(
	// Add middlware interceptors to recover in case of panics.
	recovery.StreamServerInterceptor(recoveryOpts...),
	(&activeStreamCounter{metrics: metrics}).Intercept,
),

And interceptions cannot be specified twice, otherwise you get the following panic:

panic: The stream server interceptor was already set and may not be reset. [recovered]
	panic: The stream server interceptor was already set and may not be reset.

Also, I think it might be good to have the middleware in close as possible where the server is defined, so that people can clearly see what interceptors are being used and they can easily extend the server with new interceptors.

[gmichelo]

Hi @dhiaayachi, @dnephin. FYI, this PR is ready for review based on your latest comments.

[dhiaayachi]

Hey @BigMikes , sorry for the delay. This is good to go from my perspective.
I will let @dnephin take another look before merging.

[dnephin]

I fixed the merge conflict. If CI is still happy I will merge.

Thanks again!

[dnephin]

I looking into using a newer 1.x version, but it requires updating gRPC , which I don't particularly want to do right now. I looked at the diff, and nothing important has changed since 1.0, so I think this is good.

[hc-github-team-consul-core]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/520564.