deploy build files in PR

[peterbe]

Part of #2453

Experimenting

[peterbe]

Note-to-self; this is going to clash with #589 so let's wait for that to pass first.

[nschonni]

Might be worth looking at the new "environments" in GitHub https://www.aaron-powell.com/posts/2021-01-11-using-environments-for-approval-workflows-with-github/

I previously failed to get the actual "Deployments" API working though 😆

[peterbe]

@escattone Can you make the following Secrets available here in this repo?

• DEPLOYER_DEV_AWS_ACCESS_KEY_ID
• DEPLOYER_DEV_AWS_SECRET_ACCESS_KEY

It should ONLY allow S3 upload access to the Dev bucket. It shouldn't be allowed to upload Lambda functions.

Note that I named it slightly differently to what we use in mdn/yari's for the dev-build.yml and stage-build.yml there. Over there, it's called DEPLOYER_DEV_AND_STAGE_AWS_SECRET_ACCESS_KEY but I wanted to make it extra crips that it's different.

[escattone]

@escattone Can you make the following Secrets available here in this repo?

• DEPLOYER_DEV_AWS_ACCESS_KEY_ID
• DEPLOYER_DEV_AWS_SECRET_ACCESS_KEY

It should ONLY allow S3 upload access to the Dev bucket. It shouldn't be allowed to upload Lambda functions.

Note that I named it slightly differently to what we use in mdn/yari's for the dev-build.yml and stage-build.yml there. Over there, it's called DEPLOYER_DEV_AND_STAGE_AWS_SECRET_ACCESS_KEY but I wanted to make it extra crips that it's different.

@peterbe This is done. These credentials are strictly limited to providing upload access to the dev S3 bucket only.

[peterbe]

Actually. This is not going to work I just realized. @chrisdavidmills @escattone :(

The AWS deployment secrets for uploading to S3 will never work because they're never available in PRs that come from a fork. Because, if it was allowed, someone could make a PR whose only edit is to just do echo ${{ secret.SUPER_SECRET }} and use the GitHub Actions logs to get the secret.

I guess it's time to start reading up on Environments. Thanks @nschonni and sorry guys for the false hope. How could I forget!

[nschonni]

If you want a job to have access to secrets from a fork, you an use pull_request_target https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

[peterbe]

If you want a job to have access to secrets from a fork, you an use pull_request_target https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

Oh! From skimming that, it appears that both pull_request_target and workflow_run could be useful to us. I need to do some more reading of the documentation.

[nschonni]

FYI, some examples from another repo

• Triggered by anyone commenting https://github.com/nodejs/nodejs.dev/blob/master/.github/workflows/upload-to-staging.yml
• Triggered by a maintainer labelling a PR https://github.com/nodejs/nodejs.dev/blob/master/.github/workflows/generate-preview.yml

[nschonni]

FYI, when working with pull_request_target, it only runs if the action is already landed in the default branch of the repo. Think last time I was trying to debug something like this. I changed the default branch on my fork to the working one, then sent myself the placeholder PR to my fork to test the trigger

[peterbe]

The canonical example of this in action is: peterbe#3

I can think of many things we can improve. Such as editing comments and other optimizations, but I'd rather start small and see what we hit.

@nschonni I know it's a bit of a mess but do you think you can look through it too?

[peterbe]

Now's the time to think up a much better name. Ideas?

[escattone]

What do you think of?

• PR-Post-Analysis (boring?)
• PR-Review-Support
• Review-Support
• Review-Aid
• Review-Companion

[peterbe]

• PR analyzer
• PR post-processor
• Build processor
• PR builder
  ??

[ddbeck]

Since @peterbe asked for my opinion:

Seeing the textual above comment about breaking up a catch-all workflow, I'd suggest two principles here:

• State an intention for the workflow, even if the workflow doesn't yet meet that intention
• Use the imperative to create clarity about that intention

For example, you might name this workflow, Deploy preview (with the intention that this workflow is for deployment, while other workflows will cover other things). This sets up a nice contrast for other workflows, when you split it up: Find flaws, Detect danger, Calculate stats, etc.

You're probably dissatisfied with the noun phrases you've already come up with because they don't speak to what the workflow actually does.

[peterbe]

As mentioned in Slack, the most accurate name is PR analyze and/or deploy but it just looks a bit "verbose and ugly". But it's probably the most accurate of all possible suggestions.

[peterbe]

This is the stuff that makes me slightly nervous.

So this on.workflow_run.workflows currently depends on one called "PR Test".

But imagine that someone makes a PR, from their fork, that messes "system files", then the "System file changes" workflow will fail (which is a good thing). But that doesn't mean "PR Test" will fail.

But the "System file changes" workflow only ever runs if you touched one of those files.
So, if I change the line below to:

-       workflows: ["PR Test"]
+       workflows: ["PR Test", "System file changes"]

how will that work if "System file changes" never even runs?!

I honestly don't know how this works. Perhaps we vigilantly launch this stuff as it is, then we try to mess with it and self-pen-test and if it doesn't work, we revert.

@nschonni Do you have any ideas on this?

[escattone]

@peterbe This comment sent me trying to understand GitHub Actions better. It seems to me that we do need to worry about someone messing with the workflow files in a PR. I'm wondering if we should, within this workflow file, create two jobs, with the second one dependent (requires a successful run) on the first one (see https://docs.github.com/en/actions/learn-github-actions/managing-complex-workflows#creating-dependent-jobs):

jobs:
  workflow-files-check:
    runs-on: ubuntu-latest
    if: >
      ${{ github.event.workflow_run.event == 'pull_request' &&
      github.event.workflow_run.conclusion == 'success' }}
    steps:
      # Check for any workflow files in the git diff
  review-companion:
    needs: system-files-check
    runs-on: ubuntu-latest
    steps:
      - name: 'Download artifact'
        uses: actions/github-script@v3
      ...

[escattone]

The more I think about this I'm doubting what I wrote above, that is I doubt that we have to worry about pull requests that mess with the workflow files. We can test this tomorrow, and I'm going to read a bit more tonight.

[escattone]

Ok, I'm pretty certain now that we do not have to worry about the pull request's workflow files, since the pull_request_target workflow never uses them. I think we're good here as you've written it, but we can test/discuss more tomorrow.

[peterbe]

@ddbeck perhaps you know something about this?

[peterbe]

That's correct, this work flow never runs yarn ... (or npm ...) or even python ... on any files from the PR. The files from the PR are treated like basically dumb file attachments.

Thing is, someone nasty could make a PR that will be yarn build and yarn installed, but in a context without any secrets, and that nasty content can and will be uploaded. But it won't be "executed".

[escattone]

What I finally realized last night was that a pull_request_target workflow only ever uses the accepted workflow file (and other files) from the already-accepted/reviewed stuff in the target repo, never the code from the pull request, so it's always from a trusted source. So the only concern is to never run code from the pull request because it'll have access to your secrets/tokens, and since we only use the build artifacts (zip file) from the pull_request workflow as the content we want to analyze and deploy, I think we're OK. Here's Github's description:

...behaves in an almost identical way to the pull_request event with the same set of filters and payload. However, instead of running against the workflow and code from the merge commit, the event runs against the workflow and code from the base of the pull request. This means the workflow is running from a trusted source and is given access to a read/write token as well as secrets enabling the maintainer to safely comment on or label a pull request.

[peterbe]

But this isn't a pull_request_target. It's a workflow_run.
But!! Then I go on to read https://docs.github.com/en/actions/reference/events-that-trigger-workflows#workflow_run and see that it's safe. The workflow_run is like one of those fancy pull_request_target workflows.

[escattone]

@petebe Oops, you're right, but like you said it looks like workflow_run has similar security characteristics to pull_request_target in that it only uses the workflow file from the default branch to protect against workflow files in the pull request having access to secrets/tokens.

[peterbe]

So all I have to do is to update the code comment. We do not need to be waiting for anything or be blocked.

We already have a CI workflow in place that protects PRs from being green if someone messes with the system files.

[peterbe]

PS. It's expected that "System file changes" fails in this context.

[escattone]

I don't think this should be tests right? Perhaps something like review-companion? I'm not sure how the job id is used though.

[escattone]

@peterbe This comment sent me trying to understand GitHub Actions better. It seems to me that we do need to worry about someone messing with the workflow files in a PR. I'm wondering if we should, within this workflow file, create two jobs, with the second one dependent (requires a successful run) on the first one (see https://docs.github.com/en/actions/learn-github-actions/managing-complex-workflows#creating-dependent-jobs):

jobs:
  workflow-files-check:
    runs-on: ubuntu-latest
    if: >
      ${{ github.event.workflow_run.event == 'pull_request' &&
      github.event.workflow_run.conclusion == 'success' }}
    steps:
      # Check for any workflow files in the git diff
  review-companion:
    needs: system-files-check
    runs-on: ubuntu-latest
    steps:
      - name: 'Download artifact'
        uses: actions/github-script@v3
      ...

[fiji-flo]

Overall great. Maybe remove some of the debug output, but I guess you'd do that anyway.

I'm reading the GitHub action docs to form an opinion about workflow_run topic.

[peterbe]

FYI, some examples from another repo

* Triggered by anyone commenting https://github.com/nodejs/nodejs.dev/blob/master/.github/workflows/upload-to-staging.yml

* Triggered by a maintainer labelling a PR https://github.com/nodejs/nodejs.dev/blob/master/.github/workflows/generate-preview.yml

I don't like that someone has to type something. Comment or label. The usefulness dies if you have to remember the "command".

However, those links are gold because one thing I could easily see is that someone with write permissions might wanna kill the Dev deployment if the content is terrible.
Right now, the people who have access to set labels can also delete comments. But that'd only remove the hyperlinks to the deployed site. Other than that, we only have S3 expiring the Dev build after a number of days.
So I'll try to remember this great examples when we have time to build in some fancy overrides and stuff.

Another thought, that fits in somewhere in between, is that like the dependabot[bot] it has a constant little instruction manual about what comment commands you can type. That could come in handy if you need type something like:

/rebuild

or

/rebuild-with-redirects

or

/destroy

...and you don't remember what all those clever commands are.

[escattone]

@peterbe and I discussed this at length, and although we could merge this PR as is, we decided to hold off until tomorrow. We'd like to fix the issue where the toolbar that we show in the cloud preview is not yet shown in "read-only" mode (i.e., the "open in your editor" button/links are removed).

[escattone]

@peterbe Nice work, let's give it a spin!

[escattone]

We'd like to fix the issue where the toolbar that we show in the cloud preview is not yet shown in "read-only" mode (i.e., the "open in your editor" button/links are removed).

Forgot to mention that this was resolved via mdn/yari#3416.