zeroshade commented on code in PR #2075:
URL: https://github.com/apache/iceberg-go/pull/2075#discussion_r4160158324
##########
catalog/rest/rest.go:
##########
@@ -1302,6 +1303,7 @@ func (r *Catalog) tableFromResponse(
r,
table.WithMetricsReporter(reporter),
table.WithScanPlanningIOProperties(scanPlanningConfig),
+ table.WithSavedConfig(config),
Review Comment:
Sorry for the previous AI-assisted responses, there's just too many PRs on
here for me to stay on top of without the help :smile:
The issue is that `WithSavedConfig` is supposed to hold the `caller's`
config, but with the current implementation it also collects tokens and
credentials that it shouldn't be. This is because `tableFromResponse` saves the
fully merged map and `RefreshTableCredentials` feeds that right back in.
For example:
```go
tbl := table.New(ident, meta, loc, fsF, cat,
table.WithSavedConfig(iceberg.Properties{
"s3.region": "us-west-2",
"client.factory": "...",
}))
refreshed, _ := cat.RefreshTableCredentials(ctx, tbl)
refreshed.SavedConfig()
// {
// "s3.region": "us-west-2", "client.factory": "...", <- what the caller
saved
// "token": "<catalog OAuth token>", <- from r.props
// "s3.access-key-id": "...", "s3.secret-access-key": "...",
"s3.session-token": "...", <- just vended
// }
```
The `SavedConfig()` shouldn't end up with the credentials from r.props or
the vended credentials, it should be *just* what was passed in, right?
Since `RefreshTableCredentials` rewrites the saved config on every refresh,
it actually breaks the contract you wanted, and exposes the catalog's token
through an exported accessor on every table loaded from the REST catalog. It
also causes a problem in the two-catalog case: a table loaded from Catalog A
and refreshed via Catalog B ends up with A's saved `token` overriding B's
`r.props` and so would fail.
Since the props are re-merged on every refresh, and the vended creds are
re-fetched and go stale after renewals, we don't actually have to save
*everything* in extra layers. The fix ends up being pretty narrow:
In `tableFromResponse` we stop saving the merged map:
```go
return table.New(identifier, metadata, loc, fsF, r,
append([]table.Option{
table.WithMetricsReporter(reporter),
table.WithScanPlanningIOProperties(scanPlanningConfig),
// (no WithSavedConfig here)
}, opts...)...)
```
In `LoadTable` / `CreateTable` / `RegisterTable` we save only the per-table
pieces:
```go
saved := maps.Clone(ret.Metadata.Properties())
maps.Copy(saved, ret.Config)
r.tableFromResponse(ctx, ident, ret.Metadata, loc, config, scanCfg, vended,
table.WithSavedConfig(saved))
```
In `RefreshTableCredentials` we just carry the caller's saved config through
unchanged:
```go
config := maps.Clone(r.props)
maps.Copy(config, tbl.SavedConfig())
scanCfg := maps.Clone(config) // r.props + saved, no creds
maps.Copy(config, resp)
return r.tableFromResponse(ctx, tbl.Identifier(), tbl.Metadata(),
metadataLoc,
config, scanCfg, true,
append(opts, table.WithSavedConfig(tbl.SavedConfig()))...)
```
The result is that `refreshed.SavedConfig()` will only equal what the caller
passed in and won't collect the credentials that it shouldn't have.
Catalog-loaded tables will get the exact FileIO config that `LoadTable` builds,
and `ScanPlanningConfig()` will no longer need to get exported. Then you can
flip the test at
https://github.com/apache/iceberg-go/pull/2075/changes#diff-407faac123666eb32e49f09d6bc7b33d7ef596379df8505c656bb17b5fbb6a54R232
to assert the saved config round-trips completely unchanged. It also
simplifies the whole layering I think and makes it easier to follow.
Does that make a bit more sense instead of AI slop? Lol
--
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]
