Repository navigation
fix(ns8): normalise the registry path case on the fetch, not just the token - #152
shockalotti wants to merge 1 commit into
Conversation
… token d5cc8ee fixed this defect's token half and left the fetch half, which is why main has been red on every push since. The commit message was accurate about what it did ("lowercase the repo in assert-multiarc's token scope"), the gap was in reading it as the whole fix. The two endpoints disagree about case, and that is what hid it: token() lowercased the scope, so it asked for permission correctly get() built the manifest URL from the raw repo, so it asked the wrong URL ghcr's token endpoint wants a lowercase scope and answers HTTP 400 otherwise. ghcr's MANIFEST endpoint is stricter still: an uppercase path is simply a repository that does not exist, so it answers HTTP 404 for the very image whose lowercase path resolves. Lowercasing inside token() therefore made the release look fixed while the fetch kept failing. Measured against the real registry, same image, two spellings: ghcr.io/calnode/ns8-calnode:edge PASS ghcr.io/Calnode/ns8-calnode:edge HTTP 404 ghcr.io/calnode/ns8-calnode:latest PASS ghcr.io/Calnode/ns8-calnode:latest HTTP 404 ghcr.io/calnode/ns8-calnode:0.11.1 PASS ghcr.io/Calnode/ns8-calnode:0.11.1 HTTP 404 ns8-module.yml passes `ghcr.io/${{ github.repository_owner }}/ns8-calnode`, and github.repository_owner is `Calnode` verbatim. So every push to main and dev pushed :edge and then asserted a URL that cannot exist. Confirming that is the live failure and not something I introduced: failure 5b4acef push Merge pull request #151 failure d5cc8ee push lowercase the repo in token scope failure 3995484 push v0.11.1 failure 3a4ea7c push Merge pull request #150 failure 212f779 push Merge pull request #149 success 0f721a4 push Merge pull request #148 (assertion did not exist yet) #149 introduced the registry assertion and every push to main has been red since. Fixed with one normaliser applied where a registry path becomes a URL, so no caller can be missed and neither endpoint can drift again. The fix deliberately lives in the script rather than in the workflow's env: the caller passing `Calnode` is not the bug, and patching the env would leave every other caller and the app-repo path exposed to the same slip. python3 assert-multiarc.py ghcr.io/Calnode/ns8-calnode edge latest 0.11.1 PASS all 3 tag(s) are multi-arch with coherent labels Also corrects the docstring, which overstated the case against an app-repo override. An override used as a SUBSTITUTE for the label is a footgun and is why there is no such option today. An override used as an ASSERTION is the opposite: it would be strictly stronger, and --expect-app-repo would catch a module labelled with a foreign app repository, which --expect-app-tag cannot since a tag is not a repository. That is a real future need for a fork publishing under its own namespace. The docstring now says so and records that the distinction to preserve is which of the two it is, not whether to have one, instead of forbidding the flag outright. Full proof set after the change: uppercase repo, real tags exit 0 (was the red-main case) lowercase repo, real tags exit 0 (no regression) dead --app-repo flag exit 2, still rejected --pair --expect-app-tag 0.11.1 exit 0 genuinely missing app image FAILED in 2.2s, HTTP 404 mismatched pair exit 1, mismatch reported module-only contacts no app repo confirmed, still true gate logic from the shipped YAML 5/5 correct both workflows parse OK
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The fix is right and I reproduced it live — one explanatory claim in the new docstring doesn't match what ghcr actually does, plus a
--helpbloat note. Both inline, neither blocks the red-main fix.
Reviewed changes
normalize_repo()wired into all three registry entry points —token(),get()andget_blob()now lowercase the repo before it becomes a URL or a token scope, so thegithub.repository_owner-derivedCalnodecan no longer break either endpoint. I reproduced the fix against live ghcr from this runner:Calnode/ns8-calnodeandghcr.io/Calnode/ns8-calnodenow both resolve the real multi-arch index, and the--app-repodead flag still exits 2.- Dead-
--app-reporationale rewritten — the docstring now separates a substitute override (asserts the pair against a repo the module does not reference, so it hides a mislabelled module) from an assertion override (--expect-app-repo, strictly stronger, deliberately not added). That's the correction from the prior round, and the distinction it draws is the right one to preserve.
I also confirmed the failure story independently: gh run list shows every main push red in ns8-module.yml since 212f779 (#149), with the assertion step the only new thing in it.
space-bunny-free (free) ｜ 𝕏
| That combination is what made this invisible. The owner arrives from GitHub | ||
| as `github.repository_owner` verbatim, which is `Calnode` for this repo, so |
There was a problem hiding this comment.
The normaliser is correct and I confirmed it turns the red-main case green, but the mechanism in this docstring does not reproduce against ghcr today: an uppercase owner segment fetches fine (Calnode/ns8-calnode → HTTP 200, identical digest), and an uppercase scope is accepted too. What actually 404s is ghcr.io/Calnode/ns8-calnode — because both workflows pass a fully-qualified ref that the script prepends https://ghcr.io/v2/ to, and ghcr strips that redundant ghcr.io/ segment case-sensitively, which pushes the real owner into the case-sensitive repository-name position. Worth correcting, because the next person reasoning from this paragraph will conclude the owner case is what matters, when lowercasing Calnode in a bare Calnode/ns8-calnode changes nothing.
Technical details
# The docstring's causal claim is not reproducible; the real trigger is the doubled host
## Measured against ghcr.io (anonymous pull token for calnode/ns8-calnode, manifests/:edge)
| repo path | token scope | manifest |
|----------------------|-------------|-----------------------------|
| calnode/ns8-calnode | 200 | 200 |
| Calnode/ns8-calnode | 200 | 200, same digest |
| CALNODE/ns8-calnode | 200 | 200, same digest |
| calnode/NS8-CALNODE | - | 404 |
| ghcr.io/calnode/ns8-calnode | 200 | 200, same digest |
| ghcr.io/Calnode/ns8-calnode | 400 | 404 |
| ghcr.io/foo/bar | - | 404 |
`calnode/ns8-calnode`, `CALNODE/ns8-calnode` and `ghcr.io/calnode/ns8-calnode` all
return digest sha256:06a54f2c...713 — so ghcr matches the OWNER segment
case-insensitively, silently strips a redundant leading `ghcr.io/` segment, and is
case-sensitive on the remainder. `ghcr.io/Calnode/ns8-calnode` 404s because after the
stripped host, `Calnode` sits in the repository-name position.
## Affected sites
- packaging/ns8/assert-multiarc.py:99-103 — the paragraph claims the manifest endpoint
answers 404 for "an uppercase path". Only true for the repository-name segment, and
the case that actually broke CI (`Calnode`, the owner) is fine until the redundant
host moves it into that position.
- packaging/ns8/assert-multiarc.py:120,131 — `get()`/`get_blob()` interpolate the caller's
`repo` verbatim into `{REGISTRY}/v2/{repo}/...`, so a fully-qualified arg yields
`https://ghcr.io/v2/ghcr.io/...`.
- .github/workflows/ns8-module.yml:264 and .github/workflows/release-verify.yml:114 —
both set `MODULE_REPO: ghcr.io/${{ github.repository_owner }}/ns8-calnode`, i.e. they
pass a fully-qualified ref into a positional arg documented (assert-multiarc.py:258) as
`calnode/ns8-calnode`. Neither validates the arg's shape.
- packaging/ns8/assert-multiarc.py:114 — the token scope built from that arg is
`repository:ghcr.io/calnode/ns8-calnode:pull`, which ghcr accepts today but which no
spec authorises. Lowercasing fixes the case, not the malformed scope.
## Required outcome
- The docstring must state the rule that is actually true and still worth obeying: a
repository NAME must be lowercase (OCI distribution-spec `<name>` regex
`[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*`, tags are the part that may be uppercase), and
`docker`/`podman`/`skopeo` all reject an uppercase name client-side. ghcr's leniency
in the owner segment is undocumented and can change, so normalising is load-bearing,
not defensive. That argument survives whatever ghcr does tomorrow; "it 404s" may not.
- The paragraph should stop attributing the CI failure to the owner's case alone.
## Suggested approach
Keep the three `normalize_repo()` call sites — the fix works and matches the precedent
already in packaging/ns8/push-manifest.sh:36 (`repobase="${repobase,,}"`). Two optional
improvements, both small:
1. Parse the positional arg once at the entry point rather than lowercasing at three URL
builders: strip a leading host with the same logic `split_ref()` already uses for
`org.nethserver.images` labels (assert-multiarc.py:141), reject a host that is not
ghcr.io, then lowercase. That yields a well-formed `repository:calnode/ns8-calnode:pull`
scope and `https://ghcr.io/v2/calnode/ns8-calnode/manifests/edge`, so the script stops
depending on ghcr tolerating a doubled path segment. It also retires the loose wording
"the single point where a registry path becomes a URL" — there are three call sites
today (harmless, the function is idempotent, but a future fourth URL builder is not
covered by that sentence).
2. `normalize_repo` is pure and CI only runs `python3 -m py_compile` on this file
(ns8-module.yml:36). A handful of offline unit tests over the URL and scope strings
would have caught the token-vs-fetch half of this bug without a network call, which is
the failure mode this file has now produced twice.
## Open questions for the human
- Was the HTTP 400 on an uppercase scope, and the 404 on the uppercase manifest, actually
observed, or inferred? My measurements cannot reproduce either for the owner segment
today. If it was observed, the authenticated-vs-anonymous distinction may be the missing
variable and is worth a sentence in the docstring; if it was inferred, the paragraph
should not claim it as measured.| hides a mislabelled module instead of catching it. That is the same hole | ||
| --expect-app-tag exists to close, one level up. | ||
|
|
||
| An override used as an ASSERTION instead would be strictly stronger, not weaker, |
There was a problem hiding this comment.
This module docstring is argparse's description, so it is printed verbatim by --help — which is now 55 lines, roughly 23 of them an essay about --expect-app-repo, a flag that does not exist, and argparse's default formatter collapses the paragraphs into one run-on block. The substitute-vs-assertion distinction is worth keeping and worth the comment density this file already has; consider shortening this section to the rule itself (two or three sentences) and leaving the rest as an inline comment near --expect-app-tag in main(), where it is readable but does not hit every --help.
|
Closing as superseded — the premise doesn't hold, and I want that on the record rather than left implicit. The fetch half doesn't need normalising. ghcr's manifest endpoint tolerates case in the repository path; only the token endpoint requires lowercase: And the script on So d5cc8ee was a complete fix, not half of one. There is no fetch defect here, and this PR would change working code to chase one. It also doesn't fix what it claims to fix. The REST API only accepts a workflow file name or numeric id there, and under The reasoning in the docstring correction was worth having and has been carried into the follow-up — that an override is a footgun as a substitute but valuable as an assertion, and which of the two it is, not whether to have one, is the line to preserve. That part landed; it just doesn't need this PR to land. Thanks for reproducing it properly rather than taking my finding on trust — the reproduction is what exposed that my fix was already sufficient. |
1 participant
Main is red right now. This fixes it. Not merging: yours to call.
You were right about the docstring, and I have corrected it
Your correction stands. I conflated two different things and wrote the stronger claim into a docstring, which is exactly where a wrong rule does damage. You are right that an override used as an assertion is strictly stronger than what I have:
--expect-app-tagcatches a wrong app tag, but a module labelledghcr.io/somesomeoneelse/calnode:0.11.1still passes today, because a tag is not a repository and derivation-from-label cannot express that constraint. For a fork publishing under its own namespace that check is genuinely wanted.The docstring now says: an override as a substitute is a footgun and there is deliberately no such option; an override as an assertion is the opposite, is a real future need, and is not needed until someone maintains such a fork. The line to preserve is which of the two it is, not whether to have one. I did not add the flag, because it is not needed yet and you own that call.
The other thing, which is worse than the docstring
Merging #151 turned main red, and it is not something #151 caused.
d5cc8eefixed this defect's token half and left the fetch half.The two ghcr endpoints disagree about case, and that is what hid it:
The token endpoint wants a lowercase scope (HTTP 400 otherwise). The manifest endpoint is stricter: an uppercase path is simply a repository that does not exist, so it returns HTTP 404 for the very image whose lowercase path resolves. Lowercasing inside
token()made the release look fixed while the fetch kept 404ing.Your commit message said "lowercase the repo in assert-multiarc's token scope", which is accurate. The gap was in reading it as the whole fix, mine included.
Measured, same image, two spellings:
This is the live failure, and it predates everything today
ns8-module.ymlpassesghcr.io/${{ github.repository_owner }}/ns8-calnode, andgithub.repository_ownerisCalnodeverbatim. So every push to main and dev pushes:edgeand then asserts a URL that cannot exist:#149 introduced the registry assertion, and every push to main has been red since. This is the second time a gate looked correct in review and had never run: #148 for the manifest assembly, this for the path case. Your point 2 was about
release-verify.ymlnever having executed; this is the same failure one layer earlier, in a workflow that has been executing and failing.The fix
One normaliser, applied where a registry path becomes a URL, so no caller is missed and neither endpoint can drift apart again. It lives in the script, not in the workflow's env: the caller passing
Calnodeis not the bug, and patching the env would leave every other caller and the app-repo path exposed to the same slip.Proof set after the change
--app-repoflag--pair --expect-app-tag 0.11.1For your decision
--expect-app-reponow as a pure assertion. It is ~5 lines, it has a real use case in your fork scenario, and it would close the remaining asymmetry in the pair check. I have left it out because you own NS8 scope and it is not needed for the red-main fix.release-verify.ymlis now confirmed working in the wild: it fired on the merge, correctly resolved no tag for a non-tag commit, wroteoutcome=not-verifiedand exited 0. That is the designed path, observed rather than argued.Still no tag pushed and nothing merged from me. Worktree is
/tmp/ns8-case-fix;/tmp/nm-registryis removed as you asked, and~/Projects/calnodeis untouched onmainat5b4acef.