fix(aws): reject incomplete static credential configuration

[fallintoplace]

What changed

Reject incomplete static AWS credential properties in Glue and S3 configuration. Access key and secret key must now be provided together, and a session token cannot be configured without that pair.

Why

Providing any single credential field previously replaced the default AWS provider chain with invalid static credentials containing blank fields. This delayed a configuration error until an AWS request and could obscure the actual typo.

Complete key pairs, with or without a session token, continue to use the static provider.

Testing

• go test ./catalog/glue -run TestLoadAWSConfigRejectsIncompleteStaticCredentials
• go test ./io/gocloud -run 'TestParseAWSConfigRejectsIncompleteStaticCredentials|TestResolveS3AWSConfigCredentialPrecedence'
• go vet ./catalog/glue ./io/gocloud

[laskoviymishka]

This is a clean, focused fix and the core logic is right. I walked the key/secret/token combinations and hasKey != hasSecret || (token != "" && !hasKey) rejects every partial combination without touching the valid ones (empty falls through to the default chain, key+secret installs the static provider, all three installs it). The variable cleanup in the S3 static provider is a nice incidental tidy too.

I'd hold before merging on two things, both about completeness rather than the condition itself.

First, the fix only covers one of the two paths through the S3 config. resolveS3AWSConfig calls ParseAWSConfig only when there's no ambient context config; when one is present it takes the override block that installs static creds only if key and secret are both set, and silently drops a stray s3.session-token. So the same partial props hard-error on one path and pass silently on the other depending on whether a context config happens to be present, and the "falls through" comment just above that block now contradicts what ParseAWSConfig does. Left an inline with the two options.

Second, this is a deliberate behavioral divergence worth confirming out loud. Java's AwsClientFactories and PyIceberg both fall through to the default provider chain on partial creds rather than rejecting at config time, so a table whose catalog config carries only s3.access-key-id (secret from an instance profile) reads fine under Spark/PyIceberg but will hard-error here. I think erroring is the safer default, but since ParseAWSConfig is exported and this is a behavioral break, if we keep it I'd want a godoc note calling out that both keys must be present together. Fine either way, just want it to be a conscious choice.

A couple of concrete things before merge:

• handle the context-config override path in resolveS3AWSConfig the same way (or document the intentional asymmetry and fix the stale comment), and
• fill the happy-path test gaps: empty props falling through to the default chain, and key+secret with no token.

The rest (a shared helper for the duplicated check, t.Run subtests, a sentinel error) are inline nits. Once the two above are settled, happy to take another pass.

[laskoviymishka]

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?

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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?

[laskoviymishka]

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.

[laskoviymishka]

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.