diff --git a/src/app.rs b/src/app.rs index 6d56a488cae..132858d2da5 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1,9 +1,10 @@ //! Application-wide components in a struct accessible from each request -use crate::{db, util::CargoResult, Config, Env}; +use crate::{db, Config, Env}; use std::{path::PathBuf, sync::Arc, time::Duration}; use diesel::r2d2; +use reqwest::Client; use scheduled_thread_pool::ScheduledThreadPool; /// The `App` struct holds the main components of the application like @@ -26,17 +27,24 @@ pub struct App { /// The server configuration pub config: Config, + + /// A configured client for outgoing HTTP requests + /// + /// In production this shares a single connection pool across requests. In tests + /// this is either None (in which case any attempt to create an outgoing connection + /// will panic) or a `Client` configured with a per-test replay proxy. + http_client: Option, } impl App { - /// Creates a new `App` with a given `Config` + /// Creates a new `App` with a given `Config` and an optional HTTP `Client` /// /// Configures and sets up: /// /// - GitHub OAuth /// - Database connection pools - /// - A `git2::Repository` instance from the index repo checkout (that server.rs ensures exists) - pub fn new(config: &Config) -> App { + /// - Holds an HTTP `Client` and associated connection pool, if provided + pub fn new(config: &Config, http_client: Option) -> App { let mut github = oauth2::Config::new( &config.gh_client_id, &config.gh_client_secret, @@ -90,19 +98,22 @@ impl App { session_key: config.session_key.clone(), git_repo_checkout: config.git_repo_checkout.clone(), config: config.clone(), + http_client, } } /// Returns a client for making HTTP requests to upload crate files. /// - /// The handle will go through a proxy if the uploader being used has specified one, which - /// is only done in tests with `TestApp::with_proxy()` in order to be able to record and - /// inspect the HTTP requests that tests make. - pub fn http_client(&self) -> CargoResult { - let mut builder = reqwest::Client::builder(); - if let Some(proxy) = self.config.uploader.proxy() { - builder = builder.proxy(reqwest::Proxy::all(proxy)?); - } - Ok(builder.build()?) + /// The client will go through a proxy if the application was configured via + /// `TestApp::with_proxy()`. + /// + /// # Panics + /// + /// Panics if the application was not initialized with a client. This should only occur in + /// tests that were not properly initialized. + pub fn http_client(&self) -> &Client { + self.http_client + .as_ref() + .expect("No HTTP client is configured. In tests, use `TestApp::with_proxy()`.") } } diff --git a/src/bin/render-readmes.rs b/src/bin/render-readmes.rs index ea636401a33..e3fa976e544 100644 --- a/src/bin/render-readmes.rs +++ b/src/bin/render-readmes.rs @@ -21,6 +21,7 @@ use chrono::{TimeZone, Utc}; use diesel::{dsl::any, prelude::*}; use docopt::Docopt; use flate2::read::GzDecoder; +use reqwest::Client; use tar::{self, Archive}; const DEFAULT_PAGE_SIZE: usize = 25; @@ -94,6 +95,8 @@ fn main() { total_pages + 1 }; + let client = Client::new(); + for (page_num, version_ids_chunk) in version_ids.chunks(page_size).enumerate() { println!( "= Page {} of {} ==================================", @@ -117,9 +120,10 @@ fn main() { krate_name, version.num ) }); + let client = client.clone(); let handle = thread::spawn(move || { println!("[{}-{}] Rendering README...", krate_name, version.num); - let readme = get_readme(&config, &version, &krate_name); + let readme = get_readme(&config, &client, &version, &krate_name); if readme.is_none() { return; } @@ -127,12 +131,7 @@ fn main() { let readme_path = format!("readmes/{0}/{0}-{1}.html", krate_name, version.num); config .uploader - .upload( - &reqwest::Client::new(), - &readme_path, - readme.into_bytes(), - "text/html", - ) + .upload(&client, &readme_path, readme.into_bytes(), "text/html") .unwrap_or_else(|_| { panic!( "[{}-{}] Couldn't upload file to S3", @@ -151,12 +150,17 @@ fn main() { } /// Renders the readme of an uploaded crate version. -fn get_readme(config: &Config, version: &Version, krate_name: &str) -> Option { +fn get_readme( + config: &Config, + client: &Client, + version: &Version, + krate_name: &str, +) -> Option { let location = config .uploader - .crate_location(krate_name, &version.num.to_string())?; + .crate_location(krate_name, &version.num.to_string()); - let mut response = match reqwest::get(&location) { + let mut response = match client.get(&location).send() { Ok(r) => r, Err(err) => { println!( diff --git a/src/bin/server.rs b/src/bin/server.rs index 8d97081f657..954625be6dc 100644 --- a/src/bin/server.rs +++ b/src/bin/server.rs @@ -9,6 +9,7 @@ use std::{ use civet::Server as CivetServer; use conduit_hyper::Service as HyperService; +use reqwest::Client; enum Server { Civet(CivetServer), @@ -24,7 +25,9 @@ fn main() { env_logger::init(); let config = cargo_registry::Config::default(); - let app = App::new(&config); + let client = Client::new(); + + let app = App::new(&config, Some(client)); let app = cargo_registry::build_handler(Arc::new(app)); // On every server restart, ensure the categories available in the database match diff --git a/src/config.rs b/src/config.rs index 31aaf7ec527..44b030370aa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -68,7 +68,6 @@ impl Default for Config { &api_protocol, ), cdn: dotenv::var("S3_CDN").ok(), - proxy: None, } } (Env::Production, Replica::ReadOnlyMirror) => { @@ -89,7 +88,6 @@ impl Default for Config { &api_protocol, ), cdn: dotenv::var("S3_CDN").ok(), - proxy: None, } } // In Development mode, either running as a primary instance or a read-only mirror @@ -109,7 +107,6 @@ impl Default for Config { &api_protocol, ), cdn: dotenv::var("S3_CDN").ok(), - proxy: None, } } else { // If we don't set the `S3_BUCKET` variable, we'll use a development-only diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 63eeb9000cd..726d377a8d6 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -169,8 +169,7 @@ pub fn readme(req: &mut dyn Request) -> CargoResult { .app() .config .uploader - .readme_location(crate_name, version) - .ok_or_else(|| human("crate readme not found"))?; + .readme_location(crate_name, version); if req.wants_json() { #[derive(Serialize)] diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index df0053c4e22..c7db7749610 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -24,8 +24,7 @@ pub fn download(req: &mut dyn Request) -> CargoResult { .app() .config .uploader - .crate_location(crate_name, version) - .ok_or_else(|| human("crate files not found"))?; + .crate_location(crate_name, version); if req.wants_json() { #[derive(Serialize)] diff --git a/src/github.rs b/src/github.rs index 731010bf2a7..f5b1a877407 100644 --- a/src/github.rs +++ b/src/github.rs @@ -20,8 +20,7 @@ where let url = format!("{}://api.github.com{}", app.config.api_protocol, url); info!("GITHUB HTTP: {}", url); - let client = app.http_client()?; - client + app.http_client() .get(&url) .header(header::ACCEPT, "application/vnd.github.v3+json") .header( diff --git a/src/tests/all.rs b/src/tests/all.rs index 35dea7a5aff..d5f4129aa7b 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -29,6 +29,7 @@ use std::{ use conduit::Request; use conduit_test::MockRequest; use diesel::prelude::*; +use reqwest::{Client, Proxy}; use url::Url; macro_rules! t { @@ -113,6 +114,13 @@ fn app() -> ( conduit_middleware::MiddlewareBuilder, ) { let (proxy, bomb) = record::proxy(); + let (app, handler) = simple_app(Some(proxy)); + (bomb, app, handler) +} + +fn simple_app(proxy: Option) -> (Arc, conduit_middleware::MiddlewareBuilder) { + git::init(); + let uploader = Uploader::S3 { bucket: s3::Bucket::new( String::from("alexcrichton-test"), @@ -123,16 +131,9 @@ fn app() -> ( // sniff/record it, but everywhere else we use https "http", ), - proxy: Some(proxy), cdn: None, }; - let (app, handler) = simple_app(uploader); - (bomb, app, handler) -} - -fn simple_app(uploader: Uploader) -> (Arc, conduit_middleware::MiddlewareBuilder) { - git::init(); let config = Config { uploader, session_key: "test this has to be over 32 bytes long".to_string(), @@ -149,7 +150,17 @@ fn simple_app(uploader: Uploader) -> (Arc, conduit_middleware::MiddlewareBu // sniff/record it, but everywhere else we use https api_protocol: String::from("http"), }; - let app = App::new(&config); + + let client = if let Some(proxy) = proxy { + let mut builder = Client::builder(); + builder = builder + .proxy(Proxy::all(&proxy).expect("Unable to configure proxy with the provided URL")); + Some(builder.build().expect("TLS backend cannot be initialized")) + } else { + None + }; + + let app = App::new(&config, client); t!(t!(app.diesel_database.get()).begin_test_transaction()); let app = Arc::new(app); let handler = cargo_registry::build_handler(Arc::clone(&app)); diff --git a/src/tests/util.rs b/src/tests/util.rs index 333fc453190..623b8474e77 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -26,7 +26,6 @@ use crate::{ use cargo_registry::{ middleware::current_user::AuthenticationSource, models::{ApiToken, User}, - uploaders::Uploader, App, }; use diesel::PgConnection; @@ -48,7 +47,7 @@ pub struct TestApp(Rc); impl TestApp { /// Initialize an application with an `Uploader` that panics pub fn init() -> TestAppBuilder { - let (app, middle) = crate::simple_app(Uploader::Panic); + let (app, middle) = crate::simple_app(None); let inner = Rc::new(TestAppInner { app, _bomb: None, diff --git a/src/uploaders.rs b/src/uploaders.rs index 0dbfe22e079..80b98519cf2 100644 --- a/src/uploaders.rs +++ b/src/uploaders.rs @@ -14,10 +14,6 @@ use crate::app::App; use crate::middleware::app::RequestApp; use crate::models::Crate; -fn require_test_app_with_proxy() -> ! { - panic!("No uploader is configured. In tests, use `TestApp::with_proxy()`."); -} - #[derive(Clone, Debug)] pub enum Uploader { /// For production usage, uploads and redirects to s3. @@ -25,32 +21,18 @@ pub enum Uploader { S3 { bucket: s3::Bucket, cdn: Option, - proxy: Option, }, /// For development usage only: "uploads" crate files to `dist` and serves them /// from there as well to enable local publishing and download Local, - - /// For tests using `TestApp::init()` - /// Attempts to get an outgoing HTTP handle will panic. - Panic, } impl Uploader { - pub fn proxy(&self) -> Option<&str> { - match *self { - Uploader::S3 { ref proxy, .. } => proxy.as_ref().map(String::as_str), - Uploader::Local => None, - Uploader::Panic => require_test_app_with_proxy(), - } - } - /// Returns the URL of an uploaded crate's version archive. /// /// The function doesn't check for the existence of the file. - /// It returns `None` if the current `Uploader` is `Panic`. - pub fn crate_location(&self, crate_name: &str, version: &str) -> Option { + pub fn crate_location(&self, crate_name: &str, version: &str) -> String { match *self { Uploader::S3 { ref bucket, @@ -62,18 +44,16 @@ impl Uploader { None => bucket.host(), }; let path = Uploader::crate_path(crate_name, version); - Some(format!("https://{}/{}", host, path)) + format!("https://{}/{}", host, path) } - Uploader::Local => Some(format!("/{}", Uploader::crate_path(crate_name, version))), - Uploader::Panic => require_test_app_with_proxy(), + Uploader::Local => format!("/{}", Uploader::crate_path(crate_name, version)), } } /// Returns the URL of an uploaded crate's version readme. /// /// The function doesn't check for the existence of the file. - /// It returns `None` if the current `Uploader` is `Panic`. - pub fn readme_location(&self, crate_name: &str, version: &str) -> Option { + pub fn readme_location(&self, crate_name: &str, version: &str) -> String { match *self { Uploader::S3 { ref bucket, @@ -85,10 +65,9 @@ impl Uploader { None => bucket.host(), }; let path = Uploader::readme_path(crate_name, version); - Some(format!("https://{}/{}", host, path)) + format!("https://{}/{}", host, path) } - Uploader::Local => Some(format!("/{}", Uploader::readme_path(crate_name, version))), - Uploader::Panic => require_test_app_with_proxy(), + Uploader::Local => format!("/{}", Uploader::readme_path(crate_name, version)), } } @@ -103,7 +82,7 @@ impl Uploader { format!("readmes/{}/{}-{}.html", name, name, version) } - /// Uploads a file using the configured uploader (either `S3`, `Local` or `Panic`). + /// Uploads a file using the configured uploader (either `S3`, `Local`). /// /// It returns a a tuple containing the path of the uploaded file /// and its checksum. @@ -130,7 +109,6 @@ impl Uploader { file.write_all(&body)?; Ok((filename.to_str().map(String::from), hash)) } - Uploader::Panic => require_test_app_with_proxy(), } } @@ -150,7 +128,7 @@ impl Uploader { let mut body = Vec::new(); LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut body)?; verify_tarball(krate, vers, &body, maximums.max_unpack_size)?; - self.upload(&app.http_client()?, &path, body, "application/x-tar")? + self.upload(app.http_client(), &path, body, "application/x-tar")? }; // We create the bomb for the crate file before uploading the readme so that if the // readme upload fails, the uploaded crate file is automatically deleted. @@ -160,12 +138,7 @@ impl Uploader { }; let (readme_path, _) = if let Some(rendered) = readme { let path = Uploader::readme_path(&krate.name, &vers.to_string()); - self.upload( - &app.http_client()?, - &path, - rendered.into_bytes(), - "text/html", - )? + self.upload(app.http_client(), &path, rendered.into_bytes(), "text/html")? } else { (None, vec![]) }; @@ -183,14 +156,13 @@ impl Uploader { fn delete(&self, app: &Arc, path: &str) -> CargoResult<()> { match *self { Uploader::S3 { ref bucket, .. } => { - bucket.delete(&app.http_client()?, path)?; + bucket.delete(app.http_client(), path)?; Ok(()) } Uploader::Local => { fs::remove_file(path)?; Ok(()) } - Uploader::Panic => require_test_app_with_proxy(), } } }