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.

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

Roles for our pathogen-repo-build GitHub Actions workflow, take 3 #8

Merged
merged 3 commits into from
May 21, 2024

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented May 20, 2024

A collection of templated repo-specific roles for inside a Nextstrain runtime and one role for the GitHub Actions job itself, i.e. outside the runtime.

The inside-the-runtime roles are given pathogen-specific permissions necessary for the ingest and phylogenetic workflows of a pathogen repo.

The outside-the-runtime role is only necessary/used at the moment for access to AWS Batch.

The roles can only be assumed by specific repos when performed by our centralized pathogen-repo-build workflow. While this doesn't completely prevent off-label use by other GitHub Actions workflows launched from a pathogen repo, it does make it more involved to do so, hopefully to the point of discouragement. The associated GitHub repository configuration is managed by Terraform now as well since the customization of the "sub" claim in GitHub Action's OIDC token is tightly coupled to our AWS role trust policies.

Resolves: #4
Related-to: https://github.com/nextstrain/private/issues/96, nextstrain/.github#81
Supersedes: #6, #7

Checklist

  • Checks pass

Will let us manage (and self-document) our GitHub organization and repo
settings via this Terraform configuration.
@tsibley tsibley force-pushed the trs/pathogen-repo-build-workflow-3 branch 2 times, most recently from 868c7e9 to ddc08cf Compare May 20, 2024 23:21
@tsibley
Copy link
Member Author

tsibley commented May 21, 2024

As none of the changes here should effect production usage—they're all additive or make changes to things not used in production—I've gone ahead and deployed the proposed changes (e.g. run terraform apply) in order to much more easily test them with nextstrain/.github#81.

A collection of templated repo-specific roles for inside a Nextstrain
runtime and one role for the GitHub Actions job itself, i.e. outside the
runtime.

The inside-the-runtime roles are given pathogen-specific permissions
necessary for the ingest and phylogenetic workflows of a pathogen repo.

The outside-the-runtime role is only necessary/used at the moment for
access to AWS Batch.

The roles can only be assumed by specific repos when performed by our
centralized pathogen-repo-build workflow.  While this doesn't completely
prevent off-label use by other GitHub Actions workflows launched from a
pathogen repo, it does make it more involved to do so, hopefully to the
point of discouragement.  The associated GitHub repository configuration
is managed by Terraform now as well since the customization of the "sub"
claim in GitHub Action's OIDC token is tightly coupled to our AWS role
trust policies.

Resolves: <#4>
Related-to: <nextstrain/private#96>,
            <nextstrain/.github#81>
Supersedes: <#6>,
            <#7>
@tsibley tsibley force-pushed the trs/pathogen-repo-build-workflow-3 branch from ddc08cf to a5883bd Compare May 21, 2024 16:47
@tsibley tsibley marked this pull request as ready for review May 21, 2024 16:48
@tsibley tsibley requested review from joverlee521 and a team May 21, 2024 16:48
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Thank you for iterating on this @tsibley 🙏

It's too bad that we are blocked by AWS to fully automate the management of per pathogen repo permissions, but this is definitely better than manually managing roles/credentials per repo.

Separate from this PR, we should have an internal guideline for adding new pathogen repos for automation. Seems like the new docs should live in this repo since this is where we'll have to make changes to opt-in new repos, but also seems hidden in the general infra repo...

Comment on lines +10 to +20
pathogen_repos = tomap({
# pathogen name = [repo name, …]
"dengue" = ["dengue"],
"forecasts-ncov" = ["forecasts-ncov"],
"measles" = ["measles"],
"mpox" = ["mpox"],
"ncov" = ["ncov", "ncov-ingest"],
"rsv" = ["rsv"],
"seasonal-flu" = ["seasonal-flu"],
"zika" = ["zika"],
})
Copy link
Contributor

Choose a reason for hiding this comment

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

So for any future pathogens, we would add them to this map then deploy the terraform changes with terraform apply.

Should we auto-deploy the changes on merge into the main branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, add to that map then terraform apply.

We could definitely auto-deploy… but that does require having very broad admin level access to AWS available in GitHub Actions, which makes me a little nervous. I'd say stick with manual deploys for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

When we do want to auto-deploy, the best way to gain that admin-level access would be via OIDC again, scoped down tightly to this repo (and ditto for auto-deploying nextstrain.org's Terraform config).

@tsibley
Copy link
Member Author

tsibley commented May 21, 2024

It's too bad that we are blocked by AWS to fully automate the management of per pathogen repo permissions, but this is definitely better than manually managing roles/credentials per repo.

Yeah. Either AWS or GitHub could make possible what we wanted/needed. AWS by adding more flexible claim → tag mapping. GitHub by adding customized claims.

Separate from this PR, we should have an internal guideline for adding new pathogen repos for automation. Seems like the new docs should live in this repo since this is where we'll have to make changes to opt-in new repos, but also seems hidden in the general infra repo...

Indeed. There's several scopes/levels of doc here to consider. I would think to put some brief doc adjacent to pathogen-repo-build (e.g. "Usage of this workflow requires the repository is configured in nextstrain/infra…") and some how-to doc here in this repo (e.g. "Adding a pathogen repo"). Additionally, we'll be having broader "so you want to automate things, huh?" doc in the pathogen-repo-guide, right?, and that could provide some overall contextualization of how the pieces fit together. (We could also put this overall doc in the main docs project or our internal wiki.)

@genehack
Copy link
Contributor

Separate from this PR, we should have an internal guideline for adding new pathogen repos for automation.

🎉 YES PLEASE! I would be happy to help out here, whether by beta-testing new instructions or helping to draft them or whatever. I expect I'll be wanting to do this for seasonal-cov before EOW.

Seems like the new docs should live in this repo since this is where we'll have to make changes to opt-in new repos, but also seems hidden in the general infra repo...

Maybe the pathogen-repo-guide needs to grow a "Nextstrain internal" section to document common patterns for setting up automation in repos?

@tsibley
Copy link
Member Author

tsibley commented May 21, 2024

Added documentation here for supporting a new repo: 90fec81

@tsibley
Copy link
Member Author

tsibley commented May 21, 2024

I'm going to merge this. It's already deployed as noted above and won't be used by our repos until we merge nextstrain/.github#81.

@tsibley tsibley merged commit de34b70 into main May 21, 2024
@tsibley tsibley deleted the trs/pathogen-repo-build-workflow-3 branch May 21, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new role for GH Action access to S3
3 participants