config/client: default to read-only client

Default to a read-only client if Application Default Credentials fail to load.
This issue is notable on GCE where grabbing the DefaultTokenSource will
throw an error if the machine doesn't have a service account with valid
scope. In that scenario we don't want to fail public blobstore access.

This change removes a public API and updates an existing one. Consumers
should make the following change:

-  gcsClient, err := client.NewSDK(ctx, gcsConfig)
-  if err != nil {
-     log.Fatalf("creating gcs sdk: %v\n", err)
-  }
-  blobstoreClient, err := client.New(ctx, gcsClient, &gcsConfig)
+  blobstoreClient, err := client.New(ctx, &gcsConfig)

client/client.go

@@ -34,10 +34,9 @@ var ErrInvalidROWriteOperation = errors.New("the client operates in read only mo
 
 // GCSBlobstore encapsulates interaction with the GCS blobstore
 type GCSBlobstore struct {
-	// gcsClient is a pre-configured storage.Client.
-	client *storage.Client
-	// gcscliConfig is the configuration for interactions with the blobstore
-	config *config.GCSCli
+	gcs      *storage.Client
+	config   *config.GCSCli
+	readOnly bool
 }
 
 // validateRemoteConfig determines if the configuration of the client matches
@@ -46,11 +45,11 @@ type GCSBlobstore struct {
 // If operating in read-only mode, no mutations can be performed
 // so the remote bucket location is always compatible.
 func (client *GCSBlobstore) validateRemoteConfig() error {
-	if client.config.IsReadOnly() {
+	if client.readOnly {
 		return nil
 	}
 
-	bucket := client.client.Bucket(client.config.BucketName)
+	bucket := client.gcs.Bucket(client.config.BucketName)
 	attrs, err := bucket.Attrs(context.Background())
 	if err != nil {
 		return err
@@ -60,33 +59,33 @@ func (client *GCSBlobstore) validateRemoteConfig() error {
 
 // getObjectHandle returns a handle to an object at src.
 func (client GCSBlobstore) getObjectHandle(src string) *storage.ObjectHandle {
-	handle := client.client.Bucket(client.config.BucketName).Object(src)
+	handle := client.gcs.Bucket(client.config.BucketName).Object(src)
 	if client.config.EncryptionKey != nil {
 		handle = handle.Key(client.config.EncryptionKey)
 	}
 	return handle
 }
 
-// New returns a BlobstoreClient configured to operate using the given config
-// and client.
+// New returns a GCSBlobstore configured to operate using the given config
 //
-// non-nil error is returned on invalid client or config. If the configuration
+// non-nil error is returned on invalid Client or config. If the configuration
 // is incompatible with the GCS bucket, a non-nil error is also returned.
-func New(ctx context.Context, gcsClient *storage.Client,
-	gcscliConfig *config.GCSCli) (GCSBlobstore, error) {
-	if gcsClient == nil {
-		return GCSBlobstore{}, errors.New("nil client causes invalid blobstore")
+func New(ctx context.Context, cfg *config.GCSCli) (*GCSBlobstore, error) {
+	if cfg == nil {
+		return nil, errors.New("expected non-nill config object")
 	}
-	if gcscliConfig == nil {
-		return GCSBlobstore{}, errors.New("nil config causes invalid blobstore")
+
+	storageClient, readOnly, err := newStorageClient(ctx, cfg)
+	if err != nil {
+		return nil, fmt.Errorf("creating storage client: %v", err)
 	}
 
-	return GCSBlobstore{gcsClient, gcscliConfig}, nil
+	return &GCSBlobstore{gcs: storageClient, config: cfg, readOnly: readOnly}, nil
 }
 
 // Get fetches a blob from the GCS blobstore.
 // Destination will be overwritten if it already exists.
-func (client GCSBlobstore) Get(src string, dest io.Writer) error {
+func (client *GCSBlobstore) Get(src string, dest io.Writer) error {
 	remoteReader, err := client.getObjectHandle(src).NewReader(context.Background())
 	if err != nil {
 		return err
@@ -101,8 +100,8 @@ func (client GCSBlobstore) Get(src string, dest io.Writer) error {
 // Put does not retry if upload fails. This is a change from s3cli/client
 // which does retry an upload multiple times.
 // TODO: implement retry
-func (client GCSBlobstore) Put(src io.ReadSeeker, dest string) error {
-	if client.config.IsReadOnly() {
+func (client *GCSBlobstore) Put(src io.ReadSeeker, dest string) error {
+	if client.readOnly {
 		return ErrInvalidROWriteOperation
 	}
 
@@ -122,8 +121,8 @@ func (client GCSBlobstore) Put(src io.ReadSeeker, dest string) error {
 // Delete removes a blob from from the GCS blobstore.
 //
 // If the object does not exist, Delete returns a nil error.
-func (client GCSBlobstore) Delete(dest string) error {
-	if client.config.IsReadOnly() {
+func (client *GCSBlobstore) Delete(dest string) error {
+	if client.readOnly {
 		return ErrInvalidROWriteOperation
 	}
 
@@ -135,7 +134,7 @@ func (client GCSBlobstore) Delete(dest string) error {
 }
 
 // Exists checks if a blob exists in the GCS blobstore.
-func (client GCSBlobstore) Exists(dest string) (bool, error) {
+func (client *GCSBlobstore) Exists(dest string) (bool, error) {
 	_, err := client.getObjectHandle(dest).Attrs(context.Background())
 	if err == nil {
 		log.Printf("File '%s' exists in bucket '%s'\n",

client/sdk.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"errors"
 
-	"golang.org/x/oauth2"
 	"golang.org/x/oauth2/google"
 
 	"google.golang.org/api/option"
@@ -29,39 +28,34 @@ import (
 
 	"cloud.google.com/go/storage"
 	"github.com/cloudfoundry/bosh-gcscli/config"
-	"golang.org/x/oauth2/jwt"
 )
 
 const uaString = "bosh-gcscli"
 
-// NewSDK builds the GCS SDK client from a valid config.GCSCli
-func NewSDK(ctx context.Context, c config.GCSCli) (*storage.Client, error) {
-	var client *storage.Client
-	var err error
-	ua := option.WithUserAgent(uaString)
-	var opt option.ClientOption
-	switch c.CredentialsSource {
-	case config.ApplicationDefaultCredentialsSource:
-		var tokenSource oauth2.TokenSource
-		tokenSource, err = google.DefaultTokenSource(ctx, storage.ScopeFullControl)
-		if err == nil {
+func newStorageClient(ctx context.Context, cfg *config.GCSCli) (*storage.Client, bool, error) {
+	// default to a read-only client
+	readOnly := true
+	opt := option.WithHTTPClient(http.DefaultClient)
+
+	switch cfg.CredentialsSource {
+	case config.NoneCredentialsSource:
+		// no-op
+	case config.DefaultCredentialsSource:
+		// attempt to load the application default credentials
+		if tokenSource, err := google.DefaultTokenSource(ctx, storage.ScopeFullControl); err == nil {
 			opt = option.WithTokenSource(tokenSource)
+			readOnly = false
 		}
-	case config.NoneCredentialsSource:
-		opt = option.WithHTTPClient(http.DefaultClient)
 	case config.ServiceAccountFileCredentialsSource:
-		var token *jwt.Config
-		token, err = google.JWTConfigFromJSON([]byte(c.ServiceAccountFile), storage.ScopeFullControl)
-		if err == nil {
-			tokenSource := token.TokenSource(ctx)
-			opt = option.WithTokenSource(tokenSource)
+		if token, err := google.JWTConfigFromJSON([]byte(cfg.ServiceAccountFile), storage.ScopeFullControl); err == nil {
+			opt = option.WithTokenSource(token.TokenSource(ctx))
+			readOnly = false
 		}
 	default:
-		err = errors.New("unknown credentials_source in configuration")
-	}
-	if err != nil {
-		return client, err
+		return nil, false, errors.New("unknown credentials_source in configuration")
 	}
 
-	return storage.NewClient(ctx, ua, opt)
+	gcs, err := storage.NewClient(ctx, option.WithUserAgent(uaString), opt)
+
+	return gcs, readOnly, err
 }

config/config.go

@@ -27,7 +27,7 @@ type GCSCli struct {
 	// BucketName is the GCS bucket operations will use.
 	BucketName string `json:"bucket_name"`
 	// CredentialsSource is the location of a Service Account File.
-	// If left empty, Application Default Credentials will be used.
+	// If left empty, Application Default Credentials will be used if available.
 	// If equal to 'none', read-only scope will be used.
 	// If equal to 'static', json_key will be used.
 	CredentialsSource string `json:"credentials_source"`
@@ -52,9 +52,10 @@ const (
 	defaultMultiRegionalStorageClass = "MULTI_REGIONAL"
 )
 
-// ApplicationDefaultCredentialsSource specifies that
-// Application Default Credentials should be used for authentication.
-const ApplicationDefaultCredentialsSource = ""
+// DefaultCredentialsSource specifies that credentials should be detected.
+// Application Default Credentials will be used if avaliable.
+// A read-only client will be used otherwise.
+const DefaultCredentialsSource = ""
 
 // NoneCredentialsSource specifies that credentials are explicitly empty
 // and that the client should be restricted to a read-only scope.
@@ -143,8 +144,3 @@ func (c *GCSCli) FitCompatibleLocation(loc string) error {
 
 	return validLocationStorageClass(loc, c.StorageClass)
 }
-
-// IsReadOnly returns if the configuration is limited to only read operations.
-func (c *GCSCli) IsReadOnly() bool {
-	return c.CredentialsSource == NoneCredentialsSource
-}

config/config_test.go

@@ -160,17 +160,6 @@ var _ = Describe("BlobstoreClient configuration", func() {
 		})
 	})
 
-	Describe("when credentials_source is 'none'", func() {
-		dummyJSONBytes := []byte(`{"credentials_source": "none", "bucket_name": "some-bucket"}`)
-		dummyJSONReader := bytes.NewReader(dummyJSONBytes)
-
-		It("is ReadOnly", func() {
-			c, err := NewFromReader(dummyJSONReader)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(c.IsReadOnly()).To(BeTrue())
-		})
-	})
-
 	Describe("when credentials_source is not specified", func() {
 		dummyJSONBytes := []byte(`{"credentials_source": "", "bucket_name": "some-bucket"}`)
 		dummyJSONReader := bytes.NewReader(dummyJSONBytes)

integration/assertcontext.go

@@ -151,7 +151,7 @@ func AsDefaultCredentials(ctx *AssertContext) {
 	ctx.serviceAccountPath = tempFile.Name()
 	os.Setenv(GoogleAppCredentialsEnv, ctx.serviceAccountPath)
 
-	conf.CredentialsSource = config.ApplicationDefaultCredentialsSource
+	conf.CredentialsSource = config.DefaultCredentialsSource
 }
 
 // Clone returns a new AssertContext configured using the provided options.

integration/configurations.go

@@ -17,12 +17,21 @@
 package integration
 
 import (
+	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"os"
 
+	"cloud.google.com/go/storage"
+
 	"github.com/cloudfoundry/bosh-gcscli/config"
 
 	. "github.com/onsi/ginkgo/extensions/table"
+	"golang.org/x/oauth2"
+	"golang.org/x/oauth2/google"
+	"golang.org/x/oauth2/jwt"
+	"google.golang.org/api/option"
 )
 
 const regionalBucketEnv = "REGIONAL_BUCKET_NAME"
@@ -98,3 +107,35 @@ func getInvalidStorageClassConfigs() []TableEntry {
 		Entry("Regional bucket, Multi-Region StorageClass", multiRegion),
 	}
 }
+
+// newSDK builds the GCS SDK Client from a valid config.GCSCli
+// TODO: Simplify and remove this. Tests should expect a single config and use it.
+func newSDK(ctx context.Context, c config.GCSCli) (*storage.Client, error) {
+	var client *storage.Client
+	var err error
+	var opt option.ClientOption
+	switch c.CredentialsSource {
+	case config.DefaultCredentialsSource:
+		var tokenSource oauth2.TokenSource
+		tokenSource, err = google.DefaultTokenSource(ctx, storage.ScopeFullControl)
+		if err == nil {
+			opt = option.WithTokenSource(tokenSource)
+		}
+	case config.NoneCredentialsSource:
+		opt = option.WithHTTPClient(http.DefaultClient)
+	case config.ServiceAccountFileCredentialsSource:
+		var token *jwt.Config
+		token, err = google.JWTConfigFromJSON([]byte(c.ServiceAccountFile), storage.ScopeFullControl)
+		if err == nil {
+			tokenSource := token.TokenSource(ctx)
+			opt = option.WithTokenSource(tokenSource)
+		}
+	default:
+		err = errors.New("unknown credentials_source in configuration")
+	}
+	if err != nil {
+		return client, err
+	}
+
+	return storage.NewClient(ctx, opt)
+}

integration/gcs_encryption_test.go

@@ -67,9 +67,7 @@ var _ = Describe("Integration", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(session.ExitCode()).To(BeZero())
 
-			gcsClient, err := client.NewSDK(env.ctx, *env.Config)
-			Expect(err).ToNot(HaveOccurred())
-			blobstoreClient, err := client.New(env.ctx, gcsClient, env.Config)
+			blobstoreClient, err := client.New(env.ctx, env.Config)
 			Expect(err).ToNot(HaveOccurred())
 
 			env.Config.EncryptionKey[0]++
@@ -93,9 +91,7 @@ var _ = Describe("Integration", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(session.ExitCode()).To(BeZero())
 
-			gcsClient, err := client.NewSDK(env.ctx, *env.Config)
-			Expect(err).ToNot(HaveOccurred())
-			blobstoreClient, err := client.New(env.ctx, gcsClient, env.Config)
+			blobstoreClient, err := client.New(env.ctx, env.Config)
 			Expect(err).ToNot(HaveOccurred())
 
 			env.Config.EncryptionKey = nil

integration/gcs_general_test.go

@@ -106,9 +106,7 @@ var _ = Describe("Integration", func() {
 				const twoGB = 1024 * 1024 * 1024 * 2
 				limited := newrandReadSeeker(twoGB)
 
-				gcsClient, err := client.NewSDK(env.ctx, *env.Config)
-				Expect(err).ToNot(HaveOccurred())
-				blobstoreClient, err := client.New(env.ctx, gcsClient, env.Config)
+				blobstoreClient, err := client.New(env.ctx, env.Config)
 				Expect(err).ToNot(HaveOccurred())
 
 				err = blobstoreClient.Put(&limited, env.GCSFileName)
@@ -123,9 +121,7 @@ var _ = Describe("Integration", func() {
 			func(config *config.GCSCli) {
 				env.AddConfig(config)
 
-				gcsClient, err := client.NewSDK(env.ctx, *env.Config)
-				Expect(err).ToNot(HaveOccurred())
-				blobstoreClient, err := client.New(env.ctx, gcsClient, env.Config)
+				blobstoreClient, err := client.New(env.ctx, env.Config)
 				Expect(err).ToNot(HaveOccurred())
 
 				err = blobstoreClient.Put(&badReadSeeker{}, env.GCSFileName)

integration/gcs_public_test.go

@@ -57,7 +57,7 @@ var _ = Describe("GCS Public Bucket", func() {
 				RunGCSCLI(gcsCLIPath, setupEnv.ConfigPath, "put", setupEnv.ContentFile, setupEnv.GCSFileName)
 
 				// Make the file public
-				rwClient, err := client.NewSDK(setupEnv.ctx, *setupEnv.Config)
+				rwClient, err := newSDK(setupEnv.ctx, *setupEnv.Config)
 				Expect(err).ToNot(HaveOccurred())
 				bucket := rwClient.Bucket(setupEnv.Config.BucketName)
 				obj := bucket.Object(setupEnv.GCSFileName)

main.go

@@ -114,12 +114,7 @@ func main() {
 	}
 
 	ctx := context.Background()
-	gcsClient, err := client.NewSDK(ctx, gcsConfig)
-	if err != nil {
-		log.Fatalf("creating gcs sdk: %v\n", err)
-	}
-
-	blobstoreClient, err := client.New(ctx, gcsClient, &gcsConfig)
+	blobstoreClient, err := client.New(ctx, &gcsConfig)
 	if err != nil {
 		log.Fatalf("creating gcs client: %v\n", err)
 	}