catalyst-cooperative / pudl-usage-metrics

A dagster ETL for collecting and cleaning PUDL usage metrics.
MIT License
1 stars 0 forks source link

Google auth action failing, causing all CI tests to fail #90

Open jdangerx opened 1 year ago

jdangerx commented 1 year ago

Dependabot has been faithfully opening pull requests but they fail with this error message (logs):

Error: google-github-actions/auth failed with: retry function failed after 1 attempt: the GitHub Action workflow must specify exactly one of "workload_identity_provider" or "credentials_json"! If you are specifying input values via GitHub secrets, ensure the secret is being injected into the environment. By default, secrets are not passed to workflows triggered from forks, including Dependabot.

Now our tox-pytest.yml does only use one of WIF or credentials JSON, so I'm not quite sure what they're complaining about. Maybe secrets.GCP_SA_KEY is actually empty?

      # Authentication via credentials json
      - name: Authenticate gcloud
        uses: "google-github-actions/auth@v0"
        with:
          credentials_json: "${{ secrets.GCP_SA_KEY }}"

We could also potentially use WIF instead, that'd be a little safer and is recommended by Google - what do you think @bendnorman ?

(instructions for how to do so: https://cloud.google.com/blog/products/identity-security/enabling-keyless-authentication-from-github-actions)

(if we start doing this, it might be worth looking into tools like Terraform to keep all our infra config ducks in a row)

zaneselvans commented 1 year ago

We have attempted to figure out WIF several times and... it has not gone well. If you understand how it works and can help us out that would be great!

zaneselvans commented 1 year ago

The other option is to give it the same credentials here that we use in the other repositories. I think it's probably an organization secret, but it hasn't been set up for the workflows in this repo.

bendnorman commented 1 year ago

The GCP_SA_KEY is a repository secret Dependabot has access to so I'm not sure what's going on there. I've tried to wrap my head around WIF a couple of times and failed. Do you have experience with it @jdangerx?

jdangerx commented 1 year ago

I got it working for our GH actions at my last gig - so I know it's possible, working off of that exact blog post. Not sure how much I remember, but I bet I can do it again.

jdangerx commented 1 year ago

Roughly, the steps seem like

  1. create a Workload Identity Pool in gcloud to hold providers
  2. create a Workload Identity Provider in gcloud that we will use in the auth action; tell it about how github will want to interact with it
  3. Allow the provider to impersonate the service account that we're using
  4. Update the auth action to refer to the provider

My questions are:

jdangerx commented 1 year ago

Updates:

WIF works for tox-pytest in PUDL :tada: - see https://github.com/catalyst-cooperative/pudl/pull/2259

However!

This issue is, at its root, "GitHub Actions doesn't give workflows triggered by external forks (including dependabot) enough permissions to access secrets or authenticate as us." Which I think is really good policy.

In PUDL, this is ok - we only use the GCS credentials to access the GCS cache, and I added a conditional "if you couldn't log in to GCloud, use Zenodo instead and don't even try GCS" branch in the testing flow.

In pudl-usage-metrics we have a bunch of secrets for postgres and whatnot.

Options:

I'm going to go ahead and do the second one now to clean up the existing bot PRs. @bendnorman you probably have the best context here on whether the first bullet is useful - any thoughts?

jdangerx commented 1 year ago

@zaneselvans - in response to your question on #96 - I don't think we can - dependabot PRs are explicitly treated as "external PRs" which are missing specific privileges. I don't know why these were working before.

zaneselvans commented 1 year ago

Since dependabot is part of GitHub, there's a special repo or org level dependabot secrets where you can give it credentials and trust it to run CI. Then we used merge-me-action to merge the PRs in if CI passed in the bot-auto-merge GitHub actions workflow (triggered by successful completion of the tox-pytest workflow) using an app id/key.

It's going to be annoying if we can never have any bot PRs merge automatically based on CI outcomes. Is there something analogous that we can do with WIF?

What's the best process for getting CI to run on a PR from an outside contributor?

The bot-auto-merge workflow:

name: bot-auto-merge

on:
  workflow_run:
    types: [completed]
    workflows: ["tox-pytest"]

jobs:
  bot-auto-merge:
    name: Auto-merge passing bot PRs
    runs-on: ubuntu-latest
    steps:
      - name: Impersonate auto merge PR bot
        uses: tibdex/github-app-token@v1
        id: generate-token
        with:
          app_id: ${{ secrets.BOT_AUTO_MERGE_PRS_APP_ID }}
          private_key: ${{ secrets.BOT_AUTO_MERGE_PRS_APP_KEY }}
      - name: Auto-merge passing dependabot PRs
        if: ${{ github.event.workflow_run.conclusion == 'success' }}
        uses: ridedott/merge-me-action@v2
        with:
          # For clarity only. dependabot is default login.
          GITHUB_LOGIN: dependabot
          GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }}
          ENABLED_FOR_MANUAL_CHANGES: "true"
      - name: Auto-merge passing pre-commit-ci PRs
        if: ${{ github.event.workflow_run.conclusion == 'success' }}
        uses: ridedott/merge-me-action@v2
        with:
          GITHUB_LOGIN: pre-commit-ci
          GITHUB_TOKEN: ${{ steps.generate-token.outputs.token }}
          ENABLED_FOR_MANUAL_CHANGES: "true"
jdangerx commented 1 year ago

Hmm so in theory these bot PRs should be able to pass tox-pytest - firstly because their github token should have the right permissions and secondly because if they don't have the right permissions we fall back to Zenodo-based testing.

zaneselvans commented 1 year ago

So long as the bot PRs can pass CI, then I think this secondary action (or something like it) that's kicked off by completion of one of their PR CI runs and which runs as a user internal to the organization should be able to merge the PR in.

jdangerx commented 1 year ago

@zaneselvans looks like there's a fix for the bot-auto-merge stuff you commented about https://github.com/ridedott/merge-me-action/issues/1581 !

jdangerx commented 1 year ago

OK. So I think if we want the dependabot PRs to get merged automatically, we need to do the following:

@bendnorman I think the last two are gonna be on you, or you can make me a repo admin and I can do it. Also I guess the review for the WIF PR is also on you - sorry for throwing a bunch of stuff on your plate!

zaneselvans commented 1 year ago

@zaneselvans looks like there's a fix for the bot-auto-merge stuff you commented about https://github.com/ridedott/merge-me-action/issues/1581 !

@jdangerx Yes, that's the solution I implemented in the bot-auto-merge workflow.

jdangerx commented 1 year ago

Yeah, it looks like it's not working in the pudl-usage-metrics repo though - hence thinking that there's some repo-specific mis-configuration.

bendnorman commented 1 year ago

I approved the WIF PR and I added the PG secrets to dependabot. Are these the merge-me instructions you're referring to?

jdangerx commented 1 year ago

I approved the WIF PR and I added the PG secrets to dependabot.

Sweet, thanks!

Are these the merge-me instructions you're referring to?

Right - I think we have some secrets referenced here that might not be available to dependabot in this repo?

zaneselvans commented 1 year ago

I have a vague sense that maybe this was a free-tier public vs. private repository issue...

jdangerx commented 1 year ago

Scope:

Next steps: