fix(aws): recognize S3 dual-stack endpoints in URL parsing#780
Conversation
`AmazonS3Builder::parse_url` matched S3 hostnames against a fixed set of patterns via `splitn(4, '.')`. Dual-stack endpoints insert a `dualstack` label after `s3` (e.g. `s3.dualstack.<region>.amazonaws.com` for path-style and `<bucket>.s3.dualstack.<region>.amazonaws.com` for virtual-hosted), which matched none of the patterns and fell through to `UrlNotRecognised`. Because `ObjectStoreScheme::parse` already classifies these URLs as S3, `DefaultObjectStoreRegistry::resolve` would call into the S3 builder and then fail with `ObjectStore not found`. Add handling for both dual-stack forms, setting the endpoint explicitly so the `dualstack` label is preserved (IPv6 addressing) rather than being lost when the endpoint is reconstructed from the region. Includes a registry regression test plus builder and scheme-parse coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014n2xnhDiTk4T8RESdRDgDS Signed-off-by: Andrew Duffy <andrew@a10y.dev>
a10y
changed the title
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM. TIL about dualstack.
My only nit is around strict URL parsing. I wonder if we should be as loose as possible when parsing URLs, rather than trying to determine what is “valid” ourselves. We can let the server make that decision.
I am not quite sure what you are suggesting @kevinjqliu. Are you thinking maybe preserve the original URL in the builder rather than try and recreate it? If so that seems reasonable to me However, I think this PR is consistent with the exsting pattern in the code -- so we can proceed with this one while we can investigate more flexible/futureproof configuration as a follow on PR / issue |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @a10y and @kevinjqliu
|
Please proceed with this one! The change is reasonable since the "dual-stack endpoint" conforms to a pre-defined shape. I was pointing out that This was an idea based on #711 where we once again had to allowlist a pre-defined shape into the url parsing logic for Azure/OneLake. EDIT: I created #793. Let's move the conversation there 😄 |
Thank you 🙏 |
4 participants
Which issue does this PR close?
What changes are included in this PR?
Adds support for parsing and resolving
AmazonS3stores for S3 dual-stack endpoints. Adds cases to the existing URL parsing tests.I encountered this trying to use
object_storeto read some data from a HuggingFace dataset, where one column of image URLs pointed to many values likehttps://s3.dualstack.us-east-1.amazonaws.com/....In the first table of the S3 endpoint docs you can see an example of these:
Are there any user-facing changes?
URLs that previously would error will now resolve.