Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(storage): migrate oauth2/google usages to cloud.google.com/go/auth #11191

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+72 −42
Draft

fix(storage): migrate oauth2/google usages to cloud.google.com/go/auth #11191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f8457f
fix(storage): migrate oauth2/google usages to cloud.google.com/go/auth
quartzmo Nov 25, 2024
fb2a386
fix(storage): migrate oauth2/google usages to cloud.google.com/go/auth
quartzmo Nov 25, 2024
c416ea1
update to latest api/transport surface
quartzmo Dec 2, 2024
acbc68d
update to latest api/transport surface
quartzmo Dec 2, 2024
47a6e9f
Merge branch 'main' into storage-new-auth
quartzmo Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions storage/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,11 @@ func (b *BucketHandle) SignedURL(object string, opts *SignedURLOptions) (string,
newopts.GoogleAccessID = id
}
if newopts.SignBytes == nil && len(newopts.PrivateKey) == 0 {
if b.c.creds != nil && len(b.c.creds.JSON) > 0 {
if j := b.c.credsJSON(); len(j) > 0 {
var sa struct {
PrivateKey string `json:"private_key"`
}
err := json.Unmarshal(b.c.creds.JSON, &sa)
err := json.Unmarshal(j, &sa)
if err == nil && sa.PrivateKey != "" {
newopts.PrivateKey = []byte(sa.PrivateKey)
}
Expand Down Expand Up @@ -248,11 +248,11 @@ func (b *BucketHandle) GenerateSignedPostPolicyV4(object string, opts *PostPolic
newopts.GoogleAccessID = id
}
if newopts.SignBytes == nil && newopts.SignRawBytes == nil && len(newopts.PrivateKey) == 0 {
if b.c.creds != nil && len(b.c.creds.JSON) > 0 {
if j := b.c.credsJSON(); len(j) > 0 {
var sa struct {
PrivateKey string `json:"private_key"`
}
err := json.Unmarshal(b.c.creds.JSON, &sa)
err := json.Unmarshal(j, &sa)
if err == nil && sa.PrivateKey != "" {
newopts.PrivateKey = []byte(sa.PrivateKey)
}
Expand All @@ -270,14 +270,14 @@ func (b *BucketHandle) GenerateSignedPostPolicyV4(object string, opts *PostPolic
func (b *BucketHandle) detectDefaultGoogleAccessID() (string, error) {
returnErr := errors.New("no credentials found on client and not on GCE (Google Compute Engine)")

if b.c.creds != nil && len(b.c.creds.JSON) > 0 {
if j := b.c.credsJSON(); len(j) > 0 {
var sa struct {
ClientEmail string `json:"client_email"`
SAImpersonationURL string `json:"service_account_impersonation_url"`
CredType string `json:"type"`
}

err := json.Unmarshal(b.c.creds.JSON, &sa)
err := json.Unmarshal(j, &sa)
if err != nil {
returnErr = err
} else {
Expand Down
13 changes: 9 additions & 4 deletions storage/bucket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import (
"testing"
"time"

"cloud.google.com/go/auth/credentials"
"cloud.google.com/go/compute/metadata"
"cloud.google.com/go/internal/testutil"
"cloud.google.com/go/storage/internal/apiv2/storagepb"
"github.com/google/go-cmp/cmp"
gax "github.com/googleapis/gax-go/v2"
"golang.org/x/oauth2/google"
"google.golang.org/api/googleapi"
"google.golang.org/api/option"
raw "google.golang.org/api/storage/v1"
Expand Down Expand Up @@ -1291,11 +1291,16 @@ func TestDetectDefaultGoogleAccessID(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
jsonBytes := []byte(tc.creds(tc.serviceAccount))
creds, err := credentials.DetectDefault(&credentials.DetectOptions{
CredentialsJSON: jsonBytes,
})
Comment on lines +1295 to +1297
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get auth credentials here without this function?

if tc.expectSuccess && err != nil {
t.Fatal(err)
}
bucket := BucketHandle{
c: &Client{
creds: &google.Credentials{
JSON: []byte(tc.creds(tc.serviceAccount)),
},
creds: creds,
},
name: "my-bucket",
}
Expand Down
11 changes: 5 additions & 6 deletions storage/http_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,24 +31,23 @@ import (
"strings"
"time"

"cloud.google.com/go/auth"
"cloud.google.com/go/iam/apiv1/iampb"
"cloud.google.com/go/internal/optional"
"cloud.google.com/go/internal/trace"
"github.com/googleapis/gax-go/v2/callctx"
"golang.org/x/oauth2/google"
"google.golang.org/api/googleapi"
"google.golang.org/api/iterator"
"google.golang.org/api/option"
"google.golang.org/api/option/internaloption"
raw "google.golang.org/api/storage/v1"
"google.golang.org/api/transport"
htransport "google.golang.org/api/transport/http"
)

// httpStorageClient is the HTTP-JSON API implementation of the transport-agnostic
// storageClient interface.
type httpStorageClient struct {
creds *google.Credentials
creds *auth.Credentials
hc *http.Client
xmlHost string
raw *raw.Service
Expand All @@ -65,7 +64,7 @@ func newHTTPStorageClient(ctx context.Context, opts ...storageOption) (storageCl
o := s.clientOption
config := newStorageConfig(o...)

var creds *google.Credentials
var creds *auth.Credentials
// In general, it is recommended to use raw.NewService instead of htransport.NewClient
// since raw.NewService configures the correct default endpoints when initializing the
// internal http client. However, in our case, "NewRangeReader" in reader.go needs to
Expand All @@ -83,10 +82,10 @@ func newHTTPStorageClient(ctx context.Context, opts ...storageOption) (storageCl
)
// Don't error out here. The user may have passed in their own HTTP
// client which does not auth with ADC or other common conventions.
c, err := transport.Creds(ctx, o...)
c, err := internaloption.AuthCreds(ctx, o)
if err == nil {
creds = c
o = append(o, internaloption.WithCredentials(creds))
o = append(o, option.WithAuthCredentials(creds))
}
} else {
var hostURL *url.URL
Expand Down
58 changes: 38 additions & 20 deletions storage/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import (
"testing"
"time"

"cloud.google.com/go/auth"
"cloud.google.com/go/auth/credentials"
"cloud.google.com/go/httpreplay"
"cloud.google.com/go/iam"
"cloud.google.com/go/iam/apiv1/iampb"
Expand All @@ -55,13 +57,11 @@ import (
"github.com/googleapis/gax-go/v2/apierror"
"go.opentelemetry.io/contrib/detectors/gcp"
"go.opentelemetry.io/otel/sdk/resource"
"golang.org/x/oauth2/google"
"google.golang.org/api/googleapi"
"google.golang.org/api/iterator"
itesting "google.golang.org/api/iterator/testing"
"google.golang.org/api/option"
"google.golang.org/api/option/internaloption"
"google.golang.org/api/transport"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
Expand Down Expand Up @@ -94,6 +94,13 @@ var (
controlClient *control.StorageControlClient
)

var (
testScopes = []string{
ScopeFullControl,
"https://www.googleapis.com/auth/cloud-platform",
}
)

func TestMain(m *testing.M) {
cleanup := initIntegrationTest()
cleanupEmulatorClients := initEmulatorClients()
Expand Down Expand Up @@ -5348,6 +5355,24 @@ func TestIntegration_NewReaderWithContentEncodingGzip(t *testing.T) {
})
}

type credentialsFile struct {
Type string `json:"type"`

// Service Account email
ClientEmail string `json:"client_email"`
}

func jwtConfigFromJSON(jsonKey []byte) (*credentialsFile, error) {
var f credentialsFile
if err := json.Unmarshal(jsonKey, &f); err != nil {
return nil, err
}
if f.Type != "service_account" {
return nil, fmt.Errorf("read JWT from JSON credentials: 'type' field is %q (expected service_account)", f.Type)
}
return &f, nil
}

func TestIntegration_HMACKey(t *testing.T) {
ctx := skipJSONReads(skipGRPC("hmac not implemented"), "no reads in test")
multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, _, _ string, client *Client) {
Expand All @@ -5367,13 +5392,12 @@ func TestIntegration_HMACKey(t *testing.T) {
if credentials.JSON == nil {
t.Fatal("could not read the JSON key file, is GCLOUD_TESTS_GOLANG_KEY set correctly?")
}
conf, err := google.JWTConfigFromJSON(credentials.JSON)
conf, err := jwtConfigFromJSON(credentials.JSON)
if err != nil {
t.Fatal(err)
}
serviceAccountEmail := conf.Email

hmacKey, err := client.CreateHMACKey(ctx, projectID, serviceAccountEmail)
hmacKey, err := client.CreateHMACKey(ctx, projectID, conf.ClientEmail)
if err != nil {
t.Fatalf("Failed to create HMACKey: %v", err)
}
Expand Down Expand Up @@ -5600,8 +5624,7 @@ func TestIntegration_SignedURL_WithCreds(t *testing.T) {
}

ctx := context.Background()

creds, err := findTestCredentials(ctx, "GCLOUD_TESTS_GOLANG_KEY", ScopeFullControl, "https://www.googleapis.com/auth/cloud-platform")
creds, err := findTestCredentials(ctx, "GCLOUD_TESTS_GOLANG_KEY", testScopes)
if err != nil {
t.Fatalf("unable to find test credentials: %v", err)
}
Expand All @@ -5626,7 +5649,7 @@ func TestIntegration_SignedURL_WithCreds(t *testing.T) {
if err := verifySignedURL(url, nil, contents); err != nil {
t.Fatalf("problem with the signed URL: %v", err)
}
}, option.WithCredentials(creds))
}, option.WithAuthCredentials(creds))
}

func TestIntegration_SignedURL_DefaultSignBytes(t *testing.T) {
Expand Down Expand Up @@ -5683,7 +5706,7 @@ func TestIntegration_PostPolicyV4_WithCreds(t *testing.T) {
// By default we are authed with a token source, so don't have the context to
// read some of the fields from the keyfile.
// Here we explictly send the key to the client.
creds, err := findTestCredentials(context.Background(), "GCLOUD_TESTS_GOLANG_KEY", ScopeFullControl, "https://www.googleapis.com/auth/cloud-platform")
creds, err := findTestCredentials(context.Background(), "GCLOUD_TESTS_GOLANG_KEY", testScopes)
if err != nil {
t.Fatalf("unable to find test credentials: %v", err)
}
Expand Down Expand Up @@ -5728,7 +5751,7 @@ func TestIntegration_PostPolicyV4_WithCreds(t *testing.T) {
}
})
}
}, option.WithCredentials(creds))
}, option.WithAuthCredentials(creds))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try running the integration tests without changes? I would like to make sure they pass even using the old auth before switching them over.

Signed URL stuff in particular, do you think it would be useful to test with both the old and new auth paths?


}

Expand Down Expand Up @@ -5969,16 +5992,11 @@ func verifyPostPolicy(pv4 *PostPolicyV4, obj *ObjectHandle, bytesToWrite []byte,
})
}

func findTestCredentials(ctx context.Context, envVar string, scopes ...string) (*google.Credentials, error) {
key := os.Getenv(envVar)
var opts []option.ClientOption
if len(scopes) > 0 {
opts = append(opts, option.WithScopes(scopes...))
}
if key != "" {
opts = append(opts, option.WithCredentialsFile(key))
}
return transport.Creds(ctx, opts...)
func findTestCredentials(ctx context.Context, envVar string, scopes []string) (*auth.Credentials, error) {
return credentials.DetectDefault(&credentials.DetectOptions{
CredentialsFile: os.Getenv(envVar),
Scopes: scopes,
})
}

type testHelper struct {
Expand Down
20 changes: 14 additions & 6 deletions storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"time"
"unicode/utf8"

"cloud.google.com/go/auth"
"cloud.google.com/go/internal/optional"
"cloud.google.com/go/internal/trace"
"cloud.google.com/go/storage/internal"
Expand All @@ -46,12 +47,10 @@ import (
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/sdk/metric"
"go.opentelemetry.io/otel/sdk/metric/metricdata"
"golang.org/x/oauth2/google"
"google.golang.org/api/googleapi"
"google.golang.org/api/option"
"google.golang.org/api/option/internaloption"
raw "google.golang.org/api/storage/v1"
"google.golang.org/api/transport"
htransport "google.golang.org/api/transport/http"
"google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/stats/opentelemetry"
Expand Down Expand Up @@ -117,13 +116,22 @@ type Client struct {
// xmlHost is the default host used for XML requests.
xmlHost string
// May be nil.
creds *google.Credentials
creds *auth.Credentials
retry *retryConfig

// tc is the transport-agnostic client implemented with either gRPC or HTTP.
tc storageClient
}

// credsJSON returns the raw JSON of the Client's creds, or an empty slice
// if no credentials JSON is available.
func (c Client) credsJSON() []byte {
if c.creds != nil && len(c.creds.JSON()) > 0 {
return c.creds.JSON()
}
return []byte{}
Comment on lines +128 to +132
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be nicer returning the creds and a bool so that callers can check creds, ok := credsJSON()

Suggested change
func (c Client) credsJSON() []byte {
if c.creds != nil && len(c.creds.JSON()) > 0 {
return c.creds.JSON()
}
return []byte{}
func (c Client) credsJSON() ([]byte, bool) {
if c.creds != nil && len(c.creds.JSON()) > 0 {
return c.creds.JSON(), true
}
return []byte{}, false

}

// NewClient creates a new Google Cloud Storage client using the HTTP transport.
// The default scope is ScopeFullControl. To use a different scope, like
// ScopeReadOnly, use option.WithScopes.
Expand All @@ -134,7 +142,7 @@ type Client struct {
// You may configure the client by passing in options from the [google.golang.org/api/option]
// package. You may also use options defined in this package, such as [WithJSONReads].
func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error) {
var creds *google.Credentials
var creds *auth.Credentials

// In general, it is recommended to use raw.NewService instead of htransport.NewClient
// since raw.NewService configures the correct default endpoints when initializing the
Expand All @@ -154,10 +162,10 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error

// Don't error out here. The user may have passed in their own HTTP
// client which does not auth with ADC or other common conventions.
c, err := transport.Creds(ctx, opts...)
c, err := internaloption.AuthCreds(ctx, opts)
if err == nil {
creds = c
opts = append(opts, internaloption.WithCredentials(creds))
opts = append(opts, option.WithAuthCredentials(creds))
}
} else {
var hostURL *url.URL
Expand Down
Loading