web-platform-tests / wpt.live

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

Mirroring pull requests #2

Closed jugglinmike closed 4 years ago

jugglinmike commented 6 years ago

w3c-test.org (occasionally) responds to pull requests in this repositoryWPT by mirroring the branch to a subdirectory. While considering improvements to that project, we noted how that approach invalidates tests which hard-code their own path. For now, we're not going to attempt any sort of mirroring, but I'd like to get some clarity on the approach we plan to take in the future.

Although deploying to dynamically-created subdomains would mitigate some of these problems, it would also significantly increase the complexity of the system. Given that only some pull requests use the mirroring feature, and only some of the tests in those pull requests will be effected by a change to their path, I'm starting to wonder if the complexity is worth it.

In the use case we're supporting (i.e. patch demonstration), contributors are interested in demonstrating the behavior of tests that they are actively modifying. If any one of those tests is invalidated by being relocated to a subdirectory, the going advice could be, "remove the hard-coded path from the test." Doing so would not only save us the hassle of the "dynamic subdomain" solution, it would also make WPT a little more resilient (and able to support those rare cases of subdirectory renames). I believe .well-known may be the only case where introducing a subdirectory will make testing technically impossible.

So the question is (finally): which should we plan to do in the future (e.g. Q4 2018 or beyond):

jugglinmike commented 6 years ago

There's another pattern used throughout WPT that discourages deployment to subdirectories: root-relative paths, e.g.

<script src="/resources/testharness.js"></script>

This is not invalid according to any of WPT's documentation, and I don't think that it ought to be. However, for patches that modify those resources, the "preview" deployment will fail in a way that may confuse folks since in many cases, a file will still be loaded--just the wrong file.

A full-fledged subdomain-based deployment would not suffer from this problem.

jugglinmike commented 5 years ago

Some of the limitations of w3c-test.org's directory-based solution can be classified as affecting test authors. Those mostly involves directory issues like root-relative pathing and the /.well-known directory.

The other limitations concern server maintainers. This is mostly due to running untrusted Python code; a bad actor could perform service denial through resource over-utilization, or they could leak private information (limited in this case to the server's TLS certificate).

I sketched out a solution that uses Docker to isolate servers and deploy each pull request to a dedicated set of ports. Needless to say, it's pretty convoluted. I don't think the above concerns warrant the additional complexity of a more advanced solution.

For these reasons, I'm now in favor of replicating w3c-test.org's directory-based solution. (ASCII charts of the Docker monstrosity are available on request.)

tkellen commented 5 years ago

I would love to see the ASCII charts, but only if they are nearly effortless to provide!

jugglinmike commented 5 years ago

Oh, you

ASCII This solution will make submissions available at URLs like http://submissions.web-platform-tests.live:12331, and that is a lot less memorable than, say, http://pr1233.web-platform-tests.live. Given the audience (web platform developers) and the medium (hyperlinks posted to GitHub comments), I think this is acceptable. .---------------. .------------------------------------------------------. | Let's Encrypt | | submissions.web-platform-tests.live | '---------------' | | ^ | D-----------D | '-- ACME -->== 80 =====| certifier | ----------------. | | D-----------D | | | | | .--------. | D---------D | | | GitHub |---- WebHooks ----->| control | | | | |------ code ------->| |-----------. | | | |<--- comments ------| |--. | | | '--------' | D---------D | | | | | v | | | Legend | /var/run/docker.sock v | | | | /mnt/wpt.git v | == xxx == | | | /etc/letsencrypt/ | forwarded | D---------D | | | | TCP port | | pr#1234 |<-+--------+-------+ | number XXX == 12340 ==| | | | | | == 12341 ==| | | | | | D---D | D---------D | | | | | | | | | | | D---D | D---------D | | | | Docker | | pr#1233 |<-+--------+-------+ | container == 12330 ==| | | | | | == 12331 ==| | | | | | .---. | D---------D | | | | | | | v v v | '---' | etc. | server '------------------------------------------------------'
jugglinmike commented 5 years ago

Here's what I'd like to focus on for a minimal-viable product:

  1. [x] Support nonexistent.host (see https://github.com/bocoup/infrastructure-web-platform/pull/12)
  2. [ ] Automate the retrieval of a "wildcard" TLS certificate from Let's Encrypt
  3. [ ] Internal monitoring and recovery

All of the above will benefit the main deployment of web-platform-tests.live as well. The submissions feature needs one additional step:

Nice-to-haves:

jugglinmike commented 5 years ago

Automate the retrieval of a "wildcard" TLS certificate from Let's Encrypt

I implemented this via gh-16, but ultimately decided against it (more detail on the pull request). I've once again fallen back to the simplest possible solution: gh-17.

Internal monitoring and recovery

This is done via gh-18. We'll see if it results in better performance according to the monitoring systems we have in place (both AWS and StatusCake).

That just leaves a re-implementation of the mirroring script. Plus the nice-to-have tasks, of course.

jugglinmike commented 5 years ago

@gsnedders @foolip @sideshowbarker I'm experimenting with reporting availability via GitHub's "commit status" API as suggested here, but after simulating the interaction, I'm concerned that it's too subtle.

Here are a set of screenshots for discovering the URL The discovery process would start with recognizing that there are 10 successful checks: ![screenshot from 2019-02-15 13-02-02](https://user-images.githubusercontent.com/677252/52877715-c7ad7f80-3128-11e9-9e7b-c9a5ce386af8.png) Expanding that: ![screenshot from 2019-02-15 13-01-37](https://user-images.githubusercontent.com/677252/52877723-cc723380-3128-11e9-88b8-ee46fe02ba37.png) And scrolling to the bottom: ![screenshot from 2019-02-15 13-01-19](https://user-images.githubusercontent.com/677252/52877731-d3994180-3128-11e9-9382-dfbfa989fca3.png)

I'm doubtful that most contributors will find this. Posting a comment on the issue would be far more effective, but we've had complaints about comment noise from frequent contributors.

We could make a special note of the date on which we deploy the mirroring feature. Then, whenever we mirror a pull request, we can also check if this is the first pull request submitted by the author since the initial deployment date. We'd only comment in that case, and since we know this is will be the user's introduction, we can make the explanation a lot more meaningful:

The contents of this pull request are available for a limited time at the following URL:

http://submissions.web-platform-tests.live/1234

We do this automatically for all members of the web-platform-tests organization on GitHub. This is a one-time automated message; if you'd like to find the link for your next pull request, check the GitHub commit status.

In terms of complexity, this would only require one additional request to the GitHub API, e.g.

https://api.github.com/search/issues?q=type:pr+author:capncrunch+repo:web-platform-tests/wpt+created:>2019-02-15

What do you think?

domenic commented 5 years ago

FWIW I discovered the live wpt.fyi results, which are shown as a commit status, without any prompting or messages. I think commit statuses would be fine for this purpose as well.

jugglinmike commented 5 years ago

I know you to have particularly strong attention to detail, so I'm not surprised :) Besides the less focused long-time contributors, I'm also thinking about folks who aren't so familiar with GitHub in general (hence the link to GitHub's docs). Would you excuse a one-time comment like what I've posted above (in addition to the status itself)?

sideshowbarker commented 5 years ago

I'm experimenting with reporting availability via GitHub's "commit status" API as suggested here, but after simulating the interaction, I'm concerned that it's too subtle.

Here are a set of screenshots for discovering the URL I'm doubtful that most contributors will find this.

I agree the discoverability of it in the GitHub UI is poor. And while that’s true for anything other than the first 5 or 6 items that fit into that UI, I think it’s not as important for those to be discoverable as it is for this to be.

So I’m supportive of trying out the “The contents of this pull request are available for a limited time at the following URL:… We do this automatically for all members of the web-platform-tests organization on GitHub.” one-time comment thing — given the discoverability benefit of it.

If after we try it it turns out that some of us end up feeling it’s generating excessive/intolerable noise, then at that time we could drop it and re-plan.

foolip commented 5 years ago

@jugglinmike can you elaborate on the use of port number in https://github.com/bocoup/web-platform-tests.live/issues/2#issuecomment-462503635? The latest PR is now 15567, would the port number have some relationship to PR number? Using 2 ports per PR we'd only have about 25000 to go, which we'll probably reach in the lifetime of the project. Is some kind of port recycling the intention?

Would a single VM be running up to X staging PRs, what is X, and what happens when more PRs are requested for staging?

I also wonder about the submissions.web-platform-tests.live certificates. Since wptserve uses the certificate, can anyone with access to staging PRs extract the submissions.web-platform-tests.live certificate by modifying wptserve?

jugglinmike commented 5 years ago

@jugglinmike can you elaborate on the use of port number in #2 (comment)? The latest PR is now 15567, would the port number have some relationship to PR number? Using 2 ports per PR we'd only have about 25000 to go, which we'll probably reach in the lifetime of the project. Is some kind of port recycling the intention?

The diagram simplified port assignment a bit. Each pull request would actually need something like 6 ports (two HTTP, one HTTPS, one WS, one WSS, and soon one HTTP/2). In any case, you're right: we'd want to use some modular arithmetic.

Would a single VM be running up to X staging PRs, what is X, and what happens when more PRs are requested for staging?

I don't have a sense for the upper limit, but it's something we could gauge experimentally. Traffic probably wouldn't be a limiting factor because each instance will have a viewership in the single digits. When we reached the limit, we'd have to destroy existing instances, possibly leaving a comment to alert the affected contributors.

I also wonder about the submissions.web-platform-tests.live certificates. Since wptserve uses the certificate, can anyone with access to staging PRs extract the submissions.web-platform-tests.live certificate by modifying wptserve?

Yes, though they don't even need to modify wptserve. Any Python handler could print out the certificate. As noted above, we get a lot of security from limiting functionality to WPT members.

If we don't trust members of WPT, we could go further and disable the feature for any pull request that modifies Python files. Or we could allow most Python files and instead disallow only "patches that touch the tools/ directory" if we got fancy with the certificate file (either deleting it immediately following deployment or giving wptserve a named pipe).

That said, if we don't trust members of WPT, then this is just as much a concern for the "master" deployment (nothing's stopping the tricksters from merging their nefarious patches).

jugglinmike commented 4 years ago

This project is deployed to GCP and mirrors pull requests based on the presence of git refs in the upstream git repository. The approach is documented in greater detail in the project README.md file, and the design process is detailed in this presentation.

Thanks to everyone who provided feedback!