FANNG1 commented on PR #67157:
URL: https://github.com/apache/doris/pull/67157#issuecomment-5479355161
Second review round addressed in 2eb2765.
Three of the four findings were real, and the fix for the first one turned
out to need a different shape than the review suggested:
1. **Auth tuple not reconciled after vending** — correct, and worse than
described. I had been merging the vended options over the static ones key by
key and then patching up the relationships afterwards, which is what made the
bug possible in the first place. Rather than add more patches, authentication
is now treated as one value: whichever side supplies any part of it supplies
all of it, and a single writer clears the group before writing it. A half pair,
a stale token, a vended blank and an explicit anonymous request all follow from
that one rule instead of needing a branch each.
Worth recording that `allow_anonymous` is genuinely unconditional in both
pinned OpenDAL versions — `OssCore::sign` returns the request unsigned whenever
it is set, whatever credentials sit beside it
(`opendal-service-oss-0.56.0/src/core.rs:93`; 0.58.1 folds it into
`skip_signature`). The flag is now stated either way rather than only when
true, because lance snapshots the host's `OSS_`/`AWS_` environment into the
same config map *before* storage options are applied, so omitting the key hands
the decision to an exported `OSS_ALLOW_ANONYMOUS`.
2. **Original provider failure in the log** — correct. Rather than log only
the sanitized text, which would lose the stack the previous round asked for,
the rebuilt exception now carries the stack of the exception its message came
from. It holds class, method, file and line only, so unlike the message it
cannot carry a credential, and no second copy of the raw text exists to be
logged at all.
3. **OSS-HDFS** — correct, and my earlier exemption was worse than useless:
it made the configuration look supported while still being unable to open. Now
rejected across the whole configuration rather than only the warehouse
authority, since `OSSHdfsProperties` selects on the *endpoint* —
`oss://bucket/path` with an `oss.endpoint` naming OSS-HDFS routed there just
the same.
4. **Alias normalization inside `base_<id>.` scopes** — I do not think this
one holds. The pinned lance keys per-base configuration by a nested
`HashMap<String, ObjectStoreParams>`, not by a flat dotted option; there is no
`base_` literal in the FE lance jars or in the Rust tree, so that spelling
never reaches a provider. The suffix matching in the redaction path stays, but
as defence rather than as a documented contract. Happy to be corrected if you
know of a version where the flat form is real.
Verified end to end against a real OSS bucket, including two cases the
earlier rounds could not reach: STS temporary credentials with a no-token
negative control, and the anonymous-flag-versus-vended-credential collision.
Details in the PR description.
Docs are up as apache/doris-website#4094 (English and zh-CN).
--
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]
