fix UDS without a baseURL (#165)

weissi · web-flow · commit de7421906c68 · 2020-02-24T17:52:36.000Z
Previously, UNIX Domain Sockets would only work if the URL also had a
"base URL". If it didn't have a base URL, it would try to connect to the
empty string which would fail :).

Now, we support both cases:
- URLs with a baseURL (the path to the UDS) and a path (actual path)
- URLs that just have an actual path (path to the UDS) where we'll just
  use "/" as the URL's path
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -272,7 +272,11 @@ public class HTTPClient {
                         return channel.eventLoop.makeSucceededFuture(())
                     }
                 }.flatMap {
-                    let taskHandler = TaskHandler(task: task, delegate: delegate, redirectHandler: redirectHandler, ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown)
+                    let taskHandler = TaskHandler(task: task,
+                                                  kind: request.kind,
+                                                  delegate: delegate,
+                                                  redirectHandler: redirectHandler,
+                                                  ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown)
                     return channel.pipeline.addHandler(taskHandler)
                 }
             }
@@ -282,9 +286,11 @@ public class HTTPClient {
         }
 
         let eventLoopChannel: EventLoopFuture<Channel>
-        if request.kind == .unixSocket, let baseURL = request.url.baseURL {
-            eventLoopChannel = bootstrap.connect(unixDomainSocketPath: baseURL.path)
-        } else {
+        switch request.kind {
+        case .unixSocket:
+            let socketPath = request.url.baseURL?.path ?? request.url.path
+            eventLoopChannel = bootstrap.connect(unixDomainSocketPath: socketPath)
+        case .host:
             let address = self.resolveAddress(request: request, proxy: self.configuration.proxy)
             eventLoopChannel = bootstrap.connect(host: address.host, port: address.port)
         }
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -559,12 +559,18 @@ internal class TaskHandler<Delegate: HTTPClientResponseDelegate> {
     var state: State = .idle
     var pendingRead = false
     var mayRead = true
+    let kind: HTTPClient.Request.Kind
 
-    init(task: HTTPClient.Task<Delegate.Response>, delegate: Delegate, redirectHandler: RedirectHandler<Delegate.Response>?, ignoreUncleanSSLShutdown: Bool) {
+    init(task: HTTPClient.Task<Delegate.Response>,
+         kind: HTTPClient.Request.Kind,
+         delegate: Delegate,
+         redirectHandler: RedirectHandler<Delegate.Response>?,
+         ignoreUncleanSSLShutdown: Bool) {
         self.task = task
         self.delegate = delegate
         self.redirectHandler = redirectHandler
         self.ignoreUncleanSSLShutdown = ignoreUncleanSSLShutdown
+        self.kind = kind
     }
 }
 
@@ -653,7 +659,19 @@ extension TaskHandler: ChannelDuplexHandler {
         self.state = .idle
         let request = unwrapOutboundIn(data)
 
-        var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1), method: request.method, uri: request.url.uri)
+        let uri: String
+        switch (self.kind, request.url.baseURL) {
+        case (.host, _):
+            uri = request.url.uri
+        case (.unixSocket, .none):
+            uri = "/" // we don't have a real path, the path we have is the path of the UNIX Domain Socket.
+        case (.unixSocket, .some(_)):
+            uri = request.url.uri
+        }
+
+        var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1),
+                                   method: request.method,
+                                   uri: uri)
         var headers = request.headers
 
         if !request.headers.contains(name: "Host") {
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift
@@ -28,7 +28,11 @@ class HTTPClientInternalTests: XCTestCase {
         let task = Task<Void>(eventLoop: channel.eventLoop)
 
         try channel.pipeline.addHandler(recorder).wait()
-        try channel.pipeline.addHandler(TaskHandler(task: task, delegate: TestHTTPDelegate(), redirectHandler: nil, ignoreUncleanSSLShutdown: false)).wait()
+        try channel.pipeline.addHandler(TaskHandler(task: task,
+                                                    kind: .host,
+                                                    delegate: TestHTTPDelegate(),
+                                                    redirectHandler: nil,
+                                                    ignoreUncleanSSLShutdown: false)).wait()
 
         var request = try Request(url: "http://localhost/get")
         request.headers.add(name: "X-Test-Header", value: "X-Test-Value")
@@ -56,6 +60,7 @@ class HTTPClientInternalTests: XCTestCase {
 
         XCTAssertNoThrow(try channel.pipeline.addHandler(recorder).wait())
         XCTAssertNoThrow(try channel.pipeline.addHandler(TaskHandler(task: task,
+                                                                     kind: .host,
                                                                      delegate: TestHTTPDelegate(),
                                                                      redirectHandler: nil,
                                                                      ignoreUncleanSSLShutdown: false)).wait())
@@ -74,7 +79,11 @@ class HTTPClientInternalTests: XCTestCase {
         let channel = EmbeddedChannel()
         let delegate = TestHTTPDelegate()
         let task = Task<Void>(eventLoop: channel.eventLoop)
-        let handler = TaskHandler(task: task, delegate: delegate, redirectHandler: nil, ignoreUncleanSSLShutdown: false)
+        let handler = TaskHandler(task: task,
+                                  kind: .host,
+                                  delegate: delegate,
+                                  redirectHandler: nil,
+                                  ignoreUncleanSSLShutdown: false)
 
         try channel.pipeline.addHandler(handler).wait()
 
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
@@ -92,11 +92,87 @@ internal final class RecordingHandler<Input, Output>: ChannelDuplexHandler {
     }
 }
 
+enum TemporaryFileHelpers {
+    private static var temporaryDirectory: String {
+        #if targetEnvironment(simulator)
+            // Simulator temp directories are so long (and contain the user name) that they're not usable
+            // for UNIX Domain Socket paths (which are limited to 103 bytes).
+            return "/tmp"
+        #else
+            #if os(Android)
+                return "/data/local/tmp"
+            #elseif os(Linux)
+                return "/tmp"
+            #else
+                if #available(macOS 10.12, iOS 10, tvOS 10, watchOS 3, *) {
+                    return FileManager.default.temporaryDirectory.path
+                } else {
+                    return "/tmp"
+                }
+            #endif // os
+        #endif // targetEnvironment
+    }
+
+    private static func openTemporaryFile() -> (CInt, String) {
+        let template = "\(temporaryDirectory)/ahc_XXXXXX"
+        var templateBytes = template.utf8 + [0]
+        let templateBytesCount = templateBytes.count
+        let fd = templateBytes.withUnsafeMutableBufferPointer { ptr in
+            ptr.baseAddress!.withMemoryRebound(to: Int8.self, capacity: templateBytesCount) { ptr in
+                mkstemp(ptr)
+            }
+        }
+        templateBytes.removeLast()
+        return (fd, String(decoding: templateBytes, as: Unicode.UTF8.self))
+    }
+
+    /// This function creates a filename that can be used for a temporary UNIX domain socket path.
+    ///
+    /// If the temporary directory is too long to store a UNIX domain socket path, it will `chdir` into the temporary
+    /// directory and return a short-enough path. The iOS simulator is known to have too long paths.
+    internal static func withTemporaryUnixDomainSocketPathName<T>(directory: String = temporaryDirectory,
+                                                                  _ body: (String) throws -> T) throws -> T {
+        // this is racy but we're trying to create the shortest possible path so we can't add a directory...
+        let (fd, path) = self.openTemporaryFile()
+        close(fd)
+        try! FileManager.default.removeItem(atPath: path)
+
+        let saveCurrentDirectory = FileManager.default.currentDirectoryPath
+        let restoreSavedCWD: Bool
+        let shortEnoughPath: String
+        do {
+            _ = try SocketAddress(unixDomainSocketPath: path)
+            // this seems to be short enough for a UDS
+            shortEnoughPath = path
+            restoreSavedCWD = false
+        } catch SocketAddressError.unixDomainSocketPathTooLong {
+            FileManager.default.changeCurrentDirectoryPath(URL(fileURLWithPath: path).deletingLastPathComponent().absoluteString)
+            shortEnoughPath = URL(fileURLWithPath: path).lastPathComponent
+            restoreSavedCWD = true
+            print("WARNING: Path '\(path)' could not be used as UNIX domain socket path, using chdir & '\(shortEnoughPath)'")
+        }
+        defer {
+            if FileManager.default.fileExists(atPath: path) {
+                try? FileManager.default.removeItem(atPath: path)
+            }
+            if restoreSavedCWD {
+                FileManager.default.changeCurrentDirectoryPath(saveCurrentDirectory)
+            }
+        }
+        return try body(shortEnoughPath)
+    }
+}
+
 internal final class HTTPBin {
     let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
     let serverChannel: Channel
     let isShutdown: NIOAtomic<Bool> = .makeAtomic(value: false)
 
+    enum BindTarget {
+        case unixDomainSocket(String)
+        case localhostIPv4RandomPort
+    }
+
     var port: Int {
         return Int(self.serverChannel.localAddress!.port!)
     }
@@ -112,7 +188,18 @@ internal final class HTTPBin {
         return channel.pipeline.addHandler(try! NIOSSLServerHandler(context: context), position: .first)
     }
 
-    init(ssl: Bool = false, compress: Bool = false, simulateProxy: HTTPProxySimulator.Option? = nil, channelPromise: EventLoopPromise<Channel>? = nil) {
+    init(ssl: Bool = false,
+         compress: Bool = false,
+         bindTarget: BindTarget = .localhostIPv4RandomPort,
+         simulateProxy: HTTPProxySimulator.Option? = nil,
+         channelPromise: EventLoopPromise<Channel>? = nil) {
+        let socketAddress: SocketAddress
+        switch bindTarget {
+        case .localhostIPv4RandomPort:
+            socketAddress = try! SocketAddress(ipAddress: "127.0.0.1", port: 0)
+        case .unixDomainSocket(let path):
+            socketAddress = try! SocketAddress(unixDomainSocketPath: path)
+        }
         self.serverChannel = try! ServerBootstrap(group: self.group)
             .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
             .childChannelOption(ChannelOptions.socket(IPPROTO_TCP, TCP_NODELAY), value: 1)
@@ -145,7 +232,7 @@ internal final class HTTPBin {
                         }
                     }
             }
-            .bind(host: "127.0.0.1", port: 0).wait()
+            .bind(to: socketAddress).wait()
     }
 
     func shutdown() throws {
@@ -250,6 +337,16 @@ internal final class HttpBinHandler: ChannelInboundHandler {
         case .head(let req):
             let url = URL(string: req.uri)!
             switch url.path {
+            case "/":
+                var headers = HTTPHeaders()
+                headers.add(name: "X-Is-This-Slash", value: "Yes")
+                self.resps.append(HTTPResponseBuilder(status: .ok, headers: headers))
+                return
+            case "/echo-uri":
+                var headers = HTTPHeaders()
+                headers.add(name: "X-Calling-URI", value: req.uri)
+                self.resps.append(HTTPResponseBuilder(status: .ok, headers: headers))
+                return
             case "/ok":
                 self.resps.append(HTTPResponseBuilder(status: .ok))
                 return
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift
@@ -70,6 +70,8 @@ extension HTTPClientTests {
             ("testSubsequentRequestsWorkWithServerAlternatingBetweenKeepAliveAndClose", testSubsequentRequestsWorkWithServerAlternatingBetweenKeepAliveAndClose),
             ("testManyConcurrentRequestsWork", testManyConcurrentRequestsWork),
             ("testRepeatedRequestsWorkWhenServerAlwaysCloses", testRepeatedRequestsWorkWhenServerAlwaysCloses),
+            ("testUDSBasic", testUDSBasic),
+            ("testUDSSocketAndPath", testUDSSocketAndPath),
         ]
     }
 }
diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift
@@ -1003,4 +1003,49 @@ class HTTPClientTests: XCTestCase {
             XCTAssertNil(response?.body)
         }
     }
+
+    func testUDSBasic() {
+        // This tests just connecting to a URL where the whole URL is the UNIX domain socket path like
+        //     unix:///this/is/my/socket.sock
+        // We don't really have a path component, so we'll have to use "/"
+        XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in
+            let httpBin = HTTPBin(bindTarget: .unixDomainSocket(path))
+            defer {
+                XCTAssertNoThrow(try httpBin.shutdown())
+            }
+            let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
+            defer {
+                XCTAssertNoThrow(try httpClient.syncShutdown())
+            }
+
+            let target = "unix://\(path)"
+            XCTAssertNoThrow(XCTAssertEqual(["Yes"[...]],
+                                            try httpClient.get(url: target).wait().headers[canonicalForm: "X-Is-This-Slash"]))
+        })
+    }
+
+    func testUDSSocketAndPath() {
+        // Here, we're testing a URL that's encoding two different paths:
+        //
+        //  1. a "base path" which is the path to the UNIX domain socket
+        //  2. an actual path which is the normal path in a regular URL like https://example.com/this/is/the/path
+        XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { path in
+            let httpBin = HTTPBin(bindTarget: .unixDomainSocket(path))
+            defer {
+                XCTAssertNoThrow(try httpBin.shutdown())
+            }
+            let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
+            defer {
+                XCTAssertNoThrow(try httpClient.syncShutdown())
+            }
+
+            guard let target = URL(string: "/echo-uri", relativeTo: URL(string: "unix://\(path)")),
+                let request = try? Request(url: target) else {
+                XCTFail("couldn't build URL for request")
+                return
+            }
+            XCTAssertNoThrow(XCTAssertEqual(["/echo-uri"[...]],
+                                            try httpClient.execute(request: request).wait().headers[canonicalForm: "X-Calling-URI"]))
+        })
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -272,7 +272,11 @@ public class HTTPClient {`
`272`	`272`	`return channel.eventLoop.makeSucceededFuture(())`
`273`	`273`	`}`
`274`	`274`	`}.flatMap {`
`275`		`- let taskHandler = TaskHandler(task: task, delegate: delegate, redirectHandler: redirectHandler, ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown)`
	`275`	`+ let taskHandler = TaskHandler(task: task,`
	`276`	`+ kind: request.kind,`
	`277`	`+ delegate: delegate,`
	`278`	`+ redirectHandler: redirectHandler,`
	`279`	`+ ignoreUncleanSSLShutdown: self.configuration.ignoreUncleanSSLShutdown)`
`276`	`280`	`return channel.pipeline.addHandler(taskHandler)`
`277`	`281`	`}`
`278`	`282`	`}`
`@@ -282,9 +286,11 @@ public class HTTPClient {`
`282`	`286`	`}`
`283`	`287`
`284`	`288`	`let eventLoopChannel: EventLoopFuture<Channel>`
`285`		`- if request.kind == .unixSocket, let baseURL = request.url.baseURL {`
`286`		`- eventLoopChannel = bootstrap.connect(unixDomainSocketPath: baseURL.path)`
`287`		`- } else {`
	`289`	`+ switch request.kind {`
	`290`	`+ case .unixSocket:`
	`291`	`+ let socketPath = request.url.baseURL?.path ?? request.url.path`
	`292`	`+ eventLoopChannel = bootstrap.connect(unixDomainSocketPath: socketPath)`
	`293`	`+ case .host:`
`288`	`294`	`let address = self.resolveAddress(request: request, proxy: self.configuration.proxy)`
`289`	`295`	`eventLoopChannel = bootstrap.connect(host: address.host, port: address.port)`
`290`	`296`	`}`
Original file line number	Diff line number	Diff line change
`@@ -70,6 +70,8 @@ extension HTTPClientTests {`
`70`	`70`	`("testSubsequentRequestsWorkWithServerAlternatingBetweenKeepAliveAndClose", testSubsequentRequestsWorkWithServerAlternatingBetweenKeepAliveAndClose),`
`71`	`71`	`("testManyConcurrentRequestsWork", testManyConcurrentRequestsWork),`
`72`	`72`	`("testRepeatedRequestsWorkWhenServerAlwaysCloses", testRepeatedRequestsWorkWhenServerAlwaysCloses),`
	`73`	`+ ("testUDSBasic", testUDSBasic),`
	`74`	`+ ("testUDSSocketAndPath", testUDSSocketAndPath),`
`73`	`75`	`]`
`74`	`76`	`}`
`75`	`77`	`}`