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

[CI] Build PR images on GHCR #3432

Merged
merged 54 commits into from
Nov 17, 2023
Merged

[CI] Build PR images on GHCR #3432

merged 54 commits into from
Nov 17, 2023

Conversation

jlestel
Copy link
Contributor

@jlestel jlestel commented Nov 15, 2023

I modified the CI so that the PR images are build then pushed into the GitHub packages (GHCR).

This does not interfere with how current CI works with Docker Hub.

Functioning :

  • When a PR is created or modified: the image is built on the GHCR from fork repo.
  • When a branch is pushed: the image is built on the GHCR from main repo.
  • When a PR is completed: the image is deleted in the fork repo. Additionally, untagged images are also deleted on this fork repo.

Once this PR is merged, each new / updated PR will trigger CI on the fork repo.

If necessary, anyone can then modify their docker-compose to use a PR image (eg: ghcr.io/jlestel/teslamate:master) for testing this PR instead of teslamate/teslamate:latest.

@JakobLichterfeld
Copy link
Collaborator

Thank you for this PR. Can you elaborate on why you chose to modify the Elixir CI rather than create your own workflow?

@jlestel
Copy link
Contributor Author

jlestel commented Nov 15, 2023

Thank you for this PR. Can you elaborate on why you chose to modify the Elixir CI rather than create your own workflow?

No reason, I just separated them.

Now I trigger the GHCR build on a pull_request or push master in a separate workflow.

Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

lgtm, my findings see inline comments

.github/workflows/ghcr_build.yml Show resolved Hide resolved
.github/workflows/ghcr_build.yml Outdated Show resolved Hide resolved
.github/workflows/ghcr_build.yml Show resolved Hide resolved
@JakobLichterfeld JakobLichterfeld added the github_actions Pull requests that update Github_actions code label Nov 15, 2023
@DrMichael
Copy link
Collaborator

If necessary, anyone can then modify their docker-compose to use a PR image (eg: ghcr.io/teslamate-org/teslamate:pr-123) for testing instead of teslamate/ teslamate:latest

Would that also be the case for grafana?

@jlestel
Copy link
Contributor Author

jlestel commented Nov 15, 2023

If necessary, anyone can then modify their docker-compose to use a PR image (eg: ghcr.io/teslamate-org/teslamate:pr-123) for testing instead of teslamate/ teslamate:latest

Would that also be the case for grafana?

yes I added grafana also

@adriankumpf
Copy link
Collaborator

Could this be used by a malicious PR to leak build secrets? Or does building the images require maintainer approval?

@brianmay
Copy link
Collaborator

I don't like to be the pessimist, but wondering if this will work. "With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository." https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

My experience, not even jobs triggered by dependabot created pull requests have access to secrets. I had to update all of my github workflows as a result.

But nothing wrong with trying it. :-)

Also see: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions

@JakobLichterfeld
Copy link
Collaborator

Could this be used by a malicious PR to leak build secrets? Or does building the images require maintainer approval?

Good point you raise. As far as my brief research shows, you can extract secrets using malicious github workflows and actions. Building a docker image with a verified pipeline should not be an issue, but PRs related to .github should be excluded from auto-run. But this is nothing introduced by this PR, but more of a general issue in my opinion.

Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld 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 your dedicated work! I really like the process of refactoring into separate actions. I added my findings as inline comments.

.github/actions/build/action.yaml Outdated Show resolved Hide resolved
.github/actions/grafana/action.yml Outdated Show resolved Hide resolved
.github/actions/grafana/action.yml Show resolved Hide resolved
.github/actions/merge/action.yml Outdated Show resolved Hide resolved
.github/actions/merge/action.yml Show resolved Hide resolved
Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

lgtm

@jlestel
Copy link
Contributor Author

jlestel commented Nov 16, 2023

Could this be used by a malicious PR to leak build secrets? Or does building the images require maintainer approval?

Yes there was a flaw there.

Now, I prevent workflows from being launched when there are changes to the .github directory: only a maintainer can run it manually.

@jlestel
Copy link
Contributor Author

jlestel commented Nov 16, 2023

So, i'll try to replace pull_request by pull_request_target to only run an action in the repo of the requester of a PR.

Voilà @adriankumpf @JakobLichterfeld : the GHCR build workflow is now based on the PR source user. The CI is played on the fork repo instead of the teslamate-org repo (and the generated images too).

Eg for this PR, you'll find image(s) on my fork:

It build only amd64 as others need a thrid party not everyone has install.

Let me know what you think about that.

Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

Thanks again for refactoring and your dedicated work.

I added once again my small findings.

Additionally, two general points still prevent me from merging:

  • I would vote to include - '!.github/**' # Important: Exclude PRs related to .github from auto-run in all workflows
  • Please lint the files, as there are some trailing whitespaces for example

.github/workflows/ghcr_build.yml Outdated Show resolved Hide resolved
.github/workflows/ghcr_build.yml Outdated Show resolved Hide resolved
.github/actions/merge/action.yml Show resolved Hide resolved
.github/workflows/buildx.yml Show resolved Hide resolved
.github/workflows/ghcr_build.yml Outdated Show resolved Hide resolved
.github/workflows/ghcr_build.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

lgtm

@JakobLichterfeld JakobLichterfeld merged commit b65b3f3 into teslamate-org:master Nov 17, 2023
1 of 2 checks passed
JakobLichterfeld added a commit that referenced this pull request Nov 17, 2023
JakobLichterfeld added a commit that referenced this pull request Nov 17, 2023
@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Nov 17, 2023

@jlestel Sorry, merging on mobile in the middle of the night without testing in our repo was not the best idea. I suggest opening a CI branch, including the branch in the workflow triggers, and testing it in our repo, as local testing of GitHub workflows is limited in terms of secrets and GitHub usernames. The issue lies within the login to pull buildx.

@jlestel
Copy link
Contributor Author

jlestel commented Nov 17, 2023

@jlestel Sorry, merging on mobile in the middle of the night without testing in our repo was not the best idea. I suggest opening a CI branch, including the branch in the workflow triggers, and testing it in our repo, as local testing of GitHub workflows is limited in terms of secrets and GitHub usernames. The issue lies within the login to pull buildx.

No problem. Yes it will be better to test it with a dedicated branch as well as on our repositories. Keep me informed.

JakobLichterfeld pushed a commit that referenced this pull request Feb 9, 2024
* Upgrade CI

* Keep lint and test on tags

* Keep actions/checkout@v3

* Consider CI on branches named v*

* Fix write packages

* Distinct Elixir CI and PR Build

* Rename build

* Fix type

* Add workflow_dispatch

* Add timeout to prevent stuck jobs

* Add actions for buildx and merge

* fix username ?

* Add grafana images from PR

* fix typo

* Finih merge action refactoring

* Exclude PRs related to .github from auto-run

* Don't try to auth on DockerHub when we can't

* tmp allow myself to execute

* Update ghcr_build.yml to be able to write on packages from public fork

* Revert ghcr_build.yml

* Update ghcr_build.yml

* Update to build on source repo

* Update ghcr_build.yml to test

* Use correct repo with REGISTRY_IMAGE

* Update ghcr_build.yml

* Update ghcr_build.yml

* Update ghcr_build.yml

* Update ghcr_build.yml

* Update ghcr_build.yml

* Use dynamic variables to work in all repos

* Purge in source repo

* Update ghcr_build.yml

* Update ghcr_build.yml

* Update ghcr_build.yml

* Update action.yaml

* Update ghcr_build.yml

* Update buildx.yml

* Update action.yaml

* Fix vars

* Trace digests

* Update action.yml

* Update action.yml

* Fix typo

* Fox repo name

* Update buildx.yml

* Update action.yaml

* Fix repository name

* Run on any branch pushed

* Exclude PRs related to .github from auto-run

* Add buildjet dependency

* lint

* lint actions

* fix: links in dashboards

---------

Co-authored-by: Julien <[email protected]>
JakobLichterfeld added a commit that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants