-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adopt ClientContext
changes
#56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should really add tests for this too. There are some already for the server checking its peer info:
grpc-swift-nio-transport/Tests/GRPCNIOTransportHTTP2Tests/HTTP2TransportTests.swift
Lines 1630 to 1658 in 92ad3c0
func testPeerInfoIPv4() async throws { | |
try await self.forEachTransportPair( | |
serverAddress: .ipv4(host: "127.0.0.1", port: 0) | |
) { control, _, _ in | |
let peerInfo = try await control.peerInfo() | |
let matches = peerInfo.matches(of: /ipv4:127.0.0.1:\d+/) | |
XCTAssertNotNil(matches) | |
} | |
} | |
func testPeerInfoIPv6() async throws { | |
try await self.forEachTransportPair( | |
serverAddress: .ipv6(host: "::1", port: 0) | |
) { control, _, _ in | |
let peerInfo = try await control.peerInfo() | |
let matches = peerInfo.matches(of: /ipv6:[::1]:\d+/) | |
XCTAssertNotNil(matches) | |
} | |
} | |
func testPeerInfoUDS() async throws { | |
let path = "peer-info-uds" | |
try await self.forEachTransportPair( | |
serverAddress: .unixDomainSocket(path: path) | |
) { control, _, _ in | |
let peerInfo = try await control.peerInfo() | |
XCTAssertEqual(peerInfo, "unix:peer-info-uds") | |
} | |
} |
The control service has a peer info RPC which just returns the peer info to the client as a string. We could change that type to contain the client and server local and remote peer info. The server can obviously extract that info from its own context and the client could inject it into metadata from an interceptor so that the server can then pull it out and stuff it back in the response.
Take a look here too:
grpc-swift-nio-transport/Tests/GRPCNIOTransportHTTP2Tests/ControlService.swift
Lines 65 to 75 in 92ad3c0
router.registerHandler( | |
forMethod: MethodDescriptor(fullyQualifiedService: "Control", method: "PeerInfo"), | |
deserializer: JSONDeserializer<String>(), | |
serializer: JSONSerializer<String>() | |
) { request, context in | |
return StreamingServerResponse { response in | |
let info = try await self.peerInfo(context: context) | |
try await response.write(info) | |
return [:] | |
} | |
} |
Sources/GRPCNIOTransportCore/Client/Connection/Connection.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCNIOTransportCore/Internal/Channel+AddressInfo.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCNIOTransportCore/Internal/Channel+AddressInfo.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCNIOTransportCore/Client/Connection/Connection.swift
Outdated
Show resolved
Hide resolved
5dee811
to
7ca6a4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, assuming CI turns green :)
internal import NIOCore | ||
|
||
extension NIOAsyncChannel { | ||
var remoteAddressInfo: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can deduplicate this by having one func which takes two addresses and have remoteAddressInfo
and localAddressInfo
just call it with the local/remote addresses inverted. Let's do that in a follow up though.
This PR adopts the changes introduced in grpc/grpc-swift#2158, requiring client transports to provide a
ClientContext
alongside the stream.