Improve error message for S3 `listObjects` access denied errors by nickitat · Pull Request #103463 · ClickHouse/ClickHouse · GitHub
Skip to content

Improve error message for S3 listObjects access denied errors#103463

Open
nickitat wants to merge 3 commits intomasterfrom
improve_list_bucket_exc
Open

Improve error message for S3 listObjects access denied errors#103463
nickitat wants to merge 3 commits intomasterfrom
improve_list_bucket_exc

Conversation

@nickitat
Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

nickitat and others added 3 commits April 23, 2026 19:03
When `S3ObjectStorage::listObjects` fails (e.g. `AccessDenied` on GCP/S3),
the exception now includes the bucket name, key prefix, and disk name so
users can immediately tell which path and disk caused the permission error.

Before: `Access denied. (Code: 15, S3 exception: 'AccessDenied')`
After:  `Access denied. (Code: 15, S3 exception: 'AccessDenied'),
         while listing objects in bucket 'my-bucket' with prefix 'data/' on disk 'gcp_disk'`

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Instead of wrapping `throwIfError` in a try/catch at call sites, add a
variadic overload that accepts a `fmt::format_string` context and appends
it to the exception message directly inside `throwIfError`. Update the
`listObjects` call to use the new overload, passing bucket, path prefix,
and disk name.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…perations

Four call sites where listing or existence-check operations could throw
with an opaque error message lacking disk/container/path info:

- `AzureObjectStorage::listObjects`: wrap `ListBlobs` loop in
  `try`/`catch (StorageException)`, re-throw as `DB::Exception` with
  container, prefix, and disk name.
- `AzureIteratorAsync::getBatchAndCheckNext`: same pattern for the async
  listing path; pass `container` and `disk_name` through the constructor
  so the catch block has full context.
- `AzureObjectStorage::exists`: the existing `StorageException` catch
  already handles `NotFound` correctly; now re-throws other errors
  (e.g. `AccessDenied`) as a `DB::Exception` with object path, container,
  and disk name instead of a bare `throw`.
- `S3ObjectStorage::getObjectMetadata`: the existing `addMessage` call
  only included the object path; extend it to also include the bucket
  and disk name.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 23, 2026

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 23, 2026
@nickitat nickitat marked this pull request as ready for review April 23, 2026 21:33
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 23, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 83.80% 83.80% +0.00%
Functions 91.10% 91.10% +0.00%
Branches 76.20% 76.20% +0.00%

Changed lines: 89.77% (79/88) · Uncovered code

Full report · Diff report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant