Skip to content

packaging(ns8): address code review - multi-arch module image and workflow hardening - #137

Open
shockalotti wants to merge 1 commit into
mainfrom
packaging/ns8-review-hardening-ocr
Open

shockalotti wants to merge 1 commit into
mainfrom
packaging/ns8-review-hardening-ocr

Conversation

@shockalotti

Copy link
Copy Markdown
Contributor

Follow-up to #102 and #135, from a code review of the NS8 packaging.

1. Module image was single-arch (high). The app image builds amd64+arm64 with qemu; the module image did not. NS8 runs modules as rootless containers on the node, so an arm64 node could not install Calnode at all. build-images.sh now honours TARGETARCH and pushes only per-arch :build-<arch> tags; new push-manifest.sh assembles the public tags. Publishing an assembled tag only, after every arch has landed, means an interrupted release cannot ship a single-arch image under a version tag. CI registers qemu and builds both arches.

2. The exec-bit guard failed open (medium). The awk added in #135 exits 0 on empty input, so a pathspec that stopped matching would pass vacuously and silently reintroduce the non-executable-script bug that made configure a no-op. Now asserts the file list is non-empty first. Verified three ways: passes on the real tree, fails loudly on an empty pathspec, fails on a 100644 entry.

3-6. timeout-minutes on both jobs; permissions: contents: read on validate; GITHUB_TOKEN/actor/event_name moved into env: instead of being interpolated into run:; ui/node_modules cached like upstream NS8 (every PR was re-downloading the whole Vue/Carbon tree).

7. Stale step-4 comment in 20configure.

Also aligned actions/checkout (v7) and setup-qemu-action (v4) with the rest of the repo instead of diverging.

Not verified: no buildah on this machine, so the multi-arch manifest path could not be smoke-tested locally. It needs one CI run to confirm before release. The exec-bit guard, schemas, shell syntax and YAML all validated locally.

Note on branch naming: an earlier attempt at this PR was created against packaging/ns8-review-fixes, which turned out to hold the scheduling-invitations work tracked by #129. That stray branch was left untouched and PR #136 was closed.

…kflow hardening

Code review of the NS8 module packaging found seven issues. Fixed here:

1. Module image was amd64-only while the app image is multi-arch. NS8 runs
   modules as rootless containers on the node, so an arm64 node could not
   install Calnode at all. build-images.sh now honours TARGETARCH and
   publishes only per-arch :build-<arch> tags; push-manifest.sh assembles
   the public tags, so an interrupted release cannot publish a single-arch
   image under a version tag. CI registers qemu and builds both arches.
2. The exec-bit guard added in #135 failed open: the awk exited 0 on empty
   input, so a pathspec that stopped matching would pass vacuously and
   silently reintroduce the non-executable-script bug. It now asserts the
   file list is non-empty first. Verified: passes on the real tree, fails
   loudly on an empty pathspec, fails on a 100644 entry.
3. Neither job declared timeout-minutes.
4. The validate job declared no permissions block.
5. secrets.GITHUB_TOKEN, github.actor and github.event_name were
   interpolated directly into run: blocks; they now go through env:.
6. No dependency caching for the UI build, so every branch and PR run
   re-downloaded the whole Vue/Carbon tree. Now cached like upstream NS8.
7. Stale step-4 comment in 20configure that no longer matched the code.

Also aligned actions/checkout and setup-qemu versions with the rest of the
repo (v7 / v4) instead of diverging.

Not verified: no buildah on the dev machine, so the multi-arch manifest
path could not be smoke-tested locally. It needs one CI run (or a real
buildah host) before release.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

push-manifest.sh cannot succeed as written — it calls buildah image exists (no such command) and passes a third positional argument to buildah manifest add (which errors). Because build-images.sh now pushes only the private :build-<arch> tags, this step is the only thing that publishes the public tags, so on the next main/tag push the job fails and no :edge/:X.Y.Z/:latest module tag is published at all. Both root causes are fixed inline below; the exec-bit guard, qemu/cache/env hardening, arch stamping and the comment moves are all fine.

Reviewed changes

  • Multi-arch module image — build-images.sh gains TARGETARCH, builds a pinned per-arch image (buildah from --arch) and, on PUSH=1, pushes only the private :build-<arch> tag; new push-manifest.sh is meant to assemble the public tags from those.
  • CI hardening — timeout-minutes on both jobs, permissions: contents: read on validate, REF/REF_NAME/ACTOR/EVENT_NAME/GHCR_TOKEN moved into env: instead of interpolated into run:, setup-qemu-action@v4, a yarn node_modules cache, and a per-arch build loop followed by a manifest step.
  • Exec-bit guard — asserts the action-script list is non-empty before the mode check, so a pathspec that stops matching can no longer pass vacuously.
  • Docs / comment — README multi-arch section; 20configure step-order comment.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash | 𝕏

fi

for arch in "${arches[@]}"; do
if ! buildah image exists "${repobase}/${reponame}:build-${arch}"; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildah has no image subcommand — it is buildah images (list) and buildah manifest exists (checks a manifest list). buildah image exists … fails with an unknown-command error, and because the test is if ! buildah image exists …, that failure is read as "image missing", so the loop always prints Missing local image … and the script exits 1 before assembling anything.

Technical details
# Wrong existence check in push-manifest.sh

## Affected sites
- `packaging/ns8/push-manifest.sh:39` — `buildah image exists "${repobase}/${reponame}:build-${arch}"`

## Required outcome
- The check must exit 0 when the per-arch image is present locally and non-zero when it is not.

## Suggested approach (optional)
- `buildah inspect --type image "${repobase}/${reponame}:build-${arch}" >/dev/null 2>&1`, or
- `buildah images --format '{{.Name}}:{{.Tag}}' | grep -qxF "${repobase}/${reponame}:build-${arch}"`.
- Verified against buildah source: `manifest init()` registers no `image` command (only `images`), and the `buildah(1)` command table lists no `image` entry.

Comment on lines +52 to +55
buildah manifest add \
"${manifest}" \
"${repobase}/${reponame}:build-${arch}" \
"docker.io/calnode/ns8-calnode:${imagetag}-${arch}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildah manifest add accepts exactly two positional arguments, <list> <image>; a third argument is rejected (At least two arguments are necessary: list and image to add to list on the runner's buildah, too many arguments… on current main). It is only treated as extra input when --artifact is set. With set -e this aborts the script, so no public tag is ever pushed.

The image's platform is already taken from its config, and buildah from --arch stamps that correctly, so the third argument is not needed.

Technical details
# Too many arguments to `buildah manifest add`

## Affected sites
- `packaging/ns8/push-manifest.sh:52-55` — `buildah manifest add "${manifest}" "${repobase}/${reponame}:build-${arch}" "docker.io/calnode/ns8-calnode:${imagetag}-${arch}"`

## Required outcome
- Add exactly the local per-arch image to the manifest list and let its recorded platform come from the image config.

## Suggested approach (optional)
- Drop the third argument entirely, i.e. `buildah manifest add "${manifest}" "${repobase}/${reponame}:build-${arch}"`.
- If you want the platform to be explicit rather than inferred, add `--arch "${arch}"`.
- Note the dropped value also hardcodes `docker.io/calnode/ns8-calnode`, which diverges from `${repobase}/${reponame}` (e.g. a fork's owner) — another reason to remove it rather than repoint it.

## Verified
- `buildah manifest add` non-artifact path rejects `len(args) > 2`; checked in both buildah `main` and `v1.33.7` (Ubuntu 24.04 runner).
- `buildah from --arch arm64 scratch` + `buildah commit` records `Architecture=arm64` in the config (`fixupConfig` uses `systemContext.ArchitectureChoice`), so `manifest add` reads the right platform without `--arch`.

for imagetag in "${imagetags[@]}"; do
manifest="${repobase}/${reponame}:${imagetag}"
# Drop any stale manifest so a retry cannot inherit old entries.
buildah manifest rm --all "${manifest}" 2>/dev/null || true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildah manifest rm takes no --all flag — its only arguments are the manifest-list names. The unknown-flag error is swallowed by 2>/dev/null || true, so this line never removes anything and the "drop any stale manifest" the comment promises does not happen. On a re-run with the same tag, manifest create would then fail with already exists.

Technical details
# `buildah manifest rm --all` is not a valid invocation

## Affected sites
- `packaging/ns8/push-manifest.sh:49` — `buildah manifest rm --all "${manifest}" 2>/dev/null || true`

## Required outcome
- Remove any existing local manifest list named `${manifest}` before recreating it, without erroring when none exists.

## Suggested approach (optional)
- `buildah manifest rm "${manifest}" 2>/dev/null || true`.
- `manifest rm` has no flags registered; only the `rm`/`remove` positional list names are accepted.

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.

1 participant