laskoviymishka commented on code in PR #1999:
URL: https://github.com/apache/iceberg-go/pull/1999#discussion_r3989305046
##########
catalog/rest/rest.go:
##########
@@ -1126,6 +1137,22 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
return cl, cleanup, nil
}
+// staticCredsFromProps returns a static credentials provider built from the S3
+// access-key properties. It returns (nil, nil) when neither key is set, so the
+// caller falls back to the default credential chain, and an error when only
one
+// of the pair is set rather than silently signing as a different identity.
+func staticCredsFromProps(props iceberg.Properties) (aws.CredentialsProvider,
error) {
Review Comment:
I'd delegate this to `internal/awsconfig.ValidateStaticCredentials` rather
than re-implementing the check here.
Two things it fixes: a lone `s3.session-token` with no access/secret
currently lands in the empty-pair branch and returns `(nil, nil)`, so it
silently signs with the ambient identity instead of erroring the way the shared
validator does. And the partial-pair error is a bare `fmt.Errorf` with no `%w`,
so callers can't `errors.Is` it against `ErrIncompleteStaticCredentials` like
the `io/gocloud/s3` and `catalog/glue` call sites can.
Delegating closes the token-only gap and gives us one error contract across
the codebase. wdyt?
##########
catalog/rest/options.go:
##########
@@ -88,6 +88,10 @@ 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
Review Comment:
Not blocking, but worth flagging: `s3.*` diverges from the Java client,
which reads `rest.access-key-id` / `rest.secret-access-key` for REST SigV4
signing.
You've documented `s3.*` as intentional so I'm not asking to change it, but
an operator coming from Java who sets `rest.*` will silently fall through to
the default credential chain with no signal. A line here noting we don't
recognize the `rest.*` names (or accepting them as an alias later) would save
someone a confusing debugging session. wdyt?
##########
catalog/rest/rest_internal_test.go:
##########
@@ -52,6 +53,63 @@ import (
"golang.org/x/sync/errgroup"
)
+func TestStaticCredsFromProps(t *testing.T) {
+ creds, err := staticCredsFromProps(iceberg.Properties{
+ iceio.S3AccessKeyID: "AK",
+ iceio.S3SecretAccessKey: "SK",
+ iceio.S3SessionToken: "ST",
+ })
+ require.NoError(t, err)
+ require.NotNil(t, creds)
+ got, err := creds.Retrieve(context.Background())
+ require.NoError(t, err)
+ require.Equal(t, "AK", got.AccessKeyID)
+ require.Equal(t, "SK", got.SecretAccessKey)
+ require.Equal(t, "ST", got.SessionToken)
+
+ creds, err = staticCredsFromProps(iceberg.Properties{})
+ require.NoError(t, err, "no creds must fall back to the default chain")
+ require.Nil(t, creds)
+
+ _, err = staticCredsFromProps(iceberg.Properties{iceio.S3AccessKeyID:
"AK"})
+ require.Error(t, err, "a lone access key must be an error, not the
ambient identity")
+
+ _, err =
staticCredsFromProps(iceberg.Properties{iceio.S3SecretAccessKey: "SK"})
Review Comment:
Once the token-only behavior above is settled, worth adding a
`{iceio.S3SessionToken: "ST"}` case here to pin it. Right now the covered cases
are the full triple, empty, lone-access, and lone-secret.
While we're here: this reads a bit against the file's table-driven
convention and is missing `t.Parallel()`, and
`TestSigV4SignsWithPropsCredentials` drops the `Do` response without closing
the body (bodyclose will flag it).
--
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]
