zeroshade commented on code in PR #1259:
URL: https://github.com/apache/iceberg-go/pull/1259#discussion_r3521437089
##########
io/gocloud/blob_test.go:
##########
@@ -57,42 +86,142 @@ func TestDefaultKeyExtractor(t *testing.T) {
input: "gs://my-bucket/path/to/file.parquet",
expectedKey: "path/to/file.parquet",
},
+ {
+ name: "azure URI with path",
+ bucketName: "[email protected]",
+ input:
"abfs://[email protected]/path/to/file.parquet",
+ expectedKey: "path/to/file.parquet",
+ },
+ {
Review Comment:
[MINOR] The tests exercise different-bucket rejection here, but they still
do not cover the wrong-scheme/same-authority bypass. Please add cases for an s3
FileIO receiving `gs://same-bucket/...` and a gs FileIO receiving
`s3://same-bucket/...`.
##########
io/gocloud/blob.go:
##########
@@ -69,33 +68,102 @@ func (f *blobOpenFile) Sys() any {
return f.b }
func (f *blobOpenFile) IsDir() bool { return false }
func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return f, nil }
-// KeyExtractor extracts the object key from an input path
+// KeyExtractor extracts the object key from an input path.
type KeyExtractor func(path string) (string, error)
-// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI
-// e.g., s3://bucket/path/file -> path/file
-func defaultKeyExtractor(bucketName string) KeyExtractor {
+var errEmptyObjectKey = errors.New("object key is empty")
Review Comment:
[MINOR] `errEmptyObjectKey` is now surfaced through public IO errors
(including `WalkDir` handling around `io/gocloud/blob.go:301-304`) but remains
unexported, so consumers cannot use `errors.Is` against it. Export it or
document that callers should not depend on this sentinel.
##########
io/gocloud/blob.go:
##########
@@ -69,33 +68,102 @@ func (f *blobOpenFile) Sys() any {
return f.b }
func (f *blobOpenFile) IsDir() bool { return false }
func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return f, nil }
-// KeyExtractor extracts the object key from an input path
+// KeyExtractor extracts the object key from an input path.
type KeyExtractor func(path string) (string, error)
-// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI
-// e.g., s3://bucket/path/file -> path/file
-func defaultKeyExtractor(bucketName string) KeyExtractor {
+var errEmptyObjectKey = errors.New("object key is empty")
+
+type objectLocation struct {
+ authority string
+ key string
+ uriPrefix string
+ hasAuthority bool
+}
+
+func splitObjectLocation(location string) (objectLocation, error) {
+ scheme, rest, ok := strings.Cut(location, "://")
+ if !ok {
+ return objectLocation{key: location}, nil
+ }
+
+ authorityEnd := strings.IndexAny(rest, "/?#")
+ if authorityEnd == -1 {
+ authorityEnd = len(rest)
+ }
+
+ authority := rest[:authorityEnd]
+ if authority == "" {
+ return objectLocation{}, fmt.Errorf("URI authority is empty:
%s", location)
+ }
+
+ key := ""
+ if authorityEnd < len(rest) {
+ if rest[authorityEnd] != '/' {
+ return objectLocation{}, fmt.Errorf("URI authority %q
must be followed by an object path: %s",
+ authority, location)
+ }
+
+ // Keep object keys raw. Cloud object names are opaque strings,
so this
+ // intentionally does not URL-unescape literal %, spaces, ? or
#.
+ key = strings.TrimPrefix(rest[authorityEnd:], "/")
+ }
+
+ return objectLocation{
+ authority: authority,
+ key: key,
+ uriPrefix: scheme + "://" + authority + "/",
+ hasAuthority: true,
+ }, nil
+}
+
+type objectLocationExtractor func(location string) (objectLocation, error)
+
+func keyExtractorFromObjectLocation(extract objectLocationExtractor)
KeyExtractor {
return func(location string) (string, error) {
- _, after, found := strings.Cut(location, "://")
- if found {
- location = after
+ parsed, err := extract(location)
+ if err != nil {
+ return "", err
+ }
+
+ return parsed.key, nil
+ }
+}
+
+func defaultObjectLocationExtractor(bucketName string) objectLocationExtractor
{
+ return func(location string) (objectLocation, error) {
+ parsed, err := splitObjectLocation(location)
+ if err != nil {
+ return objectLocation{}, err
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ if parsed.hasAuthority {
Review Comment:
[MAJOR] `splitObjectLocation` records only authority-derived state while
`defaultObjectLocationExtractor` validates only `authority == bucketName`, so
an S3-backed FileIO can accept `gs://same-bucket/path` (and the reverse) as
long as the authority matches. Carry the URI scheme in `objectLocation`,
validate it against the backend's allowed schemes from registration, and add
s3+gs wrong-scheme/same-authority tests.
##########
io/gocloud/azure.go:
##########
@@ -201,21 +201,48 @@ func createAzureBucket(ctx context.Context, parsed
*url.URL, props map[string]st
return azureblob.OpenBucket(ctx, client, nil)
}
-// adlsKeyExtractor creates a key extractor for Azure schemes using the
adlsURIPattern pattern
-func adlsKeyExtractor() KeyExtractor {
- return func(location string) (string, error) {
+func adlsAuthority(parsed *url.URL) string {
+ if parsed.User == nil {
+ return parsed.Hostname()
+ }
+
+ return parsed.User.Username() + "@" + parsed.Hostname()
+}
+
+func adlsObjectLocationExtractor(parsedURL *url.URL) objectLocationExtractor {
+ expectedAuthority := adlsAuthority(parsedURL)
+
+ return func(location string) (objectLocation, error) {
matches := adlsURIPattern.FindStringSubmatch(location)
if len(matches) < 4 {
- return "", fmt.Errorf("invalid ADLS location: %s",
location)
+ return objectLocation{}, fmt.Errorf("invalid ADLS
location: %s", location)
+ }
+
+ authority := matches[2]
+ if authority != expectedAuthority {
Review Comment:
[MAJOR] This hard authority check also applies to the generic blob path at
`io/gocloud/blob.go:139-143`, so read/write/delete/walk reject object URIs
whose bucket/container differs from the opened root. Iceberg tables may legally
reference data in another bucket/container. Please make an explicit design
choice: route per authority, return a documented unsupported-mode error, or
narrow the rejection to write/delete only.
--
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]
