fix(ghrel): route the asset-download endpoint to bytes, not the JSON rewriter - #203
Merged
Merged
Conversation
…rewriter
The proxy treated every path under /repos/ as JSON metadata. One of them is
not: GitHub serves asset BYTES from
/repos/{owner}/{repo}/releases/assets/{id}
asked with Accept: application/octet-stream. That is how a client fetches an
asset from a PRIVATE repo, and it is what spc's GitHubRelease downloader uses
-- so it was not an edge case, it was the main path for every `type: ghrel`
artifact.
serveMetadata ran that binary body through rewriteAssetURLs, json.Unmarshal
failed, and the handler answered 502. The client saw only a failed download
with nothing pointing at the proxy:
Download artifact 'zlib' failed. Please check your internet connection
Last failed command: curl -sfSL --max-time 3600 ... - Exit code: 22
That took out every php-sdk Linux build the day this proxy was first enabled
(run 35611830865), after 8 retries each. The proxy was disabled on the fleet to
restore builds; this is the fix.
isAPIAssetPath matches that one shape and routes it to ServeArtifact against
the API upstream, so the bytes stream and cache like any other artifact.
Metadata paths are unaffected -- /releases, /releases/latest,
/releases/tags/{tag} and /releases/assets (no id) all still go to the rewriter,
which is pinned by a table test, because misrouting the other way would serve
every release document as opaque bytes and silently stop rewriting asset URLs.
Tests: a binary response under /repos/ served intact and cached (one upstream
hit across two requests), plus the routing predicate. Revert-verified -- with
the route removed the regression test reproduces the outage exactly,
"status 502, want 200".
|
ePHPm Preview — removed Preview deployment has been torn down. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ghrel proxy took out every php-sdk Linux build the day it was first enabled. This is the fix; the proxy is currently disabled on the fleet to keep builds working.
The bug
The proxy treated every path under
/repos/as JSON metadata. One of them is not — GitHub serves asset bytes from:That is how a client fetches an asset from a private repo, and it is what spc’s
GitHubReleasedownloader uses. So it was not an edge case — it was the main path for everytype: ghrelartifact.serveMetadataran that binary body throughrewriteAssetURLs,json.Unmarshalfailed, and the handler answered 502. The build saw only:Nothing pointing at the proxy — it invented the failure and the error blamed the network. Run 35611830865, 8 retries each.
The fix
isAPIAssetPathmatches that one shape and routes it toServeArtifactagainst the API upstream, so the bytes stream and cache like any other artifact.Metadata paths are unaffected and pinned by a table test —
/releases,/releases/latest,/releases/tags/{tag}and/releases/assets(no id) all still reach the rewriter. Misrouting the other way would serve every release document as opaque bytes and silently stop rewriting asset URLs, which is the same class of failure in the opposite direction.Testing
/repos/served intact and cached (one upstream hit across two requests)status 502, want 200go build,go vet, full./pkg/proxies/... ./pkg/config/... ./cmd/...suite, andmage lintall clean (the single staticcheck hit is the pre-existing Windows-only one inpkg/networking, which Linux CI does not build).Re-enabling
After this merges and ships,
[ghrel_proxy] enabled = truegoes back in the fleet config. Metadata caching was already working before the outage — the cache had entries and logged clean hits — so only the asset path was broken.