search

web-platform-tests / wpt.live

A live version of the web-platform-tests project
https://wpt.live/
15 stars 11 forks source link

wptpr.live - Deployment of PRs from forks #27

Closed stephenmcgruer closed 3 years ago

stephenmcgruer commented 4 years ago

wprpr.live - Deployment of PRs from forks

Status: In-Review Authors: smcgruer@chromium.org Reviewers: @Hexcles, @jugglinmike Last Updated: 2020-02-07

Background

The original design of wptpr.live uses a pair of GitHub Actions to deploy pull requests to the server. The first action, pull_request_previews.yml, runs periodically and is responsible for determining:

  1. which pull requests should be mirrored,
  2. creating special refs for those pull requests (which wptpr.live detects and begins mirroring), and
  3. creating a GitHub deployment for those pull requests.

The second action, detect_pull_request_preview.yml, triggers on GitHub deployment events and is responsible for monitoring the mirroring on wptpr.live and updating the GitHub pull request with the status and target URL once the deployment is done. Polling wptpr.live was chosen as it has to be considered 'untrusted' (it hosts and executes user code), and so cannot be given the necessary GitHub tokens that would be required for it to proactively update GitHub with the mirroring status.

Unfortunately, after deploying this system it was discovered that GitHub was failing to send deployment events for deployments created for pull requests that originated from forks. Interestingly it was determined that GitHub Webhooks were still being sent a deployment event in this case. A support ticket to GitHub has so far (~4 business days at time of writing) gone unanswered.

Mirroring pull requests from forks was one of the primary purposes of wptpr.live; the design allows authorized users of wpt to label PRs from external contributors (who usually contribute from forks) as safe to be mirrored. The reviewer can then examine the newly added or modified tests at wptpr.live/[pr_number].

Proposed Fix - Periodic polling for deployments

The proposed fix is fairly simple; switch the detect_pull_request_preview.yml script to operate on a periodic basis instead of being triggered from deployment events. Instead of being passed information about a specific deployment, the script would fetch the list of deployments via GitHub APIs, and determine the status of each. It has been verified (via a manual deployment to this PR) that deployments created for pull requests from forks do show up in the list.

For each deployment, the script would check the set of associated statuses. If the list is empty or only contains 'pending' status events, then the script would check the status of wptpr.live and update the deployment as appropriate.

One downside to this approach is an increase in calls to the GitHub APIs. This fix adds one request to fetch the list of deployments, and then a call per deployment to fetch the statuses. The system today already regularly hits API limits, so those issues will likely have to be addressed before this fix can be made.

Alternatives Considered

Utilize wpt-pr-bot

As deployment events appear to be sent to webhooks without problem, we could utilize the wpt-pr-bot service (which is trusted) to handle deployment status updates. This would involve deleting the detect_pull_request_preview.yml script and associated code, and migrating the functionality to wpt-pr-bot.

This has the upside of not increasing the number of API calls being made. It has the downside of further complicating the wpt-pr-bot codebase and increasing our reliance on a bot that we had actually hoped to remove in favour of GitHub Actions.

Wait for GitHub to fix their 'bug'

At this point it is unknown whether GitHub consider not sending deployment events for pull requests from forks a bug or intended behavior. GitHub support tends to be good once they answer, but their latency can be considerable (O(weeks) is common). We could wait to see if GitHub consider this a bug, at which point we could further wait for them to fix it.

Disallow usage of wptpr.live for PRs from forks

One obvious option would be to just disallow mirroring to wptpr.live for pull requests from forks. This seems to violate one of the main goals of the project (as noted in the background section), but I note that our original goal was to provide the same service as w3c-test.org, and this comment appears to imply that w3c-test.org only mirrors patches from trusted persons. Such people are always able to make their pull requests from branches on WPT itself, and do not need to use forks (though some prefer forks).

Given there are reasonable ways to address the problem, this seems like a bad choice, but included for posterity.

stephenmcgruer commented 4 years ago

@Hexcles - PTAL.

@jugglinmike - thought you might also have thoughts, but no requirement if you don't have time :)

stephenmcgruer commented 4 years ago

Note: on 2020-02-12 I received a reply from GitHub support saying they have passed the bug report onto 'the team', but there is no guarantee of a fix or when it would be.

stephenmcgruer commented 4 years ago

Ping @Hexcles

Hexcles commented 4 years ago

Regarding API rate limits:

The alternative of utilizing wpt-pr-bot simply shifts the load from one bot to another, right? And there is a simpler way to do that: we could use different tokens for various tasks.

Otherwise, this LGTM. Remember to incorporate this into the top-level README when you send out the PR. And sorry for the delay.

stephenmcgruer commented 4 years ago

I received a reply from GitHub!

Hi Stephen,

You shared a report with us back in February around deployments and GitHub Actions.

We found that this specific issue occurs whenever a deployment is created from an unreachable commit.

We consider a commit to be "unreachable" if it's not referenced by any branches or tags in the repository. For example, this can happen when a branch or tag no longer exists due to deletion or as a result of a force-push. In addition, commits that originate in a fork of the repository are treated as unreachable.

Based on this context, our team deployed a change that would indicate this in the workflow run's messaging whenever a deployment is triggered for an unreachable commit:

> This run was triggered by the commit COMMIT_SHA which is not referenced by any branches or tags in this repository. It is either an orphaned commit or from a fork of this repository.

We hope this helps explain things. Again, we're really sorry for the inconvenience this has caused, but we hope this change serves as an improvement to your workflow on GitHub.com.

Please let us know if you have any questions for us!

I am replying to GitHub Support to make sure, but I think this means that the current status is considered working-as-intended.

It does also make me wonder if we could make this work by creating branches or tags in wpt proper for PRs from forks. I might experiment with that.

Hexcles commented 4 years ago

It does also make me wonder if we could make this work by creating branches or tags in wpt proper for PRs from forks. I might experiment with that.

I'd caution that. This effectively means you'll be pushing untrusted code to wpt on behalf of people who otherwise don't have write access to wpt. This would invalidate some key security assumptions. For example https://help.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token

stephenmcgruer commented 4 years ago

I'd caution that. This effectively means you'll be pushing untrusted code to wpt on behalf of people who otherwise don't have write access to wpt. This would invalidate some key security assumptions. For example https://help.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token

A very good point. Consider the suggestion redacted :)

foolip commented 3 years ago

https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#pull_request_target was added to GitHub Actions after we implemented mirroring of forks, and solved the secrets problem. With that, fixing this may be simple. One would have to be careful to not run any code from the fork in the workflow, however.

Hexcles commented 3 years ago

Well, that's great news. Though we'd probably need to scrape the existing workflows :)

The key part is:

except that it runs in the context of the base repository of the pull request, rather than in the merge commit. This means that you can more safely make your secrets available to the workflows triggered by the pull request, because only workflows defined in the commit on the base repository are run

I think we can create a single workflow listening to pull_request_target events, which can then synchronously push to wptpr.live and create/update a deployment on the PR. No need for periodic polling or listening to deployment.

foolip commented 3 years ago

Indeed, as long as we only copy files and never run any of the code, this might now be very simple.

stephenmcgruer commented 3 years ago

No need for periodic polling or listening to deployment.

Are you suggesting modifying the logic on the wpt.live side to avoid polling (it currently runs a polling-like script every 60s fetching origin + checking refs), or are you just referring to the current polling nature of the cronjob? The latter I agree is trivial, changing the former would require more of a rethink.

Hexcles commented 3 years ago

The cron job: https://github.com/web-platform-tests/wpt/blob/e5790de656f6108bc87c1f7d2771c3277300d35d/.github/workflows/pull_request_previews.yml#L4

TrueBrain commented 3 years ago

I just wanted to drop by and say: thank you for this detailed thread!

I ran into the exact same issue when doing something similar for OpenTTD; searching for the error I got didn't result much in terms of official documentation, but this issue gave me the exact information I needed :D Pretty sure I otherwise never would have found pull_request_target. Most excellent :)

So nothing to add to this thread, except a sincere: thank you!