[improvement](s3) Default S3 access to HTTPS#65379
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Doris object storage access defaulted to plain HTTP in BE and Cloud S3 client configuration. FE endpoint connectivity validation also added http:// when users provided an S3-compatible endpoint without an explicit scheme. Object storage services may disable HTTP and require HTTPS, so deployments that do not explicitly configure a scheme can fail or use an insecure default. This change makes Doris default S3-compatible object storage access to HTTPS while preserving explicit HTTP fallback for environments that set s3_client_http_scheme=http or provide an http:// endpoint.
### Release note
Default S3-compatible object storage access now uses HTTPS unless users explicitly configure HTTP.
### Check List (For Author)
- Test: Manual test
- Ran `git diff --cached --check` successfully before committing.
- `./run-fe-ut.sh --run org.apache.doris.common.util.S3UtilTest` was attempted but could not run because Homebrew is not installed in this macOS environment.
- Behavior changed: Yes. Default S3-compatible object storage access changes from HTTP to HTTPS. Explicit HTTP configuration remains supported.
- Does this need documentation: No
0AyanamiRei
requested review from
gavinchou,
liaoxin01 and
luwei16
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29438 ms |
TPC-DS: Total hot run time: 181808 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
ClickBench: Total hot run time: 25.14 s |
| @@ -434,6 +430,13 @@ public static void validateAndTestEndpoint(String endpoint) throws UserException | |||
| } | |||
| } | |||
|
|
|||
| static String buildEndpointUrl(String endpoint) { | |||
There was a problem hiding this comment.
One compatibility question: for users on HTTP-only storage with a scheme-less
endpoint like minio.local:9000, would existing loads start failing after upgrade
since it now defaults to HTTPS. If so, might be worth a
release-note callout
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test: the PR intends scheme-less S3-compatible access to default to HTTPS, but the implementation only updates BE/cloud config defaults plus the S3Util load/TVF validation helper. New S3Util tests cover only that helper path.
- Scope/focus: the patch is small, but it misses several parallel FE object-storage entry points. The existing GitHub thread about HTTP-only compatibility is already covered and was not duplicated.
- Parallel paths: resource/vault normalization, external stage validation, DLF Iceberg file IO, and external-catalog S3/Minio connectivity still bypass the new HTTPS-default helper.
- Config/compatibility: BE/cloud defaults changed to HTTPS, but FE paths can still force or preserve old HTTP behavior inconsistently.
- Persistence/data flow: S3Resource still persists a forced http:// endpoint into copied properties, which downstream resource, vault, policy, and object-store protobuf paths consume.
- Concurrency/lifecycle/locking: no new concurrency, lifecycle, or lock-order concerns found in the changed code.
- Testing/validation: patch-scoped whitespace check passed. FE build/unit tests were not run because this checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc.
- User focus: no additional user-provided review focus.
Subagent conclusions: OR-1 was accepted and merged into the inline comment; TSC-1 was a duplicate confirmation with test evidence; OR-2, OR-3, and OR-4 were reported during convergence and merged into the same consolidated finding. Convergence closed in round 3 when both optimizer-rewrite and tests-session-config returned NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
| if (endpoint.startsWith("http://") || endpoint.startsWith("https://")) { | ||
| return endpoint; | ||
| } | ||
| return "https://" + endpoint; |
There was a problem hiding this comment.
This only changes the load/TVF endpoint validation path, but other scheme-less object-storage endpoints still keep the old HTTP behavior. For example, S3Resource.setProperties() still rewrites and persists scheme-less resources/vaults as http://..., CreateStageCommand.tryConnect() still validates external stages with http:// + endpoint, DLFCatalog.initializeFileIO() still prepends http:// before building its S3 client, and the S3/Minio catalog connectivity tester passes a scheme-less endpoint straight to URI.create(endpoint). Those paths do not go through this helper, so users of CREATE RESOURCE, S3 storage vaults, external stages, DLF Iceberg catalogs, or catalog connectivity checks can still miss the new HTTPS default after this PR. Please route these parallel paths through the same HTTPS-default normalization while preserving explicit http://, and update the tests that currently pin or miss the old behavior.
gavinchou
left a comment
gavinchou
left a comment
There was a problem hiding this comment.
Thanks for the improvement. I think this still needs changes before merge.
-
The storage vault path still persists a bare S3 endpoint as HTTP, so the most important cloud vault/ObjectStoreInfoPB path does not actually get the new HTTPS default.
CREATE STORAGE VAULTgoes throughStorageVault.fromCommand->S3StorageVault->Resource.fromCommand->S3Resource.setProperties, andS3Resource.setPropertiesstill rewrites a bare endpoint tohttp://.... That value is then copied intoObjectStoreInfoPBbyS3Properties.getObjStoreInfoPB, persisted by meta-service as-is, and later consumed by BE/recycler throughS3Conf::get_s3_conf. As a result, a new storage vault created with a bares3.endpointwill still use/persist HTTP and will not follow the new default HTTPS behavior. -
CREATE STAGEstill has an HTTP-only preflight check.CreateStageCommand.tryConnectunconditionally buildshttp://+ endpoint before the real object-storage access. The real FE filesystem path already defaults a bare S3 endpoint to HTTPS inS3ObjStorage, so the preflight can fail for an HTTPS-only endpoint even though the actual object-storage client would use HTTPS and succeed. -
There are still FE S3-compatible client construction paths that do not use the same HTTPS-default normalization.
AbstractS3CompatibleConnectivityTesterpassesURI.create(endpoint)directly, so an explicitly configured bare endpoint is not normalized before the AWS SDK client is built.DLFCatalog.initializeFileIOalso converts the OSS endpoint to an S3-compatible endpoint and then defaults a bare endpoint tohttp://. These paths should share the same endpoint normalization rule: preserve explicithttp://orhttps://, but default a bare endpoint to HTTPS.
Testing suggestion: please add coverage for the storage vault/ObjectStoreInfoPB path, especially CREATE STORAGE VAULT with a bare s3.endpoint, and assert that it no longer persists http://... unintentionally. The existing S3ResourceTest still asserts that a bare endpoint becomes http://aaa, which is exactly the old behavior this PR should change for this path. It would also be useful to add coverage for CREATE STAGE with a bare endpoint and for the FE catalog/DLF connectivity paths so all object-storage client construction follows the same default HTTPS rule.
draft first, we will test for this behavior change. |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Doris object storage access defaulted to plain HTTP in BE and Cloud S3 client configuration. FE endpoint connectivity validation also added
http://when users provided an S3-compatible endpoint without an explicit scheme. Object storage services may disable HTTP and require HTTPS, so deployments that do not explicitly configure a scheme can fail or use an insecure default.This PR changes the default S3-compatible object storage access scheme to HTTPS. It preserves explicit HTTP fallback for environments that set
s3_client_http_scheme=httpor provide anhttp://endpoint.Release note
Default S3-compatible object storage access now uses HTTPS unless users explicitly configure HTTP.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)