Fix S3 settings configuration priorities#100975
Open
alexey-milovidov wants to merge 16 commits intomasterfrom
Open
Fix S3 settings configuration priorities#100975alexey-milovidov wants to merge 16 commits intomasterfrom
alexey-milovidov wants to merge 16 commits intomasterfrom
Conversation
Fix the priority order for S3 settings to be: 1. Global `<s3>` config section (lowest priority) 2. `<storage_configuration>` disk settings 3. Table-level, user profile, and query-level settings (highest priority) Two bugs were present: In `S3ObjectStorage::applyNewSettings`, endpoint settings from the global `<s3>` section were applied after disk config settings, overriding them. Swapped the order so disk config takes priority over global `<s3>`. In `StorageS3` configuration (`fromAST`), endpoint settings from the global `<s3>` section were applied last without re-applying profile/query settings on top. Added `updateFromSettings` after endpoint application so that user/profile/query-level settings retain highest priority. Closes: #50029 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Workflow [PR], commit [715b4c5] Summary: ❌
AI ReviewSummaryThis PR fixes S3 settings precedence by reordering merges in Findings
Tests
ClickHouse Rules
Final Verdict
|
| } | ||
|
|
||
| /// Re-apply user/profile/query-level settings on top, so they take priority over the global <s3> config section. | ||
| s3_settings->request_settings.updateFromSettings(context->getSettingsRef(), /* if_changed */ true); |
Contributor
There was a problem hiding this comment.
updateFromSettings is called here with the default validate_settings=true, so this path now validates upload settings unconditionally.
That changes behavior when s3_validate_request_settings=0: before this patch, validation was gated by that setting (loadFromConfigForObjectStorage(..., validate_settings) and the final conditional validateUploadSettings), but now invalid s3_* request settings can still throw here even when validation is explicitly disabled.
Please pass the setting explicitly, e.g.:
s3_settings->request_settings.updateFromSettings(
context->getSettingsRef(),
/* if_changed */ true,
context->getSettingsRef()[Setting::s3_validate_request_settings]);…tings` Without this, the re-applied `updateFromSettings` call would unconditionally validate upload settings, breaking the `s3_validate_request_settings = 0` path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that query-level SETTINGS (e.g., `s3_max_single_part_upload_size`) properly override session-level defaults when using the `s3()` table function. This exercises the `fromAST` code path where user/query-level settings must take priority over global `<s3>` endpoint configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| modified_settings->request_settings.proxy_resolver = DB::ProxyConfigurationResolverProvider::getFromOldSettingsFormat( | ||
| ProxyConfiguration::protocolFromString(uri.uri.getScheme()), config_prefix, config); | ||
|
|
||
| /// Apply global <s3> endpoint settings first (lowest priority). |
Contributor
There was a problem hiding this comment.
Thanks for fixing the priority order here. Please also add a regression test for the DiskS3 path (S3ObjectStorage::applyNewSettings) in addition to the new fromAST test.
This PR fixes two independent paths, but 04065_s3_settings_query_override covers only s3 table function parsing. Without a DiskS3/reload test, the storage_configuration vs global <s3> precedence can regress silently.
The test `04065_s3_settings_query_override` was passing on master because no global `<s3>` endpoint config existed with `max_single_part_upload_size` set. Without such config, the bug (endpoint settings overriding query-level settings) could not manifest. Add `tests/config/config.d/s3_settings_override.xml` with a matching endpoint that sets `max_single_part_upload_size` to 100Mi. Now on master the endpoint config overrides the query-level setting (10000) causing single-part upload, while with the fix the query-level setting is properly re-applied after endpoint config. Also remove the now-redundant session-level SET and update comments. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100975&sha=7a6d5529ced899cb58397d66d36420a5d03fad43&name_0=PR&name_1=Bugfix%20validation%20%28functional%20tests%29 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
LLVM Coverage Report
Changed lines: 100.00% (23/23) · Uncovered code |
Add `04068_s3_disk_settings_override` test that covers the `S3ObjectStorage::applyNewSettings` code path. The test creates a table on an S3 disk configured with a small `max_single_part_upload_size`, while a global `<s3>` endpoint section sets a large value for the same endpoint. The test verifies that multipart upload is used, confirming that disk config takes priority over global `<s3>` settings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…st config These config files were created but never symlinked in `install.sh`, causing bugfix validation to fail (the test passed on master because the global `<s3>` endpoint config was not loaded). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…efix The `loadFromConfigForObjectStorage` passes `"s3_"` as the setting name prefix when constructing `S3RequestSettings`, so disk config keys must use the `s3_` prefix (e.g. `s3_max_single_part_upload_size`) to be recognized. Without the prefix, the setting was silently ignored and the global `<s3>` endpoint value took precedence, causing the test to output `0 0 0` instead of `1 1 1`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…070` to avoid number conflict Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…le test
The bugfix validation failed because both tests passed on the old binary
("Failed to reproduce the bug"):
- Test `04070_s3_disk_settings_override` passed because
`S3ObjectStorage::applyNewSettings` is only called during config reload,
not during initial disk creation. Adding `SYSTEM RELOAD CONFIG` before
the INSERT triggers `applyNewSettings`, which on the old code applies
the global endpoint's 100 Mi `max_single_part_upload_size` AFTER the
disk config's 10 000 value, overriding it. With the fix, the disk
config takes priority.
- Test `04065_s3_settings_query_override` passed because
`S3ObjectStorage::writeObject` independently re-applies query-level
settings via `updateFromSettings`, compensating for the `fromAST` bug.
Since this test cannot demonstrate the bug through the write path,
remove it. The `fromAST` fix remains correct as defense-in-depth for
code paths that read the settings without the `writeObject` override
(e.g. reads, copies).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| } | ||
|
|
||
| /// Re-apply user/profile/query-level settings on top, so they take priority over the global <s3> config section. | ||
| s3_settings->request_settings.updateFromSettings( |
Contributor
There was a problem hiding this comment.
fromAST precedence path (updateFromSettings re-application), but the only remaining regression test (04070_s3_disk_settings_override) exercises DiskS3/applyNewSettings after SYSTEM RELOAD CONFIG. The earlier fromAST test was removed, so this path now has no direct regression coverage.
Please add a targeted test for the s3(...) parsing path (or another call path that consumes S3StorageParsedArguments request settings without writeObject re-applying settings), so future precedence regressions in Configuration::fromAST are caught.
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
1 participant
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.
Fix the priority order for S3 settings to be:
<s3>config section (lowest priority)<storage_configuration>disk settingsTwo bugs were present:
In
S3ObjectStorage::applyNewSettings, endpoint settings from the global<s3>section were applied after disk config settings, overriding them. Swapped the order so disk config takes priority over global<s3>.In
StorageS3configuration (fromAST), endpoint settings from the global<s3>section were applied last without re-applying profile/query settings on top. AddedupdateFromSettingsafter endpoint application so that user/profile/query-level settings retain highest priority.Closes: #50029
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix S3 settings priority so that
storage_configurationdisk settings override global<s3>section, and user/profile/query-level settings override both.Documentation entry for user-facing changes