docs-infra: move 1.3 legacy PDF upload from Cloudflare REST to R2 S3 API - #2
Draft
andamasov wants to merge 2 commits into
Draft
docs-infra: move 1.3 legacy PDF upload from Cloudflare REST to R2 S3 API#2andamasov wants to merge 2 commits into
andamasov wants to merge 2 commits into
Conversation
The PDF step gates the Worker deploy (fail-fast, spec §5 site+PDF
consistency). Two dispatches on 2026-08-15 failed with Cloudflare 10429
on the REST object endpoint, blocking unrelated Worker changes. The
cause of the 429 is not established; the reason for this change is that
Cloudflare documents the REST object endpoint as the wrong tool for bulk
object transfer ("For most production workloads, it is recommended to
use the S3-compatible API or a Cloudflare Worker with an R2 binding
instead of this direct API method").
- Existence probe: `wrangler r2 object get --pipe` downloaded the whole
29.2 MiB object just to test existence (the CLI has no head-only
verb). Replaced with `aws s3api head-object` — metadata only.
- Probe failures are classified, not swallowed: 403 shares HeadObject's
exit status 254 with 404 and connection errors exit 255, so only an
explicit 404/NoSuchKey stderr shape may mean "absent"; auth, network,
and permission failures fail the step loudly.
- Upload: `aws s3api put-object` (single-part PUT; `aws s3 cp` would
multipart above 8 MiB) with an explicit `--content-type
application/pdf`, which the S3 API does not infer from the extension.
- Request/response checksum calculation pinned to `when_required`: R2
accepts only CRC64NVME for a single-part FULL_OBJECT PUT, and the AWS
CLI opts into a computed checksum by default. A post-upload
head-object size comparison replaces the integrity check given up.
- Dropped the now-unused `npm ci` and `CLOUDFLARE_API_TOKEN` from this
step; the Deploy step keeps its own.
force_pdf_refresh semantics, the 25-35 MiB size check, the %PDF- header
check, the fail-fast ordering, and the idempotent short-circuit are
unchanged.
Phase 0 CodeRabbit: the skip path accepted any object that merely existed. Now that the fresh-upload path verifies what it stored, hold the cached object to the same bar — the existence probe reads ContentLength + ContentType in the same HEAD (no extra request) and fails loudly when the stored seed is outside the 25-35 MiB range or is not application/pdf, rather than letting a truncated or mistyped object become the deploy's §5 fallback source. min/max hoisted so the bound has one definition shared by both paths.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
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.
What
Moves the 1.3 legacy PDF seeding step in
.github/workflows/deploy.ymloff the Cloudflare REST object endpoint (wrangler r2 object get/put) and onto R2's S3-compatible API (aws s3api head-object/put-object).The connection shape follows the precedent set by
docs-preview-deploy.ymlanddocs-preview-cleanup.ymlinvyos/vyos-documentation(introduced in vyos-documentation#2140) — same endpoint form, sameAWS_DEFAULT_REGION: auto, same secret names. Precedent for the shape only: those workflows' S3 write path has never actually executed (the preview deploy has always exited at its authorization gate, and the nightly cleanup has only ever listed), so it is not evidence that the AWS CLI's defaults transact successfully against R2. See the checksum section below.Why
The PDF step runs before the Worker deploy and gates it: if the PDF cannot be secured in R2, no Worker goes live. That preserves the spec §5 site+PDF consistency invariant, but it also means the step blocks unrelated changes when it fails.
Two dispatches on 2026-08-15 — runs 31885425647 and 31886303447 — both failed with Cloudflare
10429 Too Many RequestsonPUT /accounts/*/r2/buckets/vyos-docs-artifacts/objects/legacy/1.3/vyos-documentation.pdf.The cause of that 429 is not established. Size is not it (29.2 MB against a 300 MB REST ceiling), and the identical command succeeded on 2026-07-10. This PR does not claim to fix the 429. The rationale is that the REST object endpoint is the wrong tool for bulk object transfer — Cloudflare's own documentation for that endpoint says:
Changes
Existence probe. Was
wrangler r2 object get --pipe, which downloads the entire 29.2 MB object purely to test existence — the CLI exposes no head-only verb. Nowaws s3api head-object: metadata only. This alone removes the bulk of the traffic against the rate-limited key.Probe failures are classified, not swallowed. vyos-documentation#2209 fixed a near-identical bug where a probe that caught every failure could not tell "object absent" from an auth or network error, so a transient failure silently took a safe-looking path. The failure signalling here was established empirically against the AWS CLI rather than assumed:
An error occurred (404) when calling the HeadObject operation: Not FoundAn error occurred (403) when calling the HeadObject operation: ForbiddenCould not connect to the endpoint URL: "..."404 and 403 share an exit status, so the exit code alone cannot be trusted. Only an explicit
(404)/(NoSuchKey)stderr shape falls through to fetch+upload; everything else fails the step loudly with the captured stderr.Upload.
aws s3api put-object, notaws s3 cp—cpswitches to a multipart upload above its 8 MiB threshold, which would put this object on R2's composite-checksum path;put-objectis one deterministic single-part PUT, far below the 5 GiB single-PUT ceiling.--content-type application/pdfis explicit and load-bearing:wrangler r2 object putinfers the type from the file extension, the S3 API does not, and the apex Worker serves the object's stored type. Without it the PDF would be served asapplication/octet-stream.Cached-object validation (from Phase 0 review). The skip path previously accepted any object that merely existed. The probe now reads
ContentLength+ContentTypein the same HEAD — no extra request — and refuses to short-circuit on an object outside the 25–35 MiB bound or not typedapplication/pdf, rather than letting a truncated or mistyped seed become the deploy's §5 fallback source.Post-upload verification. A
head-objectsize comparison after the PUT, replacing the on-the-wire integrity check disabled below.Removed: the
npm ci(existed only fornpx wrangler; the Deploy step keeps its own) andCLOUDFLARE_API_TOKEN(unused by this step now) from the PDF step.R2_ACCESS_KEY_IDandR2_SECRET_ACCESS_KEYdo not exist in this repository yet. The same-named pair exists invyos/vyos-documentationfrom IS-579. This workflow will fail until an operator adds them here. The account ID keeps coming from the existingCLOUDFLARE_ACCOUNT_IDsecret, viaR2_ENDPOINT.The
aws-chunked/ checksum riskR2 implements only part of S3's request-integrity surface: for a single-part (
FULL_OBJECT) PUT it supports CRC64NVME and not CRC32/CRC32C/SHA-1/SHA-256. Cloudflare's guidance for its own Java SDK is to disable chunked encoding, which otherwise produces a signature mismatch (HTTP 403). boto3/botocore ≥ 1.36 defaultsrequest_checksum_calculationtowhen_supported, and the AWS CLI v2 is built on botocore, so it is exposed to the same behaviour.What I did: pinned both
AWS_REQUEST_CHECKSUM_CALCULATIONandAWS_RESPONSE_CHECKSUM_VALIDATIONtowhen_required, so the request is a plain, unadornedPutObject. Env-var spelling and semantics verified against the AWS SDK/CLI reference; the runner (ubuntu-latest→ Ubuntu 24.04) ships AWS CLI 2.36.20, well past the 2.23 release that introduced these settings.Honest caveat: AWS CLI v2's default checksum algorithm is CRC64NVME, which R2 does support for
FULL_OBJECT— so the default might well have worked. What cannot be determined without live R2 is whether the CLI delivers it as a plain header or as anaws-chunkedtrailing checksum.when_requiredis the conservative setting, not a claim that the default would have failed.This behaviour is unverifiable outside a live run against real R2 — no local test can exercise it. It is the highest-risk part of this change. The post-upload size read-back is the compensating control for the integrity check given up.
Verification
No live R2 exercise was possible (no credentials in this repo, and dispatching a deploy to test is out of scope).
origin/productionbaseline. No new findings.SC2050on theforce_pdf_refreshliteral, an artifact of substituting the GitHub expression for the local run).awsfixtures — therun:body is extracted from the YAML withyq(not hand-copied) and run against stubbedaws/curl. 8/8 pass:put-objectattemptedforce_pdf_refresh=falseforce_pdf_refresh=trueThe auth- and network-failure cases assert that no upload was attempted — the vyos-documentation#2209 lesson in executable form.
Preserved unchanged
force_pdf_refreshsemantics, the strictly-between 25–35 MiB size check, the%PDF-header check, the fail-fast ordering ahead of the Worker deploy, and the idempotent short-circuit.Phase 0 review disposition
Two local CodeRabbit rounds. Round 1 (cached object accepted without validation) is fixed — see "Cached-object validation" above. Round 2 suggested pinning a trusted SHA-256 digest of the PDF and verifying stored objects against it via R2 checksum storage; declined: there is no independently trusted digest to pin (the artifact is fetched from RTD, so a hardcoded digest would only record an unverified fetch), and it would make the step depend on exactly the R2 checksum path this PR cannot verify without a live run.
Why merging this matters beyond the PDF
workers/branch/wrangler.legacy.jsonconvyos/vyos-documentation@rollingalready carries"html_handling": "none", but the legacy Worker was last deployed 2026-07-10 and still runsauto-trailing-slash. Docs 1.3 and 1.2 currently return307on every explicit.htmlURL. Because the PDF step gates the deploy, that fix cannot land until this step can complete.Advances: IS-572
🤖 Generated by robots