laskoviymishka commented on code in PR #1584:
URL: https://github.com/apache/iceberg-go/pull/1584#discussion_r3681109608
##########
io/gocloud/gcs.go:
##########
@@ -131,7 +131,10 @@ func gcsCredentials(ctx context.Context, props
map[string]string) (*google.Crede
return parse(data, fmt.Sprintf("%s %q", io.GCSKeyPath, path))
}
- creds, _ := gcp.DefaultCredentials(ctx)
+ creds, err := gcp.DefaultCredentials(ctx)
Review Comment:
agreed that discarding the ADC error here was a real bug — silently
returning nil creds and falling through to anonymous access hides genuine auth
failures.
the thing I'd want sorted before merge is that this also flips behavior for
anyone who was relying on that silent fallback: public buckets, the fake-gcs
emulator, or any non-GCP env without ADC used to quietly get an anonymous
client, and now they hard-fail the moment they open a `gs://` path. Java defers
ADC lazily via `StorageOptions` and never fails eagerly at FileIO init, so we'd
be diverging from the Java client here.
two ways I could see going — preserve the old `nil, nil` → anonymous
semantics when `DefaultCredentials` fails, or keep the eager error but make it
self-documenting so the migration path is obvious:
```go
return nil, fmt.Errorf("gcs: no credentials found; set %s=true for
anonymous/public bucket access: %w", io.GCSNoAuth, err)
```
I lean toward the second since failing loud on a real auth error is the
better default, but either way I'd call this out as a breaking change in the PR
description. wdyt?
##########
io/gocloud/gcs.go:
##########
@@ -131,7 +131,10 @@ func gcsCredentials(ctx context.Context, props
map[string]string) (*google.Crede
return parse(data, fmt.Sprintf("%s %q", io.GCSKeyPath, path))
}
- creds, _ := gcp.DefaultCredentials(ctx)
+ creds, err := gcp.DefaultCredentials(ctx)
+ if err != nil {
Review Comment:
small defensive one while we're here: if `DefaultCredentials` ever returns
`(nil, nil)`, we fall straight back to the anonymous path — the exact silent
fallback this PR is removing. gocloud's `gcp` doesn't do that today, but the
contract isn't enforced at the call site. a `if creds == nil` guard before the
return would close it off.
##########
io/gocloud/gcs_integration_test.go:
##########
@@ -115,6 +115,7 @@ func (s *GCSIOTestSuite) TestGCSWarehouse() {
"type": "sql",
"warehouse": fmt.Sprintf("gs://%s/iceberg/",
gcsBucketName),
io.GCSEndpoint: fmt.Sprintf("http://%s/";, gcsEndpoint),
+ io.GCSNoAuth: "true",
Review Comment:
this is the right fix for the emulator — fake-gcs-server serves no real ADC,
so without no-auth the suite would now hard-fail in CI. worth a one-line
comment saying exactly that, though; otherwise it reads as an unexplained flag
and someone may drop it later thinking it's redundant.
##########
io/gocloud/gcs_test.go:
##########
@@ -142,6 +142,19 @@ func TestGCSCredentialsNoAuth(t *testing.T) {
assert.Nil(t, creds)
}
+func TestGCSCredentialsPropagatesADCError(t *testing.T) {
+ t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(t.TempDir(),
"missing.json"))
Review Comment:
good case to have. it forces ADC to fail at the env-var step, but it doesn't
cover the more common dev failure — no env var set and no creds anywhere (fresh
machine, container with nothing mounted), where ADC falls through the
well-known-file and metadata-server lookups before failing. a sub-case with
`t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "")` would exercise that path too.
##########
io/gocloud/gcs_test.go:
##########
@@ -142,6 +142,19 @@ func TestGCSCredentialsNoAuth(t *testing.T) {
assert.Nil(t, creds)
}
+func TestGCSCredentialsPropagatesADCError(t *testing.T) {
+ t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(t.TempDir(),
"missing.json"))
+
+ creds, err := gcsCredentials(context.Background(), nil)
+ require.Error(t, err)
Review Comment:
`require.ErrorContains` already implies the error is non-nil, so the
`require.Error` above it is redundant — I'd drop 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]
