From e03f65b23ccf2a8e5e3767ba8e41f11742147b1e Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 17 Mar 2022 11:24:38 +0100 Subject: [PATCH 1/3] require the server configuration when connecting to the database --- src/admin/delete_crate.rs | 2 +- src/admin/delete_version.rs | 2 +- src/admin/migrate.rs | 11 ++++++----- src/admin/populate.rs | 2 +- src/admin/render_readmes.rs | 2 +- src/admin/transfer_crates.rs | 2 +- src/admin/verify_token.rs | 2 +- src/app.rs | 6 ++++-- src/bin/background-worker.rs | 9 ++++----- src/bin/enqueue-job.rs | 2 +- src/bin/monitor.rs | 2 +- src/boot/categories.rs | 7 ------- src/db.rs | 23 ++++++++++++++--------- 13 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/admin/delete_crate.rs b/src/admin/delete_crate.rs index f710d4b6c81..410a26c9778 100644 --- a/src/admin/delete_crate.rs +++ b/src/admin/delete_crate.rs @@ -14,7 +14,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now().unwrap(); + let conn = db::connect_now(&Default::default()).unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { delete(opts, &conn); Ok(()) diff --git a/src/admin/delete_version.rs b/src/admin/delete_version.rs index 1f3fd188541..3312a4ab517 100644 --- a/src/admin/delete_version.rs +++ b/src/admin/delete_version.rs @@ -21,7 +21,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now().unwrap(); + let conn = db::connect_now(&Default::default()).unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { delete(opts, &conn); Ok(()) diff --git a/src/admin/migrate.rs b/src/admin/migrate.rs index bf3932714a6..3bbf38032b3 100644 --- a/src/admin/migrate.rs +++ b/src/admin/migrate.rs @@ -11,13 +11,13 @@ diesel_migrations::embed_migrations!("./migrations"); pub struct Opts; pub fn run(_opts: Opts) -> Result<(), Error> { - let db_config = crate::config::DatabasePools::full_from_environment(); + let config = crate::config::Server::default(); // TODO: Refactor logic so that we can also check things from App::new() here. // If the app will panic due to bad configuration, it is better to error in the release phase // to avoid launching dynos that will fail. - if db_config.are_all_read_only() { + if config.db.are_all_read_only() { // TODO: Check `any_pending_migrations()` with a read-only connection and error if true. // It looks like this requires changes upstream to make this pub in `migration_macros`. @@ -29,13 +29,14 @@ pub fn run(_opts: Opts) -> Result<(), Error> { return Ok(()); } - println!("==> migrating the database"); // The primary is online, access directly via `DATABASE_URL`. - let conn = crate::db::connect_now()?; + let conn = crate::db::connect_now(&config)?; + + println!("==> migrating the database"); embedded_migrations::run_with_output(&conn, &mut std::io::stdout())?; println!("==> synchronizing crate categories"); - crate::boot::categories::sync(CATEGORIES_TOML).unwrap(); + crate::boot::categories::sync_with_connection(CATEGORIES_TOML, &conn).unwrap(); Ok(()) } diff --git a/src/admin/populate.rs b/src/admin/populate.rs index cb1f8afb2dc..3b56bdb3d63 100644 --- a/src/admin/populate.rs +++ b/src/admin/populate.rs @@ -14,7 +14,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now().unwrap(); + let conn = db::connect_now(&Default::default()).unwrap(); conn.transaction(|| update(opts, &conn)).unwrap(); } diff --git a/src/admin/render_readmes.rs b/src/admin/render_readmes.rs index 9fabe742888..84135c81101 100644 --- a/src/admin/render_readmes.rs +++ b/src/admin/render_readmes.rs @@ -40,7 +40,7 @@ pub struct Opts { pub fn run(opts: Opts) -> anyhow::Result<()> { let base_config = Arc::new(config::Base::from_environment()); - let conn = db::connect_now().unwrap(); + let conn = db::connect_now(&Default::default()).unwrap(); let start_time = Utc::now(); diff --git a/src/admin/transfer_crates.rs b/src/admin/transfer_crates.rs index 2ba1edc772e..f9e503f91a4 100644 --- a/src/admin/transfer_crates.rs +++ b/src/admin/transfer_crates.rs @@ -21,7 +21,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now().unwrap(); + let conn = db::connect_now(&Default::default()).unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { transfer(opts, &conn); Ok(()) diff --git a/src/admin/verify_token.rs b/src/admin/verify_token.rs index 4a393c906eb..14013b9bbd3 100644 --- a/src/admin/verify_token.rs +++ b/src/admin/verify_token.rs @@ -13,7 +13,7 @@ pub struct Opts { } pub fn run(opts: Opts) -> AppResult<()> { - let conn = db::connect_now()?; + let conn = db::connect_now(&Default::default())?; let user = User::find_by_api_token(&conn, &opts.api_token)?; println!("The token belongs to user {}", user.gh_login); Ok(()) diff --git a/src/app.rs b/src/app.rs index 3955363c559..a2eaf89977c 100644 --- a/src/app.rs +++ b/src/app.rs @@ -100,7 +100,7 @@ impl App { let thread_pool = Arc::new(ScheduledThreadPool::new(db_helper_threads)); let primary_database = if config.use_test_database_pool { - DieselPool::new_test(&config.db.primary.url) + DieselPool::new_test(&config, &config.db.primary.url) } else { let primary_db_connection_config = ConnectionConfig { statement_timeout: db_connection_timeout, @@ -116,6 +116,7 @@ impl App { DieselPool::new( &config.db.primary.url, + &config, primary_db_config, instance_metrics .database_time_to_obtain_connection @@ -126,7 +127,7 @@ impl App { let replica_database = if let Some(pool_config) = config.db.replica.as_ref() { if config.use_test_database_pool { - Some(DieselPool::new_test(&pool_config.url)) + Some(DieselPool::new_test(&config, &pool_config.url)) } else { let replica_db_connection_config = ConnectionConfig { statement_timeout: db_connection_timeout, @@ -143,6 +144,7 @@ impl App { Some( DieselPool::new( &pool_config.url, + &config, replica_db_config, instance_metrics .database_time_to_obtain_connection diff --git a/src/bin/background-worker.rs b/src/bin/background-worker.rs index 80111d66296..a160cd8e86a 100644 --- a/src/bin/background-worker.rs +++ b/src/bin/background-worker.rs @@ -24,11 +24,10 @@ use std::time::Duration; fn main() { println!("Booting runner"); - let db_config = config::DatabasePools::full_from_environment(); - let base_config = config::Base::from_environment(); - let uploader = base_config.uploader(); + let config = config::Server::default(); + let uploader = config.base.uploader(); - if db_config.are_all_read_only() { + if config.db.are_all_read_only() { loop { println!( "Cannot run background jobs with a read-only pool. Please scale background_worker \ @@ -38,7 +37,7 @@ fn main() { } } - let db_url = db::connection_url(&db_config.primary.url); + let db_url = db::connection_url(&config, &config.db.primary.url); let job_start_timeout = dotenv::var("BACKGROUND_JOB_TIMEOUT") .unwrap_or_else(|_| "30".into()) diff --git a/src/bin/enqueue-job.rs b/src/bin/enqueue-job.rs index 3c9c80ff765..c962aeb372b 100644 --- a/src/bin/enqueue-job.rs +++ b/src/bin/enqueue-job.rs @@ -7,7 +7,7 @@ use swirl::schema::background_jobs::dsl::*; use swirl::Job; fn main() -> Result<()> { - let conn = db::connect_now()?; + let conn = db::connect_now(&Default::default())?; let mut args = std::env::args().skip(1); let job = args.next().unwrap_or_default(); diff --git a/src/bin/monitor.rs b/src/bin/monitor.rs index 7cfc2874f63..d9016b6c4a7 100644 --- a/src/bin/monitor.rs +++ b/src/bin/monitor.rs @@ -11,7 +11,7 @@ use cargo_registry::{admin::on_call, db, schema::*}; use diesel::prelude::*; fn main() -> Result<()> { - let conn = db::connect_now()?; + let conn = db::connect_now(&Default::default())?; check_failing_background_jobs(&conn)?; check_stalled_update_downloads(&conn)?; diff --git a/src/boot/categories.rs b/src/boot/categories.rs index 632a585aeb6..41f45b9302d 100644 --- a/src/boot/categories.rs +++ b/src/boot/categories.rs @@ -1,8 +1,6 @@ // Sync available crate categories from `src/categories.toml`. // Runs when the server is started. -use crate::db; - use anyhow::{Context, Result}; use diesel::prelude::*; @@ -77,11 +75,6 @@ fn categories_from_toml( Ok(result) } -pub fn sync(toml_str: &str) -> Result<()> { - let conn = db::connect_now()?; - sync_with_connection(toml_str, &conn) -} - pub fn sync_with_connection(toml_str: &str, conn: &PgConnection) -> Result<()> { use crate::schema::categories::dsl::*; use diesel::dsl::all; diff --git a/src/db.rs b/src/db.rs index 68f9c049b72..7881efddeb8 100644 --- a/src/db.rs +++ b/src/db.rs @@ -9,6 +9,7 @@ use thiserror::Error; use url::Url; use crate::middleware::app::RequestApp; +use crate::{config, Env}; #[derive(Clone)] pub enum DieselPool { @@ -22,10 +23,11 @@ pub enum DieselPool { impl DieselPool { pub(crate) fn new( url: &str, - config: r2d2::Builder>, + config: &config::Server, + r2d2_config: r2d2::Builder>, time_to_obtain_connection_metric: Histogram, ) -> Result { - let manager = ConnectionManager::new(connection_url(url)); + let manager = ConnectionManager::new(connection_url(config, url)); // For crates.io we want the behavior of creating a database pool to be slightly different // than the defaults of R2D2: the library's build() method assumes its consumers always @@ -39,7 +41,7 @@ impl DieselPool { // establish any connection continue booting up the application. The database pool will // automatically be marked as unhealthy and the rest of the application will adapt. let pool = DieselPool::Pool { - pool: config.build_unchecked(manager), + pool: r2d2_config.build_unchecked(manager), time_to_obtain_connection_metric, }; match pool.wait_until_healthy(Duration::from_secs(5)) { @@ -51,9 +53,9 @@ impl DieselPool { Ok(pool) } - pub(crate) fn new_test(url: &str) -> DieselPool { + pub(crate) fn new_test(config: &config::Server, url: &str) -> DieselPool { let conn = - PgConnection::establish(&connection_url(url)).expect("failed to establish connection"); + PgConnection::establish(&connection_url(config, url)).expect("failed to establish connection"); conn.begin_test_transaction() .expect("failed to begin test transaction"); DieselPool::Test(Arc::new(ReentrantMutex::new(conn))) @@ -131,16 +133,19 @@ impl Deref for DieselPooledConn<'_> { } } -pub fn connect_now() -> ConnectionResult { - let url = connection_url(&crate::env("DATABASE_URL")); +pub fn connect_now(config: &config::Server) -> ConnectionResult { + let url = connection_url(config, &config.db.primary.url); PgConnection::establish(&url) } -pub fn connection_url(url: &str) -> String { +pub fn connection_url(config: &config::Server, url: &str) -> String { let mut url = Url::parse(url).expect("Invalid database URL"); - if dotenv::var("HEROKU").is_ok() && !url.query_pairs().any(|(k, _)| k == "sslmode") { + + // Enforce secure connections in production. + if config.base.env == Env::Production && !url.query_pairs().any(|(k, _)| k == "sslmode") { url.query_pairs_mut().append_pair("sslmode", "require"); } + url.into() } From 6ae40c3cd4892877fed0be24c84f0cf39194f3d8 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 17 Mar 2022 11:41:28 +0100 Subject: [PATCH 2/3] set TCP_USER_TIMEOUT to 15 seconds for database connections In the December 22, 2021 crates.io outage, we experienced full packet loss between the database server and the application server(s). While the crates.io application supports running without a database during outages, those mitigations kicked in only after a bit more than 15 minutes of the packet loss starting. The default parameters of the Linux network stack result in a TCP connection being marked as broken after roughly 15 minutes of no acknowledgements being received, which is what I believe happened during that outage. Broken database mitigations kicking in after 15 minutes is too long for crates.io, as we ideally want those mitigations to kick in as soon as possible (ensuring crates.io users can continue downloading crates). This commit tells libpq (the underlying PostgreSQL client used by diesel) to set the Linux network stack timeout to a configurable value, which we set as 15 seconds. With this change, if an outage like the one on Dec 22 2021 one happens again, crates.io will be fully unavailable only for 15 seconds rather than 15 minutes. --- src/config/database_pools.rs | 16 ++++++++++++++++ src/db.rs | 22 ++++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/config/database_pools.rs b/src/config/database_pools.rs index 8d2efca5307..9661689afe1 100644 --- a/src/config/database_pools.rs +++ b/src/config/database_pools.rs @@ -9,6 +9,7 @@ //! - `DB_OFFLINE`: If set to `leader` then use the read-only follower as if it was the leader. //! If set to `follower` then act as if `READ_ONLY_REPLICA_URL` was unset. //! - `READ_ONLY_MODE`: If defined (even as empty) then force all connections to be read-only. +//! - `DB_TCP_TIMEOUT_MS`: TCP timeout in milliseconds. See the doc comment for more details. use crate::env; @@ -18,6 +19,12 @@ pub struct DatabasePools { pub primary: DbPoolConfig, /// An optional follower database. Always read-only. pub replica: Option, + /// Number of seconds to wait for unacknowledged TCP packets before treating the connection as + /// broken. This value will determine how long crates.io stays unavailable in case of full + /// packet loss between the application and the database: setting it too high will result in an + /// unnecessarily long outage (before the unhealthy database logic kicks in), while setting it + /// too low might result in healthy connections being dropped. + pub tcp_timeout_ms: u64, } #[derive(Debug)] @@ -67,6 +74,11 @@ impl DatabasePools { _ => None, }; + let tcp_timeout_ms = match dotenv::var("DB_TCP_TIMEOUT_MS") { + Ok(num) => num.parse().expect("couldn't parse DB_TCP_TIMEOUT_MS"), + Err(_) => 15 * 1000, // 15 seconds + }; + match dotenv::var("DB_OFFLINE").as_deref() { // The actual leader is down, use the follower in read-only mode as the primary and // don't configure a replica. @@ -79,6 +91,7 @@ impl DatabasePools { min_idle: primary_min_idle, }, replica: None, + tcp_timeout_ms, }, // The follower is down, don't configure the replica. Ok("follower") => Self { @@ -89,6 +102,7 @@ impl DatabasePools { min_idle: primary_min_idle, }, replica: None, + tcp_timeout_ms, }, _ => Self { primary: DbPoolConfig { @@ -106,6 +120,7 @@ impl DatabasePools { pool_size: replica_pool_size, min_idle: replica_min_idle, }), + tcp_timeout_ms, }, } } @@ -119,6 +134,7 @@ impl DatabasePools { min_idle: None, }, replica: None, + tcp_timeout_ms: 1000, // 1 second } } } diff --git a/src/db.rs b/src/db.rs index 7881efddeb8..6e49149a36f 100644 --- a/src/db.rs +++ b/src/db.rs @@ -54,8 +54,8 @@ impl DieselPool { } pub(crate) fn new_test(config: &config::Server, url: &str) -> DieselPool { - let conn = - PgConnection::establish(&connection_url(config, url)).expect("failed to establish connection"); + let conn = PgConnection::establish(&connection_url(config, url)) + .expect("failed to establish connection"); conn.begin_test_transaction() .expect("failed to begin test transaction"); DieselPool::Test(Arc::new(ReentrantMutex::new(conn))) @@ -142,13 +142,27 @@ pub fn connection_url(config: &config::Server, url: &str) -> String { let mut url = Url::parse(url).expect("Invalid database URL"); // Enforce secure connections in production. - if config.base.env == Env::Production && !url.query_pairs().any(|(k, _)| k == "sslmode") { - url.query_pairs_mut().append_pair("sslmode", "require"); + if config.base.env == Env::Production { + maybe_append_url_param(&mut url, "sslmode", "require"); } + // Configure the time it takes for diesel to return an error when there is full packet loss + // between the application and the database. + maybe_append_url_param( + &mut url, + "tcp_user_timeout", + &config.db.tcp_timeout_ms.to_string(), + ); + url.into() } +fn maybe_append_url_param(url: &mut Url, key: &str, value: &str) { + if !url.query_pairs().any(|(k, _)| k == key) { + url.query_pairs_mut().append_pair(key, value); + } +} + pub trait RequestTransaction { /// Obtain a read/write database connection from the primary pool fn db_conn(&self) -> Result, PoolError>; From 399c8bba1329b2387f766564f57793e9900ab510 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 18 Mar 2022 14:48:24 +0100 Subject: [PATCH 3/3] accept the database config instead of the server config --- src/admin/delete_crate.rs | 2 +- src/admin/delete_version.rs | 2 +- src/admin/migrate.rs | 8 +++++--- src/admin/populate.rs | 2 +- src/admin/render_readmes.rs | 2 +- src/admin/transfer_crates.rs | 2 +- src/admin/verify_token.rs | 2 +- src/app.rs | 8 ++++---- src/bin/background-worker.rs | 2 +- src/bin/enqueue-job.rs | 2 +- src/bin/monitor.rs | 2 +- src/config.rs | 5 +++-- src/config/database_pools.rs | 13 +++++++++++-- src/db.rs | 24 +++++++++++++++--------- 14 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/admin/delete_crate.rs b/src/admin/delete_crate.rs index 410a26c9778..81805452b8e 100644 --- a/src/admin/delete_crate.rs +++ b/src/admin/delete_crate.rs @@ -14,7 +14,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now(&Default::default()).unwrap(); + let conn = db::oneoff_connection().unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { delete(opts, &conn); Ok(()) diff --git a/src/admin/delete_version.rs b/src/admin/delete_version.rs index 3312a4ab517..e6c0a0cf227 100644 --- a/src/admin/delete_version.rs +++ b/src/admin/delete_version.rs @@ -21,7 +21,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now(&Default::default()).unwrap(); + let conn = db::oneoff_connection().unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { delete(opts, &conn); Ok(()) diff --git a/src/admin/migrate.rs b/src/admin/migrate.rs index 3bbf38032b3..808caa58d99 100644 --- a/src/admin/migrate.rs +++ b/src/admin/migrate.rs @@ -11,13 +11,15 @@ diesel_migrations::embed_migrations!("./migrations"); pub struct Opts; pub fn run(_opts: Opts) -> Result<(), Error> { - let config = crate::config::Server::default(); + let config = crate::config::DatabasePools::full_from_environment( + &crate::config::Base::from_environment(), + ); // TODO: Refactor logic so that we can also check things from App::new() here. // If the app will panic due to bad configuration, it is better to error in the release phase // to avoid launching dynos that will fail. - if config.db.are_all_read_only() { + if config.are_all_read_only() { // TODO: Check `any_pending_migrations()` with a read-only connection and error if true. // It looks like this requires changes upstream to make this pub in `migration_macros`. @@ -30,7 +32,7 @@ pub fn run(_opts: Opts) -> Result<(), Error> { } // The primary is online, access directly via `DATABASE_URL`. - let conn = crate::db::connect_now(&config)?; + let conn = crate::db::oneoff_connection_with_config(&config)?; println!("==> migrating the database"); embedded_migrations::run_with_output(&conn, &mut std::io::stdout())?; diff --git a/src/admin/populate.rs b/src/admin/populate.rs index 3b56bdb3d63..eca1bf548dd 100644 --- a/src/admin/populate.rs +++ b/src/admin/populate.rs @@ -14,7 +14,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now(&Default::default()).unwrap(); + let conn = db::oneoff_connection().unwrap(); conn.transaction(|| update(opts, &conn)).unwrap(); } diff --git a/src/admin/render_readmes.rs b/src/admin/render_readmes.rs index 84135c81101..7b5899b0761 100644 --- a/src/admin/render_readmes.rs +++ b/src/admin/render_readmes.rs @@ -40,7 +40,7 @@ pub struct Opts { pub fn run(opts: Opts) -> anyhow::Result<()> { let base_config = Arc::new(config::Base::from_environment()); - let conn = db::connect_now(&Default::default()).unwrap(); + let conn = db::oneoff_connection().unwrap(); let start_time = Utc::now(); diff --git a/src/admin/transfer_crates.rs b/src/admin/transfer_crates.rs index f9e503f91a4..cf3bf6436cd 100644 --- a/src/admin/transfer_crates.rs +++ b/src/admin/transfer_crates.rs @@ -21,7 +21,7 @@ pub struct Opts { } pub fn run(opts: Opts) { - let conn = db::connect_now(&Default::default()).unwrap(); + let conn = db::oneoff_connection().unwrap(); conn.transaction::<_, diesel::result::Error, _>(|| { transfer(opts, &conn); Ok(()) diff --git a/src/admin/verify_token.rs b/src/admin/verify_token.rs index 14013b9bbd3..8595b75b7f3 100644 --- a/src/admin/verify_token.rs +++ b/src/admin/verify_token.rs @@ -13,7 +13,7 @@ pub struct Opts { } pub fn run(opts: Opts) -> AppResult<()> { - let conn = db::connect_now(&Default::default())?; + let conn = db::oneoff_connection()?; let user = User::find_by_api_token(&conn, &opts.api_token)?; println!("The token belongs to user {}", user.gh_login); Ok(()) diff --git a/src/app.rs b/src/app.rs index a2eaf89977c..eabc74bdf62 100644 --- a/src/app.rs +++ b/src/app.rs @@ -100,7 +100,7 @@ impl App { let thread_pool = Arc::new(ScheduledThreadPool::new(db_helper_threads)); let primary_database = if config.use_test_database_pool { - DieselPool::new_test(&config, &config.db.primary.url) + DieselPool::new_test(&config.db, &config.db.primary.url) } else { let primary_db_connection_config = ConnectionConfig { statement_timeout: db_connection_timeout, @@ -116,7 +116,7 @@ impl App { DieselPool::new( &config.db.primary.url, - &config, + &config.db, primary_db_config, instance_metrics .database_time_to_obtain_connection @@ -127,7 +127,7 @@ impl App { let replica_database = if let Some(pool_config) = config.db.replica.as_ref() { if config.use_test_database_pool { - Some(DieselPool::new_test(&config, &pool_config.url)) + Some(DieselPool::new_test(&config.db, &pool_config.url)) } else { let replica_db_connection_config = ConnectionConfig { statement_timeout: db_connection_timeout, @@ -144,7 +144,7 @@ impl App { Some( DieselPool::new( &pool_config.url, - &config, + &config.db, replica_db_config, instance_metrics .database_time_to_obtain_connection diff --git a/src/bin/background-worker.rs b/src/bin/background-worker.rs index a160cd8e86a..4bf286267fb 100644 --- a/src/bin/background-worker.rs +++ b/src/bin/background-worker.rs @@ -37,7 +37,7 @@ fn main() { } } - let db_url = db::connection_url(&config, &config.db.primary.url); + let db_url = db::connection_url(&config.db, &config.db.primary.url); let job_start_timeout = dotenv::var("BACKGROUND_JOB_TIMEOUT") .unwrap_or_else(|_| "30".into()) diff --git a/src/bin/enqueue-job.rs b/src/bin/enqueue-job.rs index c962aeb372b..b10c96a7174 100644 --- a/src/bin/enqueue-job.rs +++ b/src/bin/enqueue-job.rs @@ -7,7 +7,7 @@ use swirl::schema::background_jobs::dsl::*; use swirl::Job; fn main() -> Result<()> { - let conn = db::connect_now(&Default::default())?; + let conn = db::oneoff_connection()?; let mut args = std::env::args().skip(1); let job = args.next().unwrap_or_default(); diff --git a/src/bin/monitor.rs b/src/bin/monitor.rs index d9016b6c4a7..68136d7098d 100644 --- a/src/bin/monitor.rs +++ b/src/bin/monitor.rs @@ -11,7 +11,7 @@ use cargo_registry::{admin::on_call, db, schema::*}; use diesel::prelude::*; fn main() -> Result<()> { - let conn = db::connect_now(&Default::default())?; + let conn = db::oneoff_connection()?; check_failing_background_jobs(&conn)?; check_stalled_update_downloads(&conn)?; diff --git a/src/config.rs b/src/config.rs index 02dd28dc90d..9353f488b23 100644 --- a/src/config.rs +++ b/src/config.rs @@ -83,9 +83,10 @@ impl Default for Server { Some(s) if s.is_empty() => vec![], Some(s) => s.split(',').map(String::from).collect(), }; + let base = Base::from_environment(); Server { - db: DatabasePools::full_from_environment(), - base: Base::from_environment(), + db: DatabasePools::full_from_environment(&base), + base, session_key: env("SESSION_KEY"), gh_client_id: env("GH_CLIENT_ID"), gh_client_secret: env("GH_CLIENT_SECRET"), diff --git a/src/config/database_pools.rs b/src/config/database_pools.rs index 9661689afe1..a28b0cb8b82 100644 --- a/src/config/database_pools.rs +++ b/src/config/database_pools.rs @@ -11,7 +11,8 @@ //! - `READ_ONLY_MODE`: If defined (even as empty) then force all connections to be read-only. //! - `DB_TCP_TIMEOUT_MS`: TCP timeout in milliseconds. See the doc comment for more details. -use crate::env; +use crate::config::Base; +use crate::{env, Env}; pub struct DatabasePools { /// Settings for the primary database. This is usually writeable, but will be read-only in @@ -25,6 +26,8 @@ pub struct DatabasePools { /// unnecessarily long outage (before the unhealthy database logic kicks in), while setting it /// too low might result in healthy connections being dropped. pub tcp_timeout_ms: u64, + /// Whether to enforce that all the database connections are encrypted with TLS. + pub enforce_tls: bool, } #[derive(Debug)] @@ -49,7 +52,7 @@ impl DatabasePools { /// # Panics /// /// This function panics if `DB_OFFLINE=leader` but `READ_ONLY_REPLICA_URL` is unset. - pub fn full_from_environment() -> Self { + pub fn full_from_environment(base: &Base) -> Self { let leader_url = env("DATABASE_URL"); let follower_url = dotenv::var("READ_ONLY_REPLICA_URL").ok(); let read_only_mode = dotenv::var("READ_ONLY_MODE").is_ok(); @@ -79,6 +82,8 @@ impl DatabasePools { Err(_) => 15 * 1000, // 15 seconds }; + let enforce_tls = base.env == Env::Production; + match dotenv::var("DB_OFFLINE").as_deref() { // The actual leader is down, use the follower in read-only mode as the primary and // don't configure a replica. @@ -92,6 +97,7 @@ impl DatabasePools { }, replica: None, tcp_timeout_ms, + enforce_tls, }, // The follower is down, don't configure the replica. Ok("follower") => Self { @@ -103,6 +109,7 @@ impl DatabasePools { }, replica: None, tcp_timeout_ms, + enforce_tls, }, _ => Self { primary: DbPoolConfig { @@ -121,6 +128,7 @@ impl DatabasePools { min_idle: replica_min_idle, }), tcp_timeout_ms, + enforce_tls, }, } } @@ -135,6 +143,7 @@ impl DatabasePools { }, replica: None, tcp_timeout_ms: 1000, // 1 second + enforce_tls: false, } } } diff --git a/src/db.rs b/src/db.rs index 6e49149a36f..06242a53fa7 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,8 +8,8 @@ use std::{ops::Deref, time::Duration}; use thiserror::Error; use url::Url; +use crate::config; use crate::middleware::app::RequestApp; -use crate::{config, Env}; #[derive(Clone)] pub enum DieselPool { @@ -23,7 +23,7 @@ pub enum DieselPool { impl DieselPool { pub(crate) fn new( url: &str, - config: &config::Server, + config: &config::DatabasePools, r2d2_config: r2d2::Builder>, time_to_obtain_connection_metric: Histogram, ) -> Result { @@ -53,7 +53,7 @@ impl DieselPool { Ok(pool) } - pub(crate) fn new_test(config: &config::Server, url: &str) -> DieselPool { + pub(crate) fn new_test(config: &config::DatabasePools, url: &str) -> DieselPool { let conn = PgConnection::establish(&connection_url(config, url)) .expect("failed to establish connection"); conn.begin_test_transaction() @@ -133,16 +133,22 @@ impl Deref for DieselPooledConn<'_> { } } -pub fn connect_now(config: &config::Server) -> ConnectionResult { - let url = connection_url(config, &config.db.primary.url); +pub fn oneoff_connection_with_config( + config: &config::DatabasePools, +) -> ConnectionResult { + let url = connection_url(config, &config.primary.url); PgConnection::establish(&url) } -pub fn connection_url(config: &config::Server, url: &str) -> String { +pub fn oneoff_connection() -> ConnectionResult { + let config = config::DatabasePools::full_from_environment(&config::Base::from_environment()); + oneoff_connection_with_config(&config) +} + +pub fn connection_url(config: &config::DatabasePools, url: &str) -> String { let mut url = Url::parse(url).expect("Invalid database URL"); - // Enforce secure connections in production. - if config.base.env == Env::Production { + if config.enforce_tls { maybe_append_url_param(&mut url, "sslmode", "require"); } @@ -151,7 +157,7 @@ pub fn connection_url(config: &config::Server, url: &str) -> String { maybe_append_url_param( &mut url, "tcp_user_timeout", - &config.db.tcp_timeout_ms.to_string(), + &config.tcp_timeout_ms.to_string(), ); url.into()