zeroshade commented on code in PR #2075:
URL: https://github.com/apache/iceberg-go/pull/2075#discussion_r4158488026
##########
catalog/rest/rest.go:
##########
@@ -1302,6 +1303,7 @@ func (r *Catalog) tableFromResponse(
r,
table.WithMetricsReporter(reporter),
table.WithScanPlanningIOProperties(scanPlanningConfig),
+ table.WithSavedConfig(config),
Review Comment:
`config` here is the fully merged FileIO map: `r.props` → metadata props →
`ret.Config` → vended creds. `r.props` carries the catalog `token` and
`credential` when they are configured (`toProps`, rest.go:809-810; the field
comment at rest.go:855-856 already notes this), so with this line plus the
exported `SavedConfig()`/`ScanPlanningConfig()` (table/table.go:137-138), every
REST-loaded table hands the catalog bearer token and client secret to anything
holding a `*table.Table`, and the `WithSavedConfig` godoc invites callers to
copy that map around.
None of those layers need to be saved:
- `r.props` is merged again first in `RefreshTableCredentials`
(rest.go:1354). The saved copy only matters when a table loaded from one
catalog is refreshed through another, and then the `token` saved from the first
catalog overrides the second catalog `token` in the FileIO config (S3 uses
`token` as bearer auth, io/gocloud/s3/s3.go:58-62). That is per-catalog state
crossing into a separately constructed catalog.
- Vended creds are always re-fetched by `RefreshTableCredentials`, and the
copy saved here goes stale as soon as the refresher renews.
- Metadata properties are already on the table (`tbl.Properties()`).
The only layer that cannot be recovered from the table is the load response
`config` block, so save just that: add a `tableConfig iceberg.Properties`
parameter to `tableFromResponse` (`ret.Config` from
LoadTable/CreateTable/RegisterTable, nil from UpdateTable) and pass it to
`WithSavedConfig`. `RefreshTableCredentials` then rebuilds in LoadTable order
(`r.props` → `tbl.Properties()` → `tbl.SavedConfig()` → creds), derives the
scan-planning config from the same merge minus creds, and
`ScanPlanningConfig()` does not need to be exported.
@laskoviymishka this changes what goes into the saved config you were happy
with in round 2; the option and accessor stay, only the contents shrink.
##########
catalog/catalog.go:
##########
@@ -215,6 +215,24 @@ type Closer interface {
Close() error
}
+// RefreshableCredentialCatalog is an optional interface implemented by
catalogs that
+// support temporarily-vended credentials. Temporarily-vended credentials
scope object
+// storage access to just files allowed by the user making a related request
to the
+// catalog.
+type RefreshableCredentialCatalog interface {
+ // RefreshTableCredentials loads temporarily-vended credentials for a
previously
+ // loaded table, and returns a new/modified table instance with
up-to-date credentials
+ // if the catalog is using temporarily-vended credentials for this
table.
+ // In this case, the instance should be able to refresh its own
credentials internally
+ // making it useful for use as a long-lived instance. For tables that do
+ // not have catalog-vended storage credentials, the table can be
returned with no
+ // modifications.
+ //
+ // The main use-case for this method is to be able to make
externally-created table
+ // instances compatible with catalog-based credential refreshes.
+ RefreshTableCredentials(ctx context.Context, tbl *table.Table)
(*table.Table, error)
Review Comment:
`*rest.Catalog` always satisfies this interface, but it returns `(nil,
rest.ErrEndpointNotSupported)` when the server advertises endpoints without
`GET .../credentials` (pinned by
`TestRefreshTableCredentialsEndpointNotAdvertised`), and `ErrNoSuchTable` on a
404. The round-1 thread on this was resolved on the premise that the type
assertion makes the divergence go away; it does not, because the REST catalog
passes the assertion whether or not the server supports the endpoint. Please
document the error contract here (implementations may fail when the server
cannot vend; callers should keep using the original table in that case) and add
a type-assertion example like `TransactionalCatalog`, `PurgeableTable` and
`Closer` have.
##########
catalog/rest/rest.go:
##########
@@ -1329,6 +1331,33 @@ func (r *Catalog) fetchTableCreds(ctx context.Context,
ident []string, location
return resolveStorageCredentials(ret.StorageCredentials, location), nil
}
+// RefreshTableCredentials updates a *table.Table with newly-vended
credentials from the catalog
+// without updating any other table-internal state that a full Refresh() would.
+// Allows for a quick table credential refresh if the table was created
without any pre-seeded
+// credentials. If the catalog did not vend any credentials, the table is
returned unmodified.
+//
+// Requires that the passed-in table instance be created with the
table.WithSavedConfig() option to
+// save any table-specific configs. All tables created by this catalog pass in
that option.
+func (r *Catalog) RefreshTableCredentials(ctx context.Context, tbl
*table.Table) (*table.Table, error) {
+ metadataLoc := tbl.MetadataLocation()
+ resp, err := r.fetchTableCreds(ctx, tbl.Identifier(), metadataLoc)
+ if err != nil {
+ return nil, err
+ }
+ if len(resp) == 0 {
+ // No new credentials vended. Return as-is.
+ return tbl, nil
+ }
+
+ // Return a new *table.Table with newly-merged credentials coming from
the
+ // fetchTableCreds call.
+ config := maps.Clone(r.props)
+ maps.Copy(config, tbl.SavedConfig())
+ maps.Copy(config, resp)
Review Comment:
f1b12fe merged `tbl.Properties()` here; 2397b04 replaced it with
`tbl.SavedConfig()` instead of adding `SavedConfig()` on top. LoadTable
(rest.go:1839-1841) and `planIOBaseProps` (scan_planning.go:285-295, documented
as mirroring the LoadTable merge order) both layer metadata properties between
`r.props` and the per-table config. A `table.New`-built table, the main use
case here, therefore refreshes into a FileIO without any settings from its
metadata properties unless the caller copies them into `WithSavedConfig`; the
test at refresh_table_credentials_test.go:184-188 is exactly that workaround.
For those tables `tbl.ScanPlanningConfig()` is also nil, so derive the
scan-planning config from this merge (before `resp`) instead of passing it
through.
```suggestion
config := maps.Clone(r.props)
maps.Copy(config, tbl.Properties())
maps.Copy(config, tbl.SavedConfig())
maps.Copy(config, resp)
```
##########
catalog/rest/rest.go:
##########
@@ -1329,6 +1331,33 @@ func (r *Catalog) fetchTableCreds(ctx context.Context,
ident []string, location
return resolveStorageCredentials(ret.StorageCredentials, location), nil
}
+// RefreshTableCredentials updates a *table.Table with newly-vended
credentials from the catalog
+// without updating any other table-internal state that a full Refresh() would.
+// Allows for a quick table credential refresh if the table was created
without any pre-seeded
+// credentials. If the catalog did not vend any credentials, the table is
returned unmodified.
+//
+// Requires that the passed-in table instance be created with the
table.WithSavedConfig() option to
+// save any table-specific configs. All tables created by this catalog pass in
that option.
+func (r *Catalog) RefreshTableCredentials(ctx context.Context, tbl
*table.Table) (*table.Table, error) {
+ metadataLoc := tbl.MetadataLocation()
+ resp, err := r.fetchTableCreds(ctx, tbl.Identifier(), metadataLoc)
+ if err != nil {
+ return nil, err
+ }
+ if len(resp) == 0 {
+ // No new credentials vended. Return as-is.
+ return tbl, nil
+ }
+
+ // Return a new *table.Table with newly-merged credentials coming from
the
+ // fetchTableCreds call.
+ config := maps.Clone(r.props)
+ maps.Copy(config, tbl.SavedConfig())
+ maps.Copy(config, resp)
+
+ return r.tableFromResponse(ctx, tbl.Identifier(), tbl.Metadata(),
metadataLoc, config, tbl.ScanPlanningConfig(), true)
Review Comment:
This goes through `tableFromResponse`, which always applies
`WithMetricsReporter` with the catalog reporter (rest.go:1271, 1304), and that
is `NopReporter` unless the catalog configures one. A table built with
`table.New(..., table.WithMetricsReporter(custom))` silently loses `custom`
here. That contradicts the godoc above (no other table-internal state changes),
the test comment that only the FileIO configuration changes, and `Refresh()`,
which deliberately keeps a caller-set reporter (table/table.go:246-254). Either
carry `tbl.MetricsReporter()` through when `!metrics.IsNop(...)` (for example
an optional reporter override on `tableFromResponse`), or document that the
catalog reporter replaces it.
##########
catalog/rest/refresh_table_credentials_test.go:
##########
@@ -0,0 +1,500 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package rest_test
+
+import (
+ "context"
+ "encoding/json"
+ "maps"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "sync"
+ "sync/atomic"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/catalog/rest"
+ iceio "github.com/apache/iceberg-go/io"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// ioPropsRecorder registers an IO scheme that records the properties handed to
+// every filesystem load. That is how these tests observe whether vended
+// credentials actually reached a table's FileIO, which is otherwise private to
+// the table.
+type ioPropsRecorder struct {
+ mu sync.Mutex
+ loads []map[string]string
+}
+
+func (rec *ioPropsRecorder) register(t *testing.T, scheme string) {
+ t.Helper()
+
+ iceio.Register(scheme, func(_ context.Context, _ *url.URL, props
map[string]string) (iceio.IO, error) {
+ rec.mu.Lock()
+ defer rec.mu.Unlock()
+ rec.loads = append(rec.loads, maps.Clone(props))
+
+ return iceio.NewMemFS(), nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+}
+
+// lastLoad returns the properties the most recent filesystem load was built
+// from, failing the test if no load happened.
+func (rec *ioPropsRecorder) lastLoad(t *testing.T) map[string]string {
+ t.Helper()
+
+ rec.mu.Lock()
+ defer rec.mu.Unlock()
+ require.NotEmpty(t, rec.loads, "no filesystem was loaded")
+
+ return rec.loads[len(rec.loads)-1]
+}
+
+type credsCatalogOpts struct {
+ // endpoints is what /v1/config advertises; nil advertises every
endpoint.
+ endpoints []string
+ // defaults is the /v1/config defaults block, which the catalog folds
into
+ // the properties it later builds table FileIO from.
+ defaults map[string]string
+ // creds handles GET /v1/namespaces/db/tables/tbl/credentials. When
nil, the
+ // endpoint is left unrouted so a request to it would 404.
+ creds http.HandlerFunc
+ // loadTable handles GET /v1/namespaces/db/tables/tbl. When nil, the
+ // endpoint is left unrouted so a request to it would 404.
+ loadTable http.HandlerFunc
+}
+
+func newCredsTestCatalog(t *testing.T, opts credsCatalogOpts) *rest.Catalog {
+ t.Helper()
+
+ mux := http.NewServeMux()
+ mux.HandleFunc("/v1/config", func(w http.ResponseWriter, _
*http.Request) {
+ endpoints := opts.endpoints
+ if endpoints == nil {
+ endpoints = rest.AllEndpointStrings
+ }
+ defaults := opts.defaults
+ if defaults == nil {
+ defaults = map[string]string{}
+ }
+ assert.NoError(t, json.NewEncoder(w).Encode(map[string]any{
+ "defaults": defaults,
+ "overrides": map[string]any{},
+ "endpoints": endpoints,
+ }))
+ })
+ if opts.creds != nil {
+ mux.HandleFunc("/v1/namespaces/db/tables/tbl/credentials",
opts.creds)
+ }
+ if opts.loadTable != nil {
+ mux.HandleFunc("/v1/namespaces/db/tables/tbl", opts.loadTable)
+ }
+
+ srv := httptest.NewServer(mux)
+ t.Cleanup(srv.Close)
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", srv.URL,
rest.WithOAuthToken(TestToken))
+ require.NoError(t, err)
+ t.Cleanup(func() { assert.NoError(t, cat.Close()) })
+
+ return cat
+}
+
+// newExternalTable builds a table the way a caller outside the catalog would:
+// straight through table.New, with a plain property-based FileIO and no vended
+// credentials seeded into it. Any opts are passed through to table.New.
+func newExternalTable(t *testing.T, cat *rest.Catalog, scheme string, opts
...table.Option) (*table.Table, string) {
+ t.Helper()
+
+ // The shared fixture is written against s3://; point it at the
recording
+ // scheme so loading its FileIO stays offline and observable.
+ meta, err := table.ParseMetadataString(
+ strings.ReplaceAll(exampleTableMetadataNoSnapshotV1, "s3://",
scheme+"://"))
+ require.NoError(t, err)
+
+ metadataLoc := scheme +
"://warehouse/database/table/metadata/00000-a.metadata.json"
+
+ return table.New(
+ catalog.ToIdentifier("db", "tbl"),
+ meta,
+ metadataLoc,
+ iceio.LoadFSFunc(nil, metadataLoc),
+ cat,
+ opts...,
+ ), metadataLoc
+}
+
+func storageCredentialsBody(prefix string, config map[string]string)
map[string]any {
+ return map[string]any{
+ "storage-credentials": []any{
+ map[string]any{"prefix": prefix, "config": config},
+ },
+ }
+}
+
+// TestRefreshTableCredentialsSeedsExternallyCreatedTable covers the case a
+// caller cannot reach through LoadTable: a table handed to table.New directly,
+// whose FileIO was never seeded with vended credentials, is brought up to date
+// without a metadata reload.
+func TestRefreshTableCredentialsSeedsExternallyCreatedTable(t *testing.T) {
+ const scheme = "restcreds-seed"
+
+ rec := &ioPropsRecorder{}
+ rec.register(t, scheme)
+
+ var credsCalls atomic.Int32
+ cat := newCredsTestCatalog(t, credsCatalogOpts{
+ defaults: map[string]string{"s3.region": "us-west-2"},
+ creds: func(w http.ResponseWriter, req *http.Request) {
+ credsCalls.Add(1)
+ assert.Equal(t, http.MethodGet, req.Method)
+ assert.NoError(t,
json.NewEncoder(w).Encode(storageCredentialsBody(
+ scheme+"://warehouse/database/table",
+ map[string]string{
+ "s3.access-key-id": "vended-key",
+ "s3.secret-access-key": "vended-secret",
+ "s3.session-token": "vended-token",
+ },
+ )))
+ },
+ })
+
+ // Save config the way the catalog does for tables it loads: the table's
+ // metadata properties, which the fixture sets this codec in.
+ tbl, metadataLoc := newExternalTable(t, cat, scheme,
table.WithSavedConfig(iceberg.Properties{
+ "write.parquet.compression-codec": "zstd",
+ }))
+
+ // The premise: as built, the table's FileIO carries no credentials.
+ _, err := tbl.FS(context.Background())
+ require.NoError(t, err)
+ assert.NotContains(t, rec.lastLoad(t), "s3.access-key-id")
+
+ refreshed, err := cat.RefreshTableCredentials(context.Background(), tbl)
+ require.NoError(t, err)
+ require.NotNil(t, refreshed)
+ assert.Equal(t, int32(1), credsCalls.Load())
+
+ // Only the FileIO configuration changes: identity, metadata and the
metadata
+ // location a later commit targets all carry over untouched.
+ assert.Equal(t, catalog.ToIdentifier("db", "tbl"),
refreshed.Identifier())
+ assert.Equal(t, metadataLoc, refreshed.MetadataLocation())
+ assert.Equal(t, tbl.Metadata(), refreshed.Metadata())
+ assert.Equal(t, tbl.Location(), refreshed.Location())
+
+ _, err = refreshed.FS(context.Background())
+ require.NoError(t, err)
+
+ props := rec.lastLoad(t)
+ assert.Equal(t, "vended-key", props["s3.access-key-id"])
+ assert.Equal(t, "vended-secret", props["s3.secret-access-key"])
+ assert.Equal(t, "vended-token", props["s3.session-token"])
+ // The catalog's own config and the table's saved config survive the
merge
+ // rather than being replaced by the credentials alone.
+ assert.Equal(t, "us-west-2", props["s3.region"])
+ assert.Equal(t, "zstd", props["write.parquet.compression-codec"])
+
+ // The table the caller passed in is left alone.
+ _, err = tbl.FS(context.Background())
+ require.NoError(t, err)
+ assert.NotContains(t, rec.lastLoad(t), "s3.access-key-id")
+}
+
+// TestRefreshTableCredentialsPreservesSavedConfig checks that config a caller
+// saved on an externally created table via table.WithSavedConfig, such as a
+// region or client factory, carries over to the table RefreshTableCredentials
+// returns and reaches its FileIO alongside the vended credentials.
+func TestRefreshTableCredentialsPreservesSavedConfig(t *testing.T) {
+ const scheme = "restcreds-saved"
+
+ rec := &ioPropsRecorder{}
+ rec.register(t, scheme)
+
+ cat := newCredsTestCatalog(t, credsCatalogOpts{
+ // The saved config must win over the catalog's defaults.
+ defaults: map[string]string{iceio.S3Region: "us-west-2"},
+ creds: func(w http.ResponseWriter, _ *http.Request) {
+ assert.NoError(t,
json.NewEncoder(w).Encode(storageCredentialsBody(
+ scheme+"://warehouse/database/table",
+ map[string]string{
+ iceio.S3AccessKeyID: "vended-key",
+ iceio.S3SecretAccessKey:
"vended-secret",
+ iceio.S3SessionToken: "vended-token",
+ },
+ )))
+ },
+ })
+
+ savedConfig := map[string]string{
+ iceio.S3Region: "eu-central-1",
+ iceio.S3ClientRegion: "eu-central-1",
+ "client.factory": "com.example.CustomClientFactory",
+ iceio.S3EndpointURL: "https://s3.example.com";,
+ }
+ tbl, _ := newExternalTable(t, cat, scheme,
table.WithSavedConfig(savedConfig))
+ require.Equal(t, iceberg.Properties(savedConfig), tbl.SavedConfig())
+
+ refreshed, err := cat.RefreshTableCredentials(context.Background(), tbl)
+ require.NoError(t, err)
+ require.NotNil(t, refreshed)
+
+ // The saved config survives on the clone, now merged with the vended
+ // credentials so a further refresh starts from the full picture.
+ refreshedConfig := refreshed.SavedConfig()
+ for k, v := range savedConfig {
+ assert.Equal(t, v, refreshedConfig[k], "saved config key %q", k)
+ }
+ assert.Equal(t, "vended-key", refreshedConfig[iceio.S3AccessKeyID])
+
+ _, err = refreshed.FS(context.Background())
+ require.NoError(t, err)
+
+ props := rec.lastLoad(t)
+ for k, v := range savedConfig {
+ assert.Equal(t, v, props[k], "FileIO property %q", k)
+ }
+ assert.Equal(t, "vended-key", props[iceio.S3AccessKeyID])
+ assert.Equal(t, "vended-secret", props[iceio.S3SecretAccessKey])
+ assert.Equal(t, "vended-token", props[iceio.S3SessionToken])
+
+ // The caller's table and the map it saved are left untouched.
+ assert.Equal(t, iceberg.Properties(savedConfig), tbl.SavedConfig())
+ assert.NotContains(t, savedConfig, iceio.S3AccessKeyID)
+}
+
+// TestRefreshTableCredentialsAfterLoadTable covers a table loaded through
+// LoadTable: the per-table config block of the load response is in neither the
+// catalog's props nor the table's metadata properties, so only the table's
+// saved config can carry it through a credential refresh.
+func TestRefreshTableCredentialsAfterLoadTable(t *testing.T) {
+ const scheme = "restcreds-loaded"
+
+ rec := &ioPropsRecorder{}
+ rec.register(t, scheme)
+
+ metadataLoc := scheme +
"://warehouse/database/table/metadata/00000-a.metadata.json"
+ tableConfig := map[string]string{
+ // Overrides the catalog default below.
+ iceio.S3Region: "eu-central-1",
+ "client.factory": "com.example.CustomClientFactory",
+ }
+
+ cat := newCredsTestCatalog(t, credsCatalogOpts{
+ defaults: map[string]string{iceio.S3Region: "us-west-2"},
+ loadTable: func(w http.ResponseWriter, req *http.Request) {
+ assert.Equal(t, http.MethodGet, req.Method)
+ assert.NoError(t,
json.NewEncoder(w).Encode(map[string]any{
+ "metadata-location": metadataLoc,
+ "metadata": json.RawMessage(strings.ReplaceAll(
+ exampleTableMetadataNoSnapshotV1,
"s3://", scheme+"://")),
+ "config": tableConfig,
+ }))
+ },
+ creds: func(w http.ResponseWriter, _ *http.Request) {
+ assert.NoError(t,
json.NewEncoder(w).Encode(storageCredentialsBody(
+ scheme+"://warehouse/database/table",
+ map[string]string{
+ iceio.S3AccessKeyID: "vended-key",
+ iceio.S3SecretAccessKey:
"vended-secret",
+ iceio.S3SessionToken: "vended-token",
+ },
+ )))
+ },
+ })
+
+ tbl, err := cat.LoadTable(context.Background(),
catalog.ToIdentifier("db", "tbl"))
+ require.NoError(t, err)
+
+ // The premise: the loaded table's FileIO carries the per-table config
but no
+ // credentials, since the load response vended none.
+ _, err = tbl.FS(context.Background())
+ require.NoError(t, err)
+ props := rec.lastLoad(t)
+ for k, v := range tableConfig {
+ require.Equal(t, v, props[k], "loaded FileIO property %q", k)
+ }
+ require.NotContains(t, props, iceio.S3AccessKeyID)
+
+ refreshed, err := cat.RefreshTableCredentials(context.Background(), tbl)
+ require.NoError(t, err)
+ require.NotNil(t, refreshed)
+ assert.Equal(t, tbl.Identifier(), refreshed.Identifier())
+ assert.Equal(t, metadataLoc, refreshed.MetadataLocation())
+ assert.Equal(t, tbl.Metadata(), refreshed.Metadata())
+
+ _, err = refreshed.FS(context.Background())
+ require.NoError(t, err)
+
+ props = rec.lastLoad(t)
+ for k, v := range tableConfig {
+ assert.Equal(t, v, props[k], "refreshed FileIO property %q", k)
+ }
+ assert.Equal(t, "zstd", props["write.parquet.compression-codec"],
+ "metadata properties merged at load time should survive too")
+ assert.Equal(t, "vended-key", props[iceio.S3AccessKeyID])
+ assert.Equal(t, "vended-secret", props[iceio.S3SecretAccessKey])
+ assert.Equal(t, "vended-token", props[iceio.S3SessionToken])
+}
+
+// TestRefreshTableCredentialsSeededTableRenewsOnExpiry checks the refreshed
+// table's FileIO keeps the refresher wiring, so credentials that expire are
+// re-fetched instead of being pinned at their first value.
+func TestRefreshTableCredentialsSeededTableRenewsOnExpiry(t *testing.T) {
+ const scheme = "restcreds-renew"
+
+ rec := &ioPropsRecorder{}
+ rec.register(t, scheme)
+
+ var credsCalls atomic.Int32
+ cat := newCredsTestCatalog(t, credsCatalogOpts{
+ creds: func(w http.ResponseWriter, _ *http.Request) {
+ n := credsCalls.Add(1)
+ assert.NoError(t,
json.NewEncoder(w).Encode(storageCredentialsBody(
+ scheme+"://warehouse/database/table",
+ map[string]string{
+ "s3.access-key-id": "vended-key",
+ // Already past its expiry, so the very
next FileIO load must
+ // go back to the catalog rather than
reuse it.
+ "s3.session-token-expires-at-ms": "1",
+ "s3.session-token":
"token-" + strings.Repeat("x", int(n)),
+ },
+ )))
+ },
+ })
+
+ tbl, _ := newExternalTable(t, cat, scheme)
+
+ refreshed, err := cat.RefreshTableCredentials(context.Background(), tbl)
+ require.NoError(t, err)
+ require.Equal(t, int32(1), credsCalls.Load())
+
+ _, err = refreshed.FS(context.Background())
+ require.NoError(t, err)
+ assert.Equal(t, "token-x", rec.lastLoad(t)["s3.session-token"])
+
+ _, err = refreshed.FS(context.Background())
+ require.NoError(t, err)
+ assert.Equal(t, int32(2), credsCalls.Load(),
+ "expired credentials should be renewed through the catalog")
+ assert.Equal(t, "token-xx", rec.lastLoad(t)["s3.session-token"])
+}
+
+func TestRefreshTableCredentialsNoCredentialsVended(t *testing.T) {
+ const scheme = "restcreds-empty"
+
+ rec := &ioPropsRecorder{}
+ rec.register(t, scheme)
+
+ cat := newCredsTestCatalog(t, credsCatalogOpts{
+ creds: func(w http.ResponseWriter, _ *http.Request) {
+ assert.NoError(t,
json.NewEncoder(w).Encode(map[string]any{
+ "storage-credentials": []any{},
+ }))
+ },
+ })
+
+ tbl, _ := newExternalTable(t, cat, scheme)
+
+ refreshed, err := cat.RefreshTableCredentials(context.Background(), tbl)
+ require.NoError(t, err)
+ assert.Equal(t, tbl.Identifier(), refreshed.Identifier())
+ assert.Equal(t, tbl.Metadata(), refreshed.Metadata())
+ assert.Equal(t, tbl.MetadataLocation(), refreshed.MetadataLocation())
+ assert.Equal(t, tbl.SavedConfig(), refreshed.SavedConfig(),
+ "a server that vends nothing should leave the table as-is")
Review Comment:
On this path `refreshed` is the same pointer as `tbl`, so these four
assertions compare an object with itself and cannot fail; same at 451-455 in
`TestRefreshTableCredentialsPrefixMismatch`. Now that returning the input table
is the documented contract, assert it directly with `assert.Same(t, tbl,
refreshed)` in both tests. @laskoviymishka this reverses your round-1 ask to
drop `assert.Same`, which was right while the aliasing was an implementation
detail; it matches your round-2 ask and closes the open thread on rest.go:1349.
Smaller: the message at 455 is copy-pasted (that server does vend a
credential, just for another prefix); `ioPropsRecorder` is registered but never
read in this test, PrefixMismatch, EndpointNotAdvertised and NotFound (407-408,
435-436, 461-462, 479-480); and SeedsExternallyCreatedTable and
PreservesSavedConfig mostly exercise the same path, so one of them can go.
--
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]
