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

Enabling PR Previews with Azure Storage #189

Merged
merged 38 commits into from Aug 30, 2023
Merged

Enabling PR Previews with Azure Storage #189

merged 38 commits into from Aug 30, 2023

Conversation

nerddtvg
Copy link
Contributor

@nerddtvg nerddtvg commented Aug 29, 2023

This PR implements temporary Azure Storage based static sites for PR previews. A new storage account will be created for each PR (ocelotprpreview##). Every run of the workflow will build and upload the latest version so updated PRs will push changes similar to today's setup.

When a deployment is pushed, a comment on the PR will be added with a link to the static website.

Once a PR is closed, whether merged or not, the storage account will be deleted. When the account deleted, a comment will be added stating that. If the storage account cannot be deleted, a comment will note that as well.

The specific app registration for this only has Reader and Storage Account Contributor access to a single RG. This will ensure it cannot be manipulated to break something else in the sandbox environment.

The storage accounts will be tagged to indicate they are managed by GitHub Actions with a reference to the appropriate PR number.

Fixes #183

@nerddtvg nerddtvg self-assigned this Aug 29, 2023
@netlify
Copy link

netlify bot commented Aug 29, 2023

Deploy Preview for romantic-austin-f85522 failed.

Name Link
🔨 Latest commit f81b93f
🔍 Latest deploy log https://app.netlify.com/sites/romantic-austin-f85522/deploys/64ef36bf399bc50008a80d57

@nerddtvg nerddtvg marked this pull request as ready for review August 29, 2023 20:53
@nerddtvg nerddtvg requested a review from a team as a code owner August 29, 2023 20:53
@nerddtvg
Copy link
Contributor Author

Woo! Progress. Just need to check why some files are missing.

@nerddtvg
Copy link
Contributor Author

Latest preview works. Adding parallelism to the upload so it can go a little faster. Then I'm leaving it for tonight and picking up tomorrow.

@Techneaux
Copy link
Member

This is a neat idea. Your end-to-end flow makes sense to me. The main area I was pondering was security. With anyone in the world being able to create a PR from a fork and trigger this, I was trying to think through if there's any security vulnerabilities potentially introduced with a DIY preview solution. Anything that perhaps netlify was handling. I haven't come up with anything specific so maybe there isn't any concerns?

@nerddtvg
Copy link
Contributor Author

GitHub Actions abuse is a definite concern. I believe we are relatively safe here though because the secrets are not accessible from forked repositories and any runs in those repositories run in those contexts. The ODIC credentials for Azure will only operate if the workflow is run from the base repository.

While this is a public repository, forks with PRs from those also should not automatically run using the pull_request trigger: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

I believe we do not have any concerns along those lines but it is a good thing to verify.

@PMByrne
Copy link
Member

PMByrne commented Aug 30, 2023

This is a neat idea. Your end-to-end flow makes sense to me. The main area I was pondering was security. With anyone in the world being able to create a PR from a fork and trigger this, I was trying to think through if there's any security vulnerabilities potentially introduced with a DIY preview solution. Anything that perhaps netlify was handling. I haven't come up with anything specific so maybe there isn't any concerns?

This actually isn't a concern. The only time the secret values will be available is if the PR is generated from a branch within the repo. If the PR is from a branch in a forked version of the repo, the Azure secret values will not be populated. Since only org members have write access, the only time a PR comes from inside the repo, should be from helper bots or actual Ocelots.

@nerddtvg instead of failing explosively, can you add an if conditional to the PR workflow so that it will only run if the required env variables exist? The rest looks good to me!

@nerddtvg
Copy link
Contributor Author

@PMByrne What env vars are you referring to?

@PMByrne
Copy link
Member

PMByrne commented Aug 30, 2023

The 3 azure ones. They should not exist if the PR comes from a forked repo.

@nerddtvg
Copy link
Contributor Author

@PMByrne A test was done with an empty string in place of an empty secret and the job was skipped: https://github.com/ocelotconsulting/ocelotconsulting.github.io/actions/runs/6026431339/job/16349350518

At this time, I think the work on this is completed. I've already disabled Netlify so it would stop bugging us on this PR.

Technically we need to test deleting the storage account but that will be done with the PR closure, and I don't expect an issue there.

@PMByrne PMByrne merged commit 07fa36a into main Aug 30, 2023
4 checks passed
@github-actions
Copy link

The preview storage account has been deleted.

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.

Consider Changing PR Preview Configuration
3 participants