AWS, Core: Read S3 files via catalog-vended HTTPS URLs by williamhyun · Pull Request #17457 · apache/iceberg

williamhyun · 2026-07-31T23:46:10Z

Summary

Adds support for reading object-store files over catalog-vended HTTP(S) URLs without requiring native cloud credentials on the reader, and wires it into S3FileIO.

Core: Introduces HttpUrlClient, HttpInputFile, and HttpInputStream — a seekable, range-readable HTTPS input path built on the httpclient5 dependency iceberg-core already depends on. Reads are served via chunked range GETs whose size is configurable via io.http.read.chunk-size-bytes (default 8 MB for S3FileIO); range fetches are retried with exponential backoff on transient network and retryable HTTP errors (e.g. 429/503), while terminal statuses (403/404) fail fast. Content length is probed via Range: bytes=0-0 when unknown, since pre-signed GET URLs typically reject HEAD. Pre-signed URLs are treated as bearer secrets and are redacted in logs.
AWS: S3FileIO#newInputFile(String) / newInputFile(String, long) now detect an HTTP(S) location and read it via HttpUrlClient, after validating (S3PresignedReadValidation) that it is HTTPS on an allow-listed host — s3.presigned-read.allowed-hosts (default amazonaws.com, amazonaws.com.cn, plus the configured S3 endpoint host); plain http:// or an untrusted host is rejected with ValidationException. s3:///s3a:///s3n:// reads stay on the native, credentialed S3 client path unchanged. S3FileIO#close() now also closes the shared HTTP client.
Tests: Unit coverage for the dispatch decision (TestS3FileIOHttpUrlDispatch injects a throwing S3 supplier to prove the native client is never invoked and that http:///untrusted hosts are rejected); host-allow-list validation (TestS3PresignedReadValidation); HttpInputFile/HttpInputStream behavior against a real local HttpServer (TestHttpInputFile, including multi-chunk reads crossing the chunk-size boundary, retry/backoff on transient statuses, terminal-status handling, and redirect-not-followed); chunk-size property parsing, per-FileIO default precedence, redaction, and client config (TestHttpUrlClient); status classification and content-length parsing (TestHttpStatusCategory, TestHttpHeaderUtil); and an optional live-AWS integration test (TestS3FileIOLivePresignedUrls) gated on AWS credential env vars.
This is the FileIO building block for catalogs that vend a pre-signed URL as a file's file-path in scan planning, so readers can fetch bytes over HTTPS with auth encoded in the URL itself. Delete-file handling, scan-planning integration, and other cloud providers are out of scope for this PR (see below).

Scope / compatibility notes

The catalog is expected to place a usable, already-signed URL directly in the file's location (file-path); the client uses that location unchanged as the fetch URL. There is no separate signing lookup or location-to-URL mapping in this PR.
Reads are restricted to HTTPS on operator-trusted hosts via s3.presigned-read.allowed-hosts, bounding a compromised or malicious catalog to naming files on the expected storage hosts rather than turning the reader into a general-purpose URL fetcher.
Only S3FileIO is updated. ResolvingFileIO and the GCS/ADLS equivalents are unchanged — a catalog that wants this behavior today needs to vend io-impl: S3FileIO per-table via the REST LoadTableResponse.config() map (the specified mechanism for a catalog to select a FileIO for a table), rather than relying on ResolvingFileIO's scheme-based dispatch. Extending the other cloud FileIOs to the same short-circuit pattern is a natural follow-up.
Positional deletes are not supported.
SplitScanTasks are already unsupported on the remote scan-plan read path, as are multiple deletion vectors in the same deletion file. These are known limitations of the remote scan-plan endpoint and are an orthogonal fix. (Core, REST: Fix delete file references for DVs in the same Puffin file #17497)
Error handling has a single classification point, HttpStatusCategory.classify(int), mapping raw status codes into coarse categories; only transient categories are retried (retry is driven by exception type). A cloud-specific refiner could later inspect error-response bodies to reclassify an otherwise-terminal status (e.g. a throttling 403) as retryable, without exposing the package-private enum.

Test plan

Pass the CIs
Optionally, with AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION, and AWS_TEST_BUCKET set: ./gradlew :iceberg-aws:integrationTest --tests org.apache.iceberg.aws.s3.TestS3FileIOLivePresignedUrls

AI Disclosure

Model: Claude Opus 4.8
Platform/Tool: Cursor
Human Oversight: fully reviewed
Prompt Summary: Implement a delegation-based HTTP(S) read path for S3FileIO (a shared core HttpUrlClient plus per-FileIO dispatch) so readers can fetch bytes from catalog-vended pre-signed URLs without native S3 credentials, with an HTTPS host allow-list and unit/integration/live-AWS test coverage.

singhpk234

Thanks for the work @williamhyun !

Added first round of comments

williamhyun · 2026-08-03T19:44:26Z

Thank you @singhpk234 for the comments, I have pushed with the most recent changes, please feel free to review again!

williamhyun · 2026-08-05T18:51:19Z

cc: @danielcweeks @RussellSpitzer @kevinjqliu

steveloughran

Commented.

Error handling is a lot more complex and s3FileIO has historically been weak here; not seen enough errors or layers above have recovered and people haven't noticed/complained about the performance penalties. Factoring it all out into one place lines up for better maintenance and adding handlers for the failure situations which haven't been handled yet.
Curious whether that 4MB block size came from; I think 8 MB is what others use.
It would be really interesting to consider the merits of prefetching or not. Parquet files do NOT want prefetching, they want parallel ranged reads in separate threads. If puffin file readers are using vector read APIs that is what they will want too. Otherwise, reading those files may benefit from prefetching. Avro files do, FWIW.

steveloughran · 2026-08-05T20:01:36Z

+  public boolean exists() {
+    try {
+      HttpGet request = new HttpGet(url);
+      request.setHeader(HttpHeaders.RANGE, "bytes=0-0");


Nice trick, curious if there's any performance penalty against aws s3 though

Thanks Steve. As far as I'm aware I don't expect a material performance penalty against S3 for this. Since GET and HEAD requests are categorized under the same per-request pricing. A partial GET that returns only one byte should be limited in extra cost. pricing

One thing to note is that GET can incur data-retrieval charges for storage classes such as Standard-IA, One Zone-IA, and Glacier Instant Retrieval, and bytes=0-0 is not satisfiable for a zero-byte object. For normal S3 Standard objects, though, I don’t expect a meaningful performance or cost difference. S3 storage classes

why don't we just use HEAD call here?

Hey @xndai, there are a couple things in our model of catalog-vended presigned URLs that limit our use of HEAD requests.

S3's remote signed header is method-bound

The catalog holds the credentials to sign the object and provide it to the client via URL.

Hence the catalog-vended URL is method bound to GET in our read use case and to do a HEAD request on the same object, we would need the catalog to vend a new, separate URL signed for the HEAD request. Effectively doubling sign calls, response payload, and introducing additional spec changes for carrying two URLs.

williamhyun · 2026-08-06T10:18:05Z

Thank you @steveloughran for the thoughtful comments,

I'm on board with the direction for the end state error handling model having classifying vs handling separation.
Given that, I'd like to keep this PR to the current minimal handling and do the RTE mapping as a dedicated follow-up: a generic HTTP-status -> RTE mapper in core to keep it cloud-agnostic, and the S3-specific bits (throttle-403 detection, isThrottlingException, the OpenSSL exception surface) layered in aws where the SDK types are available. Happy to open a tracking issue for that and tag you, would love your input on the design.
The 4MB block size came from matching ContentCache.BUFFER_CHUNK_SIZE, which is the precedent for a read chunk here, so I went with that for consistency. I see that 8MB is a common ecosystem default so I'm not strongly opinionated here. Happy to bump it to 8MB if you'd prefer we align with that convention.
Agreed that parquet wants parallel ranged reads. Currently, HTTPInputStream inherits the default readVectored, which runs them serially, which avoids prefetch but doesn't yet use the separate threads for parallel reading (would be a great follow up for reading performance).

Puffin is more nuanced to my knowledge:

Reading deletion vectors bypasses prefetch since the DV DeleteFile has exact offset and size, so reading does one positional range read for exactly the blob. Parallelism comes from the delete worker pool meaning one GET per DV, at the file/task level, rather than grouping multiple ranges into a single stream's vectored read.
Reading table stats (NDV) doesn't open the file at all as the statistics are recorded into table metadata JSON at write time.
The only path that actually prefetches is bulk blob scans via PuffinReader.readAll. Blobs are written contiguously and read in order, so prefetching coalesces them into fewer GETs. Only caller is the offline rewrite-table-path.

For this PR, I'd propose we keep the prefetch for the sequential consumers (Avro + Puffin bulk scans) and leave readVectored on its serial default for now with a follow up for enabling parallel reading. Noting that S3InputStream also relies on the serial default today, this is consistent with existing behavior.

steveloughran · 2026-08-06T22:12:56Z

The 8 MB came from AWS S3 engineers, so I'd go with that.

As for the other comments, unambitious is more likely to get reviewed by a committer. I am not one, no longer actively doing any software dev, just doing spare time reviewing.

I would at least recommend designing the http response handling such that if it were factored out then it's easy to find. And that 403 response is probably the one to go for. The SDK will retry internally, briefly, but under very heavy load or if something odd is happening like shard rebalancing capacity will drop and suddenly it will become very visible.

If the other stuff is comparable to what's already there (i.e. not a regression, w.r.t. vector reads), then all is good, separate pr work

williamhyun · 2026-08-07T21:25:24Z

Thank you @steveloughran that's a helpful way to scope it!
I've bumped the block size to be 8MB and kept the HTTP response handling changes concentrated on a single easy-to-find seam rather than attempting full error classification here. Would appreciate if you took another look at it!

To summarize what changed:

Added a HttpStatusCategory in core to map status code to a vendor-agnostic category such that call sites (sequential/positional reads, getLength(), and exists()) classify through it and then apply their own policy.
Coarse mapping for now:
- 403 -> terminal ForbiddenException on the read/length paths (exists() returns false [1])
- 404 -> NotFoundException
- 416 -> empty read
- 5xx -> retried as transient on the read path (the one-shot length probe and exists() don't retry beyond Apache HttpClient's own default retry)
- Anything else -> terminal
  Response-body parsing, store-specific throttle detection, and Retry-After/backoff have deliberately been left out to keep the PR minimal.

Thanks for the great context on throttling 403s. Since the pre-signed path bypasses the SDK, its brief internal retries don't apply here; but because the response handler already has the whole response in scope, a store-aware refiner (e.g. in the aws module) can later read the body and reclassify a throttling 403 as transient as follow-up work, without touching these core call sites.

[1] exists() classifies the same way but its policy reduces every non-2xx category to false instead of throwing. A cleaner future policy might let exists() distinguish between "definitely absent" (NOT_FOUND → false) and "couldn't verify" (FORBIDDEN/SERVER_ERROR → throw)

williamhyun · 2026-08-12T22:55:52Z

Thank you @xndai for the comments,
I have applied the feedback and would appreciate if you could take another look!

Changes made:

Centralized all HTTP status handling in a single HttpStatusCategory.classify()
Tightened classification for transient and terminal errors.
Allow-listed S3 host check before reads
Replaced the manual retry loop with Tasks-based exponential backoff

steveloughran · 2026-08-16T14:30:55Z

@williamhyun I like the classify() code design, FWIW. Elegant

xndai · 2026-08-18T16:19:04Z

+  public boolean exists() {
+    try {
+      HttpGet request = new HttpGet(url);
+      request.setHeader(HttpHeaders.RANGE, "bytes=0-0");


why don't we just use HEAD call here?

xndai · 2026-08-18T16:22:50Z

+  private static final int MAX_RETRIES = 3;
+  private static final int MIN_RETRY_WAIT_MS = 100;
+  private static final int MAX_RETRY_WAIT_MS = 5_000;
+  private static final int MAX_RETRY_DURATION_MS = 30_000;
+  private static final double RETRY_SCALE_FACTOR = 2.0;


can we make these configurable?

danielcweeks · 2026-09-02T16:36:14Z


  @Override
  public InputFile newInputFile(String path) {
+    if (HttpUrlClient.isHttpUrl(path)) {


This should probably be pushed up to a BaseFileIO implementation as opposed to duplicating these checks across all implementations.

danielcweeks · 2026-09-02T17:44:28Z

+ * schemes s3a, s3n are also treated as s3 file paths. HTTP(S) URL locations (e.g. a catalog-vended
+ * pre-signed URL) are read directly over HTTP(S) instead of through the native, credentialed S3
+ * client; see {@link #newInputFile(String)}. Using this FileIO with other schemes will result in
+ * {@link org.apache.iceberg.exceptions.ValidationException}.


I don't think this clarification is necessary. We're exposing implementation which isn't necessary. The existing documentation states https is supported, so we don't need this addition context.

danielcweeks · 2026-09-02T17:45:05Z

  private static final String ROOT_PREFIX = "s3";
+  // Default HTTP read chunk size for pre-signed URL reads, tuned for S3; overridable via the
+  // io.http.read.chunk-size-bytes property.
+  private static final int HTTP_READ_CHUNK_SIZE_BYTES_DEFAULT = 8 * 1024 * 1024; // 8 MB


This shouldn't be in S3FileIO since it's a property of the HTTP implementation, not the S3 implementation.

This is saying s3 prefers 8 MB read ... it might happen ADLS prefers 64 MB ... the generic HTTPInputStream is configurable to what the objectstore provider FileIO prefers, so s3 here is sending this HTTPInputStream it need to read it 8 MB chunks ... rather than HTTPInputstream fixing the http_read_chunk_bytes for everyone.

danielcweeks · 2026-09-02T17:45:50Z

  private SerializableMap<String, String> properties = null;
  private MetricsContext metrics = MetricsContext.nullMetrics();
  private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+  private transient volatile HttpUrlClient httpUrlClient;


The http client shouldn't be at the S3FileIO, but rather managed by the HTTPInput/Output Class structure.

danielcweeks · 2026-09-02T17:46:26Z


  @Override
  public InputFile newInputFile(String path, long length) {
+    if (HttpUrlClient.isHttpUrl(path)) {


Similar to above, this should be delegated to a superclass implementation that can be shared across IO implementations.

danielcweeks · 2026-09-02T17:51:46Z

  @Override
  public void initialize(Map<String, String> props) {
    this.properties = SerializableMap.copyOf(props);
+    // reset so the next access rebuilds from the new properties


This should be pushed to a super class as well.

danielcweeks · 2026-09-02T17:53:30Z

+  static final Set<String> PRESIGNED_READ_ALLOWED_HOSTS_DEFAULT =
+      Collections.unmodifiableSet(Sets.newHashSet("amazonaws.com", "amazonaws.com.cn"));


Why is this necessary? We shouldn't be prescribing the hosts or managing them from the client. The host is a catalog concern, the client should only process the request.

danielcweeks · 2026-09-02T17:54:07Z

+  public Set<String> presignedReadAllowedHosts() {
+    return presignedReadAllowedHosts;
+  }
+
+  private static Set<String> parsePresignedReadAllowedHosts(
+      Map<String, String> properties, String endpoint) {
+    Set<String> hosts = Sets.newHashSet();
+    String configured = properties.get(PRESIGNED_READ_ALLOWED_HOSTS);
+    if (configured == null || configured.trim().isEmpty()) {
+      hosts.addAll(PRESIGNED_READ_ALLOWED_HOSTS_DEFAULT);
+    } else {
+      for (String suffix : Splitter.on(',').trimResults().omitEmptyStrings().split(configured)) {
+        hosts.add(suffix.toLowerCase(Locale.ROOT));
+      }
+    }
+
+    // Always trust the configured S3 endpoint host, so custom, VPC, or S3-compatible endpoints
+    // work without extra configuration.
+    if (endpoint != null) {
+      try {
+        String endpointHost = URI.create(endpoint).getHost();
+        if (endpointHost != null) {
+          hosts.add(endpointHost.toLowerCase(Locale.ROOT));
+        }
+      } catch (IllegalArgumentException e) {
+        // An unparseable endpoint is validated elsewhere; ignore it for host allow-listing.
+      }
+    }
+
+    return hosts;
+  }
+


This isn't necessary and we should remove it.

danielcweeks · 2026-09-02T17:55:06Z

+/**
+ * Restricts catalog-vended pre-signed reads to HTTPS URLs on operator-trusted hosts, before {@link
+ * S3FileIO} fetches them over HTTP with no S3 credentials. This bounds a compromised or malicious
+ * catalog to naming files on the expected storage hosts rather than turning the reader into a
+ * general-purpose fetcher of arbitrary URLs.
+ */
+class S3PresignedReadValidation {


We shouldn't be protecting from the catalog. This is unnecessary protection because the catalog is the one delegating access.

I was worried about this before because I'm terrified of driving a client to a uri that somehow gives access to the client. But I now think we can't actually do anything intelligent here because we don't really now what the endpoints should be.

danielcweeks · 2026-09-02T20:54:41Z

+ *
+ * <p>A length that cannot be determined is reported as {@link #UNKNOWN_LENGTH}.
+ */
+class HttpHeaderUtil {


This util class doesn't strike me as necessary. This is logic that should be part of the HTTPInputFile implementation. I don't think we need a separate class for this functionality?

Generally, utility classes are created when there are multiple independent use cases, but that doesn't seem to be the case here.

danielcweeks · 2026-09-02T20:59:02Z

+    if (length == HttpHeaderUtil.UNKNOWN_LENGTH) {
+      this.length = fetchContentLength();
+    }
+
+    return length;


This logic doesn't look quite right. If we see UNKNOWN_LENGTH then we try to fetch the content length, but that can result in an UNKNOWN_LENGTH which would then produce the incorrect length from this call.

If can't establish the length, we should throw.

danielcweeks · 2026-09-02T21:44:38Z

+ * status that is actually transient (for example a throttling {@code 403}, which some object stores
+ * return under heavy load) so that it is retried too.
+ */
+enum HttpStatusCategory {


I don't understand why we need this class. If this is all isolated to the Http implementation, we don't need an agnostic mapping (we just use the codes from the client).

This is doing a classification of the error code to states ... for example which as transient (throttling) / which are persistent (403..) and based on that its taking action in stream .... i wonder why can't we do this in the https://github.com/apache/iceberg/pull/17457/changes#diff-88dbd57947faabb967a76beac0e51b21b52df326a25086cde68f983c43256708R238 ?
will think this more

danielcweeks · 2026-09-02T21:53:45Z

+ * When the property is not set, the default passed to {@link #HttpUrlClient(Map, int)} is used,
+ * letting a {@link org.apache.iceberg.io.FileIO} supply a value tuned for its backing object store.
+ */
+public class HttpUrlClient implements Serializable {


This class doesn't make sense to me. We already have a HttpClient interface in the project and this seems to create a separate wrapper interface over a specific implementation.

I don't think we should have this. The client can be initialized and held by the HttpInputFile class directly. This gives the impression that we need a separate standard way of building a client, but it doesn't abstract the underlying implementation and seems like an unnecessary and exposes things in a strange way. It's not actually an HTTP client and only eposes access to InputFile.

williamhyun added 2 commits July 31, 2026 16:04

AWS, Core: Read S3 files via catalog-vended HTTPS location

e34da8c

Comments

550ded1

github-actions Bot added core AWS labels Jul 31, 2026

singhpk234 requested review from RussellSpitzer and danielcweeks August 1, 2026 15:35

singhpk234 reviewed Aug 1, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpUrlClient.java

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpInputStream.java

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpUrlSupport.java Outdated

Add configurable properties

73b5897

Fix CIs

d5342f4

williamhyun changed the title ~~AWS, Core: Read S3 files via catalog-vended HTTPS location~~ AWS, Core: Read S3 files via catalog-vended HTTPS URLs Aug 3, 2026

steveloughran suggested changes Aug 5, 2026

View reviewed changes

Comments

dbbfb24

williamhyun requested a review from steveloughran August 6, 2026 10:18

Centralize HTTP response classification for pre-signed URLs

4fb0d28

williamhyun added 2 commits August 7, 2026 15:47

message assertion

a5e8b8e

rerun CIs

b0596af

singhpk234 reviewed Aug 11, 2026

View reviewed changes

xndai reviewed Aug 11, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpUrlSupport.java Outdated

xndai reviewed Aug 11, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpUrlClient.java

xndai reviewed Aug 11, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/io/http/HTTPInputStream.java Outdated

xndai reviewed Aug 11, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/io/http/HttpInputStream.java

williamhyun added 2 commits August 11, 2026 14:59

impl HTTP read chunk size configurable

e0b5d2a

Address retryable error + exponential retry comments

6f294ab

singhpk234 requested a review from xndai August 12, 2026 05:36

Rerun CIs

0b2929f

williamhyun added 2 commits August 12, 2026 14:12

rerun CIs

b0ae357

Make naming scheme uniform

75f323f

xndai reviewed Aug 18, 2026

View reviewed changes

danielcweeks reviewed Sep 2, 2026

View reviewed changes

		static final Set<String> PRESIGNED_READ_ALLOWED_HOSTS_DEFAULT =
		Collections.unmodifiableSet(Sets.newHashSet("amazonaws.com", "amazonaws.com.cn"));

Uh oh!

Conversation

williamhyun commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Scope / compatibility notes

Test plan

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williamhyun commented Aug 3, 2026

Uh oh!

williamhyun commented Aug 5, 2026

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williamhyun commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveloughran commented Aug 6, 2026

Uh oh!

williamhyun commented Aug 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williamhyun commented Aug 12, 2026

Uh oh!

steveloughran commented Aug 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamhyun commented Jul 31, 2026 •

edited

Loading

williamhyun commented Aug 6, 2026 •

edited

Loading