laskoviymishka commented on code in PR #1574:
URL: https://github.com/apache/iceberg-go/pull/1574#discussion_r3690095025
##########
catalog/glue/glue.go:
##########
@@ -123,7 +123,11 @@ func toAwsConfig(ctx context.Context, p
iceberg.Properties) (aws.Config, error)
}
key, secret, token := p[AccessKeyID], p[SecretAccessKey],
p[SessionToken]
- if key != "" || secret != "" || token != "" {
+ hasKey, hasSecret := key != "", secret != ""
+ if hasKey != hasSecret || (token != "" && !hasKey) {
+ return aws.Config{}, errors.New("glue.access-key-id and
glue.secret-access-key must be configured together")
Review Comment:
minor: this same message fires for the token-only case, where it reads a
little oddly, the user set `session-token` and gets told about
`access-key-id`/`secret-access-key`. A distinct message for
token-without-a-keypair would point at the actual offending field.
##########
io/gocloud/s3.go:
##########
@@ -68,9 +68,13 @@ func ParseAWSConfig(ctx context.Context, props
map[string]string) (*aws.Config,
accessKey, secretAccessKey := props[io.S3AccessKeyID],
props[io.S3SecretAccessKey]
token := props[io.S3SessionToken]
- if accessKey != "" || secretAccessKey != "" || token != "" {
+ hasAccessKey, hasSecretAccessKey := accessKey != "", secretAccessKey !=
""
+ if hasAccessKey != hasSecretAccessKey || (token != "" && !hasAccessKey)
{
+ return nil, errors.New("s3.access-key-id and
s3.secret-access-key must be configured together")
Review Comment:
I think this only guards one of the two paths into the S3 config, and the
other one behaves differently for the same input.
`resolveS3AWSConfig` calls `ParseAWSConfig` only when there's no ambient
context config. When a context config is present it takes the override block
lower down (around 198-205), which installs static creds only when both key and
secret are set and silently drops a lone `s3.session-token`. So identical
partial props now hard-error on the no-context path but pass silently on the
context path, the outcome depends on whether a context config happens to exist,
which is invisible to the caller. The `// A partial set ... falls through`
comment right above that block now also contradicts what `ParseAWSConfig` does.
I'd either apply the same check at the override block and make that the
single enforcement point where creds actually get installed, or keep the
asymmetry deliberately and update that comment to explain it. wdyt?
##########
catalog/glue/glue_test.go:
##########
@@ -50,6 +50,33 @@ import (
var errGluePurgeRemove = errors.New("glue purge remove failed")
+func TestLoadAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) {
+ t.Parallel()
+
+ tests := []iceberg.Properties{
+ {AccessKeyID: "access"},
+ {SecretAccessKey: "secret"},
+ {SessionToken: "token"},
+ {AccessKeyID: "access", SessionToken: "token"},
+ {SecretAccessKey: "secret", SessionToken: "token"},
+ }
+
+ for _, props := range tests {
Review Comment:
the loop asserts five cases without a `t.Run`, so a failure points at this
one line with no way to tell which combination broke. Wrapping each case in a
named `t.Run` (with `t.Parallel()` moved inside) would localize it, the other
table tests in this file already do that. Same applies to
`TestParseAWSConfigRejectsIncompleteStaticCredentials` in the s3 test.
##########
catalog/glue/glue.go:
##########
@@ -123,7 +123,11 @@ func toAwsConfig(ctx context.Context, p
iceberg.Properties) (aws.Config, error)
}
key, secret, token := p[AccessKeyID], p[SecretAccessKey],
p[SessionToken]
- if key != "" || secret != "" || token != "" {
+ hasKey, hasSecret := key != "", secret != ""
+ if hasKey != hasSecret || (token != "" && !hasKey) {
Review Comment:
this exact condition shows up verbatim in `ParseAWSConfig` too, just with
`hasAccessKey`/`hasSecretAccessKey` instead of `hasKey`/`hasSecret`. Two call
sites isn't much, but the expression is dense enough that the copies could
drift, and neither has a comment explaining why a token requires both keys.
Could we pull it into a small `validateStaticAWSCredentials(key, secret,
token) error` shared by both builders? That unifies the naming and gives us one
place for the explanatory comment. Thoughts?
##########
io/gocloud/s3_test.go:
##########
@@ -114,6 +114,33 @@ func TestResolveS3AWSConfigCredentialPrecedence(t
*testing.T) {
})
}
+func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) {
+ t.Parallel()
+
+ tests := []map[string]string{
+ {io.S3AccessKeyID: "access"},
+ {io.S3SecretAccessKey: "secret"},
+ {io.S3SessionToken: "token"},
+ {io.S3AccessKeyID: "access", io.S3SessionToken: "token"},
+ {io.S3SecretAccessKey: "secret", io.S3SessionToken: "token"},
+ }
+
+ for _, props := range tests {
+ _, err := ParseAWSConfig(context.Background(), props)
+ require.ErrorContains(t, err, "s3.access-key-id and
s3.secret-access-key must be configured together")
Review Comment:
these assert the error by literal string against an inline `errors.New`, so
renaming a property-key constant would invalidate the assertion while still
compiling, and callers can't distinguish this error programmatically. The
package leans on `errors.Is` sentinels elsewhere (`ErrNoSuchTable` and
friends), a package-level `ErrIncompleteStaticCredentials` used here and
asserted with `errors.Is` would be sturdier. Not blocking.
##########
io/gocloud/s3_test.go:
##########
@@ -114,6 +114,33 @@ func TestResolveS3AWSConfigCredentialPrecedence(t
*testing.T) {
})
}
+func TestParseAWSConfigRejectsIncompleteStaticCredentials(t *testing.T) {
+ t.Parallel()
+
+ tests := []map[string]string{
Review Comment:
the only happy path covered is all-three-fields. The two most common
production cases aren't asserted: empty props falling through to the default
chain (IAM role / env creds), and key+secret with no token. Without the
empty-props case, an over-eager future edit to the condition (say `==` instead
of `!=`) would reject every default-chain user and this suite wouldn't catch
it. Worth adding both, with the empty case asserting no error and that
`cfg.Credentials` isn't a static provider.
--
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]
