nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.25k stars 322 forks source link

[WIP] CI: Test packaging on debian-based systems #1200

Closed thresheek closed 3 days ago

thresheek commented 3 months ago

This uses F5/nginx self-hosted runners to run the jobs.

This is a WIP PR. It will be amended and fixed going forward as this not only changes Unit CI, but will likely require some changes to F5-hosted runners as well.

callahad commented 3 months ago

We'll want to consider the security implications of exposing self-hosted runners to this repo. GitHub itself recommends against using self-hosted runners on public repos:

We recommend that you only use self-hosted runners with private repositories. This is because forks of your public repository can potentially run dangerous code on your self-hosted runner machine by creating a pull request that executes the code in a workflow.

This is not easy to lock down, as a community discussion explains:

The problem is that everyone, even outside users, can change the workflow to use "on pull_request" and create a pull request and then the pipeline will run.

The only way to guard against this in a public repo is to require manual approval every time an outside collaborator submits a pull request before workflows can run.

thresheek commented 3 months ago

That's all true.

Currently, our runners are ephemeral and re-created for each job run, somewhat reducing the attack surface.

Manual approval seems to be an ok guard I guess.

I also think it's possible to run a part of a workflow without an approval, and then require it to run the next steps: https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment#about-environments

e.g. think of style checks / smoke tests that we currently run on public runners be executed as a first step; and then after an explicit approval self-hosted runners will pick up the rest of integration / packaging jobs.

callahad commented 3 months ago

Manual approval seems to be an ok guard I guess.

The more I think about it, I don't think the friction is justifiable in this repo: Every time a community member submits or updates a pull request, they'll have to prod us to click a button before they get any CI/CD feedback. This regresses the feedback cycle from single-digit minutes right now, to potentially unbounded delays and manual intervention.

I don't think Deployments can be bent into achieving this, but I'd love to be proven wrong. :)

Per your message in Slack... the prospect of splitting packaging into its own repo is really intriguing, and could be the best path forward.

ac000 commented 3 months ago

Can we block the CI runs if say anything under .github/ changes? As that seems to be where the concern lies...

Maybe the runners themselves could check? I believe you are able to have some sort of feedback loop here...

thresheek commented 3 months ago

I don't think we can selectively block that in some github setting that is not accessible by the PR author. And exposing that check in the .github will allow a PR author to change that as a part of PR...

That said, we're now exploring an idea of:

This however will mean that the self-hosted checks will happen after the PR gets merged. Which is probably OK for packaging-related things as it's the main target for self-hosted runners...

The rest of checks will still run on public runners, too, so you'd get a good idea if proposed code is buildable or passes the tests.

ac000 commented 3 months ago

And exposing that check in the .github will allow a PR author to change that as a part of PR...

Well the check wouldn't be in the github workflow files. It would be something on the runners themselves, a simple did anything under .github/ change? If so (and you could maybe skip the check for the @nginx org) prompt for manual intervention...

This however will mean that the self-hosted checks will happen after the PR gets merged.

That is what we'd really like to avoid...

Will that also not make it harder to test the workflows out if you have to merge them first?

Right now you can open a PR and see the results, you can make some tweaks to the workflow, push up the change and see the results, rinse, repeat...

thresheek commented 3 months ago

And exposing that check in the .github will allow a PR author to change that as a part of PR...

Well the check wouldn't be in the github workflow files. It would be something on the runners themselves, a simple did anything under .github/ change? If so (and you could maybe skip the check for the @nginx org) prompt for manual intervention...

That's not really possible to implement outside of a workflow. There is no way to inject a "pre-job" hook into GH self-hosted runners (well, without forking that project, and that's not something we should do).

This however will mean that the self-hosted checks will happen after the PR gets merged.

That is what we'd really like to avoid...

Will that also not make it harder to test the workflows out if you have to merge them first?

I don't really expect packaging checks to be more than "cd pkg/deb && make all", and that's fairly triivial to implement.

Right now you can open a PR and see the results, you can make some tweaks to the workflow, push up the change and see the results, rinse, repeat...

The current approach will stay as is - and all publicly-running jobs will still be easy to tweak. You can amend and change those during a normal PR push/test/fix flow.

The only downside of a tiered approach is that it will not be possible to know if your changes break packaging during the PR run - but we didnt really have that with Buildbot or a current approach either. We will know if it did a few minutes after the merge though - and currently we rely on nightly build runs for that so time delta can be as high as 24 hours.

thresheek commented 3 days ago

Closing since we've chosen a different approach. PR to follow soon-ish.