Auto merge of #3884 - pietroalbini:pa-force-unconditional-redirects, r=Turbo87

Add `FORCE_UNCONDITIONAL_REDIRECTS`

This PR adds a new environment variable, `FORCE_UNCONDITIONAL_REDIRECTS`, which forces unconditional redirects even with a fully healthy database. The environment variable is meant to be used by an on-call operator to reduce the database load when the primary database is overloaded, and should only be set during incidents.

Author: bors

src/config.rs

@@ -27,6 +27,7 @@ pub struct Server {
     pub metrics_authorization_token: Option<String>,
     pub use_test_database_pool: bool,
     pub instance_metrics_log_every_seconds: Option<u64>,
+    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(),
         }
     }
 }

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

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");
+}

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,
     }
 }