From a049aff03acf7801fd91fe7e71761437055a805f Mon Sep 17 00:00:00 2001 From: Gwynne Raskind Date: Tue, 26 May 2020 12:27:29 -0500 Subject: [PATCH] `Environment` cleanups (#2353) * Remove numerous unnecessary code stanzas. * Don't check `isRelease` in `Environment.==`, it will always be equal because the value is effectively a compile-time constant. Also improve the use of MARK comments so Xcode renders the intended divisions nicely. * Add warning to `Environment.isRelease` explaining the seemingly unusual behavior of its value. * Redo the documentation comments for `Environment.secret()` (both versions). Significantly simplify the implementation of the path-taking version. IMPORTANT NOTE: This is an interim step of cleanup while a much more complete revamping of this API is worked on. * Correctly sanitize the excess arguments Xcode passes to test invocations for the testing environment. I forgot to mention in a previous commit that support was added for `VAPOR_ENV` too... * Add sanitization of raw SwiftPM's invocation of the xctest runner binary. This is necessarily a little specific to the version of SwiftPM and Xcode involved, but should at least be specific enough a check to not interfere with normal operations if the call sequence changes. * There is no need to hardcode all the logger levels for `LosslessStringConvertible` conformance. `Logger.Level` is already `RawRepresentable` as `String` and that conformance can be used transparently. (And it has `CaseIterable` for good measure, which provides yet another way to scale this particular elevation.) --- .../Environment/Environment+Process.swift | 14 +-- .../Environment/Environment+Secret.swift | 74 +++++++----- Sources/Vapor/Environment/Environment.swift | 111 ++++++++++++------ .../Logging/LoggingSystem+Environment.swift | 26 +--- 4 files changed, 128 insertions(+), 97 deletions(-) diff --git a/Sources/Vapor/Environment/Environment+Process.swift b/Sources/Vapor/Environment/Environment+Process.swift index ae14f6f897..66aaaadc80 100644 --- a/Sources/Vapor/Environment/Environment+Process.swift +++ b/Sources/Vapor/Environment/Environment+Process.swift @@ -17,12 +17,9 @@ extension Environment { /// Environment.process.DATABASE_PORT // 3306 public subscript(dynamicMember member: String) -> T? where T: LosslessStringConvertible { get { - guard let raw = self._info.environment[member], let value = T(raw) else { - return nil - } - - return value + return self._info.environment[member].flatMap { T($0) } } + nonmutating set (value) { if let raw = value?.description { setenv(member, raw, 1) @@ -38,12 +35,9 @@ extension Environment { /// Environment.process.DATABASE_USER // "root" public subscript(dynamicMember member: String) -> String? { get { - guard let value = self._info.environment[member] else { - return nil - } - - return value + return self._info.environment[member] } + nonmutating set (value) { if let raw = value { setenv(member, raw, 1) diff --git a/Sources/Vapor/Environment/Environment+Secret.swift b/Sources/Vapor/Environment/Environment+Secret.swift index 4855301227..eafd3a5d9e 100644 --- a/Sources/Vapor/Environment/Environment+Secret.swift +++ b/Sources/Vapor/Environment/Environment+Secret.swift @@ -1,43 +1,61 @@ extension Environment { - /// Reads a file's content for a secret. The secret key represents the name of the environment variable that holds the path for the file containing the secret + /// Reads a file's content for a secret. The secret key is the name of the environment variable that is expected to + /// specify the path of the file containing the secret. + /// /// - Parameters: - /// - key: Environment name for the path to the file containing the secret - /// - fileIO: FileIO handler provided by NIO - /// - on: EventLoop to operate on while opening the file - /// - Throws: Error.environmentVariableNotFound if the environment variable with the key name does not exist + /// - key: The environment variable name + /// - fileIO: `NonBlockingFileIO` handler provided by NIO + /// - eventLoop: `EventLoop` for NIO to use for working with the file + /// + /// Example usage: + /// + /// ```` + /// func configure(_ app: Application) { + /// // ... + /// + /// let databasePassword = try Environment.secret( + /// key: "DATABASE_PASSWORD_FILE", + /// fileIO: app.fileio, + /// on: app.eventLoopGroup.next() + /// ).wait() + /// + /// ```` + /// + /// - Important: Do _not_ use `.wait()` if loading a secret at any time after the app has booted, such as while + /// handling a `Request`. Chain the result as you would any other future instead. public static func secret(key: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture { - guard let filePath = self.get(key) else { return eventLoop.future(nil) } + guard let filePath = self.get(key) else { + return eventLoop.future(nil) + } return self.secret(path: filePath, fileIO: fileIO, on: eventLoop) } - /// Reads a file's content for a secret. The path is a file path to the file that contains the secret in plain text + /// Load the content of a file at a given path as a secret. + /// /// - Parameters: - /// - path: Path to the file that contains the secret - /// - fileIO: FileIO handler provided by NIO - /// - on: EventLoop to operate on while opening the file - /// - Throws: Error.environmentVariableNotFound if the environment variable with the key name does not exist + /// - path: Path to the file containing the secret + /// - fileIO: `NonBlockingFileIO` handler provided by NIO + /// - eventLoop: `EventLoop` for NIO to use for working with the file + /// + /// - Returns: + /// - On success, a succeeded future with the loaded content of the file. + /// - On any kind of error, a succeeded future with a value of `nil`. It is not currently possible to get error details. public static func secret(path: String, fileIO: NonBlockingFileIO, on eventLoop: EventLoop) -> EventLoopFuture { return fileIO .openFile(path: path, eventLoop: eventLoop) - .flatMap({ (arg) -> EventLoopFuture in + .flatMap { handle, region in return fileIO - .read(fileRegion: arg.1, allocator: .init(), eventLoop: eventLoop) - .flatMapThrowing({ (buffer) -> ByteBuffer in - try arg.0.close() - return buffer - }) - }) - .map({ (buffer) -> (String) in - var buffer = buffer - return buffer.readString(length: buffer.writerIndex) ?? "" - }) - .map({ (secret) -> (String) in - secret.trimmingCharacters(in: .whitespacesAndNewlines) - }) - .recover ({ (_) -> String? in + .read(fileRegion: region, allocator: .init(), eventLoop: eventLoop) + .always { _ in try? handle.close() } + } + .map { buffer -> String in + return buffer + .getString(at: buffer.readerIndex, length: buffer.readableBytes)! + .trimmingCharacters(in: .whitespacesAndNewlines) + } + .recover { _ -> String? in nil - }) + } } } - diff --git a/Sources/Vapor/Environment/Environment.swift b/Sources/Vapor/Environment/Environment.swift index 067054b29c..0461b88d3f 100644 --- a/Sources/Vapor/Environment/Environment.swift +++ b/Sources/Vapor/Environment/Environment.swift @@ -1,3 +1,5 @@ +import ConsoleKit + /// The environment the application is running in, i.e., production, dev, etc. All `Container`s will have /// an `Environment` that can be used to dynamically register and configure services. /// @@ -12,6 +14,8 @@ /// print(Environment.get("DB_PASSWORD")) /// public struct Environment: Equatable { + // MARK: - Detection + /// Detects the environment from `CommandLine.arguments`. Invokes `detect(from:)`. /// - parameters: /// - arguments: Command line arguments to detect environment from. @@ -21,73 +25,106 @@ public struct Environment: Equatable { return try Environment.detect(from: &commandInput) } - /// Detects the environment from `CommandInput`. Parses the `--env` flag. + /// Detects the environment from `CommandInput`. Parses the `--env` flag, with the + /// `VAPOR_ENV` environment variable as a fallback. /// - parameters: /// - arguments: `CommandInput` to parse `--env` flag from. /// - returns: The detected environment, or default env. public static func detect(from commandInput: inout CommandInput) throws -> Environment { + self.sanitize(commandInput: &commandInput) + struct EnvironmentSignature: CommandSignature { @Option(name: "env", short: "e", help: "Change the application's environment") var environment: String? - init() { } } + var env: Environment - if let value = try EnvironmentSignature(from: &commandInput).environment { - switch value { + switch try EnvironmentSignature(from: &commandInput).environment ?? + Environment.process.VAPOR_ENV + { case "prod", "production": env = .production - case "dev", "development": env = .development + case "dev", "development", .none: env = .development case "test", "testing": env = .testing - default: env = .init(name: value) - } - } else { - env = .development + case .some(let name): env = .init(name: name) } env.commandInput = commandInput return env } - // MARK: Presets + /// Performs stripping of user defaults overrides where and when appropriate. + private static func sanitize(commandInput: inout CommandInput) { + #if Xcode + // Strip all leading arguments matching the pattern for assignment to the `NSArgumentsDomain` + // of `UserDefaults`. Matching this pattern means being prefixed by `-NS` or `-Apple` and being + // followed by a value argument. Since this is mainly just to get around Xcode's habit of + // passing a bunch of these when no other arguments are specified in a test scheme, we ignore + // any that don't match the Apple patterns and assume the app knows what it's doing. + while (commandInput.arguments.first?.prefix(6) == "-Apple" || commandInput.arguments.first?.prefix(3) == "-NS"), + commandInput.arguments.count > 1 { + commandInput.arguments.removeFirst(2) + } + #elseif os(macOS) || os(iOS) || os(tvOS) || os(watchOS) + // When tests are invoked directly through SwiftPM using `--filter`, SwiftPM will pass `-XCTest ` to the + // runner binary, and also the test bundle path unconditionally. These must be stripped for Vapor to be satisifed + // with the validity of the arguments. We detect this case reliably the hard way, by looking for the `xctest` + // runner executable and a leading argument with the `.xctest` bundle suffix. + if commandInput.executable.hasSuffix("/usr/bin/xctest") { + if commandInput.arguments.first?.lowercased() == "-xctest" && commandInput.arguments.count > 1 { + commandInput.arguments.removeFirst(2) + } + if commandInput.arguments.first?.hasSuffix(".xctest") ?? false { + commandInput.arguments.removeFirst() + } + } + #endif + } + + /// Invokes `sanitize(commandInput:)` over a set of raw arguments and returns the + /// resulting arguments, including the executable path. + private static func sanitizeArguments(_ arguments: [String] = CommandLine.arguments) -> [String] { + var commandInput = CommandInput(arguments: arguments) + sanitize(commandInput: &commandInput) + return commandInput.executablePath + commandInput.arguments + } + + // MARK: - Presets /// An environment for deploying your application to consumers. - public static var production: Environment { - return .init(name: "production") - } + public static var production: Environment { .init(name: "production") } /// An environment for developing your application. - public static var development: Environment { - return .init(name: "development", arguments: ["vapor"]) - } + public static var development: Environment { .init(name: "development") } /// An environment for testing your application. - public static var testing: Environment { - return .init(name: "testing", arguments: ["vapor"]) - } + /// + /// Performs an explicit sanitization step because this preset is often used directly in unit tests, without the + /// benefit of the logic usually invoked through either form of `detect()`. This means that when `--env test` is + /// explicitly specified, the sanitize logic is run twice, but this should be harmless. + public static var testing: Environment { .init(name: "testing", arguments: sanitizeArguments()) } /// Creates a custom environment. - public static func custom(name: String) -> Environment { - return .init(name: name, arguments: ["vapor"]) - } + public static func custom(name: String) -> Environment { .init(name: name) } - // MARK: Env + // MARK: - Env /// Gets a key from the process environment public static func get(_ key: String) -> String? { return ProcessInfo.processInfo.environment[key] } - // MARK: Equatable - - /// See `Equatable` - public static func ==(lhs: Environment, rhs: Environment) -> Bool { - return lhs.name == rhs.name && lhs.isRelease == rhs.isRelease - } - /// The current process of the environment. public static var process: Process { return Process() } - // MARK: Properties + // MARK: - Equatable + + /// See `Equatable` + public static func ==(lhs: Environment, rhs: Environment) -> Bool { + return lhs.name == rhs.name + } + + // MARK: - Properties /// The environment's unique name. public let name: String @@ -96,9 +133,13 @@ public struct Environment: Equatable { /// /// This usually means reducing logging, disabling debug information, and sometimes /// providing warnings about configuration states that are not suitable for production. - public var isRelease: Bool { - return !_isDebugAssertConfiguration() - } + /// + /// - Warning: This value is determined at compile time by configuration; it is not + /// based on the actual environment name. This can lead to unxpected results, such + /// as `Environment.production.isRelease == false`. This is done intentionally to + /// allow scenarios, such as testing production environment behaviors while retaining + /// availability of debug information. + public var isRelease: Bool { !_isDebugAssertConfiguration() } /// The command-line arguments for this `Environment`. public var arguments: [String] @@ -109,7 +150,7 @@ public struct Environment: Equatable { set { arguments = newValue.executablePath + newValue.arguments } } - // MARK: Init + // MARK: - Init /// Create a new `Environment`. public init(name: String, arguments: [String] = CommandLine.arguments) { diff --git a/Sources/Vapor/Logging/LoggingSystem+Environment.swift b/Sources/Vapor/Logging/LoggingSystem+Environment.swift index 4c685ec721..51e0ef6c30 100644 --- a/Sources/Vapor/Logging/LoggingSystem+Environment.swift +++ b/Sources/Vapor/Logging/LoggingSystem+Environment.swift @@ -22,28 +22,6 @@ extension LoggingSystem { } extension Logger.Level: LosslessStringConvertible { - public init?(_ description: String) { - switch description.lowercased() { - case "trace": self = .trace - case "debug": self = .debug - case "info": self = .info - case "notice": self = .notice - case "warning": self = .warning - case "error": self = .error - case "critical": self = .critical - default: return nil - } - } - - public var description: String { - switch self { - case .trace: return "trace" - case .debug: return "debug" - case .info: return "info" - case .notice: return "notice" - case .warning: return "warning" - case .error: return "error" - case .critical: return "critical" - } - } + public init?(_ description: String) { self.init(rawValue: description.lowercased()) } + public var description: String { self.rawValue } }