Continue #2724: replace Url-based PgConnectionConfig with a hand-crafted struct

Downsides are:

* We store all components of the config separately. `Url` stores them inside a single
  `String` and a bunch of ints which point to different parts of the URL, which is
  probably more efficient.
* It is now impossible to pass arbitrary connection strings to the configuration file,
  one has to support all components explicitly. However, we never supported anything
  except for `host:port` anyway.

Upsides are:

* This significantly restricts the space of possible connection strings, some of which
  may be either invalid or unsupported. E.g. Postgres' connection strings may include
  a bunch of parameters as query (e.g. `connect_timeout=`, `options=`). These are nether
  validated by the current implementation, nor passed to the postgres client library,
  Hence, storing separate fields expresses the intention better.
* The same connection configuration may be represented as a URL in multiple ways
  (e.g. either `password=` in the query part or a standard URL password).
  Now we have a single canonical way.
* Escaping is provided for `options=`.

Other possibilities considered:

* `newtype` with a `String` inside and some validation on creation.
  This is more efficient, but harder to log for two reasons:
  * Passwords should never end up in logs, so we have to somehow
  * Escaped `options=` are harder to read, especially if URL-encoded,
    and we use `options=` a lot.

Cargo.lock

@@ -601,6 +601,7 @@ dependencies = [
  "once_cell",
  "pageserver_api",
  "postgres",
+ "postgres_connection",
  "regex",
  "reqwest",
  "safekeeper_api",
@@ -2405,6 +2406,18 @@ dependencies = [
  "postgres-protocol",
 ]
 
+[[package]]
+name = "postgres_connection"
+version = "0.1.0"
+dependencies = [
+ "anyhow",
+ "once_cell",
+ "postgres",
+ "tokio-postgres",
+ "url",
+ "workspace_hack",
+]
+
 [[package]]
 name = "postgres_ffi"
 version = "0.1.0"

control_plane/Cargo.toml

@@ -23,6 +23,7 @@ url = "2.2.2"
 # Note: Do not directly depend on pageserver or safekeeper; use pageserver_api or safekeeper_api
 # instead, so that recompile times are better.
 pageserver_api = { path = "../libs/pageserver_api" }
+postgres_connection = { path = "../libs/postgres_connection" }
 safekeeper_api = { path = "../libs/safekeeper_api" }
 utils = { path = "../libs/utils" }
 workspace_hack = { version = "0.1", path = "../workspace_hack" }

control_plane/src/connection.rs

@@ -1,57 +0,0 @@
-use url::Url;
-
-#[derive(Debug)]
-pub struct PgConnectionConfig {
-    url: Url,
-}
-
-impl PgConnectionConfig {
-    pub fn host(&self) -> &str {
-        self.url.host_str().expect("BUG: no host")
-    }
-
-    pub fn port(&self) -> u16 {
-        self.url.port().expect("BUG: no port")
-    }
-
-    /// Return a `<host>:<port>` string.
-    pub fn raw_address(&self) -> String {
-        format!("{}:{}", self.host(), self.port())
-    }
-
-    /// Connect using postgres protocol with TLS disabled.
-    pub fn connect_no_tls(&self) -> Result<postgres::Client, postgres::Error> {
-        postgres::Client::connect(self.url.as_str(), postgres::NoTls)
-    }
-}
-
-impl std::str::FromStr for PgConnectionConfig {
-    type Err = anyhow::Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let mut url: Url = s.parse()?;
-
-        match url.scheme() {
-            "postgres" | "postgresql" => {}
-            other => anyhow::bail!("invalid scheme: {other}"),
-        }
-
-        // It's not a valid connection url if host is unavailable.
-        if url.host().is_none() {
-            anyhow::bail!(url::ParseError::EmptyHost);
-        }
-
-        // E.g. `postgres:bar`.
-        if url.cannot_be_a_base() {
-            anyhow::bail!("URL cannot be a base");
-        }
-
-        // Set the default PG port if it's missing.
-        if url.port().is_none() {
-            url.set_port(Some(5432))
-                .expect("BUG: couldn't set the default port");
-        }
-
-        Ok(Self { url })
-    }
-}

control_plane/src/lib.rs

@@ -9,7 +9,6 @@
 
 mod background_process;
 pub mod compute;
-pub mod connection;
 pub mod etcd;
 pub mod local_env;
 pub mod pageserver;

control_plane/src/pageserver.rs

@@ -6,11 +6,11 @@ use std::path::{Path, PathBuf};
 use std::process::Child;
 use std::{io, result};
 
-use crate::connection::PgConnectionConfig;
 use anyhow::{bail, Context};
 use pageserver_api::models::{
     TenantConfigRequest, TenantCreateRequest, TenantInfo, TimelineCreateRequest, TimelineInfo,
 };
+use postgres_connection::{parse_host_port, PgConnectionConfig};
 use reqwest::blocking::{Client, RequestBuilder, Response};
 use reqwest::{IntoUrl, Method};
 use thiserror::Error;
@@ -77,30 +77,24 @@ pub struct PageServerNode {
 
 impl PageServerNode {
     pub fn from_env(env: &LocalEnv) -> PageServerNode {
+        let (host, port) = parse_host_port(&env.pageserver.listen_pg_addr)
+            .expect("Unable to parse listen_pg_addr");
+        let port = port.unwrap_or(5432);
         let password = if env.pageserver.auth_type == AuthType::NeonJWT {
-            &env.pageserver.auth_token
+            Some(env.pageserver.auth_token.clone())
         } else {
-            ""
+            None
         };
 
         Self {
-            pg_connection_config: Self::pageserver_connection_config(
-                password,
-                &env.pageserver.listen_pg_addr,
-            ),
+            pg_connection_config: PgConnectionConfig::new_host_port(host, port)
+                .set_password(password),
             env: env.clone(),
             http_client: Client::new(),
             http_base_url: format!("http://{}/v1", env.pageserver.listen_http_addr),
         }
     }
 
-    /// Construct libpq connection string for connecting to the pageserver.
-    fn pageserver_connection_config(password: &str, listen_addr: &str) -> PgConnectionConfig {
-        format!("postgresql://no_user:{password}@{listen_addr}/no_db")
-            .parse()
-            .unwrap()
-    }
-
     pub fn initialize(
         &self,
         create_tenant: Option<TenantId>,

control_plane/src/safekeeper.rs

@@ -5,12 +5,12 @@ use std::sync::Arc;
 use std::{io, result};
 
 use anyhow::Context;
+use postgres_connection::PgConnectionConfig;
 use reqwest::blocking::{Client, RequestBuilder, Response};
 use reqwest::{IntoUrl, Method};
 use thiserror::Error;
 use utils::{http::error::HttpErrorBody, id::NodeId};
 
-use crate::connection::PgConnectionConfig;
 use crate::pageserver::PageServerNode;
 use crate::{
     background_process,
@@ -86,10 +86,7 @@ impl SafekeeperNode {
 
     /// Construct libpq connection string for connecting to this safekeeper.
     fn safekeeper_connection_config(port: u16) -> PgConnectionConfig {
-        // TODO safekeeper authentication not implemented yet
-        format!("postgresql://no_user@127.0.0.1:{port}/no_db")
-            .parse()
-            .unwrap()
+        PgConnectionConfig::new_host_port(url::Host::parse("127.0.0.1").unwrap(), port)
     }
 
     pub fn datadir_path_by_id(env: &LocalEnv, sk_id: NodeId) -> PathBuf {

libs/postgres_connection/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+name = "postgres_connection"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+anyhow = "1.0"
+postgres = { git = "https://github.com/neondatabase/rust-postgres.git", rev = "d052ee8b86fff9897c77b0fe89ea9daba0e1fa38" }
+tokio-postgres = { git = "https://github.com/neondatabase/rust-postgres.git", rev="d052ee8b86fff9897c77b0fe89ea9daba0e1fa38" }
+url = "2.2.2"
+workspace_hack = { version = "0.1", path = "../../workspace_hack" }
+
+[dev-dependencies]
+once_cell = "1.13.0"

libs/postgres_connection/src/lib.rs

@@ -0,0 +1,196 @@
+use anyhow::{bail, Context};
+use std::fmt;
+use url::Host;
+
+/// Parses a string of format either `host:port` or `host` into a corresponding pair.
+/// The `host` part should be a correct `url::Host`, while `port` (if present) should be
+/// a valid decimal u16 of digits only.
+pub fn parse_host_port<S: AsRef<str>>(host_port: S) -> Result<(Host, Option<u16>), anyhow::Error> {
+    let (host, port) = match host_port.as_ref().rsplit_once(':') {
+        Some((host, port)) => (
+            host,
+            // +80 is a valid u16, but not a valid port
+            if port.chars().all(|c| c.is_ascii_digit()) {
+                Some(port.parse::<u16>().context("Unable to parse port")?)
+            } else {
+                bail!("Port contains a non-ascii-digit")
+            },
+        ),
+        None => (host_port.as_ref(), None), // No colons, no port specified
+    };
+    let host = Host::parse(host).context("Unable to parse host")?;
+    Ok((host, port))
+}
+
+#[cfg(test)]
+mod tests_parse_host_port {
+    use crate::parse_host_port;
+    use url::Host;
+
+    #[test]
+    fn test_normal() {
+        let (host, port) = parse_host_port("hello:123").unwrap();
+        assert_eq!(host, Host::Domain("hello".to_owned()));
+        assert_eq!(port, Some(123));
+    }
+
+    #[test]
+    fn test_no_port() {
+        let (host, port) = parse_host_port("hello").unwrap();
+        assert_eq!(host, Host::Domain("hello".to_owned()));
+        assert_eq!(port, None);
+    }
+
+    #[test]
+    fn test_ipv6() {
+        let (host, port) = parse_host_port("[::1]:123").unwrap();
+        assert_eq!(host, Host::<String>::Ipv6(std::net::Ipv6Addr::LOCALHOST));
+        assert_eq!(port, Some(123));
+    }
+
+    #[test]
+    fn test_invalid_host() {
+        assert!(parse_host_port("hello world").is_err());
+    }
+
+    #[test]
+    fn test_invalid_port() {
+        assert!(parse_host_port("hello:+80").is_err());
+    }
+}
+
+pub struct PgConnectionConfig {
+    host: Host,
+    port: u16,
+    password: Option<String>,
+}
+
+/// A simplified PostgreSQL connection configuration. Supports only a subset of possible
+/// settings for simplicity. A password getter or `to_connection_string` methods are not
+/// added by design to avoid accidentally leaking password through logging, command line
+/// arguments to a child process, or likewise.
+impl PgConnectionConfig {
+    pub fn new_host_port(host: Host, port: u16) -> Self {
+        PgConnectionConfig {
+            host,
+            port,
+            password: None,
+        }
+    }
+
+    pub fn host(&self) -> &Host {
+        &self.host
+    }
+
+    pub fn port(&self) -> u16 {
+        self.port
+    }
+
+    pub fn set_host(mut self, h: Host) -> Self {
+        self.host = h;
+        self
+    }
+
+    pub fn set_port(mut self, p: u16) -> Self {
+        self.port = p;
+        self
+    }
+
+    pub fn set_password(mut self, s: Option<String>) -> Self {
+        self.password = s;
+        self
+    }
+
+    /// Return a `<host>:<port>` string.
+    pub fn raw_address(&self) -> String {
+        format!("{}:{}", self.host(), self.port())
+    }
+
+    /// Build a client library-specific connection configuration.
+    /// Used for testing and when we need to add some obscure configuration
+    /// elements at the last moment.
+    pub fn to_tokio_postgres_config(&self) -> tokio_postgres::Config {
+        // Use `tokio_postgres::Config` instead of `postgres::Config` because
+        // the former supports more options to fiddle with later.
+        let mut config = tokio_postgres::Config::new();
+        config.host(&self.host().to_string()).port(self.port);
+        if let Some(password) = &self.password {
+            config.password(password);
+        }
+        config
+    }
+
+    /// Connect using postgres protocol with TLS disabled.
+    pub fn connect_no_tls(&self) -> Result<postgres::Client, postgres::Error> {
+        postgres::Config::from(self.to_tokio_postgres_config()).connect(postgres::NoTls)
+    }
+}
+
+impl fmt::Debug for PgConnectionConfig {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        // We want `password: Some(REDACTED-STRING)`, not `password: Some("REDACTED-STRING")`
+        // so even if the password is `REDACTED-STRING` (quite unlikely) there is no confusion.
+        // Hence `format_args!()`, it returns a "safe" string which is not escaped by `Debug`.
+        f.debug_struct("PgConnectionConfig")
+            .field("host", &self.host)
+            .field("port", &self.port)
+            .field(
+                "password",
+                &self
+                    .password
+                    .as_ref()
+                    .map(|_| format_args!("REDACTED-STRING")),
+            )
+            .finish()
+    }
+}
+
+#[cfg(test)]
+mod tests_pg_connection_config {
+    use crate::PgConnectionConfig;
+    use once_cell::sync::Lazy;
+    use url::Host;
+
+    static STUB_HOST: Lazy<Host> = Lazy::new(|| Host::Domain("stub.host.example".to_owned()));
+
+    #[test]
+    fn test_no_password() {
+        let cfg = PgConnectionConfig::new_host_port(STUB_HOST.clone(), 123);
+        assert_eq!(cfg.host(), &*STUB_HOST);
+        assert_eq!(cfg.port(), 123);
+        assert_eq!(cfg.raw_address(), "stub.host.example:123");
+        assert_eq!(
+            format!("{:?}", cfg),
+            "PgConnectionConfig { host: Domain(\"stub.host.example\"), port: 123, password: None }"
+        );
+    }
+
+    #[test]
+    fn test_ipv6() {
+        // May be a special case because hostname contains a colon.
+        let cfg = PgConnectionConfig::new_host_port(Host::parse("[::1]").unwrap(), 123);
+        assert_eq!(
+            cfg.host(),
+            &Host::<String>::Ipv6(std::net::Ipv6Addr::LOCALHOST)
+        );
+        assert_eq!(cfg.port(), 123);
+        assert_eq!(cfg.raw_address(), "[::1]:123");
+        assert_eq!(
+            format!("{:?}", cfg),
+            "PgConnectionConfig { host: Ipv6(::1), port: 123, password: None }"
+        );
+    }
+
+    #[test]
+    fn test_with_password() {
+        let cfg = PgConnectionConfig::new_host_port(STUB_HOST.clone(), 123)
+            .set_password(Some("password".to_owned()));
+        assert_eq!(cfg.host(), &*STUB_HOST);
+        assert_eq!(cfg.port(), 123);
+        assert_eq!(cfg.raw_address(), "stub.host.example:123");
+        assert_eq!(
+            format!("{:?}", cfg),
+            "PgConnectionConfig { host: Domain(\"stub.host.example\"), port: 123, password: Some(REDACTED-STRING) }"
+        );
+    }
+}