Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions misc/docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ output; don't put prose here that a build step will overwrite.)
| [0009](decisions/0009-scope-and-deferrals.md) | v1 scope, deferrals, the `s3dol`/`botodol` line | proposing a feature |
| [0010](decisions/0010-bucket-and-bulk-operations.md) | Bucket-existence policy, `delete_many`, cascading delete | touching bucket lifecycle or bulk ops |
| [0011](decisions/0011-keyed-capability-surface.md) | **No key-taking methods; capabilities are sibling stores** | adding any capability, or wondering where `url_for` went |
| [0012](decisions/0012-credential-and-endpoint-resolution.md) | **Credential + endpoint resolution**: a pure `resolve()`, a picklable spec, two endpoint rungs | touching credentials, endpoints, presets, `anon`, or the migration warning |

## The five things most likely to bite you

Expand Down
49 changes: 35 additions & 14 deletions misc/docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,28 +85,49 @@ load-bearing rather than decorative:
### Layer A — `s3dol.connection`

Owns the expensive resource (the botocore client) and *all* credential/endpoint
resolution. See [decisions/0002](decisions/0002-boto3-as-engine.md) and
[decisions/0003](decisions/0003-provider-presets-and-capabilities.md).
resolution. See [decisions/0002](decisions/0002-boto3-as-engine.md),
[decisions/0003](decisions/0003-provider-presets-and-capabilities.md) and — for the full
resolution design — [decisions/0012](decisions/0012-credential-and-endpoint-resolution.md).

```python
@dataclass(frozen=True)
class S3Connection:
preset: str | Preset | None = None # 'aws' | 'minio' | 'r2' | ...
profile_name: str | None = None
# WHERE
endpoint_url: str | None = None
region_name: str | None = None
credentials: Credentials | None = None # explicit; None => resolve the chain
anon: bool | Literal["auto"] = False
client_config: dict = field(default_factory=dict)
# WHO
profile: str | None = None
credentials: CredentialProvider | None = None # normalised in __post_init__
anon: bool = False # 'auto' deferred to v1.x — ADR-0012 §D5
# HOW ("None" means do not override; each is a ladder — ADR-0012 §D3)
signature_version: str = "s3v4"
addressing_style: Literal["path", "virtual"] | None = None
checksum: Literal["when_supported", "when_required"] | None = None
payload_signing_enabled: bool | None = None
verify: bool | str | None = None
client_kwargs: tuple[tuple[str, Any], ...] = ()
# POLICY
deny_means_absent: bool = False # ADR-0004 §3
```

Three properties matter and are each tested:

- **Lazy.** The client is a `cached_property`; constructing a connection performs no I/O and
never raises for missing credentials.
- **Picklable.** The connection carries a *spec*, not a client. `__getstate__` drops the
cached client so stores survive `ProcessPoolExecutor` and Dask.
- **Redacting.** No secret appears in `repr`, `str`, or any surviving local.
`preset` is an argument to the factory, not a field: it resolves to concrete values in
`resolve()`, so the spec records *what was resolved and from where*, not *what to look up later*.

Four properties matter and are each tested:

- **Lazy, and built under a lock.** Constructing a connection performs no I/O and never raises
for missing credentials. The client is **not** a bare `cached_property` — measured, 8
concurrent threads produce 8 clients, which for a callable credential is 8 token fetches
([ADR-0012](decisions/0012-credential-and-endpoint-resolution.md) §D1).
- **Picklable, always.** The connection carries a *spec*, never a live object — there is no
`connection=<boto3 client>` escape hatch. `__getstate__`/`__setstate__` are mandatory for
*every* connection, because the boto3 client is unpicklable regardless of `anon`.
- **Redacting.** No secret appears in `repr`, `str`, or any surviving local. Note `repr()` of a
botocore provider chain dumps the entire `os.environ`, and a `str` subclass is not sufficient
redaction — it leaks through `json`, concatenation and `join`.
- **Purely resolvable.** `resolve(spec, environ, aws_config) -> Resolution` does no I/O and
carries per-field provenance, so the whole ladder is exhaustively testable with a fake environ
dict and `s3dol.diagnose()` can print where every value came from.

### Layer B — `s3dol.base`

Expand Down
20 changes: 20 additions & 0 deletions misc/docs/decisions/0003-provider-presets-and-capabilities.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,16 @@ Two caveats we must not forget:

### 4. `signature_version` is never left to botocore — this is the single most load-bearing default in the package

> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The mandate is right;
> its stated *scope and cause* are wrong. Measured across 16 regions on botocore 1.43.68: the
> presign downgrade fires in exactly the regions `endpoints.json` still lists as v2-capable —
> **12 region strings**, not 8, counting `aws-global` and three FIPS regions. **A custom endpoint
> is irrelevant**: `us-east-1` + MinIO → SigV2, `eu-south-2` + MinIO → SigV4. *Region* is the
> discriminator. The practical impact stands only because `us-east-1` is the conventional MinIO
> default. Pin this as a conformance test **against `endpoints.json`**, not against a literal
> region list — and note a user's own AWS-config profile setting `s3.signature_version = s3v4`
> already suppresses the downgrade, so `diagnose()` must report the source.

botocore silently **downgrades presigning to SigV2** whenever the user has not *explicitly*
set `signature_version` (`botocore/client.py::_set_s3_presign_signature_version`). Measured:

Expand Down Expand Up @@ -177,6 +187,16 @@ This is a live bug in v0, not merely a design gap: `s3dol` today presigns with S

### 5. Anonymous access

> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D5, in two places.**
> (1) **`url_for` returns `None` when anonymous — it does not raise.**
> `dol.SupportsUrlFor.url_for` is typed `-> Optional[str]`, and raising turns `lacing`'s
> documented streaming fallback into a 500 on a *public* bucket, the one case `anon` exists for.
> ADR-0011 D3b's unreachable-wrapper guard still raises; the two refusal conditions must not be
> merged. (2) **The pickle rationale is incomplete.** Keeping `UNSIGNED` out of the dataclass is
> necessary but not sufficient — the boto3 client is unpicklable full stop, so
> `__getstate__`/`__setstate__` are mandatory for *every* connection, not only anonymous ones.
> Also: `anon='auto'` is **deferred to v1.x**; v1 ships `anon: bool`.

`anon` is `bool | 'auto'` on the connection **and surfaced directly on `s3_store(...)`** —
reading a public bucket must not require learning Layer A. It is translated to
`botocore.UNSIGNED` *inside* the `cached_property` that builds the client: the singleton is
Expand Down
11 changes: 11 additions & 0 deletions misc/docs/decisions/0004-error-taxonomy.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,17 @@ produce**.

### 3. The taxonomy

> **Extended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D1** with the
> configuration/spec-time branch, which resolves before any request is made and therefore has no
> `(operation, code, status)` row: `ConfigurationError(S3Error, ValueError)` with
> `PresetConflict`, `PresetHostMismatch`, `MissingEndpoint`, `MissingPresetParam`,
> `UnknownPresetParam`, `InvalidEndpoint`. Plus a warning tree, `S3DolWarning(UserWarning)` with
> `S3DolResolutionChanged`, `AmbiguousResolution`, `AnonymousFallback` —
> `filterwarnings('error', category=S3DolWarning)` is the strict mode, so there is no `strict=`
> kwarg. Two bans belong here too: `anon=True` with credentials or a profile, and `anon=True`
> with `deny_means_absent=True` (measured under moto, anonymous HEAD gives 403 on a present key
> and 404 on an absent one, so every key reads as absent — a silently empty store).

| Condition | Raises | Rationale |
|---|---|---|
| object absent | `ObjectNotFound(KeyError)` | Mapping contract |
Expand Down
17 changes: 17 additions & 0 deletions misc/docs/decisions/0007-naming-and-compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ what v0 lacked was a statement of which were *public*.)

### 3. `s3dol.store.S3Store` becomes a deprecated shim, removed in v2

> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D7:** the shim signature
> below **omits `is_supabase_endpoint=`, which v0 accepts** (`s3dol/store.py:26`). As written it
> is a `TypeError` on upgrade for anyone passing it. Add it, map it to the supabase preset, and
> preserve `False` as an opt-out.

It keeps its current signature exactly — `bucket_name` accepted **both positionally and by
keyword**, `path=` (not renamed), plus `aws_access_key_id`, `aws_secret_access_key`,
`aws_session_token`, `endpoint_url`, `region_name`, `profile_name`, `make_bucket`,
Expand Down Expand Up @@ -99,6 +104,18 @@ hard-coded endpoint will silently redirect on upgrade. That is why §5 has a ste

### 4. Behaviour changes that the shim deliberately does NOT preserve

> **Corrected by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The ladder below is
> kept, but the sentence "v0's effective precedence is `AWS_ENDPOINT_URL_S3 > … > explicit
> kwarg`, and the corrected ladder **inverts the top of it**" describes a **deletion, not a
> ranking**. Measured: botocore already ranks `explicit > AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL
> > config file > resolver`. v0 behaves otherwise because it *drops* the argument
> (`_find_default_credentials` never forwards `endpoint_url`). **v1 re-ranks nothing; it stops
> discarding an argument** — which deletes the "vendor a precedence ladder" work item entirely.
> Two further v0 behaviour changes are missing from the table below: v0 silently **nulls an
> explicitly-passed `aws_session_token`** and silently **changes an explicitly-passed
> `region_name`**. And `http_cosmo_prep` moves on **three axes at once** (endpoint honoured,
> credential identity, checksum policy) — one row, one release-note bullet, not three.

These are bug fixes, and preserving them would mean preserving data-misrouting:

| v0 behaviour | v1 | shim keeps v0? |
Expand Down
11 changes: 11 additions & 0 deletions misc/docs/decisions/0008-testing-architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@ own stores.

### What the conformance suite must assert

> **Extended by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The pickle conformance
> test must run **after** touching `.client` (the cache is what breaks it) and must **also assert
> `hash(conn)`** — measured, a container-valued field pickles happily but raises `TypeError` on
> `hash`, so the pickle test alone misses it. Add to the moto-fidelity list: **moto lets an
> anonymous client `list_objects_v2` and `put_object` against a private bucket**, so no tier
> below 4 can validate anonymous semantics. Any test of credential fallback must scrub CI's
> `AWS_*` variables. And presign assertions must check `'X-Amz-Algorithm'` in the URL across
> **both** a v2-legacy region and a v4-only region — `client.meta.config.signature_version`
> reports `'s3v4'` while emitting SigV2, and a `us-east-1`-only test cannot distinguish "we
> forced s3v4" from "this region defaults to v4 anyway".

Beyond the obvious Mapping laws, these are the ones that would have caught real bugs:

1. **Prefix scoping round-trip.** Fixture must include sibling, non-matching, marker and
Expand Down
5 changes: 5 additions & 0 deletions misc/docs/decisions/0011-keyed-capability-surface.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ argument, where a method could not be called on a store lacking it. Free-functio

### D5 — A conformance test, not a registry

> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D5:** the allowlisted
> `url_for` returns `str | None`, where `None` means "no presign capability" (an anonymous
> connection). Its *other* refusal — the unreachable-wrapper guard in D3b — still **raises**.
> Two distinct conditions; do not merge them.

No `_key_methods` registry. A **reflective conformance test** enumerates the public methods of
every Layer B class and fails on any that takes a key — allowlist: `url_for` (D3b), and nothing
else.
Expand Down
Loading
Loading