Envoy proxy requires the authority header

[cdvr1993]

If envoy receives a header frame without the authority header it drops
the frame.

The errors is the following:
[2020-09-29 22:34:48.118][1962226][debug][http2]
[external/envoy/source/common/http/http2/codec_impl_legacy.cc:786] [C3]
invalid http2: Invalid HTTP header field was received: frame type: 1,
stream: 1, name: [:authority], value: []
[2020-09-29 22:34:48.118][1962226][debug][http2]
[external/envoy/source/common/http/http2/codec_impl_legacy.cc:792] [C3]
invalid frame: Invalid HTTP header field was received on stream 1

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #307 into dev will decrease coverage by 1.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #307      +/-   ##
==========================================
- Coverage   81.05%   80.02%   -1.04%     
==========================================
  Files          59       58       -1     
  Lines        3236     2818     -418     
==========================================
- Hits         2623     2255     -368     
+ Misses        514      464      -50     
  Partials       99       99              

Impacted Files	Coverage Δ
protobuf/source_reflection.go	100.00% <100.00%> (ø)
encoding/meta.go	77.77% <0.00%> (-6.84%)	⬇️
testdata/protobuf/simple/simple.pb.go	31.25% <0.00%> (-6.53%)	⬇️
testdata/gen-go/integration/ttypes.go	7.69% <0.00%> (-4.81%)	⬇️
internal/thrifttest/thrifttest.go	87.50% <0.00%> (-4.17%)	⬇️
transport/grpc.go	76.92% <0.00%> (-3.35%)	⬇️
templateargs/interpolate/types.go	80.95% <0.00%> (-3.05%)	⬇️
encoding/protobuf_health.go	82.60% <0.00%> (-2.58%)	⬇️
testdata/gen-go/integration/foo.go	17.87% <0.00%> (-2.36%)	⬇️
peerprovider/http.go	71.42% <0.00%> (-1.91%)	⬇️
... and 50 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 843ed98...2f41d26. Read the comment docs.

[prashantv]

Thanks for the contribution, a few questions:

• This adds authority by default, should it be an option? Should we allow the caller to specify it?
• This only adds authority to the connection made for the reflection call, not the actual call with the RPC. Is that intentional, shouldn't we ensure the same headers are used for the RPC call as well?
• Why do we pass the full list of host:ports as the authority? I'm not sure what the value should be, whether a host:port is even appropriate.

[cdvr1993]

1.- I think it should be an option to add more flexibility. Although many clients put a default value if there is none provided by the user. For instance grpcurl:

grpcurl ...
"authority":"127.0.0.1:20000"

# if I set a different host header
grpcurl -H 'host: some' ....
"authority":"127.0.0.1:20000,some"

2.- I think it sets the same. Example:

':method', 'POST'
':scheme', 'http'
':path', '/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo'
':authority', '127.0.0.1:20000'
'content-type', 'application/grpc'

':method', 'POST'
':scheme', 'http'
':path', '/ratelimiter.Ratelimiter/GetTrackedEndpoints'
':authority', '127.0.0.1:20000'

3.- Most clients (if not all of them) set the authority header to host header. Although, as you can see from my grpcurl command if I set another host header it appends it to the authority header using a comma, that is why I decided to do a join by comma, but I'm open to suggestions.

For more information on the issue you can look at this one: envoyproxy/envoy#5944

[prashantv]

1.- I think it should be an option to add more flexibility. Although many clients put a default value if there is none provided by the user.

Agree, that seems ideal, default with a value based on the peer, and option to set a custom value.

2.- I think it sets the same.

I just took a packet capture using the current yab version (without this change), and it looks like:

• the reflection call has a blank authority header:
  

• the RPC call has a default value with the peer information already:
  

So it looks like the authority header is set to the peer for the RPC (through some combination of the yarpc transport / grpc-go, not sure exactly how).

The current change only impacts the authority header used for the reflection call. If we add an option to set the value, it should also control the authority header for the RPC call.

3.- Most clients (if not all of them) set the authority header to host header. Although, as you can see from my grpcurl command if I set another host header it appends it to the authority header using a comma, that is why I decided to do a join by comma, but I'm open to suggestions.

I think using a single host:port may be better, since yab can be used with huge host files lists (e.g., 10000 peers), and joining + sending that could cause issues. I think we should use a single host:port which matches the peer that we are connecting to.

[cdvr1993]

1.- I think adding the option can be done in a separate diff.
2.- I will try and fix if we can set the host:port for the actual peer chosen.

What do you think?

[prashantv]

1.- I think adding the option can be done in a separate diff.
2.- I will try and fix if we can set the host:port for the actual peer chosen.

What do you think?

Yep, I think that's totally fine, can do the option as a follow-up, thanks.

[cdvr1993]

It seems that the current API doesn't support to set the authority per host.

The current version that yab uses has the following (https://github.com/grpc/grpc-go/blob/v1.24.x/clientconn.go#L262):

	if creds != nil && creds.Info().ServerName != "" {
		cc.authority = creds.Info().ServerName
	} else if cc.dopts.insecure && cc.dopts.authority != "" {
		cc.authority = cc.dopts.authority
	} else {
		// Use endpoint from "scheme://authority/endpoint" as the default
		// authority for ClientConn.
		cc.authority = cc.parsedTarget.Endpoint
	}

cc.dopts.authority is the hardcoded value form WithAuthority().

The newest version provides a way to set the authority per host but it is only for tls connections. I tried using it by mocking a tls interface but it doesn't work because it only uses that information for tls handshake not to set the authority header.

What do you think? Should we set the authority by default to one host? to the service name? Or should we instead use multiple grpc reflection clients (one per host) and handle ourselves to which one to connect. In the end these clients are only used for the reflection mechanism, not sure how often do we call them... I imagine only the first time. Should we ask to fix this in the grpc loadbalancer?

[cdvr1993]

I check one more time and yes it seems there is no way to inject the right authority because the API doesn't provide a way. The authority header comes from: https://github.com/grpc/grpc-go/blob/master/stream.go#L206.

I think the easiest solution is setting the authority to the service name (this removes my issue where authority is empty and I don't care about the value) and adding a flag to set a different authority which is going to be useful if you require the authority to match the virtual hosts. The only issue would be that if the backends have different virtual hosts it is not going to work.

In order to remove the previous issue, the only solution is to use our own load balancing mechanism or asking to the grpc library to provide an API to set authority per host.

[prashantv]

It seems that the current API doesn't support to set the authority per host.

I think YARPC creates a separate ClientConn per-host (and I think the same is true for the reflection call made), so I don't think this should be an issue.

I'm actually OK with using the service name, but it has to be consistent: If we use the service name for the reflection call, the RPC should also use the same.

[cdvr1993]

Yeah, it shouldn't be an issue. The problem is that because of the load balancing that it has, the grpc AP doesn't provide a way to set an authority per host. We would need to call DialContext with each host to specify the authority for them, but that means we would need to handle the load balancing for the reflection calls.

We could try and leverage the yarpc-go library but I think there are enough differences between the API which makes this approach too complex.

At the moment the reflection client is only talking to the first peer because by default it has the first_pick load balancing policy. So, if we implement our own load balancing, should we do it in a round robin approach or just use the other servers as fallback when there is an error?

I have something like this:

func (s *grpcreflectSource) getClient() *grpcreflect.Client {
	s.peerState.Lock()
	defer s.peerState.Unlock()
	peer := s.peerState.peers[s.peerState.currentPeer]
	if s.peerState.hadError {
		// clean error
		s.peerState.hadError = false
		s.peerState.currentPeer = (s.peerState.currentPeer+1) % len(s.peerState.peers)
		peer = s.peerState.peers[s.peerState.currentPeer]

                 // this is for async connection
		if s.clients[peer] == nil {
			cli, err := s.connecToPeer(peer)
			if err != nil {
				panic(err)
			}

			s.clients[peer] = cli
		}
	}

	return s.clients[peer]
}

func (s *grpcreflectSource) FindService(fullyQualifiedName string) (service *desc.ServiceDescriptor, err error) {
	defer func() {
		if r := recover(); r != nil {
			if e, ok := r.(error); ok {
				err = e
			}
		}
	}()

	service, err = s.getClient().ResolveService(fullyQualifiedName)

....

[prashantv]

I think implementing first-available makes sense, although implementing it ourselves does seem a little more complex than what we had.

cc @robbertvanginkel who worked on this integration, any suggestions on how to support Authority header (and ensure it's consistent between the RPC calls and the reflection calls)?

[robbertvanginkel]

Not really, I'm not very familiar with grpc-go's connection. Reading up on this thread and grpc/grpc-go#3444 it does seem like there is no way to set authority for a single request.

I think the cleanest way of achieving getting rpc calls and and reflection calls to use the same parameters is make sure both requests are made through yarpc, instead of one using grpc-go. This would probably involve implementing a version of https://godoc.org/github.com/jhump/protoreflect/grpcreflect#NewClient that can take a yarpc client. I think it might be possible without re-implementing the whole package but instead using a small wrapper using a yarpc connection that implements https://godoc.org/google.golang.org/grpc/reflection/grpc_reflection_v1alpha#ServerReflectionClient, as I believe yarpc streams expose their grpc.ClientStream, which is what the package really relies on.

If the above isn't possible, then I suppose to make the same requests yab would need to implement similar peer/connection logic as yarpc in the absence of being able to set it per request.

[alxn]

Codecov Report

Merging #307 into dev will decrease coverage by 1.03%.
The diff coverage is 100.00%.

What ... what is the math behind this calculation?

[crazyboycjr]

This PR has been years. I question the ambition that the authority header in gRPC reflection call and actual gRPC calls should be consistent. Previously we don't even have the authority header in gRPC reflection call, right? That seems to me a kind of inconsistence for years.

Disclaimer: I'm testing Envoy with yab. Without this single line of change, Envoy cannot proxy yab's request. This PR is super helpful to me and people who want to use Envoy with yab.

@prashantv What's your opinion if we merge this single line of change and move discussion of the need of consistency between gRPC reflection calls and actual gRPC calls to another thread?