Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add API for crates.io to trigger rebuilds #2534

Merged
merged 8 commits into from
Jul 11, 2024
Merged

add API for crates.io to trigger rebuilds #2534

merged 8 commits into from
Jul 11, 2024

Conversation

pflanze
Copy link
Contributor

@pflanze pflanze commented Jun 24, 2024 •
edited by syphar
Loading

relates to #2442.

  • adds config variable DOCSRS_TRIGGER_REBUILD_TOKEN /
    Config.trigger_rebuild_token

  • adds build_trigger_rebuild_handler and route
    "/crate/:name/:version/rebuild"

Note: does not yet contain any kind of rate limiting!

@pflanze pflanze requested a review from a team as a code owner June 24, 2024 13:28
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jun 24, 2024
syphar
syphar requested changes Jun 24, 2024
View reviewed changes
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some first comments.

I'm not 100% sure about the Json/APi error design, but I need a less tired moment to think about it.

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 24, 2024
@syphar syphar changed the title Rebuild api add API for crates.io to trigger crate rebuilds Jun 24, 2024
@syphar syphar changed the title add API for crates.io to trigger crate rebuilds add API for crates.io to trigger rebuilds Jun 24, 2024
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 25, 2024
@pflanze
web/builds: add crate_version_exists
8fc46ab 
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 25, 2024
@pflanze
web/builds: add crate_version_exists
5cd562c 
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
@pflanze
Copy link
Contributor Author

pflanze commented Jun 25, 2024

I think I've handled all of your suggestions, but I forgot about Clippy. I'll need to get back to that another evening.

@pflanze pflanze requested a review from syphar June 25, 2024 19:42
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 25, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jun 25, 2024

Oh build failures:

error: set DATABASE_URL to use query macros online, or run cargo sqlx prepare to update the query cache

Not sure right now what to do about this (need to stop for tonight).

@syphar
Copy link
Member

syphar commented Jun 26, 2024

Oh build failures:

error: set DATABASE_URL to use query macros online, or run cargo sqlx prepare to update the query cache

Not sure right now what to do about this (need to stop for tonight).

I've put the necessary command into our Justfile, so calling just sqlx-prepare is fine

syphar
syphar requested changes Jun 26, 2024
View reviewed changes
@syphar
Copy link
Member

syphar commented Jun 26, 2024

Thank you for all the work! This will be awesome for crate authors

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 26, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jun 27, 2024

I've so far avoided to force-push and instead pushed 3 commits (those with subject lines starting with "sq", standing for squash) with changes that I will squash into the earlier commits where they belong; the idea is that this makes it easier for you to see what I changed; this assumes that diffing is easier as a last step after I do the squashing together with the right commits and force-pushing at the end. Let me know once you're happy and I'll do that. And if that was useful or rather a complication.

@pflanze pflanze requested a review from syphar June 27, 2024 05:33
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 27, 2024
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 27, 2024
@pflanze
web/builds: add crate_version_exists
449ec34 
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
@pflanze
Copy link
Contributor Author

pflanze commented Jun 27, 2024

I've realized that you can still see the commit from before me squashing the commits anyway and have now force-pushed db8b363...b7deb0d (i.e. if you'd like to see the incremental changes I did easily, look at db8b363, for the state I wish to be merged b7deb0d).

@pflanze
Copy link
Contributor Author

pflanze commented Jun 30, 2024

I've pushed 3 commits with (a) the 'optimized' SQL query, FWIW, (b)+(c) add and use the constant_time_eq crate for token comparisons. Without it, it would likely be possible for attackers to find the token via response times.

syphar
syphar approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really awesome, thank you for the continued work on this.

I'll do some local testing and then merge & deploy this.

( and probably already start using it to queue rebuilds )

@syphar
Copy link
Member

syphar commented Jul 10, 2024

@pflanze do you want to squash some commits before I merge?

pflanze added 8 commits July 11, 2024 09:28
@pflanze
preparations for API to request rebuild of a crate version
bb0212b 
- web: `axum_cached_redirect`: change return type to be concrete
- web/routes: add `post_internal`
- test: add `TestFrontent::post`
@pflanze
web/error: add JsonAxumNope for JSON APIs
aaf7ea5 
This aims to satisfy these wishes:

- Use the same error enum (`AxumNope`) for both handlers returning
  HTML or JSON.

- Yet allow those handlers to specify whether HTML or JSON should be
  returned.

- Not change the exising code returning `AxumNope`--still convert to
  HTML by default.

Because `AxumNope` also contains a case (`Search`) that is fixed to
producing HTML, but shouldn't ever be used by handlers returning JSON,
that works with a runtime error in case the latter assumption isn't
being followed.

The approach is to add a new wrapper around `AxumNope` called
`JsonAxumNope` and matching type def `JsonAxumResult`. The wrapper
also implements `IntoResponse`, but produces JSON. Endpoints wishing
to return JSON errors need to use that wrapper and specify
`JsonAxumResult` as their return type.
@pflanze
web/error: add AxumNope::Unauthorized
cf58d70
@pflanze
web/builds: add API to request rebuild of a crate version
464a4c0 
This resolves rust-lang#2442.

- adds config variable `DOCSRS_TRIGGER_REBUILD_TOKEN` /
  `Config.trigger_rebuild_token`

- adds `build_trigger_rebuild_handler` and route
  "/crate/:name/:version/rebuild"

Note: does not yet contain any kind of rate limiting!
@pflanze
web/builds: add crate_version_exists to query the database directly
005a072
@pflanze
web/error: simplify JsonAxumNope implementation
d5e6da8 
This removes the intermediate `ErrorResponse` type and instead pattern
matches AxumNope on both levels, using a new function
`redirect_with_policy` to avoid duplication.

(Saves 11 lines of code, 7 comment lines, and the indirection via
intermediate type, but adds the function instead.)
@pflanze
Cargo,web/builds: use constant_time_eq for security
247d86b
@pflanze
Run "cargo sqlx prepare"
108356a
@pflanze
Copy link
Contributor Author

pflanze commented Jul 11, 2024

@pflanze do you want to squash some commits before I merge?

@syphar, yes, some more squashing was warranted. I think now it's at the proper resolution.

@syphar syphar merged commit 2eb0516 into rust-lang:master Jul 11, 2024
8 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jul 11, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jul 11, 2024

This looks really awesome, thank you for the continued work on this.

Thank you, too, for your help during https://rustfest.ch/ and your feedback afterwards! It was the perfect introduction.

Now I just need to get a bit more time.

@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Jul 12, 2024
@syphar syphar mentioned this pull request Jul 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syphar syphar syphar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pflanze @syphar