Skip to content
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

Merged
merged 10 commits into from
Jan 16, 2025
Merged

Adopt ClientContext changes #56

merged 10 commits into from
Jan 16, 2025

Conversation

gjcairo
Copy link
Collaborator

@gjcairo gjcairo commented Jan 15, 2025

This PR adopts the changes introduced in grpc/grpc-swift#2158, requiring client transports to provide a ClientContext alongside the stream.

@gjcairo gjcairo added the ⚠️ semver/major Breaks existing public API. label Jan 15, 2025
@gjcairo gjcairo requested a review from glbrntt January 15, 2025 14:57
Copy link
Collaborator

@glbrntt glbrntt left a 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:

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:

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 [:]
}
}

@gjcairo gjcairo force-pushed the client-context-changes branch from 5dee811 to 7ca6a4b Compare January 15, 2025 17:57
Copy link
Collaborator

@glbrntt glbrntt left a 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 :)

@gjcairo gjcairo requested a review from glbrntt January 16, 2025 16:22
Tests/GRPCNIOTransportHTTP2Tests/ControlService.swift Outdated Show resolved Hide resolved
internal import NIOCore

extension NIOAsyncChannel {
var remoteAddressInfo: String {
Copy link
Collaborator

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.

@glbrntt glbrntt merged commit b8a3c3b into main Jan 16, 2025
21 of 28 checks passed
@glbrntt glbrntt deleted the client-context-changes branch January 16, 2025 17:04
@gjcairo gjcairo added 🔨 semver/patch No public API change. and removed ⚠️ semver/major Breaks existing public API. labels Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants