zghong commented on PR #66595:
URL: https://github.com/apache/doris/pull/66595#issuecomment-5506752683
@gavinchou @Gabriel39 Thanks for both comments. I agree that STS is the
better option when the caller controls the object-store identity and Doris is
accessing the storage service through its native credentials. This PR is not
intended to replace that model.
The compatibility gap here is specific to the generic HTTP TVF. We migrate
from ClickHouse to Doris, and the URL is an opaque, HEAD-limited capability
issued by a third-party platform. Doris cannot obtain STS credentials or change
how that platform signs URLs.
The resource is readable with the operation that HTTP TVF ultimately
performs, but Doris **currently rejects it during planning** because it
additionally must require the URL to authorize `HEAD`, which we cannot do.
However, ClickHouse follows the same general compatibility principle in
`ReadWriteBufferFromHTTP::getFileInfo()`:
```cpp
ReadWriteBufferFromHTTP::HTTPFileInfo ReadWriteBufferFromHTTP::getFileInfo()
{
if (file_info)
return *file_info;
/// May be disabled in case the user knows in advance that the server
doesn't support HEAD requests.
/// Allows to avoid making unnecessary requests in such cases.
if (!read_settings.http_settings.make_head_request)
return HTTPFileInfo{};
Poco::Net::HTTPResponse response;
try
{
getHeadResponse(response);
}
catch (const HTTPException & e)
{
/// Maybe the web server doesn't support HEAD requests.
/// E.g. webhdfs reports status 400.
/// We should proceed in hopes that the actual GET request will
succeed.
/// (Unless the error in transient. Don't want to
nondeterministically sometimes
/// fall back to slow whole-file reads when HEAD is actually
supported; that sounds
/// like a nightmare to debug.)
if (e.getHTTPStatus() >= 400 && e.getHTTPStatus() <= 499 &&
e.getHTTPStatus() !=
Poco::Net::HTTPResponse::HTTP_TOO_MANY_REQUESTS &&
e.getHTTPStatus() !=
Poco::Net::HTTPResponse::HTTP_REQUEST_TIMEOUT &&
e.getHTTPStatus() !=
Poco::Net::HTTPResponse::HTTP_MISDIRECTED_REQUEST)
{
return HTTPFileInfo{};
}
throw;
}
file_info = parseFileInfo(response, 0);
return *file_info;
}
```
>
https://github.com/ClickHouse/ClickHouse/blob/23c30347d281bf558807e7c01066e0515836a03e/src/IO/ReadWriteBufferFromHTTP.cpp#L752-L788
What's more, Doris is currently using `GET` to detect whether range-reader
is supported, so no additional requests are added:
https://github.com/apache/doris/blob/ddbaaab13882dab3dfa51ffb8038218df869e360/be/src/io/fs/http_file_reader.cpp#L479-L484
That said, I agree that the current implementation needs to be tightened,
and I have updated:
1. fall back when HEAD returns 200 without a usable Content-Length;
2. require a valid, known Content-Range total for a 206 size probe instead
of leaving the BE size as `SIZE_MAX`;
3. recognize 416 with Content-Range: bytes */0 as an empty resource in both
FE and BE;
4. use `mask_token` in `util/security.h` to mask security info about url in
logs, but I think a new PR is needed for further optimizationin about security
policy;
5. make `HttpFileReader` consume the file size already carried from FE
through `TFileRangeDesc`/`FileReaderOptions`, avoiding duplicate probes when
the size is known;
6. add FE and BE tests for the missing/malformed/unknown Content-Range,
HEAD-without-length, empty-file, metadata-reuse, and redaction cases.
With those changes: `HEAD` remains the normal metadata path, and a bounded
Range: bytes=0-0 `GET` is used only when `HEAD` cannot provide the size. The
fallback must either determine a finite size, recognize an empty resource, or
fail explicitly.
I hope this keeps the generic HTTP compatibility needed by existing users
without introducing an object-storage-specific credential assumption, while
also addressing the correctness, security, and efficiency concerns raised here.
--
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]
