feat: pro rock in ACR - #1153
Conversation
b79f5c6 to
224baa8
Compare
Jobs to be done: Rebuild, Continuous scanning
224baa8 to
27d800d
Compare
Test-Rock previously either cached the decrypted, unpacked image (as on main) or, for encrypted Pro artifacts, skipped the cache and re-ran a per-job prepare-rock-for-testing action to download + decrypt + convert. The former persists Pro (private) image bytes unencrypted at rest in the Actions cache, violating the Pro invariant that image archives stay gpg-encrypted while exchanged and retained; the latter duplicated logic across every test job and disabled caching for Pro. Unify both paths: configure-tests now caches the OCI archive exactly as downloaded (still gpg-encrypted for Pro) under the run-scoped key, and each test job restores it, decrypts locally into its ephemeral workspace when a passphrase is present, then converts to the layout it needs. The cache therefore only ever holds encrypted bytes for Pro images, while public images are unaffected. - Drop the redundant `encrypted-artifact` input; Pro is detected solely by the presence of `pro-artifact-passphrase`, matching main and the rocks-template caller (which never set the input). - Remove the "Determine artifact type" / "Validate passphrase" steps and all `encrypted-artifact == ...` cache gating. - Delete the now-superseded prepare-rock-for-testing composite action. - Image.yaml: stop passing `encrypted-artifact` to Test-Rock. - Update workflow contract tests to assert the unified flow.
| - name: Encrypt pro artifact | ||
| if: ${{ matrix.pro-services != '' }} | ||
| uses: ./.github/actions/crypt-artifact | ||
| with: | ||
| mode: encrypt | ||
| input-path: ${{ steps.rename-oci-archive.outputs.name }} | ||
| passphrase: ${{ secrets.ROCKS_PRO_ARTIFACT_PASSPHRASE }} | ||
| output-path: ${{ steps.rename-oci-archive.outputs.name }}.gpg | ||
|
|
There was a problem hiding this comment.
See if this can be optimized, we can just rename the downloaded artifact, and choose to keep the encrypted artifact during the decryption.
| is-public-release: ${{ steps.release-type.outputs.is-public-release }} | ||
| is-pro-release: ${{ steps.release-type.outputs.is-pro-release }} |
There was a problem hiding this comment.
Consider refactoring these two boolean names to contains-x-release to reduce the ambiguity.
There was a problem hiding this comment.
Revisit this to see if we can avoid any changes to this established workflow.
There was a problem hiding this comment.
These changes seem more like a drive-by to me. Afaict they are not really required for any of the workflows to run correctly right? Since the inputs and outputs remain the same.
If so, I'd move this to a separate refactor PR.
There was a problem hiding this comment.
These changes aren't a drive-by refactor — they're required for Pro support, which is why the inputs/outputs are unchanged (the behavior change is internal to how Pro artifacts are handled).
Previously configure-tests unpacked the rock once and cached the unpacked OCI layout, which every downstream test job restored. For Pro images that would mean the decrypted image content sits in the shared, run-scoped cache — defeating the reason we GPG-encrypt the artifact in the first place.
To keep the property "cache encrypted archives, decrypt only in ephemeral test jobs", the restructuring:
- caches the raw archive (
<name>/<name>.gpg) instead of the unpacked layout, and - moves the
Decrypt Rock+Unpack Rocksteps out of the shared job into each individual test job, so decryption happens only inside the short-lived job that needs it and the plaintext is never persisted to the cache.
Public (non-Pro) images are unaffected: the Decrypt Rock step is gated on ARTIFACT_PASSPHRASE != '', so for them it's a no-op and the unpack behaves as before.
Given that, I'd prefer to keep it in this PR rather than split it, since the Pro test flow doesn't work correctly without it. Happy to revisit if you still see a piece here that's genuinely independent of Pro.
There was a problem hiding this comment.
Yes, this is essentially what I flagged back in: https://github.com/canonical/oci-factory/pull/706/changes#r2623388879 but wasn't ever addressed. Happy to keep it in this PR 👍
There was a problem hiding this comment.
as discussed on 1-1, overall approach looks good. a couple of drive-bys but i personally don't mind. i found a small pile of things to look at though:
- worth failing fast in the matrix on an
upload[*]entry withpro:but norelease: - pro build metadata in Swift records
digest: ""(the GHCR push is skipped), so a Pro revision has no content identity anywhere once the artifact expires. - pro archives are uploaded with default retention (90 day).
retention-days: 1?? Create Git tagruns for Pro too. just flagging.upload-rock/action.ymlstill decrypts with a<name>*glob while everything else moved to<name>.gpg.dockeris unpinned intests/etc/requirements.txtwhile everything else there is pinned.
and inline comments ofc
Co-authored-by: Marcin Konowalczyk <marcin.konowalczyk@canonical.com>
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
lczyk
left a comment
There was a problem hiding this comment.
#1153 (comment) was really my only blocker. everything else is flags / nice-to-have's so +1 from me
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
alesancor1
left a comment
There was a problem hiding this comment.
First pass
Overall the code looks good, I can see it's quite polished already after a couple iterations with @lczyk (thanks!).
For now, just a couple of nits and a bigger concern, check below:
There was a problem hiding this comment.
These changes seem more like a drive-by to me. Afaict they are not really required for any of the workflows to run correctly right? Since the inputs and outputs remain the same.
If so, I'd move this to a separate refactor PR.
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
|
The Docker repository for this image does not exist yet. Merging of this repo is blocked. See logs. |
alesancor1
left a comment
There was a problem hiding this comment.
Happy with the current state of this, thanks!
Is there any chance you could provide a test run before merging?
There was a problem hiding this comment.
Yes, this is essentially what I flagged back in: https://github.com/canonical/oci-factory/pull/706/changes#r2623388879 but wasn't ever addressed. Happy to keep it in this PR 👍
Jobs to be done: Rebuild, Continuous scanning
Description
Add support for building and releasing Ubuntu Pro Rocks to a private Azure Container Registry.
Changes
upload[*].pro.services(user-facing) andpro-release(no user-facing)._pro_releases.json. (no uploads to GHCR)Limitations
pro-release; releases must be done via a rebuild.Testing
All PR checks pass, including
pytest,bats, workflow contract tests, and mock-rock build, test, and release workflows.Related issues
Picture of a cool rock: