[improvement](s3) Default S3 access to HTTPS#65379
Open
0AyanamiRei wants to merge 7 commits into
Open
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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29438 ms |
Contributor
TPC-DS: Total hot run time: 181808 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
ClickBench: Total hot run time: 25.14 s |
spaces-X
reviewed
Jul 8, 2026
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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.
gavinchou
requested changes
Jul 8, 2026
gavinchou
left a comment
gavinchou
left a comment
Contributor
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.
Contributor
Author
draft first, we will test for this behavior change. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65379 Problem Summary: Scheme-less S3 endpoints were still normalized inconsistently in FE paths after changing the BE and Cloud defaults to HTTPS. S3 resources and storage vaults persisted bare endpoints as HTTP, ALTER RESOURCE could write bare endpoints, external stage preflight and metadata handling were inconsistent, and catalog connectivity and DLF file IO bypassed the HTTPS default. This change centralizes endpoint normalization so bare endpoints use HTTPS while explicitly configured schemes are preserved across these paths. It also updates resource and stage serialization coverage to verify persisted endpoints. ### Release note Scheme-less S3-compatible endpoints now default to HTTPS across resources, storage vaults, external stages, catalog connectivity checks, and DLF file IO. HTTP-only endpoints must be configured explicitly with the http:// scheme. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.common.util.S3UtilTest,org.apache.doris.catalog.S3ResourceTest,org.apache.doris.nereids.trees.plans.commands.CreateStageCommandTest - ./build.sh --fe -j48 - Behavior changed: Yes. Scheme-less S3-compatible endpoints now default to HTTPS consistently across FE object-storage paths; explicitly configured schemes remain unchanged. - Does this need documentation: No
0AyanamiRei
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test: the PR goal is to default scheme-less S3-compatible endpoints to HTTPS. The current head covers the earlier missing FE parallel paths, but the S3 resource alter path has a regression for blank endpoint updates. Added tests cover normal endpoint normalization, but not this empty-value modification case.
- Scope/focus: the change is focused on S3 endpoint normalization and default BE/cloud client scheme values.
- Parallel paths: I did not resubmit the existing comments about S3Resource/storage vault/stage/DLF/connectivity paths; current head now routes those paths through the shared HTTPS-default helper or through S3Resource-backed storage vault creation.
- Config/compatibility: explicit http:// is preserved, while scheme-less endpoints now default to HTTPS. HTTP-only deployments still need explicit HTTP configuration as described in the PR release note.
- Persistence/data flow: one remaining issue is that ALTER RESOURCE can persist the invalid endpoint https:// when a blank endpoint value is supplied with validity checking disabled.
- Concurrency/lifecycle/locking: no new concurrency, lifecycle, or lock-order concern found in the changed code.
- Testing/validation: patch-scoped whitespace check passed with
git diff --check -- $(cat .code-review.VRLyVZ/pr_changed_files.txt). I did not run FE unit tests locally because this checkout lacks.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - User focus: no additional user-provided review focus.
Subagent conclusions: optimizer-rewrite and tests-session-config both completed initial focused passes with no new candidates. The main agent accepted M-001 as the only inline comment. The previous endpoint-propagation concern was classified as duplicate/stale against the existing thread after verifying the current head. Convergence round 1 ended with both live subagents returning NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set after M-001 was added.
### What problem does this PR solve? Issue Number: None Related PR: apache#65379 Problem Summary: Normalizing an empty S3 endpoint during ALTER RESOURCE converted the value to `https://`. Because that result was non-empty, it bypassed the existing effective-value guard and replaced the persisted endpoint. Only normalize effective endpoint values so empty updates continue to preserve the current endpoint for both standard and legacy property keys. ### Release note None ### Check List (For Author) - Test: Unit Test not run because `thirdparty/installed/bin/protoc` is unavailable; added S3ResourceTest coverage for empty standard and legacy endpoint updates - Behavior changed: No; restores the existing behavior of ignoring empty resource property updates - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29079 ms |
Contributor
TPC-DS: Total hot run time: 180068 ms |
Contributor
ClickBench: Total hot run time: 24.88 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65379 Problem Summary: The P0 regression still expected a scheme-less S3 resource endpoint after endpoint normalization began defaulting to HTTPS. The vault P0 MinIO test also passed scheme-less endpoints for an HTTP-only MinIO service, causing the storage vault validity check to attempt TLS and fail with an unrecognized SSL message. Update the expected normalized resource endpoint and explicitly configure HTTP for the HTTP-only MinIO test endpoints. ### Release note None ### Check List (For Author) - Test: Regression test not run locally because no Doris cloud/MinIO regression cluster is available; fixes were matched against TeamCity failures from builds 992206 and 992209 - Behavior changed: No; adapts tests to the intended HTTPS-default behavior while preserving explicit HTTP coverage - Does this need documentation: No
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65379 Problem Summary: The cold heat separation ALTER RESOURCE regression has two result checks for scheme-less S3 endpoints. The first check was updated for the new HTTPS default, but the check after binding the resource to a storage policy still expected HTTP. Doris correctly persists the scheme-less endpoint as HTTPS, so update the missed expectation to match the intended normalization behavior. ### Release note None ### Check List (For Author) - Test: No need to test; this is a one-line regression expectation update matched directly against the pipeline result, and git diff --check passed - Behavior changed: No; aligns the test with the intended HTTPS-default behavior - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29223 ms |
Contributor
TPC-DS: Total hot run time: 180452 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65379 Problem Summary: Changing the in-code S3 scheme defaults from HTTP to HTTPS also changes behavior for upgraded clusters whose existing FE and BE configuration files do not contain the new setting. Keep the FE, BE, and Cloud in-code defaults on HTTP for compatibility, add the FE counterpart of s3_client_http_scheme, and set both shipped FE and BE configuration templates to HTTPS. New deployments therefore use HTTPS for scheme-less S3 endpoints without requiring user action, while upgrades preserve their existing behavior and endpoints with an explicit scheme remain unchanged. ### Release note New deployments default scheme-less S3 endpoints to HTTPS while upgraded clusters retain their existing S3 scheme behavior unless configured otherwise. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.common.util.S3UtilTest,org.apache.doris.catalog.S3ResourceTest,org.apache.doris.nereids.trees.plans.commands.CreateStageCommandTest - ./run-fe-ut.sh --run org.apache.doris.common.util.S3UtilTest - build-support/check-format.sh - Behavior changed: Yes; only newly generated FE and BE configurations default scheme-less S3 endpoints to HTTPS - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)