zeroshade commented on code in PR #1503:
URL: https://github.com/apache/iceberg-go/pull/1503#discussion_r3647330951
##########
io/gocloud/gcs.go:
##########
@@ -67,18 +71,60 @@ func ParseGCSConfig(props map[string]string)
*gcsblob.Options {
}
}
+const gcsScope = "https://www.googleapis.com/auth/devstorage.read_write";
+
+func gcsCredentials(ctx context.Context, props map[string]string)
(*google.Credentials, error) {
+ credType := google.ServiceAccount
+ if v, ok := resolveGCSCredType(props); ok {
+ credType = google.CredentialsType(v)
+ }
+
+ // CredentialsFromJSONWithType requires the key's type to equal
credType, so
+ // hint at gcs.credtype when a non-service-account key fails against it.
+ parse := func(data []byte, source string) (*google.Credentials, error) {
+ creds, err := google.CredentialsFromJSONWithType(ctx, data,
credType, gcsScope)
+ if err != nil {
+ return nil, fmt.Errorf("gcs: parsing %s: set %s if the
key is not a service account: %w", source, io.GCSCredType, err)
+ }
+
+ return creds, nil
+ }
+
+ if jsonKey := props[io.GCSJSONKey]; jsonKey != "" {
+ return parse([]byte(jsonKey), io.GCSJSONKey)
+ }
+
+ if path := props[io.GCSKeyPath]; path != "" {
+ data, err := os.ReadFile(path)
+ if err != nil {
+ return nil, fmt.Errorf("gcs: reading %s %q: %w",
io.GCSKeyPath, path, err)
+ }
+
+ return parse(data, fmt.Sprintf("%s %q", io.GCSKeyPath, path))
+ }
+
+ creds, _ := gcp.DefaultCredentials(ctx)
+
+ return creds, nil
+}
+
// Construct a GCS bucket from a URL
func createGCSBucket(ctx context.Context, parsed *url.URL, props
map[string]string) (*blob.Bucket, error) {
gcscfg := ParseGCSConfig(props)
- creds, _ := gcp.DefaultCredentials(ctx)
+
+ creds, err := gcsCredentials(ctx, props)
+ if err != nil {
+ return nil, err
+ }
+
var client *gcp.HTTPClient
if creds == nil {
client = gcp.NewAnonymousHTTPClient(gcp.DefaultTransport())
} else {
Review Comment:
No explicit anonymous/no-auth mode: with `gcs.no-auth=true` and ADC present,
this still builds an authenticated client. Consider honoring `gcs.no-auth`
explicitly via `option.WithoutAuthentication`.
##########
io/gocloud/gcs.go:
##########
@@ -28,34 +30,36 @@ import (
"gocloud.dev/blob"
"gocloud.dev/blob/gcsblob"
"gocloud.dev/gcp"
+ "golang.org/x/oauth2/google"
"google.golang.org/api/option"
)
-var allowedGCSCredTypes = map[string]option.CredentialsType{
- "service_account": option.ServiceAccount,
- "authorized_user": option.AuthorizedUser,
- "impersonated_service_account": option.ImpersonatedServiceAccount,
- "external_account": option.ExternalAccount,
+var allowedGCSCredTypes = map[string]struct{}{
+ "service_account": {},
+ "authorized_user": {},
+ "impersonated_service_account": {},
+ "external_account": {},
}
-// ParseGCSConfig parses GCS properties and returns a configuration.
+func resolveGCSCredType(props map[string]string) (string, bool) {
+ v := props[io.GCSCredType]
+ if v == "" {
+ return "", false
+ }
+ if _, ok := allowedGCSCredTypes[v]; !ok {
+ return "", false
+ }
+
+ return v, true
+}
+
+// ParseGCSConfig parses non-credential GCS blob options; credentials are set
on
+// the client by gcsCredentials, which supersedes any credential option here.
func ParseGCSConfig(props map[string]string) *gcsblob.Options {
var o []option.ClientOption
Review Comment:
Endpoint override keys off `gcs.endpoint`; Java/PyIceberg use
`gcs.service.host`. Consider accepting the standard name as an alias or
documenting the difference.
##########
io/gocloud/gcs.go:
##########
@@ -67,18 +71,60 @@ func ParseGCSConfig(props map[string]string)
*gcsblob.Options {
}
}
+const gcsScope = "https://www.googleapis.com/auth/devstorage.read_write";
+
+func gcsCredentials(ctx context.Context, props map[string]string)
(*google.Credentials, error) {
+ credType := google.ServiceAccount
+ if v, ok := resolveGCSCredType(props); ok {
+ credType = google.CredentialsType(v)
+ }
+
+ // CredentialsFromJSONWithType requires the key's type to equal
credType, so
+ // hint at gcs.credtype when a non-service-account key fails against it.
+ parse := func(data []byte, source string) (*google.Credentials, error) {
+ creds, err := google.CredentialsFromJSONWithType(ctx, data,
credType, gcsScope)
+ if err != nil {
+ return nil, fmt.Errorf("gcs: parsing %s: set %s if the
key is not a service account: %w", source, io.GCSCredType, err)
+ }
+
+ return creds, nil
+ }
+
+ if jsonKey := props[io.GCSJSONKey]; jsonKey != "" {
+ return parse([]byte(jsonKey), io.GCSJSONKey)
Review Comment:
Only `gcs.jsonkey`/`gcs.keypath` are handled before falling back to ADC.
Iceberg/PyIceberg and REST-vended creds use `gcs.oauth2.token` (+
`gcs.oauth2.token-expires-at`, already recognized here); that token is never
turned into an `option.WithTokenSource`, so a vended GCS token is silently
dropped.
--
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]
