diff --git a/src/config.rs b/src/config.rs index 2566576c7d7..e3b844aa527 100644 --- a/src/config.rs +++ b/src/config.rs @@ -27,6 +27,7 @@ pub struct Server { pub metrics_authorization_token: Option, pub use_test_database_pool: bool, pub instance_metrics_log_every_seconds: Option, + pub force_unconditional_redirects: bool, } impl Default for Server { @@ -55,6 +56,8 @@ impl Default for Server { /// will occur. /// - `INSTANCE_METRICS_LOG_EVERY_SECONDS`: How frequently should instance metrics be logged. /// If the environment variable is not present instance metrics are not logged. + /// - `FORCE_UNCONDITIONAL_REDIRECTS`: Whether to force unconditional redirects in the download + /// endpoint even with a healthy database pool. /// /// # Panics /// @@ -96,6 +99,7 @@ impl Default for Server { metrics_authorization_token: dotenv::var("METRICS_AUTHORIZATION_TOKEN").ok(), use_test_database_pool: false, instance_metrics_log_every_seconds: env_optional("INSTANCE_METRICS_LOG_EVERY_SECONDS"), + force_unconditional_redirects: dotenv::var("FORCE_UNCONDITIONAL_REDIRECTS").is_ok(), } } } diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index 05fde11a9c3..9b85a7e987e 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -18,62 +18,76 @@ pub fn download(req: &mut dyn RequestExt) -> EndpointResult { let mut crate_name = req.params()["crate_id"].clone(); let version = req.params()["version"].as_str(); - let mut log_metadata = None; - match req.db_conn() { - Ok(conn) => { - use self::versions::dsl::*; - - // Returns the crate name as stored in the database, or an error if we could - // not load the version ID from the database. - let (version_id, canonical_crate_name) = app - .instance_metrics - .downloads_select_query_execution_time - .observe_closure_duration(|| { - versions - .inner_join(crates::table) - .select((id, crates::name)) - .filter(Crate::with_name(&crate_name)) - .filter(num.eq(version)) - .first::<(i32, String)>(&*conn) - })?; - - if canonical_crate_name != crate_name { - app.instance_metrics - .downloads_non_canonical_crate_name_total - .inc(); - log_metadata = Some(("bot", "dl")); - } - crate_name = canonical_crate_name; - - // The increment does not happen instantly, but it's deferred to be executed in a batch - // along with other downloads. See crate::downloads_counter for the implementation. - app.downloads_counter.increment(version_id); + // When no database connection is ready unconditional redirects will be performed. This could + // happen if the pool is not healthy or if an operator manually configured the application to + // always perform unconditional redirects (for example as part of the mitigations for an + // outage). See the comments below for a description of what unconditional redirects do. + let conn = if app.config.force_unconditional_redirects { + None + } else { + match req.db_conn() { + Ok(conn) => Some(conn), + Err(PoolError::UnhealthyPool) => None, + Err(err) => return Err(err.into()), } - Err(PoolError::UnhealthyPool) => { - // The download endpoint is the most critical route in the whole crates.io application, - // as it's relied upon by users and automations to download crates. Keeping it working - // is the most important thing for us. - // - // The endpoint relies on the database to fetch the canonical crate name (with the - // right capitalization and hyphenation), but that's only needed to serve clients who - // don't call the endpoint with the crate's canonical name. - // - // Thankfully Cargo always uses the right name when calling the endpoint, and we can - // keep it working during a full database outage by unconditionally redirecting without - // checking whether the crate exists or the rigth name is used. Non-Cargo clients might - // get a 404 response instead of a 500, but that's worth it. - // - // Without a working database we also can't count downloads, but that's also less - // critical than keeping Cargo downloads operational. + }; + let mut log_metadata = None; + if let Some(conn) = &conn { + use self::versions::dsl::*; + + // Returns the crate name as stored in the database, or an error if we could + // not load the version ID from the database. + let (version_id, canonical_crate_name) = app + .instance_metrics + .downloads_select_query_execution_time + .observe_closure_duration(|| { + versions + .inner_join(crates::table) + .select((id, crates::name)) + .filter(Crate::with_name(&crate_name)) + .filter(num.eq(version)) + .first::<(i32, String)>(&**conn) + })?; + + if canonical_crate_name != crate_name { app.instance_metrics - .downloads_unconditional_redirects_total + .downloads_non_canonical_crate_name_total .inc(); - log_metadata = Some(("unconditional_redirect", "true")); + log_metadata = Some(("bot", "dl")); } - Err(err) => return Err(err.into()), + crate_name = canonical_crate_name; + + // The increment does not happen instantly, but it's deferred to be executed in a batch + // along with other downloads. See crate::downloads_counter for the implementation. + app.downloads_counter.increment(version_id); + } else { + // The download endpoint is the most critical route in the whole crates.io application, + // as it's relied upon by users and automations to download crates. Keeping it working + // is the most important thing for us. + // + // The endpoint relies on the database to fetch the canonical crate name (with the + // right capitalization and hyphenation), but that's only needed to serve clients who + // don't call the endpoint with the crate's canonical name. + // + // Thankfully Cargo always uses the right name when calling the endpoint, and we can + // keep it working during a full database outage by unconditionally redirecting without + // checking whether the crate exists or the rigth name is used. Non-Cargo clients might + // get a 404 response instead of a 500, but that's worth it. + // + // Without a working database we also can't count downloads, but that's also less + // critical than keeping Cargo downloads operational. + + app.instance_metrics + .downloads_unconditional_redirects_total + .inc(); + log_metadata = Some(("unconditional_redirect", "true")); } + // Ensure the connection is released to the pool as soon as possible, as the download endpoint + // covers the majority of our traffic and we don't want it to starve other requests. + drop(conn); + let redirect_url = req .app() .config diff --git a/src/tests/krate/downloads.rs b/src/tests/krate/downloads.rs index b45e6c708de..d66bcb77ce7 100644 --- a/src/tests/krate/downloads.rs +++ b/src/tests/krate/downloads.rs @@ -105,3 +105,34 @@ fn download_noncanonical_crate_name() { anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download") .assert_redirect_ends_with("/crates/foo_download/foo_download-1.0.0.crate"); } + +#[test] +fn force_unconditional_redirect() { + let (app, anon, user) = TestApp::init() + .with_config(|config| { + config.force_unconditional_redirects = true; + }) + .with_user(); + + app.db(|conn| { + CrateBuilder::new("foo-download", user.as_model().id) + .version(VersionBuilder::new("1.0.0")) + .expect_build(conn); + }); + + // Any redirect to an existing crate and version works correctly. + anon.get::<()>("/api/v1/crates/foo-download/1.0.0/download") + .assert_redirect_ends_with("/crates/foo-download/foo-download-1.0.0.crate"); + + // Redirects to crates with wrong capitalization are performed unconditionally. + anon.get::<()>("/api/v1/crates/Foo_downloaD/1.0.0/download") + .assert_redirect_ends_with("/crates/Foo_downloaD/Foo_downloaD-1.0.0.crate"); + + // Redirects to missing versions are performed unconditionally. + anon.get::<()>("/api/v1/crates/foo-download/2.0.0/download") + .assert_redirect_ends_with("/crates/foo-download/foo-download-2.0.0.crate"); + + // Redirects to missing crates are performed unconditionally. + anon.get::<()>("/api/v1/crates/bar-download/1.0.0/download") + .assert_redirect_ends_with("/crates/bar-download/bar-download-1.0.0.crate"); +} diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index 12ef95d8aa5..1e6ec426bc9 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -334,6 +334,7 @@ fn simple_config() -> config::Server { metrics_authorization_token: None, use_test_database_pool: true, instance_metrics_log_every_seconds: None, + force_unconditional_redirects: false, } }