zeroshade commented on code in PR #1999:
URL: https://github.com/apache/iceberg-go/pull/1999#discussion_r4158451147
##########
catalog/rest/options.go:
##########
@@ -88,6 +88,14 @@ func WithMetadataLocation(loc string) Option {
}
}
+// WithSigV4 enables AWS SigV4 request signing for the REST catalog. The
signing
+// identity is resolved in order: an explicit WithAwsConfig, then the s3.*
catalog
+// credential properties (s3.access-key-id / s3.secret-access-key /
s3.session-token),
+// then the AWS default credential chain.
+//
+// The Java-client property names (rest.access-key-id / rest.secret-access-key
/
+// rest.session-token) are accepted as aliases, resolved per field with the
s3.*
+// keys taking precedence when both are set.
Review Comment:
This is stale after d401049: resolution is no longer per field.
`staticCredsFromProps` uses the `s3.*` tuple if any `s3.*` key is set (and
errors if that tuple is incomplete), otherwise the `rest.*` tuple. Fields never
mix across namespaces. The same wording is in the const comment at
rest.go:97-99 and in configuration.md:59.
Suggested text: "If any s3.* credential key is set, only the s3.* tuple is
used and it must be complete; otherwise the rest.* tuple is used."
##########
website/src/configuration.md:
##########
@@ -56,7 +56,7 @@ catalog:
| `catalog.<name>.aws-profile` | AWS named profile for the Glue catalog. When
unset, the AWS SDK default credential chain is used. |
| `catalog.<name>.sql-driver` | `database/sql` driver name for the SQL
catalog. Maps to the `sql.driver` property. The default CLI binary only
compiles in `sqliteshim`; other drivers require a custom build. |
| `catalog.<name>.sql-dialect` | SQL dialect for the SQL catalog (`postgres`,
`mysql`, `sqlite`, `mssql`, `oracle`). Maps to the `sql.dialect` property. The
default CLI binary only ships `sqlite` via `sqliteshim`; other dialects need a
custom build with their drivers. |
-| `catalog.<name>.rest.sigv4-enabled` | Enable AWS SigV4 signing for REST. |
+| `catalog.<name>.rest.sigv4-enabled` | Enable AWS SigV4 signing for REST.
When enabled, requests are signed with the `s3.*` credential properties if set
(`s3.access-key-id` / `s3.secret-access-key` / `s3.session-token`), otherwise
with the AWS default credential chain. The Java-client names
(`rest.access-key-id` / `rest.secret-access-key` / `rest.session-token`) are
accepted as aliases, resolved per field with the `s3.*` keys taking precedence.
|
Review Comment:
Apart from the stale "resolved per field" wording (see options.go), this row
is in the CLI YAML table, and the CLI config cannot set `s3.*` or `rest.*`
credentials. `config.RestOptions` only has sigv4-enabled / signing-name /
signing-region, and cmd/iceberg/main.go:392-398 only maps those. I'd keep this
row as "Enable AWS SigV4 signing for REST." and move the credential-resolution
text to the REST catalog options section (the AWS SigV4 row at line 74), where
`WithAdditionalProps` can actually supply these keys.
##########
catalog/rest/rest.go:
##########
@@ -1106,26 +1139,64 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
if opts.enableSigv4 {
cfg := opts.awsConfig
if !opts.awsConfigSet {
+ creds, err := staticCredsFromProps(opts.additionalProps)
+ if err != nil {
+ cleanup()
+
+ return nil, nil, err
+ }
// If no config provided, load defaults from
environment.
- var err error
cfg, err = config.LoadDefaultConfig(ctx)
if err != nil {
cleanup()
return nil, nil, err
}
+ // Sign with the S3 credentials carried in the catalog
properties when
+ // present, rather than only the AWS default credential
chain.
+ if creds != nil {
+ cfg.Credentials = creds
+ }
}
if opts.sigv4Region != "" {
cfg.Region = opts.sigv4Region
}
session.cfg, session.service = cfg, opts.sigv4Service
session.signer, session.newHash = v4.NewSigner(), sha256.New
+ session.signingOrigin = r.baseURI
}
return cl, cleanup, nil
}
+// staticCredsFromProps returns a static credentials provider built from the
+// signing-credential properties. It prefers the s3.* keys and falls back to
the
+// Java-compatible rest.* aliases, resolving the tuple atomically from a single
+// namespace so a partial pair is never completed with fields from the other
one.
+// It returns (nil, nil) when neither namespace sets any credential property,
so
+// the caller falls back to the default credential chain, and an
+// ErrIncompleteStaticCredentials error when the chosen namespace is
incomplete.
+func staticCredsFromProps(props iceberg.Properties) (aws.CredentialsProvider,
error) {
+ namespaces := [][3]string{
+ {iceio.S3AccessKeyID, iceio.S3SecretAccessKey,
iceio.S3SessionToken},
+ {keyRestAccessKeyID, keyRestSecretAccessKey,
keyRestSessionToken},
+ }
Review Comment:
Non-blocking, but worth settling before this ships because it is
user-visible. With both namespaces set, `s3.*` signs and `rest.*` is ignored
(pinned by the "s3.* keys take precedence over rest.* aliases" case in the
test).
Java's `AwsProperties.restCredentialsProvider()` signs only with
`rest.access-key-id` / `rest.secret-access-key` / `rest.session-token`, then
`client.credentials-provider`, then the default chain. It never reads `s3.*`.
pyiceberg signs with `client.access-key-id` etc. So if someone sets `s3.*` for
FileIO (MinIO, or a scoped data principal) and `rest.*` for the catalog,
catalog requests get signed with the data credentials. The same happens if a
server returns `s3.*` in `/v1/config` defaults/overrides, since those end up in
`additionalProps`.
I'd flip this to `rest.*` > `s3.*` > default chain: swap these two entries
and invert the precedence assertion in `TestStaticCredsFromProps`. This builds
on @laskoviymishka's Java-parity note. Fine here if you prefer, otherwise a
follow-up.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -52,6 +54,159 @@ import (
"golang.org/x/sync/errgroup"
)
+func TestStaticCredsFromProps(t *testing.T) {
Review Comment:
Optional, as @laskoviymishka noted earlier: this reads better as a
table-driven test with `t.Parallel()`, which matches the rest of the file. Not
blocking.
--
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]
