laskoviymishka commented on code in PR #1423:
URL: https://github.com/apache/iceberg-go/pull/1423#discussion_r3557590530
##########
io/gocloud/s3.go:
##########
@@ -156,6 +158,70 @@ func applyS3TransportTuning(t *http.Transport) {
// path-style addressing. It defaults to virtual-hosted style for
// standard AWS S3 and path-style for custom endpoints (e.g. MinIO).
// The s3.force-virtual-addressing property can override either default.
+func applyS3ClientOptions(endpoint string, props map[string]string)
func(*s3.Options) {
+ return func(o *s3.Options) {
+ if endpoint != "" {
Review Comment:
The trigger being `endpoint != ""` treats every custom endpoint as if it
were GCS, and I don't think that holds.
MinIO, R2, Ceph, and LocalStack all support SigV4 request checksums and
don't choke on the `Amz-Sdk-*` signed headers, so this quietly disables
checksum-on-write for all of them — and it does it by overloading
`s3.endpoint`/`AWS_S3_ENDPOINT`, a stable documented property, so every
existing MinIO/R2 deployment picks up the new behavior on upgrade with no way
to opt out. PyIceberg and iceberg-rust both gate this behind an explicit
"S3-compatible mode" knob rather than inferring it from the endpoint being set.
I'd gate the workaround behind either a dedicated property (`s3.compat-mode`
/ `s3.disable-checksum-headers`, defaulting off) or a `storage.googleapis.com`
host check, so we're not changing behavior for backends that don't need it.
Which direction do you prefer?
##########
io/gocloud/s3.go:
##########
@@ -156,6 +158,70 @@ func applyS3TransportTuning(t *http.Transport) {
// path-style addressing. It defaults to virtual-hosted style for
// standard AWS S3 and path-style for custom endpoints (e.g. MinIO).
// The s3.force-virtual-addressing property can override either default.
+func applyS3ClientOptions(endpoint string, props map[string]string)
func(*s3.Options) {
+ return func(o *s3.Options) {
+ if endpoint != "" {
+ o.BaseEndpoint = aws.String(endpoint)
+ o.RequestChecksumCalculation =
aws.RequestChecksumCalculationWhenRequired
+ o.APIOptions = append(o.APIOptions,
stripS3InputChecksumAlgorithm)
+ o.APIOptions = append(o.APIOptions,
stripGCSIncompatibleSignedHeaders)
+ }
+ o.UsePathStyle = resolveUsePathStyle(endpoint, props)
+ o.DisableLogOutputChecksumValidationSkipped = true
+ }
+}
+
+func stripS3InputChecksumAlgorithm(stack *smithymiddleware.Stack) error {
+ m := smithymiddleware.InitializeMiddlewareFunc(
+ "iceberg-go/strip-s3-input-checksum-algorithm",
+ func(ctx context.Context, in smithymiddleware.InitializeInput,
next smithymiddleware.InitializeHandler) (smithymiddleware.InitializeOutput,
smithymiddleware.Metadata, error) {
+ switch v := in.Parameters.(type) {
+ case *s3.PutObjectInput:
+ v.ChecksumAlgorithm = ""
+ case *s3.UploadPartInput:
+ v.ChecksumAlgorithm = ""
+ case *s3.CreateMultipartUploadInput:
+ v.ChecksumAlgorithm = ""
+ }
+
+ return next.HandleInitialize(ctx, in)
+ },
+ )
+
+ if err := stack.Initialize.Insert(m, "AWSChecksum:SetupInputContext",
smithymiddleware.Before); err != nil {
+ return stack.Initialize.Add(m, smithymiddleware.Before)
+ }
+
+ return nil
+}
+
+var gcsIncompatibleSignedHeaders = []string{
+ "Amz-Sdk-Invocation-Id",
+ "Amz-Sdk-Request",
+ "Accept-Encoding",
Review Comment:
I think there's a read-side regression hiding in the `Accept-Encoding` entry
here.
`stripGCSIncompatibleSignedHeaders` is registered before `Signing`, and
`Signing` runs on every op — `GetObject`, `ListObjectsV2`, not just writes. So
this deletes `Accept-Encoding` on reads too. The SDK installs
`DisableAcceptEncodingGzip`, which sets `Accept-Encoding: identity`
specifically so Go's `http.Transport` won't auto-negotiate gzip and
transparently decompress the body. Deleting the header entirely (rather than
just omitting it from the signed set) means `net/http` is free to add
`Accept-Encoding: gzip` itself and silently decompress the response — which
corrupts the Content-Length-bounded footer/range reads we lean on all over the
Parquet/Avro/manifest read path against any endpoint that gzips.
Two ways out: scope this middleware to write ops with the same input-type
switch `stripS3InputChecksumAlgorithm` uses, or re-set `Accept-Encoding:
identity` after deleting it instead of leaving it absent. The first feels
cleaner since the GCS signing problem is a write concern. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
