tanmayrauth commented on code in PR #1503:
URL: https://github.com/apache/iceberg-go/pull/1503#discussion_r3626361425
##########
io/gocloud/gcs.go:
##########
@@ -67,15 +70,58 @@ func ParseGCSConfig(props map[string]string)
*gcsblob.Options {
}
}
+// gcsScope grants read/write access to GCS objects.
+const gcsScope = "https://www.googleapis.com/auth/devstorage.read_write";
+
+// gcsCredentials builds credentials from the gcs.jsonkey / gcs.keypath
+// properties (defaulting to a service-account key, honouring gcs.credtype),
+// falling back to Application Default Credentials when neither is set.
+func gcsCredentials(ctx context.Context, props map[string]string)
(*google.Credentials, error) {
+ credType := google.ServiceAccount
+ if v := props[io.GCSCredType]; v != "" {
+ credType = google.CredentialsType(v)
Review Comment:
This casts gcs.credtype raw with no validation, while ParseGCSConfig
validates it against allowedGCSCredTypes and silently drops unknown values —
two parsers for one property, over two different type spaces
(option.CredentialsType vs google.CredentialsType). A typo'd credtype here
surfaces as the confusing "expected credential type X, found Y" instead of
being ignored. Normalize/validate gcs.credtype once and share it between both
paths.
##########
io/gocloud/gcs.go:
##########
@@ -67,15 +70,58 @@ func ParseGCSConfig(props map[string]string)
*gcsblob.Options {
}
}
+// gcsScope grants read/write access to GCS objects.
+const gcsScope = "https://www.googleapis.com/auth/devstorage.read_write";
+
+// gcsCredentials builds credentials from the gcs.jsonkey / gcs.keypath
+// properties (defaulting to a service-account key, honouring gcs.credtype),
+// falling back to Application Default Credentials when neither is set.
+func gcsCredentials(ctx context.Context, props map[string]string)
(*google.Credentials, error) {
+ credType := google.ServiceAccount
Review Comment:
Defaulting to service_account means a gcs.jsonkey / gcs.keypath holding an
authorized_user, external_account, or impersonated_service_account credential
fails unless gcs.credtype is also set — CredentialsFromJSONWithType enforces an
exact type match. With gcs.jsonkey = an authorized_user JSON and no
gcs.credtype you get:
gcs: parsing gcs.jsonkey: google: expected credential type
"service_account", found "authorized_user"
Not a regression (those keys were ignored before too), but the error
doesn't hint that gcs.credtype is the fix. Either document that gcs.credtype is
required for non-service-account keys, or wrap the mismatch error with a hint
naming gcs.credtype.
##########
io/gocloud/gcs_test.go:
##########
@@ -45,3 +52,37 @@ func TestParseGCSConfigUseJSONAPI(t *testing.T) {
assert.Len(t, cfg.ClientOptions, 0)
})
}
+
+// Inline JSON key (gcs.jsonkey) yields explicit credentials, not ADC.
+func TestGCSCredentialsFromInlineJSONKey(t *testing.T) {
+ creds, err := gcsCredentials(context.Background(), map[string]string{
+ io.GCSJSONKey: authorizedUserJSON,
+ io.GCSCredType: "authorized_user",
Review Comment:
Both credential tests always pass gcs.credtype explicitly, so neither
exercises the default (service_account) path — which is exactly where a non-SA
key fails. Add a case with a service_account-typed JSON and no gcs.credtype
(should succeed) and one with an authorized_user JSON and no gcs.credtype
(locks the current behavior) so the default path is covered.
##########
io/gocloud/gcs.go:
##########
@@ -67,15 +70,58 @@ func ParseGCSConfig(props map[string]string)
*gcsblob.Options {
}
}
+// gcsScope grants read/write access to GCS objects.
+const gcsScope = "https://www.googleapis.com/auth/devstorage.read_write";
+
+// gcsCredentials builds credentials from the gcs.jsonkey / gcs.keypath
+// properties (defaulting to a service-account key, honouring gcs.credtype),
+// falling back to Application Default Credentials when neither is set.
+func gcsCredentials(ctx context.Context, props map[string]string)
(*google.Credentials, error) {
+ credType := google.ServiceAccount
+ if v := props[io.GCSCredType]; v != "" {
+ credType = google.CredentialsType(v)
+ }
+
+ if jsonKey := props[io.GCSJSONKey]; jsonKey != "" {
+ creds, err := google.CredentialsFromJSONWithType(ctx,
[]byte(jsonKey), credType, gcsScope)
Review Comment:
ParseGCSConfig still appends
WithAuthCredentialsJSON/WithAuthCredentialsFile into gcscfg.ClientOptions, but
those are dead now: OpenBucket wraps the pre-built client as WithHTTPClient and
the api transport (dial.go:44) returns that client and ignores credential
options. Harmless today but misleading — anyone reading ParseGCSConfig will
think jsonkey/keypath authenticate through it when gcsCredentials is the only
thing that matters. Drop the credential options from ParseGCSConfig, or add a
comment noting they're superseded.
--
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]
