diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..a51aeec --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,14 @@ +version: 2 +updates: + - package-ecosystem: cargo + directory: "/" + schedule: + interval: daily + time: "07:00" + open-pull-requests-limit: 10 + - package-ecosystem: "github-actions" + directory: ".github/workflows" + schedule: + interval: "daily" + time: "07:00" + open-pull-requests-limit: 10 diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml new file mode 100644 index 0000000..36eed5d --- /dev/null +++ b/.github/workflows/rust.yml @@ -0,0 +1,69 @@ +name: Rust + +on: + push: + pull_request: + +jobs: + format: + name: Format + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt + - name: Check formatting + run: cargo fmt --all -- --check + + clippy: + name: Clippy + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - name: Checkout sources + uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: clippy + - run: cargo clippy --locked -- --deny warnings + + docs: + name: Check for documentation errors + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - name: Checkout sources + uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + - name: cargo doc + run: cargo doc --locked --no-deps --document-private-items + env: + RUSTDOCFLAGS: -Dwarnings + + stable: + name: Stable Rust + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - uses: actions/checkout@v4 + - name: Install stable rust toolchain + uses: dtolnay/rust-toolchain@stable + - name: Install valgrind + run: sudo apt-get install -y valgrind + - name: Build + run: cargo build --locked --verbose + - name: Run tests + run: cargo test --locked --verbose diff --git a/ci-bench-runner/Cargo.lock b/ci-bench-runner/Cargo.lock index f28690e..17936f4 100644 --- a/ci-bench-runner/Cargo.lock +++ b/ci-bench-runner/Cargo.lock @@ -557,21 +557,6 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" -[[package]] -name = "foreign-types" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" -dependencies = [ - "foreign-types-shared", -] - -[[package]] -name = "foreign-types-shared" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" - [[package]] name = "form_urlencoded" version = "1.2.0" @@ -945,19 +930,6 @@ dependencies = [ "tokio-io-timeout", ] -[[package]] -name = "hyper-tls" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6183ddfa99b85da61a140bea0efc93fdf56ceaa041b37d553518030827f9905" -dependencies = [ - "bytes", - "hyper", - "native-tls", - "tokio", - "tokio-native-tls", -] - [[package]] name = "iana-time-zone" version = "0.1.58" @@ -1188,24 +1160,6 @@ dependencies = [ "windows-sys", ] -[[package]] -name = "native-tls" -version = "0.2.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07226173c32f2926027b63cce4bcd8076c3552846cbe7925f3aaffeac0a3b92e" -dependencies = [ - "lazy_static", - "libc", - "log", - "openssl", - "openssl-probe", - "openssl-sys", - "schannel", - "security-framework", - "security-framework-sys", - "tempfile", -] - [[package]] name = "nom" version = "7.1.3" @@ -1347,50 +1301,12 @@ version = "1.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dd8b5dd2ae5ed71462c540258bedcb51965123ad7e7ccf4b9a8cafaa4a63576d" -[[package]] -name = "openssl" -version = "0.10.58" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a9dfc0783362704e97ef3bd24261995a699468440099ef95d869b4d9732f829a" -dependencies = [ - "bitflags 2.4.1", - "cfg-if", - "foreign-types", - "libc", - "once_cell", - "openssl-macros", - "openssl-sys", -] - -[[package]] -name = "openssl-macros" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.38", -] - [[package]] name = "openssl-probe" version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" -[[package]] -name = "openssl-sys" -version = "0.9.94" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f55da20b29f956fb01f0add8683eb26ee13ebe3ebd935e49898717c6b4b2830" -dependencies = [ - "cc", - "libc", - "pkg-config", - "vcpkg", -] - [[package]] name = "overload" version = "0.1.1" @@ -1671,12 +1587,10 @@ dependencies = [ "http-body", "hyper", "hyper-rustls", - "hyper-tls", "ipnet", "js-sys", "log", "mime", - "native-tls", "once_cell", "percent-encoding", "pin-project-lite", @@ -1687,7 +1601,6 @@ dependencies = [ "serde_urlencoded", "system-configuration", "tokio", - "tokio-native-tls", "tokio-rustls", "tower-service", "url", @@ -2526,16 +2439,6 @@ dependencies = [ "syn 2.0.38", ] -[[package]] -name = "tokio-native-tls" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbae76ab933c85776efabc971569dd6119c580d8f5d448769dec1764bf796ef2" -dependencies = [ - "native-tls", - "tokio", -] - [[package]] name = "tokio-rustls" version = "0.24.1" diff --git a/ci-bench-runner/Cargo.toml b/ci-bench-runner/Cargo.toml index 6e0cb48..49a6b93 100644 --- a/ci-bench-runner/Cargo.toml +++ b/ci-bench-runner/Cargo.toml @@ -27,5 +27,5 @@ uuid = { version = "1.4.1", features = ["v4", "serde"] } [dev-dependencies] ctor = "0.2.5" -reqwest = "0.11.22" +reqwest = { version = "0.11.22", default-features = false, features = ["rustls-tls-webpki-roots"] } wiremock = "0.5.19" diff --git a/ci-bench-runner/build.rs b/ci-bench-runner/build.rs new file mode 100644 index 0000000..3026ceb --- /dev/null +++ b/ci-bench-runner/build.rs @@ -0,0 +1,7 @@ +fn main() { + // In some cases if new migrations are added without accompanying Rust src changes + // `cargo build` isn't smart enough to detect the need for recompilation to pick up + // the new embedded migrations. This build script is the recommended workaround. + // See + println!("cargo:rerun-if-changed=migrations"); +} diff --git a/ci-bench-runner/src/db.rs b/ci-bench-runner/src/db.rs index 7b3ab7d..8af5503 100644 --- a/ci-bench-runner/src/db.rs +++ b/ci-bench-runner/src/db.rs @@ -12,6 +12,7 @@ use tokio::sync::Mutex; use uuid::Uuid; /// An enqueued GitHub event +#[derive(Debug)] pub struct QueuedEvent { /// An internal id for this event (i.e. not GitHub's) pub id: Uuid, @@ -63,7 +64,7 @@ pub struct BenchJob { } /// A result for a specific benchmark scenario -#[derive(sqlx::FromRow)] +#[derive(Debug, sqlx::FromRow)] pub struct BenchResult { /// The scenario's name pub scenario_name: String, @@ -75,6 +76,7 @@ pub struct BenchResult { } /// The results of a comparison between two branches of rustls +#[derive(Debug)] pub struct ComparisonResult { /// The diffs, per scenario pub diffs: Vec, @@ -123,7 +125,7 @@ impl TryFrom for ScenarioKind { fn try_from(value: i64) -> Result { match value { - 0 => Ok(ScenarioKind::Icount), + 0 => Ok(Self::Icount), kind => bail!("invalid scenario kind: {kind}"), } } diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 5481f63..e72bbf5 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -1,3 +1,4 @@ +use std::fmt::{Debug, Formatter}; use std::path::PathBuf; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -41,9 +42,8 @@ impl EventQueue { ) -> Self { let (worker_tx, event_enqueued_rx) = tokio::sync::mpsc::unbounded_channel(); - let active_job_id = Arc::new(Mutex::new(None)); let queue = Self { - active_job_id, + active_job_id: Arc::new(Mutex::new(None)), event_enqueued_tx: worker_tx, db, }; @@ -53,18 +53,17 @@ impl EventQueue { config, bench_runner, octocrab, - ); - queue + ) } /// Starts and supervises the background queue processing task fn start_and_supervise_queue_processing( - &self, + self, event_enqueued_rx: UnboundedReceiver<()>, config: Arc, bench_runner: Arc, octocrab: CachedOctocrab, - ) { + ) -> Self { let active_job_id = self.active_job_id.clone(); let queue = self.clone(); let event_enqueued_rx = Arc::new(tokio::sync::Mutex::new(event_enqueued_rx)); @@ -77,7 +76,7 @@ impl EventQueue { octocrab.clone(), ); - info!("background task started for event queue"); + info!("background task started for event queue job {active_job_id:?}"); match background_task.await { Ok(Ok(_)) => { // The task finished normally, no need to restart it @@ -87,14 +86,14 @@ impl EventQueue { // The task finished with an error error!( cause = e.to_string(), - "job queue background task errored, restarting in 1s" + "job queue background task {active_job_id:?} errored, restarting in 1s" ); } Err(e) => { // The task panicked or was cancelled error!( cause = e.to_string(), - "job queue background task crashed, restarting in 1s" + "job queue background task {active_job_id:?} crashed, restarting in 1s" ); } } @@ -103,6 +102,8 @@ impl EventQueue { tokio::time::sleep(Duration::from_secs(1)).await; } }); + + self } /// Spawns a tokio task to process queued events in the background. @@ -236,16 +237,14 @@ pub enum AllowedEvent { } impl AllowedEvent { - fn from_event_string(event: &str) -> Option { - let kind = match event { - "issue_comment" => AllowedEvent::IssueComment, - "push" => AllowedEvent::Push, - "pull_request" => AllowedEvent::PullRequest, - "pull_request_review" => AllowedEvent::PullRequestReview, + fn from_event_string(event: &str) -> Option { + Some(match event { + "issue_comment" => Self::IssueComment, + "push" => Self::Push, + "pull_request" => Self::PullRequest, + "pull_request_review" => Self::PullRequestReview, _ => return None, - }; - - Some(kind) + }) } } @@ -266,7 +265,17 @@ pub struct JobContext<'a> { pub db: Db, } -#[derive(Serialize, Deserialize)] +impl<'a> Debug for JobContext<'a> { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("JobContext") + .field("event", &self.event) + .field("job_id", &self.job_id) + .field("job_output_dir", &self.job_output_dir) + .finish() + } +} + +#[derive(Debug, Serialize, Deserialize)] pub struct JobView { #[serde(with = "time::serde::rfc3339")] pub created_utc: OffsetDateTime, diff --git a/ci-bench-runner/src/github.rs b/ci-bench-runner/src/github.rs index d692a97..e1e1519 100644 --- a/ci-bench-runner/src/github.rs +++ b/ci-bench-runner/src/github.rs @@ -6,7 +6,6 @@ use hmac::{Hmac, Mac}; use jsonwebtoken::EncodingKey; use octocrab::models::{InstallationId, StatusState}; use octocrab::Octocrab; -use sha2::digest::FixedOutput; use sha2::Sha256; use tracing::{error, trace, warn}; @@ -19,21 +18,21 @@ pub mod api { use octocrab::models::pulls::PullRequest; use serde::Deserialize; - #[derive(Deserialize)] + #[derive(Debug, Clone, PartialEq, Deserialize)] pub struct PullRequestReviewEvent { pub action: String, pub review: Review, pub pull_request: PullRequest, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Review { pub author_association: String, pub state: String, pub commit_id: String, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct PushEvent { #[serde(rename = "ref")] pub git_ref: String, @@ -42,44 +41,45 @@ pub mod api { pub deleted: bool, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct CommentEvent { pub action: String, pub comment: Comment, pub issue: Issue, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Comment { pub author_association: String, pub body: String, pub user: GitHubUser, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Issue { pub number: u64, pub pull_request: Option, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct PullRequestLite { pub url: String, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct GitHubUser { pub login: String, + pub id: u64, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Repo { pub clone_url: String, } } /// Provides access to an authenticated `Octocrab` client -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct CachedOctocrab { /// Initial GitHub client, authenticated as our GitHub App app_client: Octocrab, @@ -210,15 +210,8 @@ pub fn verify_webhook_signature(body: &[u8], signature: &str, secret: &str) -> b // Safe to unwrap because any key is valid let mut mac = Hmac::::new_from_slice(secret.as_bytes()).unwrap(); - mac.update(body); - let output = mac.finalize_fixed(); - trace!( - "verifying webhook signature; provided = {signature}; computed = {:?}", - hex::encode(output.as_slice()) - ); - - signature_bytes == output.as_slice() + mac.verify_slice(signature_bytes.as_slice()).is_ok() } #[cfg(test)] diff --git a/ci-bench-runner/src/job/bench_pr.rs b/ci-bench-runner/src/job/bench_pr.rs index 081e0a9..40a494f 100644 --- a/ci-bench-runner/src/job/bench_pr.rs +++ b/ci-bench-runner/src/job/bench_pr.rs @@ -63,8 +63,8 @@ pub async fn handle_issue_comment(ctx: JobContext<'_>) -> anyhow::Result<()> { trace!("ignoring event for `{}` action", payload.action); return Ok(()); } - if payload.comment.user.login == "rustls-bench" { - trace!("ignoring comment from rustls-bench"); + if payload.comment.user.id == ctx.config.github_app_id { + trace!("ignoring comment from ourselves"); return Ok(()); } if !ALLOWED_AUTHOR_ASSOCIATIONS.contains(&payload.comment.author_association.as_str()) { @@ -426,18 +426,19 @@ fn calculate_significance_thresholds(historical_results: Vec) -> Ha outlier_bounds } -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct PrBranches { pub baseline: CommitIdentifier, pub candidate: CommitIdentifier, } +#[derive(Debug)] struct BenchPrError { error: anyhow::Error, logs: BenchPrLogs, } -#[derive(Default)] +#[derive(Debug, Default)] struct BenchPrLogs { base: Vec, candidate: Vec, diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index 735513d..1b7b378 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -43,7 +43,7 @@ struct AppState { } /// The application's configuration -#[derive(Deserialize)] +#[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct AppConfig { /// Base URL used for the GitHub API (used to mock out the API in tests) pub github_api_url_override: Option, @@ -142,24 +142,24 @@ async fn get_cachegrind_diff( ) -> axum::response::Result { // Extract commit hashes from URL let mut commit_parts = compared_commits.split(':'); - let baseline_commit = commit_parts.next().ok_or("malformed URL")?; - let candidate_commit = commit_parts.next().ok_or("malformed URL")?; - if commit_parts.next().is_some() { - Err((StatusCode::BAD_REQUEST, "malformed URL"))?; - } + let (baseline_commit, candidate_commit) = match ( + commit_parts.next(), + commit_parts.next(), + commit_parts.next(), + ) { + (Some(baseline), Some(candidate), None) => (baseline, candidate), + _ => Err((StatusCode::BAD_REQUEST, "malformed URL"))?, + }; - let result = state + Ok(state .db .cachegrind_diff(baseline_commit, candidate_commit, &scenario_name) .await - .map_err(|_| "internal server error")?; - - let result = result.ok_or(( - StatusCode::NOT_FOUND, - "comparison not found for the provided commit hashes and scenario", - ))?; - - Ok(result) + .map_err(|_| "internal server error")? + .ok_or(( + StatusCode::NOT_FOUND, + "comparison not found for the provided commit hashes and scenario", + ))?) } /// Handles an incoming GitHub webhook @@ -181,7 +181,7 @@ async fn handle_github_webhook( }; if !verify_webhook_signature(&body, signature, &state.config.webhook_secret) { - trace!("invalid signature, ignoring event"); + trace!("{WEBHOOK_SIGNATURE_HEADER} invalid signature, ignoring event"); return StatusCode::BAD_REQUEST; } @@ -222,7 +222,7 @@ pub static WEBHOOK_SIGNATURE_HEADER: &str = "X-Hub-Signature-256"; pub static WEBHOOK_EVENT_HEADER: &str = "X-GitHub-Event"; /// Identifies a specific commit in a repository -#[derive(Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] pub struct CommitIdentifier { /// The URL at which the repository can be cloned pub clone_url: String, diff --git a/ci-bench-runner/src/runner.rs b/ci-bench-runner/src/runner.rs index a83c1ce..73e0a05 100644 --- a/ci-bench-runner/src/runner.rs +++ b/ci-bench-runner/src/runner.rs @@ -21,6 +21,7 @@ pub trait BenchRunner: Send + Sync { } /// A bench runner that runs benchmarks locally +#[derive(Debug)] pub struct LocalBenchRunner; impl BenchRunner for LocalBenchRunner { @@ -161,6 +162,7 @@ fn run_command(mut command: Command, logs: &mut Vec) -> anyhow::Result<()> } /// Logs for a specific command +#[derive(Debug)] pub struct Log { /// The command in question pub command: String, @@ -178,17 +180,13 @@ pub fn write_logs_for_run(s: &mut String, logs: &[Log]) { } for log in logs { - write_log(s, log); + write_log_part(s, "command", &log.command); + write_log_part(s, "cwd", &log.cwd); + write_log_part(s, "stdout", &String::from_utf8_lossy(&log.stdout)); + write_log_part(s, "stderr", &String::from_utf8_lossy(&log.stderr)); } } -fn write_log(s: &mut String, log: &Log) { - write_log_part(s, "command", &log.command); - write_log_part(s, "cwd", &log.cwd); - write_log_part(s, "stdout", &String::from_utf8_lossy(&log.stdout)); - write_log_part(s, "stderr", &String::from_utf8_lossy(&log.stderr)); -} - fn write_log_part(s: &mut String, part_name: &str, part: &str) { write!(s, "{part_name}:").ok(); if part.trim().is_empty() { diff --git a/flake.nix b/flake.nix index bcd7e83..1c83ad3 100644 --- a/flake.nix +++ b/flake.nix @@ -15,7 +15,7 @@ cargoToml = builtins.fromTOML (builtins.readFile "${crateDir}/Cargo.toml"); - devDeps = with pkgs; [ ansible ansible-lint ]; + devDeps = with pkgs; [ ansible ansible-lint valgrind ]; mkDevShell = rustc: pkgs.mkShell { @@ -35,9 +35,7 @@ packages.ci-bench-runner = pkgs.rustPlatform.buildRustPackage { inherit (cargoToml.package) name version; src = crateDir; - cargoLock = { - lockFile = "${crateDir}/Cargo.lock"; - }; + cargoLock = { lockFile = "${crateDir}/Cargo.lock"; }; doCheck = false; # Some tests require platform certs. };