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

Allow seasonal-flu/* to assume GitHubActionsRoleNextstrainBatchJobs #19

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

joverlee521
Copy link
Contributor

We cannot use the usual pathogen-repo-build workflow for the seasonal flu deploy-private-nextflu workflow because these are private builds that should not be surfaced through public GH Action artifacts.¹

¹ https://github.com/nextstrain/private/issues/110#issuecomment-2155212036

Checklist

  • Checks pass

# from AWS Batch before bundling/deploying them through Netlify.
# This special case can be removed when we finally sunset the private site.
# -Jover, 07 June 2024
"repo:nextstrain/seasonal-flu:*:job_workflow_ref:nextstrain/seasonal-flu/.github/workflows/deploy-private-nextflu.yaml@*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, just reading the OIDC docs more closely, job_workflow_ref is for called reusable workflows only...what we need here would be workflow_ref

Copy link
Member

Choose a reason for hiding this comment

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

Oy. Really wishing GitHub allowed mapping of other claims in its token so we could use PrincipalTags here without a bunch of effort. But they don't, so.

We could add workflow_ref to the sub claim and adjust other conditions to account for it too, e.g.

…/pathogen-repo-build@*:workflow_ref:*

or change nothing about sub and simply let everything in seasonal-flu access Batch for now.

repo:nextstrain/seasonal-flu:*

Copy link
Member

Choose a reason for hiding this comment

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

The first option would look like 3dd92dd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first option would look like 3dd92dd.

Thanks! I'll cherry-pick and deploy the changes to give it a try.

Copy link
Member

Choose a reason for hiding this comment

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

Here's hoping it works…

Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♂️ sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying the second option with 81b8676

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

\o/ Kinda surprised that adding the missing @* wasn't enough... wonder what it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaving that mystery for another time...

We cannot use the usual `pathogen-repo-build` workflow for the
seasonal flu deploy-private-nextflu workflow because these are private
builds that should not be surfaced through public GH Action artifacts.¹

We attempted to use the custom claim `workflow_ref` in
538385e but that did not work as
expected, so just allow any seasonal-flu GH Action workflow to
access AWS Batch.

¹ <nextstrain/private#110 (comment)>
@joverlee521 joverlee521 changed the title Allow seasonal-flu/deploy-private-nextflu to assume GitHubActionsRoleNextstrainBatchJobs Allow seasonal-flu/* to assume GitHubActionsRoleNextstrainBatchJobs Jun 7, 2024
@joverlee521 joverlee521 marked this pull request as ready for review June 7, 2024 23:35
@joverlee521 joverlee521 merged commit f69478e into main Jun 7, 2024
1 check passed
@joverlee521 joverlee521 deleted the seasonal-flu-batch branch June 7, 2024 23:37
joverlee521 added a commit that referenced this pull request Jun 10, 2024
Follow up to #19 where I rebased
and accidentally included unintended changes.

I should have known better to not merge/deploy on a Friday afternoon,
especially when my brain was fried.
]
"token.actions.githubusercontent.com:sub": flatten([
[for repo in keys(local.repo_pathogens):
"repo:nextstrain/${repo}:*:job_workflow_ref:nextstrain/.github/.github/workflows/pathogen-repo-build.yaml@*:workflow_ref:*"],
Copy link
Contributor Author

@joverlee521 joverlee521 Jun 10, 2024

Choose a reason for hiding this comment

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

Unintended change here fixed in cefc83e 🤦‍♀️

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.

2 participants