-
Notifications
You must be signed in to change notification settings - Fork 11
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
Implement DNS resolver #4
Conversation
Motivation: The GRPCNIOTransportCore module needs a DNS resolver, as currently, IP addresses have to be passed in manually. Modifications: - Add a new type `DNSResolver` with a method `resolve(host:port:)` that calls the `getaddrinfo` `libc` function to resolve a hostname and port number to a list of socket addresses. `resolve(host:port:)` is non-blocking and asynchronous. Result: The GRPCHTTP2Core module will have a DNS resolver.
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.
It looks like the CI is failing because some symbols are missing, I think we need to add a C shim for Linux to get hold of these, let me do that in a separate PR.
package import Darwin | ||
#elseif canImport(Glibc) | ||
package import Glibc | ||
#elseif canImport(Musl) | ||
package import Musl |
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.
I think these should be private
, not sure it makes sense for types from lib C to make it beyond the scope of this file.
@Test("Resolve hostname") | ||
func resolve() async throws { | ||
// Note: This test checks the IPv4 and IPv6 addresses separately (instead of | ||
// `DNSResolver.resolve(host: "localhost", port: 80)`) because the ordering of the resulting | ||
// list containing both IP address versions can be different. | ||
|
||
let expectedIPv4Result: [SocketAddress] = [.ipv4(host: "127.0.0.1", port: 80)] | ||
let expectedIPv6Result: [SocketAddress] = [.ipv6(host: "::1", port: 80)] | ||
|
||
let ipv4Result = try await DNSResolver.resolve(host: "127.0.0.1", port: 80) | ||
let ipv6Result = try await DNSResolver.resolve(host: "::1", port: 80) | ||
|
||
#expect(ipv4Result == expectedIPv4Result) | ||
#expect(ipv6Result == expectedIPv6Result) | ||
} |
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.
This can be a paremeterized test, can be something like this:
@Test("Resolve hostname") | |
func resolve() async throws { | |
// Note: This test checks the IPv4 and IPv6 addresses separately (instead of | |
// `DNSResolver.resolve(host: "localhost", port: 80)`) because the ordering of the resulting | |
// list containing both IP address versions can be different. | |
let expectedIPv4Result: [SocketAddress] = [.ipv4(host: "127.0.0.1", port: 80)] | |
let expectedIPv6Result: [SocketAddress] = [.ipv6(host: "::1", port: 80)] | |
let ipv4Result = try await DNSResolver.resolve(host: "127.0.0.1", port: 80) | |
let ipv6Result = try await DNSResolver.resolve(host: "::1", port: 80) | |
#expect(ipv4Result == expectedIPv4Result) | |
#expect(ipv6Result == expectedIPv6Result) | |
} | |
@Test( | |
"Resolve hostname", | |
arguments: [ | |
("127.0.0.1", .ipv4(host: "127.0.0.1", port: 80)), | |
("::1", .ipv6(host: "::1", port: 80)), | |
] as [(String, SocketAddress)].self | |
) | |
func resolve(host: String, expected: SocketAddress) async throws { | |
// Note: This test checks the IPv4 and IPv6 addresses separately (instead of | |
// `DNSResolver.resolve(host: "localhost", port: 80)`) because the ordering of the resulting | |
// list containing both IP address versions can be different. | |
let result = try await DNSResolver.resolve(host: host, port: 80) | |
#expect(result == expected) | |
} |
Okay, multiple problems here:
I have an alternative proposal to adding a C-shim module and doing all of the platform shenanigans: just call package struct GetAddrInfoError: Error, Hashable, CustomStringConvertible {
package let description: String
package init(code: CInt) {
if let cString = gai_strerror(code) {
self.description = String(cString: cString)
} else {
self.description = "Unknown error: \(code)"
}
}
} |
package enum InetNetworkToPresentationError: Error, Hashable { | ||
case systemError(errno: errno_t) | ||
} |
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.
Let's just make this a struct
which stores the errno
as a CInt
|
||
var hints = addrinfo() | ||
hints.ai_socktype = SOCK_STREAM | ||
hints.ai_protocol = IPPROTO_TCP |
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 need to convert to CInt
here (which is expected by ai_protocol
) as IPPROTO_TCP
isn't always defined as a CInt
:
hints.ai_protocol = IPPROTO_TCP | |
hints.ai_protocol = CInt(IPPROTO_TCP) |
Tests/GRPCNIOTransportCoreTests/Client/Resolver/DNSResolverTests.swift
Outdated
Show resolved
Hide resolved
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.
Great, thanks @clintonpi
Motivation:
The GRPCNIOTransportCore module needs a DNS resolver, as currently, IP addresses have to be passed in manually.
Modifications:
DNSResolver
with a methodresolve(host:port:)
that calls thegetaddrinfo
libc
function to resolve a hostname and port number to a list of socket addresses.resolve(host:port:)
is non-blocking and asynchronous.Result:
The GRPCHTTP2Core module will have a DNS resolver.
This PR re-opens grpc/grpc-swift#2060.