laskoviymishka commented on code in PR #1259:
URL: https://github.com/apache/iceberg-go/pull/1259#discussion_r3474015374
##########
io/gocloud/blob.go:
##########
@@ -188,25 +284,52 @@ func (bfs *blobFileIO) NewWriter(ctx context.Context,
path string, overwrite boo
nil
}
-func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor) icebergio.IO {
- return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor, ctx: ctx}
+func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor, extractObject ...objectLocationExtractor) icebergio.IO {
Review Comment:
I'd make `extractObject` a required parameter rather than variadic.
The variadic reads as composable but isn't — only `[0]` is ever used, a
second arg is silently dropped, and `createBlobFS(ctx, bucket, ext, nil)` is
legal and quietly installs a nil extractor that sends every call down the
fallback path below. The only callers relying on the optionality are a few old
3-arg test helpers; I'd rather update those four call sites than keep a
fallback that papers over a real semantic difference.
##########
io/config.go:
##########
@@ -17,6 +17,14 @@
package io
+// Constants for generic object store configuration options.
+const (
+ // ObjectStoreStrictAuthorityValidation rejects fully-qualified object
paths
+ // whose URI authority differs from the bucket or container used to
create
+ // the FileIO.
+ ObjectStoreStrictAuthorityValidation =
"object-store.strict-authority-validation"
Review Comment:
This sits at the top level next to `s3.*`/`gcs.*`/`adls.*`, all of which are
mirrored in Java and PyIceberg — but `object-store.strict-authority-validation`
is Go-only. Java's `S3FileIO`/`ADLSFileIO` and PyIceberg's fsspec create
per-authority FileIO instances and have no cross-authority knob, so a user who
sets this in table properties expecting portable behavior gets it honored only
by the gocloud backend and silently ignored elsewhere.
I'd either move it into the gocloud package unexported and wire it through
the functional option you already have, or, if it stays exported, add a doc
comment that it's honored only by the gocloud FileIO and ignored by
Java/PyIceberg/Rust. wdyt?
##########
io/gocloud/blob.go:
##########
@@ -69,33 +68,130 @@ 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)
+ }
+
+ key = strings.TrimPrefix(rest[authorityEnd:], "/")
+ }
+
+ return objectLocation{
+ authority: authority,
+ key: key,
+ uriPrefix: scheme + "://" + authority + "/",
+ hasAuthority: true,
+ }, nil
+}
+
+type keyExtractorConfig struct {
+ strictAuthorityValidation bool
+}
+
+type keyExtractorOption func(*keyExtractorConfig)
+
+func withStrictAuthorityValidation() keyExtractorOption {
+ return func(cfg *keyExtractorConfig) {
+ cfg.strictAuthorityValidation = true
+ }
+}
+
+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 legacyAuthorityKey(parsed objectLocation) string {
+ if parsed.key == "" {
+ return parsed.authority + "/"
+ }
+
+ return parsed.authority + "/" + parsed.key
+}
+
+func defaultObjectLocationExtractor(bucketName string, opts
...keyExtractorOption) objectLocationExtractor {
+ var cfg keyExtractorConfig
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ return func(location string) (objectLocation, error) {
+ parsed, err := splitObjectLocation(location)
+ if err != nil {
+ return objectLocation{}, err
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ if parsed.hasAuthority {
+ if parsed.authority != bucketName {
+ if cfg.strictAuthorityValidation {
+ return objectLocation{},
fmt.Errorf("URI authority %q does not match configured authority %q",
+ parsed.authority, bucketName)
+ }
- if key == "" {
- return "", fmt.Errorf("URI path is empty: %s", location)
+ parsed.key = legacyAuthorityKey(parsed)
Review Comment:
In non-strict mode a cross-authority URI silently becomes a key inside the
configured bucket, which I think is riskier than the old behavior.
`s3://data-bucket/snap-1.avro` against a FileIO bound to `meta-bucket`
writes an object at key `data-bucket/snap-1.avro` inside `meta-bucket` and
reports success, where the SDK previously returned a clear not-found. I'd
either make strict the default with an explicit
`object-store.allow-cross-authority` opt-out, or at minimum emit a structured
warning here so a misconfigured FileIO leaves a trace instead of silently
misplacing data. Which way would you prefer — strict-by-default, or keep the
legacy fold but make it loud?
##########
io/gocloud/blob.go:
##########
@@ -188,25 +284,52 @@ func (bfs *blobFileIO) NewWriter(ctx context.Context,
path string, overwrite boo
nil
}
-func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor) icebergio.IO {
- return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor, ctx: ctx}
+func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor, extractObject ...objectLocationExtractor) icebergio.IO {
+ var extractor objectLocationExtractor
+ if len(extractObject) > 0 {
+ extractor = extractObject[0]
+ }
+
+ return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor,
extractObject: extractor, ctx: ctx}
+}
+
+func (bfs *blobFileIO) objectLocation(root string) (objectLocation, error) {
+ if bfs.extractObject != nil {
+ return bfs.extractObject(root)
+ }
+
+ location, err := splitObjectLocation(root)
+ if err != nil {
+ return objectLocation{}, err
+ }
+
+ key, err := bfs.preprocess(root)
+ location.key = key
+
+ return location, err
}
func (bfs *blobFileIO) WalkDir(root string, fn fs.WalkDirFunc) error {
- parsed, err := url.Parse(root)
+ location, err := bfs.objectLocation(root)
+ walkPath := location.key
if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
+ if !errors.Is(err, errEmptyObjectKey) {
+ return &fs.PathError{Op: "walk dir", Path: root, Err:
err}
+ }
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
walkPath = "."
}
- parsed.Path = ""
-
return fs.WalkDir(bfs.Bucket, walkPath, func(path string, d
fs.DirEntry, err error) error {
- return fn(parsed.JoinPath(path).String(), d, err)
+ if location.hasAuthority {
+ if path == "." {
+ path = location.uriPrefix
+ } else {
+ path = location.uriPrefix + path
Review Comment:
The same doubled-authority bug surfaces here on the non-strict path even
with `extractObject` set: `defaultObjectLocationExtractor` folds the authority
into `parsed.key` via `legacyAuthorityKey` but leaves `uriPrefix` as the
original `s3://other-bucket/`, so `walkPath = "other-bucket/data"`, the walk
yields `other-bucket/data/file.parquet`, and this line prepends
`s3://other-bucket/` to give `s3://other-bucket/other-bucket/data/file.parquet`.
A table case would lock this down: write a file at
`other-bucket/data/file.parquet`, walk `s3://other-bucket/data` non-strict
against a FileIO bound to `my-bucket`, and assert no returned path contains a
duplicated authority. Every current WalkDir test uses a matching authority or
strict mode, so this path is unguarded.
##########
io/gocloud/blob.go:
##########
@@ -188,25 +284,52 @@ func (bfs *blobFileIO) NewWriter(ctx context.Context,
path string, overwrite boo
nil
}
-func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor) icebergio.IO {
- return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor, ctx: ctx}
+func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor, extractObject ...objectLocationExtractor) icebergio.IO {
+ var extractor objectLocationExtractor
+ if len(extractObject) > 0 {
+ extractor = extractObject[0]
+ }
+
+ return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor,
extractObject: extractor, ctx: ctx}
+}
+
+func (bfs *blobFileIO) objectLocation(root string) (objectLocation, error) {
+ if bfs.extractObject != nil {
+ return bfs.extractObject(root)
+ }
+
+ location, err := splitObjectLocation(root)
+ if err != nil {
+ return objectLocation{}, err
+ }
+
+ key, err := bfs.preprocess(root)
Review Comment:
This fallback computes the key two incompatible ways and they disagree on
cross-authority input.
For `s3://other-bucket/path`, `splitObjectLocation` leaves
`location.uriPrefix = "s3://other-bucket/"` with `key = "path"`, but
`bfs.preprocess(root)` (non-strict `defaultKeyExtractor`) then overwrites
`location.key` with `legacyAuthorityKey` = `"other-bucket/path"` while
`uriPrefix` still points at `other-bucket`. WalkDir reconstructs
`s3://other-bucket/other-bucket/path` — authority doubled. If we make
`extractObject` required (see the `createBlobFS` comment) this branch goes away
entirely; if it stays, the key needs to come from `splitObjectLocation` too so
prefix and key agree.
--
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]
