feat: add EDL support for Icechunk stores - #147
Merged
Conversation
Add an opt-in 'earthdata' access mode to the typed virtual chunk
authorization config: an entry like
{"s3://asdc-prod-protected/": {"earthdata": true}} exchanges the
service's Earthdata Login identity for the DAAC's temporary S3
credentials via earthaccess-auth's CMR-derived bucket registry and hands
icechunk a refreshable credential that renews as the ~1h STS credentials
expire. Earthdata credentials apply to virtual chunk containers only;
icechunk stores and zarr/NetCDF sources keep using the service's ambient
credentials. Entries are validated at startup (unregistered buckets fail
the deploy; 'earthdata' is exclusive with other access options).
EDL identity sources, in precedence order: usable ambient EARTHDATA_*
variables, .netrc, or TITILER_MULTIDIM_EARTHDATA_SECRET_ARN pointing at
a Secrets Manager secret (plain token string or JSON with EARTHDATA_*
keys; plain secret names accepted as well as ARNs). The secret is
resolved lazily at first use (SnapStart-safe) and re-read every 10
minutes, rebuilding the shared credential manager when it changed, so
rotating the ~60-day EDL token needs neither a redeploy nor a restart.
A failed fetch is retried rather than latched, and a failed refresh
keeps serving on the still-valid identity. Client-facing errors never
contain the ARN, AWS errors, or EDL responses; details go to logs.
Credentials are fetched eagerly in Python before icechunk's Rust layer
runs, so failures stay typed: an unaccepted DAAC EULA surfaces as HTTP
403 with the EULA URLs, a missing EDL identity as a clean 500. The
Redis dataset cache key now fingerprints the authorization config so
authorization changes take effect immediately, and cache hits for
earthdata-authorized datasets establish the EDL identity before
unpickling (fixes fresh Lambda environments sharing a cache).
Deployment: new earthdata_secret_arn stack setting (reader role needs
secretsmanager:GetSecretValue); must run in us-west-2 (Earthdata S3
credentials are region-locked). The dev deploy authorizes ASDC as the
first production use. New dependency: earthaccess-auth 0.2.0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings on the earthdata feature (efda9ef), most-severe first: - Scope earthdata coupling to containers a repo actually declares: EDL identity and credential priming move out of to_credential (now pure local construction) into opener_icechunk, which intersects the configured earthdata entries with repo.config.virtual_chunk_containers via the new chunk_access.earthdata_endpoints(). A public icechunk store — or any plain zarr/NetCDF request — no longer touches Secrets Manager, EDL, or EULA state just because an earthdata entry exists in the service config. - Prime the cache-hit path: the opener records the endpoints it primed in ds.encoding (pickled with the dataset, never served in responses), and _open_cached re-primes exactly those on a hit, so a fresh worker unpickling from shared Redis surfaces typed 403/500 errors instead of opaque Rust-wrapped storage errors. Replaces the request-wide _has_earthdata_entries ensure. - Never fail requests on a bad rotation: applying a rotated secret (parse, export, EDL re-login) now restores the previous identity and backs off _RETRY_INTERVAL on failure instead of raising with no backoff after having popped the warm identity. - Swap EARTHDATA_* env vars write-before-remove (_swap_env), so a concurrent reader never observes an empty identity mid-rotation. - Skip falsy secret values: an empty EARTHDATA_TOKEN no longer shadows a working username/password with an empty bearer token, and JSON null no longer exports the literal string "None"; blank secrets raise. - Sanitize client-visible auth errors: S3CredentialsRequestFailure (403) now returns a fixed message pointing at the EDL EULA/application pages instead of echoing up to 1000 chars of the DAAC's raw response body, and LoginAttemptFailure (raw EDL response body) is mapped to a sanitized 500 instead of falling through to the str(exc) catch-all. - Drop the hand-rolled registry re-check in to_credential in favor of earthaccess-auth's own typed S3CredentialsEndpointUnresolved. - Scope the icechunk-builder parity test's 'earthdata' exemption to S3ChunkAccess, so an earthdata field added to another model without a matching model_dump exclude fails CI instead of at request time. - Repair tests/test_app.py: the merge of main (8562b7a) left test_errors_not_cacheable indented inside the previous test, an IndentationError that broke collection of the whole module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to 122398b, from a second adversarial review pass: - Reject partial-identity secrets: _parse_secret now requires a usable pairing (EARTHDATA_TOKEN, or both EARTHDATA_USERNAME and EARTHDATA_PASSWORD), mirroring _usable_env_identity. Previously a username-without-password secret exported cleanly, then latched a broken identity until the secret *content* changed (the unchanged secret short-circuits _apply_secret on every refresh), failing every request with a misleading error. - Back off failed first loads: both first-load failure paths (Secrets Manager fetch failure, unusable secret) now set _RETRY_INTERVAL before raising, so a misconfigured deployment fails fast for 60s instead of issuing a fresh GetSecretValue per request serialized on the module lock. Calls inside the window return without fetching; credential use fails downstream with a typed, sanitized error. - Don't stall concurrent requests behind a refresh fetch: the deadline is pushed to now+_RETRY_INTERVAL before the network call, so requests arriving mid-refresh take the pre-lock fast path on the still-valid warm identity instead of queueing behind a possibly-hung Secrets Manager call. - Correct _swap_env's docstring: it guarantees a never-empty environment, not an atomic swap; a racing reader can briefly see a mixed identity (one transient, self-healing failure). Marked as a deliberate ceiling. - Parse the chunk-access config once per open: opener_icechunk parses and hands the models to both build_virtual_chunk_access and earthdata_endpoints (which now takes parsed entries). - Drop the dead prefix parameter from Gcs/Azure to_credential; only the S3 model resolves its prefix, and build_virtual_chunk_access branches at the single call site. - Make the Redis cache entry an explicit (endpoints, dataset) pair: the cross-process re-prime contract lives in the cache layer instead of being smuggled through ds.encoding across pickle (encoding remains only the opener->reader in-process handoff, popped at cache write). - Remove the [options.exclude-newer-package] earthaccess-auth override from uv.lock: it bypassed the package-release cooldown. The locked 0.2.0 entry is unchanged and installs via `uv sync --locked`; re-locking will admit the release once the cooldown passes. - Extract _secret_arn_unless_latched from ensure_earthdata_credentials (complexity limit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third review round found seven correctness issues surviving from the original feature commit (efda9ef), all in paths the earlier rounds never exercised, plus three cleanups. This commit, with the companion earthaccess-auth branch fix/s3credentials-status-and-lock-granularity (status-aware S3CredentialsRequestFailure; per-endpoint fetch locks in S3CredentialManager), addresses all ten: - Sanitize the Rust-side error path: icechunk stringifies exceptions raised inside its credential callbacks (the steady-state refresh at chunk-read time), chaining raw DAAC bodies into IcechunkError. A new handler logs the full text and serves fixed messages, keeping wrapped S3CredentialsRequestFailure identifiable (EULA 403; "status 401" -> service-credential 500). - Distinguish service-side credential rejection from EULA problems: a 401 from the s3credentials endpoint (expired ~60-day token, bad secret) now returns a sanitized 500 that alarms as a server error, instead of telling end users to accept EULAs. - Validate token-shaped rotations: EDL marks any non-empty token authenticated without a network call, so _rebuild_default_auth now probes one configured earthdata endpoint; a definitive 401 rejects the identity (rotation rolls back to the previous one; first load raises with backoff). EULA 403s, outages, or no earthdata entries never reject. - Make the configured secret authoritative: a stale-but-truthy ambient EARTHDATA_TOKEN no longer latches permanently and silently disables rotation; ambient credentials remain the fallback while the secret is unreachable and win outright when no ARN is configured. - Rebuild the default manager on every successful load (first load included), so an identity cached during a backoff window (netrc, ambient) can never keep shadowing the secret's identity. - Treat SecretBinary-only secrets as a typed failure with backoff (the SecretString subscript moved inside the guarded fetch). - Decode JSON string-scalar secrets to the intended token instead of exporting quote-wrapped garbage that latches. - Pop the ds.encoding endpoints marker unconditionally so it never rides datasets served with caching disabled. - build_virtual_chunk_access now takes parsed entries (config parsed exactly once per open, matching the comment), and the process-constant default authorization fingerprint is computed once, not per request. - ensure_earthdata_credentials fetch-failure handling extracted to _on_fetch_failure (complexity limit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_probe_identity validated a candidate identity with a raw, uncached fetch_s3_credentials call, then set_default_auth built a cold manager — so every cold process hit the DAAC s3credentials endpoint twice (probe, then prime). Build the S3CredentialManager first, probe through manager.get_credentials, and install that same manager via the new earthaccess-auth set_default_manager: a successful probe now leaves the credentials cached where prime_earthdata_endpoints and icechunk's refresh callable read them — one fetch per endpoint per cold process. Probe-before-install ordering is unchanged, so a 401-rejected identity still never becomes the default manager. Requires earthaccess-auth with set_default_manager (branch fix/s3credentials-status-and-lock-granularity). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch depends on 0.3.0's set_default_manager and the status_code attribute on S3CredentialsRequestFailure; with 0.2.x the app fails at identity-rebuild time with an ImportError. 0.3.0 is now on PyPI, so pin it instead of shimming around the gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The existing smoke cases read public, from_env, or anonymous stores, so the earthdata chain this branch adds (Secrets Manager secret -> EDL login -> identity probe -> s3credentials -> virtual chunk reads) was never exercised after a deploy. One TEMPO NO2 tile from the ASDC protected bucket covers it end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers what to check by hand once a deploy-dev deployment is live: finding the endpoint, the ordered TEMPO checks (metadata needs IAM only, tiles need the full EDL chain), how to read the 403/500 failure mappings, the two cache layers behind freshness, and the map viewer. Linked from the README's deployment section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two dev-deployment failures triaged this session were both config, not code, and neither was findable from the guide: - an empty-extension NotImplementedError, which is really identify_storage_backend listing a prefix and finding nothing - LoginStrategyUnavailable, which is really a Lambda deployed without TITILER_MULTIDIM_EARTHDATA_SECRET_ARN (an unset Actions variable expands to "" and model_post_init then omits the env var) Adds both to the failure-mode table, plus a section on verifying the externally managed reader role's grants: assume-role (the only check that sees bucket and KMS key policies), simulate-principal-policy with its blind spots, and CMK detection. Records that the role needs nothing on asdc-prod-protected -- those chunk reads use DAAC temporary credentials, not ambient IAM. Also notes that /variables never uses the Redis dataset cache, so it disagrees with /tiles when a store moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The TEMPO smoke case and the manual smoke-testing guide passed `sel_method=nearest` alongside an exact `sel=time=...`. That parameter was removed in the titiler 2.2 upgrade (c17dfa7); FastAPI drops unknown query parameters silently, so the requests were asking for an exact timestamp match rather than the nearest scan. Fold the method into the selector as `sel=time=nearest::<value>`, matching the syntax documented in the README and already used by the MUR SST zarr case.
main removed the Redis dataset cache and VPC (#149) and added mosaic support (#140, #138 release 0.8.0). Conflict resolutions: - reader.py: kept the earthdata priming in opener_icechunk (now also reusing main's `containers` local for its per-container logging) and main's timing/structured logs. Dropped everything that existed only to serve the Redis cache: `_open_cached`, the cache-key fingerprint helpers, and the `ds.encoding["earthdata_endpoints"]` handoff, whose sole consumer was the cache-hit re-prime path. - main.py: kept the earthdata exception handlers; dropped the now-unused `Depends` import, which only fed the removed `/clear_cache` endpoint. - tests/test_reader.py: main deleted the file (it was all cache tests); kept it with this branch's non-cache tests and dropped the cache and encoding-marker assertions with the code they covered. - deploy-dev.yml: union of both authorized-chunk-access entries (main's noaa-nws-naqfc-pds, this branch's asdc-prod-protected) plus the earthdata secret ARN env var. - README/docs: removed the Redis cache and /clear_cache paragraphs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a66aba1 raised the floor to >=0.3.0 but left uv.lock pinning 0.2.0, so `uv lock --check` failed. Regenerated with the release visible to the resolver; only the earthaccess-auth entry and its specifier move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ror bodies Switch the walkthrough to the HCHO trial store with a store-derived scan time instead of a hardcoded one, add warm-path/second-tile and credential-refresh checks, correct the empty-extension failure to the 501 it actually returns, and make test_deployment.py include the response body when a request fails (str(HTTPError) is only the status).
0.3.1 turns the bare JSONDecodeError from a TEA s3credentials endpoint serving a non-JSON body (observed: ASDC returning its EDL login page with a 200) into S3CredentialsRequestFailure carrying the status code and the first 500 bytes of the body, so the deployed service logs what the endpoint actually sent instead of an opaque 'Expecting value: line 1 column 1 (char 0)' 500.
The automated smoke test exercised the earthdata credential chain through the NO2 store only; HCHO is a separate icechunk repository with its own time axis and variable set, so a regression limited to one store could pass unnoticed. Request a tile from both.
The text-block URL invited pasting into a shell, where unquoted & is a parse error and <T> is redirection. Reuse the $TILE query from step 3 in a quoted echo/open command instead.
…ON handoff Parameterize the TEMPO walkthrough over both stores (TEMPO/VAR/CMAP blocks, run once per store) instead of HCHO-only prose. Switch rendering to rescale=0,3e16 with viridis (HCHO) / magma_r (NO2), matching the GIBS palettes Worldview uses (both run 0-3.0e16 molecules/cm2), so tiles are visually comparable to the reference. Add a Worldview sanity-check section with verified layer IDs and a permalink built from the tested scan time, and a TileJSON section with the tilejson.json request, an example response, and the caveats frontends need (baked-in sel time, store prefix lifetime).
Docstrings added on this branch mixed Google-style Args/Returns/Raises sections with prose descriptions of parameters, return values, and raised exceptions. The repo's existing convention is Google style (reader.py, chunk_access.py), so move those prose descriptions into proper sections in earthdata.py and chunk_access.py. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
'earthdata': true routes credential building away from icechunk.s3_credentials entirely, so documenting it only as a field docstring left the S3ChunkAccess class docstring misleading and the one documented field looking like an afterthought. Also: capitalize _next_refresh's docstring and document _last_secret to match its sibling module globals, and clarify prime_earthdata_endpoints' 'in Python' to 'in the Python layer'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
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.
Summary
We need a way to wire through temporary S3 credentials for the TEMPO virtual store, because we cannot rely on role based access. This PR adds EDL based access via configuration options like:
{"s3://asdc-prod-protected/": {"earthdata": true}}.PR checks
run-cdk-checkslabel to this PR.deploy-devlabel. It smoke-tests tiles from the native MUR, virtual MUR, and virtual NLDAS Icechunk stores after deployment.