moomindani commented on PR #17979: URL: https://github.com/apache/iceberg/pull/17979#issuecomment-5611227157
Thanks @sungwy — good catch, added in `1f471cbb1`. You were right that this is a separate defect and not covered by the `HTTPClient` change: `S3V4RestSignerClient` copies the response headers into its own plain `HashMap`, so `canBeCached` looked `Cache-Control` up in a map keyed by whatever spelling the server used. It is the same second-copy pattern as `RESTSessionCatalog`. The consequence is worse than the ETag miss — with a lowercase `cache-control` the signed component is never cached, so every S3 request signs remotely again. The test is `TestS3V4RestSignerClient.signedComponentIsCachedRegardlessOfCacheControlHeaderCase`, parameterized over both spellings; on an unfixed tree `[cache-control]` fails and `[Cache-Control]` passes. Worth noting for anyone who tries the same: I first wrote this in `TestS3RestSigner` and it could not express the case at all. Jetty rewrites the field name to `Cache-Control` on the wire even when the servlet sets it lowercase, so an integration test there passes with or without the fix. On the other half of what you linked, `reconstructHeaders`: its input is `remoteSignResponse.headers()`, the JSON body of the sign response rather than HTTP header fields, so RFC 9110's case-insensitivity does not settle it. If a signer service did put `cache-control` in that body it would not be stripped and would be forwarded to S3, which could break the signature. I would rather not fold that into a PR titled after HTTP response headers — happy to open a separate issue and PR if you think it is worth fixing. Retitled to "Core, AWS" since it now touches both modules. -- 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]
