diff options
| -rw-r--r-- | etc/brep-module.conf | 2 | ||||
| -rw-r--r-- | etc/private/install/brep-module.conf | 23 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.cxx | 5 | ||||
| -rw-r--r-- | mod/mod-ci-github-gh.hxx | 12 | ||||
| -rw-r--r-- | mod/mod-ci-github-gq.cxx | 26 | ||||
| -rw-r--r-- | mod/mod-ci-github-service-data.hxx | 4 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 143 | ||||
| -rw-r--r-- | mod/mod-ci-github.hxx | 21 | ||||
| -rw-r--r-- | mod/module.cli | 3 |
9 files changed, 133 insertions, 106 deletions
diff --git a/etc/brep-module.conf b/etc/brep-module.conf index 37220a3..0143434 100644 --- a/etc/brep-module.conf +++ b/etc/brep-module.conf @@ -459,7 +459,7 @@ menu About=?about # The GitHub App's configured webhook secret. If not set, then the GitHub CI -# service is disabled. +# service is disabled. Note: make sure to choose a strong (random) secret. # # ci-github-app-webhook-secret diff --git a/etc/private/install/brep-module.conf b/etc/private/install/brep-module.conf index 8cf546f..e1f921d 100644 --- a/etc/private/install/brep-module.conf +++ b/etc/private/install/brep-module.conf @@ -461,6 +461,29 @@ submit-handler-timeout 120 # ci-handler-timeout +# The GitHub App ID. Found in the app's settings on GitHub. +# +# ci-github-app-id + + +# The GitHub App's configured webhook secret. If not set, then the GitHub CI +# service is disabled. Note: make sure to choose a strong (random) secret. +# +# ci-github-app-webhook-secret + + +# The private key used during GitHub API authentication. Created in the GitHub +# App's settings. +# +# ci-github-app-private-key + + +# The number of seconds a JWT (authentication token) should be valid for. The +# maximum allowed by GitHub is 10 minutes. +# +# ci-github-jwt-validity-period 600 + + # The directory to save upload data to for the specified upload type. If # unspecified, the build artifacts upload functionality will be disabled for # this type. diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx index fc5cf82..f0de991 100644 --- a/mod/mod-ci-github-gh.cxx +++ b/mod/mod-ci-github-gh.cxx @@ -356,7 +356,7 @@ namespace brep { p.next_expect (event::begin_object); - bool ni (false), nm (false), fn (false), cu (false); + bool ni (false), fn (false), cu (false); // Skip unknown/uninteresting members. // @@ -368,14 +368,12 @@ namespace brep }; if (c (ni, "node_id")) node_id = p.next_expect_string (); - else if (c (nm, "name")) name = p.next_expect_string (); else if (c (fn, "full_name")) path = p.next_expect_string (); else if (c (cu, "clone_url")) clone_url = p.next_expect_string (); else p.next_expect_value_skip (); } if (!ni) missing_member (p, "gh_repository", "node_id"); - if (!nm) missing_member (p, "gh_repository", "name"); if (!fn) missing_member (p, "gh_repository", "full_name"); if (!cu) missing_member (p, "gh_repository", "clone_url"); } @@ -384,7 +382,6 @@ namespace brep operator<< (ostream& os, const gh_repository& rep) { os << "node_id: " << rep.node_id - << ", name: " << rep.name << ", path: " << rep.path << ", clone_url: " << rep.clone_url; diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx index eeda871..392c0e8 100644 --- a/mod/mod-ci-github-gh.hxx +++ b/mod/mod-ci-github-gh.hxx @@ -21,8 +21,6 @@ namespace butl namespace brep { - // @@@ Check if any data members are unused (once the dust settles). - using build_queued_hints = tenant_service_build_queued::build_queued_hints; // GitHub request/response types (all start with gh_). @@ -87,15 +85,12 @@ namespace brep string node_id; unsigned int number; - // @@ TMP The unused base/head members may be useful for trace output when - // we receive the pull_request webhook. - string base_path; // Repository path (<org>/<repo>) under github.com. - string base_ref; // @@ TODO Remove if remains unused. - string base_sha; // @@ TODO Remove if remains unused. + string base_ref; + string base_sha; string head_path; - string head_ref; // @@ TODO Remove if remains unused. + string head_ref; string head_sha; explicit @@ -133,7 +128,6 @@ namespace brep struct gh_repository { string node_id; - string name; string path; // Repository path (<org>/<repo>) under github.com. string clone_url; diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx index 7b7e464..774eeed 100644 --- a/mod/mod-ci-github-gq.cxx +++ b/mod/mod-ci-github-gq.cxx @@ -17,9 +17,11 @@ namespace brep // bottom). // static const string& gq_name (const string&); + static string gq_name (string&&); static string gq_str (const string&); static string gq_bool (bool); static const string& gq_enum (const string&); + static string gq_enum (string&&); [[noreturn]] static void throw_json (json::parser& p, const string& m) @@ -273,11 +275,6 @@ namespace brep // Note that GitHub won't allow us to change a built check run to // any other state (but all other transitions are allowed). // - // @@ Are we handling the case where the resulting state (built) - // differs from what we expect? - // - // @@@ Does built-to-built transition updates status? - // if (rst != st && rst != build_state::built) { error << "unexpected check_run status: received '" << rcr.status @@ -830,8 +827,6 @@ namespace brep // // Return the name or throw invalid_argument if it is invalid. // - // @@ TODO: dangerous API. - // static const string& gq_name (const string& v) { @@ -850,6 +845,13 @@ namespace brep return v; } + static string + gq_name (string&& v) + { + gq_name (v); + return move (v); + } + // Serialize a string to GraphQL. // // Return the serialized string or throw invalid_argument if the string is @@ -904,8 +906,6 @@ namespace brep // // Return the enum value or throw invalid_argument if it is invalid. // - // @@ TODO: dangerous API. - // static const string& gq_enum (const string& v) { @@ -914,4 +914,12 @@ namespace brep return gq_name (v); } + + static string + gq_enum (string&& v) + { + gq_enum (v); + return move (v); + } + } diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx index 776ec8d..0f4c760 100644 --- a/mod/mod-ci-github-service-data.hxx +++ b/mod/mod-ci-github-service-data.hxx @@ -11,8 +11,6 @@ namespace brep { - // @@@ Check if any data members are unused (once the dust settles). - // Service data associated with the tenant (corresponds to GH check suite). // // It is always a top-level JSON object and the first member is always the @@ -99,7 +97,7 @@ namespace brep // The following two are only used for pull requests. // - // @@ TODO/LATER: maybe put them in a struct? + // @@ TODO/LATER: maybe put them in a struct, if more members? // optional<string> pr_node_id; optional<uint32_t> pr_number; diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 113da2e..394638a 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -19,26 +19,6 @@ #include <stdexcept> -// @@ Remaining TODOs -// -// - Rerequested checks -// -// - check_suite (action: rerequested): received when user re-runs all -// checks. -// -// - check_run (action: rerequested): received when user re-runs a -// specific check or all failed checks. -// -// @@ TMP I have confirmed that the above is accurate. -// -// Will need to extract a few more fields from check_runs, but the layout -// is very similar to that of check_suite. -// -// - Choose strong webhook secret (when deploying). -// -// - Check that delivery UUID has not been received before (replay attack). -// - // Resources: // // Creating an App: @@ -280,9 +260,6 @@ namespace brep // is that we want be "notified" of new actions at which point we can // decide whether to ignore them or to handle. // - // @@ There is also check_run even (re-requested by user, either - // individual check run or all the failed check runs). - // if (event == "check_suite") { gh_check_suite_event cs; @@ -316,13 +293,10 @@ namespace brep else if (cs.action == "completed") { // GitHub thinks that "all the check runs in this check suite have - // completed and a conclusion is available". Looks like this one we - // ignore? + // completed and a conclusion is available". Check with our own + // bookkeeping and log an error if there is a mismatch. // - // What if our bookkeeping says otherwise? But then we can't even - // access the service data easily here. @@ TODO: maybe/later. - // - return true; + return handle_check_suite_completed (move (cs), warning_success); } else { @@ -558,10 +532,6 @@ namespace brep l3 ([&]{trace << "installation_access_token { " << *iat << " }";}); - // @@ What happens if we call this functions with an already existing - // node_id (e.g., replay attack). See the UUID header above. - // - // While it would have been nice to cancel CIs of PRs with this branch as // base not to waste resources, there are complications: Firstly, we can // only do this for remote PRs (since local PRs will most likely share the @@ -690,6 +660,23 @@ namespace brep return true; } + bool ci_github:: + handle_check_suite_completed (gh_check_suite_event cs, bool warning_success) + { + // The plans is as follows: + // + // 1. Load the service data. + // + // 2. Verify it is completed. + // + // 3. Verify (like in build_built()) that all the check runs are + // completed. + // + // 4. Verify the result matches what GitHub thinks it is (if easy). + + return true; + } + // Create a gq_built_result. // // Throw invalid_argument in case of invalid result_status. @@ -795,6 +782,7 @@ namespace brep // archived. // service_data sd; + string tenant_id; { // Service id that uniquely identifies the CI tenant. // @@ -868,6 +856,8 @@ namespace brep { fail << "failed to parse service data: " << e; } + + tenant_id = d->tenant_id; } else { @@ -1015,7 +1005,8 @@ namespace brep // only if the build is actually restarted). // auto update_sd = [&error, &new_iat, &race, - &cr, &bcr, &ccr] (const string& /*tenant_id*/, + tenant_id = move (tenant_id), + &cr, &bcr, &ccr] (const string& ti, const tenant_service& ts, build_state) -> optional<string> { @@ -1024,6 +1015,16 @@ namespace brep race = false; // Reset. + if (tenant_id != ti) + { + // The tenant got replaced since we loaded it but we managed to + // trigger a rebuild in the new tenant. Who knows whose check runs are + // visible, so let's fail ours similar to the cases below. + // + race = true; + return nullopt; + } + service_data sd; try { @@ -1320,9 +1321,8 @@ namespace brep return true; } - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: - build_unloaded (const string& /*tenant_id*/, + function<optional<string> (const string&, const tenant_service&)> ci_github:: + build_unloaded (const string& ti, tenant_service&& ts, const diag_epilogue& log_writer) const noexcept { @@ -1342,12 +1342,11 @@ namespace brep } return sd.pre_check - ? build_unloaded_pre_check (move (ts), move (sd), log_writer) - : build_unloaded_load (move (ts), move (sd), log_writer); + ? build_unloaded_pre_check (move (ts), move (sd), log_writer) + : build_unloaded_load (ti, move (ts), move (sd), log_writer); } - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: + function<optional<string> (const string&, const tenant_service&)> ci_github:: build_unloaded_pre_check (tenant_service&& ts, service_data&& sd, const diag_epilogue& log_writer) const noexcept @@ -1547,9 +1546,9 @@ namespace brep return nullptr; } - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: - build_unloaded_load (tenant_service&& ts, + function<optional<string> (const string&, const tenant_service&)> ci_github:: + build_unloaded_load (const string& tenant_id, + tenant_service&& ts, service_data&& sd, const diag_epilogue& log_writer) const noexcept try @@ -1751,15 +1750,19 @@ namespace brep return nullptr; // Nothing to save (but potentially retry on next call). return [&error, + tenant_id, iat = move (new_iat), cni = move (conclusion_node_id)] - (const string& /*tenant_id*/, + (const string& ti, const tenant_service& ts) -> optional<string> { // NOTE: this lambda may be called repeatedly (e.g., due to // transaction being aborted) and so should not move out of its // captures. + if (tenant_id != ti) + return nullopt; // Do nothing if the tenant has been replaced. + service_data sd; try { @@ -1812,9 +1815,9 @@ namespace brep // them when notifying GitHub. The first is not important (we expect the // state to go back to building shortly). The second should normally not // happen and would mean that a completed check suite may go back on its - // conclusion (which would be pretty confusing for the user). @@@ This - // can/will happen on check run rebuild. Distinguish between internal - // and external rebuilds? + // conclusion (which would be pretty confusing for the user). Note that + // the ->queued state transition of a check run rebuild triggered by + // us is handled directly in handle_check_run_rerequest(). // // So, for GitHub notifications, we only have the following linear // transition sequence: @@ -1891,9 +1894,8 @@ namespace brep // if we have node_id, then we update, otherwise, we create (potentially // overriding the check run created previously). // - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: - build_queued (const string& /*tenant_id*/, + function<optional<string> (const string&, const tenant_service&)> ci_github:: + build_queued (const string& tenant_id, const tenant_service& ts, const vector<build>& builds, optional<build_state> istate, @@ -2019,16 +2021,20 @@ namespace brep } } - return [bs = move (bs), + return [tenant_id, + bs = move (bs), iat = move (new_iat), crs = move (crs), error = move (error), - warn = move (warn)] (const string& /*tenant_id*/, + warn = move (warn)] (const string& ti, const tenant_service& ts) -> optional<string> { // NOTE: this lambda may be called repeatedly (e.g., due to transaction // being aborted) and so should not move out of its captures. + if (tenant_id != ti) + return nullopt; // Do nothing if the tenant has been replaced. + service_data sd; try { @@ -2077,9 +2083,8 @@ namespace brep return nullptr; } - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: - build_building (const string& /*tenant_id*/, + function<optional<string> (const string&, const tenant_service&)> ci_github:: + build_building (const string& tenant_id, const tenant_service& ts, const build& b, const diag_epilogue& log_writer) const noexcept @@ -2187,15 +2192,19 @@ namespace brep } } - return [iat = move (new_iat), + return [tenant_id, + iat = move (new_iat), cr = move (*cr), error = move (error), - warn = move (warn)] (const string& /*tenant_id*/, + warn = move (warn)] (const string& ti, const tenant_service& ts) -> optional<string> { // NOTE: this lambda may be called repeatedly (e.g., due to transaction // being aborted) and so should not move out of its captures. + if (tenant_id != ti) + return nullopt; // Do nothing if the tenant has been replaced. + service_data sd; try { @@ -2241,9 +2250,8 @@ namespace brep return nullptr; } - function<optional<string> (const string& tenant_id, - const tenant_service&)> ci_github:: - build_built (const string& /*tenant_id*/, + function<optional<string> (const string&, const tenant_service&)> ci_github:: + build_built (const string& tenant_id, const tenant_service& ts, const build& b, const diag_epilogue& log_writer) const noexcept @@ -2253,9 +2261,8 @@ namespace brep NOTIFICATION_DIAG (log_writer); - // @@ TODO Include service_data::event_node_id and perhaps ts.id in - // diagnostics? E.g. when failing to update check runs we print the - // build ID only. + // @@ TODO Include ts.id in diagnostics? Check run build ids alone seem + // kind of meaningless. Log lines get pretty long this way however. service_data sd; try @@ -2569,16 +2576,20 @@ namespace brep } } - return [iat = move (new_iat), + return [tenant_id, + iat = move (new_iat), cr = move (cr), completed = completed, error = move (error), - warn = move (warn)] (const string& /*tenant_id*/, + warn = move (warn)] (const string& ti, const tenant_service& ts) -> optional<string> { // NOTE: this lambda may be called repeatedly (e.g., due to transaction // being aborted) and so should not move out of its captures. + if (tenant_id != ti) + return nullopt; // Do nothing if the tenant has been replaced. + service_data sd; try { diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx index 9b9814e..104f889 100644 --- a/mod/mod-ci-github.hxx +++ b/mod/mod-ci-github.hxx @@ -42,26 +42,23 @@ namespace brep virtual const cli::options& cli_options () const {return options::ci_github::description ();} - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + virtual function<optional<string> (const string&, const tenant_service&)> build_unloaded (const string& tenant_id, tenant_service&&, const diag_epilogue& log_writer) const noexcept override; - function<optional<string> (const string& tenant_id, - const tenant_service&)> + function<optional<string> (const string&, const tenant_service&)> build_unloaded_pre_check (tenant_service&&, service_data&&, const diag_epilogue&) const noexcept; - function<optional<string> (const string& tenant_id, - const tenant_service&)> - build_unloaded_load (tenant_service&&, + function<optional<string> (const string&, const tenant_service&)> + build_unloaded_load (const string& tenant_id, + tenant_service&&, service_data&&, const diag_epilogue&) const noexcept; - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + virtual function<optional<string> (const string&, const tenant_service&)> build_queued (const string& tenant_id, const tenant_service&, const vector<build>&, @@ -69,15 +66,13 @@ namespace brep const build_queued_hints&, const diag_epilogue& log_writer) const noexcept override; - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + virtual function<optional<string> (const string&, const tenant_service&)> build_building (const string& tenant_id, const tenant_service&, const build&, const diag_epilogue& log_writer) const noexcept override; - virtual function<optional<string> (const string& tenant_id, - const tenant_service&)> + virtual function<optional<string> (const string&, const tenant_service&)> build_built (const string& tenant_id, const tenant_service&, const build&, diff --git a/mod/module.cli b/mod/module.cli index ca750e9..274ecd4 100644 --- a/mod/module.cli +++ b/mod/module.cli @@ -860,7 +860,8 @@ namespace brep { "<secret>", "The GitHub App's configured webhook secret. If not set, then the - GitHub CI service is disabled." + GitHub CI service is disabled. Note: make sure to choose a strong + (random) secret." } path ci-github-app-private-key |