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

Implement DNS resolver #4

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Implement DNS resolver #4

merged 3 commits into from
Sep 23, 2024

Conversation

clintonpi
Copy link
Contributor

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.


This PR re-opens grpc/grpc-swift#2060.

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.
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.

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.

Comment on lines 20 to 24
package import Darwin
#elseif canImport(Glibc)
package import Glibc
#elseif canImport(Musl)
package import Musl
Copy link
Collaborator

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.

Comment on lines 22 to 36
@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)
}
Copy link
Collaborator

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:

Suggested change
@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)
}

@glbrntt
Copy link
Collaborator

glbrntt commented Sep 23, 2024

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.

Okay, multiple problems here:

  • EAI_ADDRFAMILY and EAI_NODATA aren't available on Linux without including <netdb.h> on and defining the _GNU_SOURCE feature test macro. We can work around this by adding a C shim library. All good.
  • EAI_BADHINTS and EAI_PROTOCOL don't exist in Linux. We could declare them in a platform specific way.

I have an alternative proposal to adding a C-shim module and doing all of the platform shenanigans: just call gai_strerror with the error code instead. It's job is to provide a string description of the error, and that's ultimately what we want in our error:

  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)"
      }
    }
  }

Comment on lines 222 to 224
package enum InetNetworkToPresentationError: Error, Hashable {
case systemError(errno: errno_t)
}
Copy link
Collaborator

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
Copy link
Collaborator

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:

Suggested change
hints.ai_protocol = IPPROTO_TCP
hints.ai_protocol = CInt(IPPROTO_TCP)

@clintonpi clintonpi requested a review from glbrntt September 23, 2024 18:08
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.

Great, thanks @clintonpi

@glbrntt glbrntt enabled auto-merge (squash) September 23, 2024 19:10
@glbrntt glbrntt merged commit 1cea701 into grpc:main Sep 23, 2024
3 of 4 checks passed
@clintonpi clintonpi deleted the dns-resolver branch September 24, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants