feat(metrics-emf): warn once when AWS_ENV/ENV missing#2835
Open
abhishekgarg18 wants to merge 2 commits into
Open
feat(metrics-emf): warn once when AWS_ENV/ENV missing#2835abhishekgarg18 wants to merge 2 commits into
abhishekgarg18 wants to merge 2 commits into
Conversation
When neither `AWS_ENV` nor `ENV` is set on the Lambda, `resolveEnvironment`
silently returns `'dev'` — meaning every metric routes to the dev dashboard
while prod dashboards read zero traffic. That's a P1-shaped failure mode with
no observable signal today: on-call notices the wrong-tier dashboard is
suddenly noisy hours after a deploy, or worse, doesn't notice at all because
they only watch prod.
Change `resolveEnvironment(env, {log})` to log.warn once per Lambda instance
when the default fallback fires, so a broken env-var manifest surfaces on the
first cold-start invocation. Rate-limited via a module-scoped latch so a
mis-configured deploy doesn't flood the log with one warn per request.
Defensive: skip the warn if `log.warn` is absent (some helix / Lambda
contexts only guarantee `log.info` and `log.error`) so the request path never
crashes.
Callers threaded through:
- src/controllers/redirects.js (ASO overlay)
- src/controllers/webhooks.js (GitHub webhook enqueue + errorHandler)
- src/support/aso-overlay-key-handler.js
- src/support/github-webhook-hmac-handler.js
Backwards-compatible for legacy callers that pass no options — return value
and precedence rules unchanged. Test-only reset export so the warn-once latch
doesn't cross-contaminate tests.
Follow-up to PR #2832 post-review audit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ent call Post-review finding on this PR: I missed a 5th call site — `BrandsController` emits `BrandDemotionBlocked` metrics and would fall back to `'dev'` silently if the Lambda's env-var manifest dropped AWS_ENV/ENV. Threading `log` through here means all metric-emitting controllers now surface the mis-config warning on cold start. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
abhishekgarg18
changed the title
|
This PR will trigger no release when merged. |
abhishekgarg18
temporarily deployed
to
dev-branches
July 16, 2026 06:31 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Jul 16, 2026
abhishekgarg18
changed the title
miakobchuk
added a commit
that referenced
this pull request
Jul 16, 2026
…2832) > **Defense-in-depth positioning** (2026-07-16). Cornel Isbiceanu (Skyline dispatcher team) noted on Slack that in steady state preview pods aren't supposed to be polling this endpoint — the ASO API key is only injected in publish pods, not preview. This PR is *not* meant to normalize preview traffic; it's meant to make api-service resilient to whatever URL shape the dispatcher sends. Rationale: > > 1. **Auth is still required.** Accepting the `-prev` URL shape at the route regex does not weaken auth: `AsoOverlayKeyHandler` still requires a valid `X-ASO-API-Key` regardless of URL shape. Preview pods without the key are still rejected — just as 401 from the origin handler instead of Fastly edge, or 401 from Fastly edge for canonical URLs. Same posture, different layer. > 2. **Fastly edge remains the primary signal.** Skyline's diagnostic surface for "preview pods are misconfigured" is the Fastly edge log (401 rate by URL). That signal is unchanged by this PR — we only affect what happens *if* a request reaches origin. Preview pods still get 401'd at edge today because they lack the key; that alarm still fires. > 3. **Zero-cost when preview eventually gets the key.** If Skyline / AEM CS ever provision the API key on preview pods (planned or not), this code just works with no follow-up patch. Symmetric to how we already handle the same overlay across dev/stage/prod tiers. > 4. **Future-proof against dispatcher changes.** If Skyline's URL construction changes to strip `-prev` upstream, this PR becomes a no-op. If it doesn't, origin behavior is deterministic instead of a shape-based 400. > > This is a resilience-hardening PR, not a functional change. ## Summary AEM Cloud Service preview publish pods send `AEM_SERVICE=cm-pN-eN-prev`. Skyline dispatcher builds the overlay URL from that env var verbatim, so preview traffic hits us with `/config/cm-pN-eN-prev/redirects.txt`. Two api-service regexes reject the shape today: 1. `AsoOverlayKeyHandler.OVERLAY_ROUTE` does not match `-prev`, so the request falls through the auth chain and 401s at `authWrapper` before the controller runs. 2. Even with correct edge auth, `RedirectsController.SERVICE_RE` would 400 the shape. Mystique writes exactly one overlay per (program, environment) — publish and preview tiers share it. This PR accepts the suffix at both boundaries and strips it before all downstream lookups (Site lookup, S3 key, Surrogate-Key). Origin-side stripping avoids requiring a Fastly VCL change and lands the fix on our side alone. **Auth model is preserved end-to-end.** `AsoOverlayKeyHandler` still requires `X-ASO-API-Key` regardless of URL shape — accepting `-prev` at the route regex does not weaken any authentication guarantee. A future auth-chain regression that accidentally auto-allowed keyless overlay-route requests would be the actual security problem, and that risk exists for `cm-pN-eN` as much as for `cm-pN-eN-prev`. ## Fastly cache footprint Fastly caches on raw URL, so preview and publish create two separate cache entries per tenant. Trade-off is acceptable because: 1. Both entries share `Surrogate-Key: aso-overlay-<canonical>`, so mystique's purge-on-Deploy invalidates both in a single call — no divergence risk. 2. Both fetch the same S3 object (canonical), so upstream is not doubled beyond initial per-URL fills — subsequent polls are cache hits. 3. At 10k tenants × ~20% preview coverage the extra entries are cheap. 4. No cache-poisoning risk — publish and preview have distinct cache keys. A future normalization at Fastly VCL (`regsub` in `vcl_recv` before cache-key derivation) would collapse to one entry per tenant, but that requires Fastly-admin territory. Origin-side is the pragmatic bridge. ## Changes - `SERVICE_RE` (`src/controllers/redirects.js:43`): accept optional trailing `-prev`, case-sensitive - `RedirectsController.getRedirects`: derive `canonicalService` from captured program/environment ids; use it for S3 key + Surrogate-Key on both 200 and 304 branches - `AsoOverlayKeyHandler.OVERLAY_ROUTE` (`src/support/aso-overlay-key-handler.js:33`): accept `-prev` so the auth handler doesn't 401 preview traffic before it reaches the controller - **`s3DurationEmitted` idempotency guard** (audit follow-up): fix double-emission of `AsoOverlayS3ReadDurationMs` when the body stream throws post-`send()`. Test in `redirects.test.js` asserts exactly one emission with SUCCESS result (first-writer-wins). - **OpenAPI `Surrogate-Key` documentation** (audit follow-up): document the header on both 200 and 304 response schemas so the mystique purge-on-Deploy contract is discoverable - Unit tests: `-prev` happy path, 304 on `-prev`, malformed variants still 400 (`-preview`, `-PREV`, `-prev-2`, trailing hyphen), auth handler accepts `-prev` - IT test: `-prev` end-to-end against the seeded canonical overlay - OpenAPI: pattern + description updated to document optional `-prev` Jira: [SITES-48140](https://jira.corp.adobe.com/browse/SITES-48140) ## Test plan - [x] 58 unit tests in `redirects.test.js` pass, all auth handler tests green - [x] Full suite green (2 pre-existing flakes unrelated to this PR; pass in isolation) - [x] Lint + type-check clean - [x] OpenAPI validates + docs rebuilt - [ ] Post-deploy: verify `-prev` URL returns the same canonical body + `surrogate-key: aso-overlay-cm-pN-eN` header via curl on dev - [ ] Post-deploy: verify Fastly edge 401 rate for `-prev` URLs is unchanged (the edge signal for upstream misconfig is preserved) ## Post-review audit findings Self-review by staff-engineer-level reviewer flagged: - **I1 (Important):** double-emit of `AsoOverlayS3ReadDurationMs` — **fixed in this PR** via `s3DurationEmitted` guard - **I4 (Important):** OpenAPI missing `Surrogate-Key` doc — **fixed in this PR** - **I3 (Important):** `resolveEnvironment` silent fallback — **split out to #2835** (independent concern) - **I2 + M1-M5 (Minor):** tracked as follow-up tasks; not blocking merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Abhishek Garg <abhigarg+adobe@adobe.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: miakobchuk <miakobchuk@gmail.com>
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.
Summary
Fix a silent metric mis-labeling failure mode. When neither
AWS_ENVnorENVis set on the Lambda,resolveEnvironmentreturns'dev'— meaning every prod metric routes to the dev dashboard while prod dashboards read zero traffic.Today this fails silently: on-call notices the wrong-tier dashboard is unexpectedly noisy hours after a deploy, or worse, doesn't notice because they only watch prod. Making it log-observable so a broken env-var manifest surfaces on the first cold-start invocation.
Change
resolveEnvironment(env, {log})now emitslog.warnonce per Lambda instance when the default fallback fires. Rate-limited via a module-scoped latch so a mis-configured deploy doesn't flood the log with one warn per request.Defensive: skips the warn if
log.warnis absent (some helix / Lambda contexts only guaranteelog.infoandlog.error) so the request path never crashes.Call sites threaded (all 5 that had access to
log)src/controllers/redirects.js:144(ASO overlay)src/controllers/webhooks.js:86, 101(GitHub webhook enqueue + error handler)src/controllers/brands.js:156(BrandsController — post-review fix, was missed initially)src/support/aso-overlay-key-handler.js:78src/support/github-webhook-hmac-handler.js:83Backwards-compatible for legacy callers that pass no options — return value and precedence rules unchanged.
Design decisions
resetEnvFallbackWarnedForTest): required to isolate the latch across unit tests without cross-test bleed. Named to make its scope obvious — no production caller should reach for it.typeof log.warn === 'function'guard: repo has no shared log-shape convention (grep confirms), and helix contexts historically only guaranteelog.info. Best-effort path must never throw.Origin
Follow-up to a post-review audit on #2832. Split out here because the concern is orthogonal to the overlay-tier fix in that PR — this hardens metric labeling repo-wide.
Test plan
AWS_ENVset, warn suppressed whenENVset, warn fires exactly once when both missing, no-op when log absent, warn skipped whenlog.warnis undefinedresolveEnvironmentthreadlogthroughRelated
.env.exampledocumentation forASO_OVERLAY_API_KEY_PREVIOUS— same audit sweep🤖 Generated with Claude Code