feat(llmo): add Cloudflare Logpush onboarding endpoint (LLMO-5869)#2784
Draft
IrisAlexandrescu wants to merge 5 commits into
Draft
feat(llmo): add Cloudflare Logpush onboarding endpoint (LLMO-5869)#2784IrisAlexandrescu wants to merge 5 commits into
IrisAlexandrescu wants to merge 5 commits into
Conversation
added 2 commits
July 9, 2026 14:49
New POST /sites/:siteId/llmo/cdn-onboard/cloudflare/zones/:zoneId/logpush, completing the Cloudflare CDN-log-automation flow: idempotency check against existing Logpush jobs -> trigger an ownership challenge -> fetch the resulting token from auth-service (LLMO-6116, forwarding the caller's own bearer token; api-service never touches the customer's cdn-logs S3 bucket directly) -> create the Logpush job. Recovers from a create-time race by re-listing jobs rather than pattern-matching an unverified Cloudflare error message (the exact duplicate-destination error shape is an open E2E-only confirm per the ticket). Route/capability/FACS-param wiring, OpenAPI block, and an AUTH_SERVICE_BASE_URL env var doc. Depends on (not yet released): @adobe/spacecat-shared-cloudflare-client's new requestLogpushOwnership/listLogpushJobs/createLogpushJob methods and @adobe/spacecat-shared-utils' new CLOUDFLARE_LOGPUSH_FIELDS export (both in-flight PRs on spacecat-shared). Verified locally via npm link against the real unpublished source; package.json version bumps are deliberately deferred to a follow-up once those publish, rather than pinning to versions that don't exist yet.
…iew feedback) auth-service's cloudflare-ownership-token endpoint switched from GET to POST (MysticatBot flagged GET on a side-effecting endpoint as an HTTP-semantics issue). Updates the caller in lockstep: siteId/filename now go in a JSON body instead of URL query params.
|
This PR will trigger a minor release when merged. |
nit23uec
reviewed
Jul 10, 2026
| site-llmo-cloudflare-logpush: | ||
| parameters: | ||
| - $ref: './parameters.yaml#/siteId' | ||
| - name: zoneId |
Contributor
There was a problem hiding this comment.
please avoid cloudflare owned terms like zoneId in path segments. they should be captured in the body.
nit23uec
reviewed
Jul 10, 2026
| 'GET /sites/:siteId/llmo/cdn-onboard/cloudflare/zones': llmoCloudflareController.listZones, | ||
| 'POST /sites/:siteId/llmo/cdn-onboard/cloudflare/deploy': llmoCloudflareController.deployWorker, | ||
| 'POST /sites/:siteId/llmo/cdn-onboard/cloudflare/routes': llmoCloudflareController.addRoute, | ||
| 'POST /sites/:siteId/llmo/cdn-onboard/cloudflare/zones/:zoneId/logpush': llmoCloudflareController.createLogpush, |
Contributor
There was a problem hiding this comment.
Suggested change
| 'POST /sites/:siteId/llmo/cdn-onboard/cloudflare/zones/:zoneId/logpush': llmoCloudflareController.createLogpush, | |
| 'POST /sites/:siteId/llmo/cdn-onboard/cloudflare/logpush': llmoCloudflareController.createLogpush, |
nit23uec
reviewed
Jul 10, 2026
| }, 409); | ||
| } | ||
|
|
||
| const siteId = site.getId(); |
Contributor
There was a problem hiding this comment.
we should check if the registrable domain of the zone in the zoneId matches with the given site base url or not.
… site (review feedback) nit23uec review on this PR: 1. zoneId moved from a path segment to the request body — it's a Cloudflare identifier, not a SpaceCat entity, matching addRoute's existing convention in this same file. Route is now POST /sites/:siteId/llmo/cdn-onboard/cloudflare/logpush. Updated routing, S2S capability, FACS param classification (removed the now- unneeded zoneId non-resource-param entry), OpenAPI, and tests accordingly. 2. Added a registrable-domain check: fetches the zone via the new CloudflareClient.getZone(zoneId) (spacecat-shared, companion PR) and rejects with 400 if the zone's domain doesn't match the site's own domain, before doing anything else. This also let a dead code path be removed: the Logpush job's "name" field previously recomputed registrableDomain(site.getBaseURL()) a second time with a "|| siteId" fallback that could never fire once the new guard (using the identical computation) already validates it's non-null. Depends on @adobe/spacecat-shared-cloudflare-client's new getZone() method (not yet released — same publish-then-bump sequencing as the existing CLOUDFLARE_LOGPUSH_FIELDS dependency).
Author
|
@nit23uec Addressed both in `d904f110`:
One side effect worth flagging: the domain check let me delete a small piece of dead code — the Logpush job's `name` field used to recompute `registrableDomain(site.getBaseURL())` a second time with a `|| siteId` fallback. That fallback can never fire now, since the new guard performs the identical computation earlier and already guarantees it's non-null before we get there. Simplified to reuse the guard's result directly. 98 tests passing, 100% coverage on the controller, verified locally against the real (linked, not-yet-published) `getZone` implementation. |
added 2 commits
July 13, 2026 18:22
R3 — corrected stale JSDoc on createLogpush that still described a path-param
zoneId / .../zones/:zoneId/logpush route; the shipped route is .../cloudflare/logpush
with zoneId in the body.
E2 — idempotency + TOCTOU re-check now match an existing Logpush job's
destination_conf component-wise (bucket + path prefix + region, query-param-order-
insensitive) via sameLogpushDestination(), instead of exact string equality.
Cloudflare may store/echo destination_conf normalized (reordered/extra params,
resolved {DATE}); exact-string matching would miss it and create a duplicate job.
E3 — bounded retry (default 1x, 2s, overridable via CF_OWNERSHIP_TOKEN_RETRY_DELAY_MS,
default needs no deploy wiring) on the ownership-token read-back to absorb the
Cloudflare challenge-file read-after-write race. Re-reads the same filename only;
never re-triggers the ownership challenge (which would orphan challenge files).
Tests: normalized-destination match, different-region non-match, malformed-conf
non-match, retry-then-succeed (asserts no second ownership challenge), default-delay
branch. 104 passing, 100% coverage on the controller.
…prefix)
The .env.example default was wrong (auth.experiencecloud.live, no /api prefix).
Corrected to the real value shape and documented the load-bearing constraint,
verified against spacecat-infrastructure's Fastly VCL:
- must be the LLMO gateway host (llmo.experiencecloud.live), per-env /api/{ci|stage|v1};
- NOT the spacecat host — the edge overwrites x-product from Host (llmo->LLMO,
spacecat->ASO), and the endpoint requires x-product: LLMO or 403s;
- routing through the public gateway path also injects the API-Gateway auth token,
so a direct/internal call would be rejected.
Comment + .env.example only; no behavior change.
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
2 participants
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.
DRAFT — blocked on 2 upstream repos publishing
This PR is intentionally opened as a draft. It depends on two in-flight, not-yet-merged/published PRs:
spacecat-sharedfeat: add filterByTopOrganicSearchPages parameter for RUM metrics #1796 — addsCLOUDFLARE_LOGPUSH_FIELDSexport (this repo'soutput_options.field_namessource)spacecat-auth-servicechore(deps): update adobe fixes #609 (LLMO-6116) — adds the ownership-token endpoint this repo callsI verified this PR's code locally via
npm linkagainst the real unpublished source in both repos (not mocks-only), so the logic is proven — butpackage.jsonstill pins the old published versions of@adobe/spacecat-shared-cloudflare-client(1.2.1) and@adobe/spacecat-shared-utils(1.124.1), neither of which has the new exports yet. CI will fail on this PR as-is until those two publish and this PR's dependency versions are bumped for real. Please review the code now; hold the merge until the bump.Summary
POST /sites/:siteId/llmo/cdn-onboard/cloudflare/zones/:zoneId/logpush, completing the Cloudflare log-forwarding onboarding flow:http_requestsdataset, matched on the computeddestination_conf— Cloudflare job creation is not inherently idempotent, unlike CloudWatch LogsCreateDelivery, so this is the primary guard.requestLogpushOwnership) — Cloudflare writes a token file to the customer's cdn-logs S3 bucket.spacecat-auth-service's new endpoint (LLMO-6116), forwarding the caller's ownAuthorizationheader rather than a service credential — api-service never touches the S3 bucket directly (least-privilege boundary; only auth-service has that access).createLogpushJobfailure, re-lists jobs and treats a now-matching job as success, rather than pattern-matching an unverified Cloudflare error message — the exact duplicate-destination error shape is called out in the ticket as an open, E2E-only confirm, not something to guess at.cdnBucketConfig) — returns409with an actionable message otherwise.zoneId) + OpenAPI (site-llmo-cloudflare-logpush) wiring..env.exampledoc for the newAUTH_SERVICE_BASE_URLenv var (per-environment, not yet in real deploy config).Related Issues
LLMO-5869 (epic LLMO-5809)
Test plan
llmo-cloudflare.test.js— happy path, idempotent no-op, race recovery, 409 missing-config (both variants), 400s (zoneId format/missing, CF token missing), 404 site-not-found, 502s (job listing / ownership challenge / auth-service call failure incl. non-ok + missing-token / job creation with failed re-check), Authorization-header-forwarding assertion, null-list defensive branchesllmo-cloudflare.test.jssuite — 92 passing, 100% statement/branch/line/function coverage onllmo-cloudflare.jsnpm test) — 14422 passing, coverage thresholds greennpm run docs:lint— valid, no new warningsspacecat-sharedsource — not mocked aroundNot yet verified (needs a dev deploy + real Cloudflare zone, tracked as follow-up, not blocking this review)