podvm: remove quay.io secret from podvm builds - #3249
Conversation
|
I'm fine to change the registries for the PR runs, but I don't think this change as is make sense for the release as it looks like we'll have some of our release images in quay and the podvm in ghcr? Can you leave the secrets in the release flow and keep quay.io for the release builds, but ghcr for pr/nightly ones? Either that or we should move all our images to ghcr.io, rather than having half and half? |
If we want to enable runs of e2e on pull-requests workflows If we need to also push to quay.io in the release workflow, I think the correct thing to do is to skopeo cp oci://ghcr.io/... oci://quay.io/... in the release wf. would that work? |
I don't think we need to push to quay.io in the release workflow, but that means updating the other image build and pushes (e.g. caa, peerpodctrl, webhook) to switch to ghcr.io as well, which we need to do anyway based on your logic that we shouldn't have the secrets in e2e_run_all? Or is this part 1 and those changes will follow before we next release? |
hmm, good point. I think those default to ghcr atm, otherwise e2e tests wouldn't work on a fork, which they do for me (I do not have the quay secret). If that is correct, we can do it iteratively. I'm open about dropping the quay pushes, but maybe it breaks something downstream. I think we can copy-push in the release workflow for now |
e6ca6ff to
c8cdb4d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the PodVM GitHub Actions workflows to avoid passing quay.io credentials through PodVM builds, defaulting PodVM artifact publishing to GHCR and only performing an optional post-build copy to quay.io when requested. This aligns with the security goal in #2214 of enabling safer fork-based PR execution without relying on pull_request_target purely for registry secrets.
Changes:
- Default PodVM builds to GHCR and remove quay login/secret usage from the
podvm_mkosireusable workflow. - Add a follow-up job that (optionally) copies the built PodVM ORAS artifact from GHCR to quay.io when
inputs.registrytargets quay. - Remove
QUAY_PASSWORDpassing from PodVM build jobs inside the e2e “run all” workflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/podvm_publish.yaml | Switches PodVM publishing flow to build/push to GHCR first and optionally copy to quay.io. |
| .github/workflows/podvm_mkosi.yaml | Removes quay login/secret requirements and defaults registry to GHCR-centric usage. |
| .github/workflows/e2e_run_all.yaml | Stops passing QUAY_PASSWORD into PodVM mkosi builds in e2e orchestration. |
Suppressed comments (1)
.github/workflows/podvm_publish.yaml:55
- The
registryinput is described as an alternative registry to push images to, but thepodvmreusable-workflow call no longer forwardsinputs.registryintopodvm_mkosi.yaml. As a result, settingregistryto an alternate GHCR namespace has no effect (only thepush-to-quaycopy uses it). Forward a GHCR registry override while keeping quay requests on the GHCR->quay copy path.
strategy:
fail-fast: false
matrix:
arch: [amd64, s390x, arm64]
with:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c8cdb4d to
c960ef0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
.github/workflows/podvm_publish.yaml:59
- The reusable
podvm_mkosi.yamlworkflow is no longer passed aregistryinput here, so it falls back to its own default. That default is currently not safe for forks and (as written) is also using an expression in an input default. Pass an explicit GHCR registry here, and avoid passingquay.io/...(since quay publishing is handled by the follow-up copy job).
with:
git_ref: ${{ inputs.git_ref || github.sha }}
image_tag: ${{ inputs.image_tag || github.sha }}
arch: ${{ matrix.arch}}
debug: false
.github/workflows/podvm_publish.yaml:65
push-to-quaycurrently (1) copies only a single image vianeeds.podvm.outputs...even thoughpodvmis a matrix job (so there are multiple arch-specific images), and (2) does not authenticate to GHCR beforeoras cp, which will fail for private packages. Copy the expected per-arch images by tag and add a GHCR login step.
push-to-quay:
name: Push podvm image to quay.io
permissions:
packages: read # Required to copy container images
needs: podvm
.github/workflows/podvm_mkosi.yaml:117
- This workflow still accepts an arbitrary
inputs.registry, but it now only authenticates to GHCR. If a caller supplies a non-GHCR registry (e.g.quay.io/...), theoras pushstep will fail with a confusing auth error. Consider failing fast with a clear message unless the registry isghcr.io/....
- name: Login to the ghcr Container registry
uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 # v4.4.0
with:
registry: ghcr.io
username: ${{ github.actor }}
291691c to
5bea1df
Compare
5bea1df to
41ca3a2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
.github/workflows/podvm_publish.yaml:66
push-to-quaycan be triggered withinputs.registrystarting withquay.ioeven whenQUAY_PASSWORD/QUAY_USERNAMEare not provided (e.g., reusable workflow caller omits the optional secret, or forks don’t have it). That will fail later during the quay login step with a non-obvious error; it’s better to gate the job on the credentials being present.
needs: podvm
if: ${{ startsWith(inputs.registry, 'quay.io') }}
.github/workflows/podvm_mkosi.yaml:118
- The workflow still exposes a generic
registryinput (and uses it in theoras pushstep), but it now always logs into GHCR. If a caller setsinputs.registryto a non-GHCR registry, the workflow will proceed and then fail at push time with an auth error. Adding an explicit validation (or reintroducing registry-specific login) would make failures deterministic and easier to diagnose.
- name: Login to the ghcr Container registry
uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 # v4.4.0
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
Currently the builds pass through a secret to quay.io. This will
prohibit the building of the images on forks for pull request builds,
forcing the use of the insecure pull-request-target mechanism in e2e
tests, even though podvm builds can be executed safely on forks.
This change removes the secret and its uses at the call site and
defaults to ghcr.io, which can be used in forks. For subsequent
pull request workflows registry should be set to
"ghcr.io/${{ github.repository_owner }}".
Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
41ca3a2 to
91df061
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
.github/workflows/podvm_publish.yaml:28
- The
registryinput description suggests it can push to an arbitrary registry, but this workflow only copies toquay.io...(and otherwise always publishes to GHCR). This is misleading for workflow_call users.
description: 'Alternative container registry to push images to (e.g., quay.io/your-username)'
.github/workflows/podvm_publish.yaml:70
push-to-quaycan be triggered even whenQUAY_PASSWORDis not provided (it is optional for workflow_call), which will make the job fail at login in forks/misconfigured callers. Gate the job on the secret being present (or fail fast with a clear message).
if: ${{ startsWith(inputs.registry, 'quay.io') }}
.github/workflows/podvm_publish.yaml:98
- This step is also mis-indented under
steps:and references$GHCR_IMAGE, which is never set (the script definesghcr_image). This will fail at runtime.
- name: Push podvm image to quay.io
env:
QUAY_REGISTRY: ${{ inputs.registry }}
GHCR_REGISTRY: ghcr.io/${{ github.repository_owner }}
ARCH: ${{ matrix.arch }}
IMAGE_TAG: ${{ inputs.image_tag || github.sha }}
run: |
ghcr_image="${GHCR_REGISTRY}/podvm-${ARCH}:${IMAGE_TAG}"
quay_image="${QUAY_REGISTRY}/$(basename "$ghcr_image")"
oras cp "$GHCR_IMAGE" "$quay_image"
stevenhorsman
left a comment
There was a problem hiding this comment.
I think this is fine to try and we should consider whether we can migrate everything over to ghcr.io as a follow on step, which I guess we need to check with downstream, or do as a deprecation notice?
91df061 to
325a841
Compare
325a841 to
1ce4167
Compare
In the release podvm publish worfklow we mirror the ghcr image to quay. Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
Currently the builds pass through a secret to quay.io. This will prohibit the building of the images on forks for pull request builds, forcing the use of the insecure pull-request-target mechanism in e2e tests, even though podvm builds can be executed safely on forks.
This change removes the secret and its uses at the call site and hard codes it to ghcr.io, which can be used in forks.
We can specify an additional registry in the podvm_publish step that will mirror the built images from ghcr.io to that registry. If it's a quay.io registry we login to that registry.
related to #2214