Propagate the authority pseudoheader #33
Conversation
Motivation: We should be sending the ":authority" pseudoheader, somehow this was missed when doing the stream state machine. The strategy for propagating the authority will be to: - Use any override value set on the client transport, otherwise - Use the value provided by a name resolver (if any), otherwise - Derive a value from the target address. Modifications: - Add client config allowing the authority to be overridden - Add authority to the name resolver interface - Propagate the authority through to the stream state machine - Add a percent encoder, as the authority should be percent encoded - Remove hostname from the client TLS config and use the authority instead. This is generally a usability win as clients generally won't have to set this value themselves. Result: Authority is set automatically, and can be overridden if necessary.
| return try await self.http2Connector.establishConnection(to: self.address) | ||
| return try await self.http2Connector.establishConnection( | ||
| to: self.address, | ||
| authority: self.authority ?? self.address.sniHostname |
There was a problem hiding this comment.
Here you use self.authority ?? self.address.sniHostname, but further down the file you use self.authority ?? self.address.authority - is that intentional?
There was a problem hiding this comment.
Yes, it's intentional here. The authority here is used as part of the TLS handshake, but valid hostnames for that don't include raw IP addresses, so sniHostname returns nil if address is an IPv4/IPv6 address.
Further down the authority being used is for the :authority pseudo-header.
I'll add some comments explaining this.
| @@ -82,6 +86,12 @@ package final class GRPCChannel: ClientTransport { | |||
| self.input = AsyncStream.makeStream() | |||
| self.connector = connector | |||
|
|
|||
| if let authority = config.http2.authority ?? resolver.authority { | |||
| self.authority = PercentEncoding.encodeAuthority(authority) | |||
There was a problem hiding this comment.
Why does the percent-encoding happen at this level of the hierarchy rather than when it is first obtained?
There was a problem hiding this comment.
If it remains sometimes percent-encoded and sometimes not when stored as a property on objects I think that should be documented.
There was a problem hiding this comment.
This is effectively where it's first obtained: config.http2.authority is the 'global' override and resolver.authority is the first fallback option. If we have a value at this point it'll be used for all connections created by the GRPCChannel.
If we don't have a value at this point then we have to derive it from the address of each connection the GRPCChannel attempts to establish.
You're right about the documentation though, I'll add that and some comments explaining this.
| isValidCharacter: (UInt8) -> Bool | ||
| ) -> String { | ||
| var output: [UInt8] = [] | ||
| output.reserveCapacity(input.utf8.count) |
There was a problem hiding this comment.
Do you think it's worth reserving by some amount greater than the count here, anticipating some conversions?
There was a problem hiding this comment.
I don't think so – I think the common case will be for no additional encoding. We'll get some slack anyway because reserveCapacity should reserve at least that much capacity.
| if isValidCharacter(char) { | ||
| output.append(char) | ||
| } else { | ||
| output.append(UInt8(ascii: "%")) |
There was a problem hiding this comment.
Will the UInt8 conversion of % be computed and stored at compile time? Is it worth doing that ourselves and hardcoding it e.g. UInt8(0x25) // %
There was a problem hiding this comment.
It'll be computed at compile time in release builds: https://godbolt.org/z/rz1Macabv
|
API breakage is expected and okay. |
Motivation:
We should be sending the ":authority" pseudoheader, somehow this was missed when doing the stream state machine.
The strategy for propagating the authority will be to:
Modifications:
Result:
Authority is set automatically, and can be overridden if necessary.