FANNG1 commented on issue #66772:
URL: https://github.com/apache/doris/issues/66772#issuecomment-5299773752
## Correction to the fix guidance above
Digging further into how Lance actually consumes `storage_options`, the
suggestion in the issue body — teach `LanceStorageOptions.forBackend` to accept
the unprefixed aliases — is **necessary but not sufficient**, and one of its
premises was wrong. Everything below is verified against the exact Lance
revision that `lance-c` v0.1.6 pins (`f69324596`), i.e. what the BE on
`branch-4.1` actually compiles, not a newer release.
### 1. The outbound spelling is wrong too, so an inbound-only fix still
breaks
The BE emits `aws_`-prefixed option names (`kStorageKeys` in
`be/src/format_v2/table/lance_reader.cpp`). Those silently stop working the
moment Lance takes its OpenDAL path:
```rust
// rust/lance-io/src/object_store/providers/aws.rs — build_opendal_s3_store
let mut config_map: HashMap<String, String> = storage_options.0.clone();
config_map.insert("bucket".to_string(), bucket);
let operator = Operator::from_iter::<S3>(config_map)
```
There is no alias normalization on this path — the map goes straight into
OpenDAL's `S3Config`, whose field names are unprefixed, and OpenDAL's
deserializer ignores unknown keys without error.
| | object_store path (default) | OpenDAL path (`use_opendal=true`) |
|---|---|---|
| `access_key_id` | works | works |
| `aws_access_key_id` | works | **silently dropped** |
To be precise about the current blast radius: `use_opendal` is itself read
from the storage options (`aws.rs:139-143`), and the same five-key filter drops
it, so **this path is not reachable through Doris today** — the prefixed
spelling is a latent hazard rather than a live bug. It becomes live exactly
when the filter is opened up, which is what any real fix to this issue does.
That is why it belongs in the same change: widening the inbound aliases while
continuing to emit the prefixed form outbound would make `use_opendal=true`
reachable and silently credential-less at the same moment.
The unprefixed short names are the only spelling both paths accept, so
`forJavaSdk` and the BE's `kStorageKeys` are the place to converge.
### 2. Both spellings have first-party sources — the real principle is that
the consumer must be permissive
The issue body implied Gravitino's unprefixed spelling is the more standard
one. That is not accurate, and the correct argument is stronger.
Lance's own credential vendor emits the **prefixed** form:
```rust
// rust/lance-namespace-impls/src/credentials/aws.rs
storage_options.insert("aws_access_key_id".to_string(), access_key_id);
storage_options.insert("aws_session_token".to_string(), session_token);
```
while Lance's own consumer accepts **both**, prefixed first:
```rust
// rust/lance-io/src/object_store/dynamic_credentials.rs:172-186
.get("aws_access_key_id").or_else(|| credentials.0.get("access_key_id"))
.get("aws_session_token")
.or_else(|| credentials.0.get("session_token"))
.or_else(|| credentials.0.get("token"))
```
So both spellings legitimately occur in the wild from first-party sources,
and Lance's answer is: vendors emit one, consumers accept all of them. Doris is
a consumer that accepts one. That is the defect, and it holds regardless of
which namespace server is on the other end — this is not a Gravitino-specific
quirk.
Worth noting the spec is explicit that this field is opaque: *"Configuration
options to be used to access storage… These will be **passed directly to
Lance** to initialize storage access."* The namespace protocol deliberately
does not define the key vocabulary, so a client cannot assume one.
### 3. The bigger loss: the filter also discards Lance's credential
auto-refresh
I under-reported `expires_at_millis` above as merely another dropped key. It
is the entry point to a subsystem that is present in the revision the BE builds:
```
rust/lance-io/src/object_store/storage_options.rs
EXPIRES_AT_MILLIS_KEY (:31), REFRESH_OFFSET_MILLIS_KEY (:34)
LanceNamespaceStorageOptionsProvider -> issues its own
DescribeTableRequest
StorageOptionsAccessor -> caches, and refreshes
refresh_offset_millis
(60s default) before expiry
```
Lance already re-vends expiring credentials through the namespace by itself.
Because the five-key filter drops `expires_at_millis`, Doris silently opts out
of it: vended short-lived credentials expire mid-scan with no renewal. That is
a limitation Lance does not actually have — Doris introduces it by filtering.
### 4. The filter also locks the Lance catalog to S3
`S3_KEYS` (FE) and `kStorageKeys` (BE) are both S3-only, five entries each.
Lance vends Azure and GCP credentials as well (`azure_storage_sas_token`,
`google_storage_token` — `credentials/azure.rs`, `credentials/gcp.rs`). Against
a namespace serving Lance tables on Azure or GCS, **100% of the vended
credentials are dropped** — not a spelling mismatch, simply no entry in the
table. As written, the Lance catalog cannot support a non-S3 backend through
credential vending at all.
### 5. Revised suggestion
Rather than widening the lookup table, stop translating server-supplied
options at all:
- **FE** — pass `DescribeTable`'s `storage_options` through untranslated.
- **BE** — hand them to `lance-c` as-is and drop the second translation.
Note today's round trip is `access_key_id` → `AWS_ACCESS_KEY` →
`aws_access_key_id`: Lance vocabulary out, Lance vocabulary back in, with a
lossy filter in between and no information gained.
- **Static catalog properties** (`s3.access_key` and friends) keep their
conversion — they genuinely are in Doris vocabulary — but should emit the
unprefixed spelling.
On the safety concern that presumably motivated an allowlist in the first
place: Lance's own model for exactly this problem is a **denylist of
location-controlling keys**, not an allowlist of credential keys.
```rust
// rust/lance-io/src/object_store/dynamic_opendal.rs
protected_keys: Vec<&'static str>, // stripped from server-supplied options
```
That defends the thing actually worth defending — a namespace server
redirecting where data is read from — while scaling to other clouds and to keys
added later, which an allowlist cannot.
If a smaller change is preferred for now, widening `forBackend`'s aliases
still fixes the immediate breakage and is worth doing on its own; it just
leaves items 1, 3 and 4 open.
### Testing note
The coverage gap noted in the issue body is a little wider than described:
`lance_rest_server.py` vends only `aws_`-prefixed keys, so neither the
unprefixed alias path nor `expires_at_millis` is exercised anywhere today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
