Added more tests and cleaned up GCP provider logic

Signed-off-by: pa250194 <pa250194@ncr.com>

controllers/bucket_controller.go

@@ -29,6 +29,7 @@ import (
 	"github.com/minio/minio-go/v7"
 	"github.com/minio/minio-go/v7/pkg/credentials"
 	"github.com/minio/minio-go/v7/pkg/s3utils"
+	"google.golang.org/api/option"
 	corev1 "k8s.io/api/core/v1"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -417,8 +418,7 @@ func (r *BucketReconciler) authGCP(ctx context.Context, bucket sourcev1.Bucket)
 		if err := gcp.ValidateSecret(secret.Data, secret.Name); err != nil {
 			return nil, err
 		}
-		serviceAccount := gcp.InitCredentialsWithSecret(secret.Data)
+		client, err = gcp.NewClient(ctx, option.WithCredentialsJSON(secret.Data["serviceaccount"]))
-		client, err = gcp.NewClientWithSAKey(ctx, serviceAccount)
 		if err != nil {
 			return nil, err
 		}

go.mod

@@ -23,7 +23,6 @@ require (
 	github.com/go-git/go-git/v5 v5.4.2
 	github.com/go-logr/logr v0.4.0
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
-	github.com/golang/mock v1.6.0 // indirect
 	github.com/googleapis/gax-go/v2 v2.1.0 // indirect
 	github.com/libgit2/git2go/v31 v31.6.1
 	github.com/minio/minio-go/v7 v7.0.10

go.sum

@@ -419,7 +419,6 @@ github.com/golang/mock v1.4.1/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt
 github.com/golang/mock v1.4.3/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw=
 github.com/golang/mock v1.4.4/go.mod h1:l3mdAwkq5BuhzHwde/uurv3sEJeZMXNpwsxVWU71h+4=
 github.com/golang/mock v1.5.0/go.mod h1:CWnOUgYIOo4TcNZ0wHX3YZCqsaM1I1Jvs6v3mP3KVu8=
-github.com/golang/mock v1.6.0 h1:ErTB+efbowRARo13NNdxyJji2egdxLGQhRaY+DUumQc=
 github.com/golang/mock v1.6.0/go.mod h1:p6yTPP+5HYm5mzsMV8JkE6ZKdX+/wYM6Hr+LicevLPs=
 github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=

pkg/gcp/gcp.go

@@ -18,7 +18,6 @@ package gcp
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -30,13 +29,6 @@ import (
 	"google.golang.org/api/option"
 )
 
-const (
-	ServiceAccount          = "service_account"
-	AuthUri                 = "https://accounts.google.com/o/oauth2/auth"
-	TokenUri                = "https://oauth2.googleapis.com/token"
-	AuthProviderX509CertUrl = "https://www.googleapis.com/oauth2/v1/certs"
-)
-
 var (
 	// IteratorDone is returned when the looping of objects/content
 	// has reached the end of the iteration.
@@ -61,27 +53,12 @@ type GCPClient struct {
 	EndRange int64
 }
 
-// CredentialsFile struct representing the GCP Service Account
-// JSON file.
-type CredentialsFile struct {
-	Type                    string `json:"type"`
-	ProjectID               string `json:"project_id"`
-	PrivateKeyID            string `json:"private_key_id"`
-	PrivateKey              string `json:"private_key"`
-	ClientEmail             string `json:"client_email"`
-	ClientID                string `json:"client_id"`
-	AuthUri                 string `json:"auth_uri"`
-	TokenUri                string `json:"token_uri"`
-	AuthProviderX509CertUrl string `json:"auth_provider_x509_cert_url"`
-	ClientX509CertUrl       string `json:"client_x509_cert_url"`
-}
-
 // NewClient creates a new GCP storage client
 // The Google Storage Client will automatically
 // look for the Google Application Credential environment variable
 // or look for the Google Application Credential file.
-func NewClient(ctx context.Context) (*GCPClient, error) {
+func NewClient(ctx context.Context, opts ...option.ClientOption) (*GCPClient, error) {
-	client, err := gcpStorage.NewClient(ctx)
+	client, err := gcpStorage.NewClient(ctx, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -89,70 +66,11 @@ func NewClient(ctx context.Context) (*GCPClient, error) {
 	return &GCPClient{Client: client, StartRange: 0, EndRange: -1}, nil
 }
 
-// NewClientWithSAKey creates a new GCP storage client
-// It uses the provided JSON file with service account details
-// To authenticate.
-func NewClientWithSAKey(ctx context.Context, credentials *CredentialsFile) (*GCPClient, error) {
-	saAccount, err := credentials.credentailsToJSON()
-	if err != nil {
-		return nil, err
-	}
-
-	client, err := gcpStorage.NewClient(ctx, option.WithCredentialsJSON(saAccount))
-	if err != nil {
-		return nil, err
-	}
-
-	return &GCPClient{Client: client, StartRange: 0, EndRange: -1}, nil
-}
-
-// credentailsToJSON converts GCP service account credentials struct to JSON.
-func (credentials *CredentialsFile) credentailsToJSON() ([]byte, error) {
-	credentialsJSON, err := json.Marshal(credentials)
-	if err != nil {
-		return nil, err
-	}
-
-	return credentialsJSON, nil
-}
-
-// InitCredentialsWithSecret creates a new credential
-// by initializing a new CredentialsFile struct
-func InitCredentialsWithSecret(secret map[string][]byte) *CredentialsFile {
-	return &CredentialsFile{
-		Type:                    ServiceAccount,
-		ProjectID:               string(secret["projectid"]),
-		PrivateKeyID:            string(secret["privatekeyid"]),
-		PrivateKey:              string(secret["privatekey"]),
-		ClientEmail:             string(secret["clientemail"]),
-		ClientID:                string(secret["clientid"]),
-		AuthUri:                 AuthUri,
-		TokenUri:                TokenUri,
-		AuthProviderX509CertUrl: AuthProviderX509CertUrl,
-		ClientX509CertUrl:       string(secret["certurl"]),
-	}
-}
-
 // ValidateSecret validates the credential secrets
 // It ensures that needed secret fields are not missing.
 func ValidateSecret(secret map[string][]byte, name string) error {
-	if _, exists := secret["projectid"]; !exists {
+	if _, exists := secret["serviceaccount"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'projectid'", name)
+		return fmt.Errorf("invalid '%s' secret data: required fields 'serviceaccount'", name)
-	}
-	if _, exists := secret["privatekeyid"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'privatekeyid'", name)
-	}
-	if _, exists := secret["privatekey"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'privatekey'", name)
-	}
-	if _, exists := secret["clientemail"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'clientemail'", name)
-	}
-	if _, exists := secret["clientid"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'clientid'", name)
-	}
-	if _, exists := secret["certurl"]; !exists {
-		return fmt.Errorf("invalid '%s' secret data: required fields 'certurl'", name)
 	}
 
 	return nil

pkg/gcp/gcp_test.go

@@ -47,54 +47,61 @@ const (
 )
 
 var (
-	Client *gcpStorage.Client
+	hc     *http.Client
+	client *gcpStorage.Client
+	close  func()
 	err    error
 )
 
 func TestMain(m *testing.M) {
-	hc, close := newTestServer(func(w http.ResponseWriter, r *http.Request) {
+	hc, close = newTestServer(func(w http.ResponseWriter, r *http.Request) {
 		io.Copy(ioutil.Discard, r.Body)
-		w.WriteHeader(200)
 		if r.RequestURI == fmt.Sprintf("/storage/v1/b/%s?alt=json&prettyPrint=false&projection=full", bucketName) {
+			w.WriteHeader(200)
 			response := getBucket()
-			jsonedResp, err := json.Marshal(response)
+			jsonResponse, err := json.Marshal(response)
 			if err != nil {
-				log.Fatalf("error marshalling resp %v\n", err)
+				log.Fatalf("error marshalling response %v\n", err)
 			}
-			_, err = w.Write(jsonedResp)
+			_, err = w.Write(jsonResponse)
 			if err != nil {
-				log.Fatalf("error writing jsonedResp %v\n", err)
+				log.Fatalf("error writing jsonResponse %v\n", err)
 			}
 		} else if r.RequestURI == fmt.Sprintf("/storage/v1/b/%s/o/%s?alt=json&prettyPrint=false&projection=full", bucketName, objectName) {
+			w.WriteHeader(200)
 			response := getObject()
-			jsonedResp, err := json.Marshal(response)
+			jsonResponse, err := json.Marshal(response)
 			if err != nil {
-				log.Fatalf("error marshalling resp %v\n", err)
+				log.Fatalf("error marshalling response %v\n", err)
 			}
-			_, err = w.Write(jsonedResp)
+			_, err = w.Write(jsonResponse)
 			if err != nil {
-				log.Fatalf("error writing jsonedResp %v\n", err)
+				log.Fatalf("error writing jsonResponse %v\n", err)
 			}
 		} else if r.RequestURI == fmt.Sprintf("/storage/v1/b/%s/o?alt=json&delimiter=&endOffset=&pageToken=&prefix=&prettyPrint=false&projection=full&startOffset=&versions=false", bucketName) {
+			w.WriteHeader(200)
 			response := getObject()
-			jsonedResp, err := json.Marshal(response)
+			jsonResponse, err := json.Marshal(response)
 			if err != nil {
-				log.Fatalf("error marshalling resp %v\n", err)
+				log.Fatalf("error marshalling response %v\n", err)
 			}
-			_, err = w.Write(jsonedResp)
+			_, err = w.Write(jsonResponse)
 			if err != nil {
-				log.Fatalf("error writing jsonedResp %v\n", err)
+				log.Fatalf("error writing jsonResponse %v\n", err)
 			}
 		} else if r.RequestURI == fmt.Sprintf("/%s/test.yaml", bucketName) || r.RequestURI == fmt.Sprintf("/storage/v1/b/%s/o/%s?alt=json&prettyPrint=false&projection=full", bucketName, objectName) {
+			w.WriteHeader(200)
 			response := getObjectFile()
 			_, err = w.Write([]byte(response))
 			if err != nil {
-				log.Fatalf("error writing jsonedResp %v\n", err)
+				log.Fatalf("error writing response %v\n", err)
 			}
+		} else {
+			w.WriteHeader(404)
 		}
 	})
 	ctx := context.Background()
-	Client, err = gcpStorage.NewClient(ctx, option.WithHTTPClient(hc))
+	client, err = gcpStorage.NewClient(ctx, option.WithHTTPClient(hc))
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -103,9 +110,15 @@ func TestMain(m *testing.M) {
 	os.Exit(run)
 }
 
+func TestNewClient(t *testing.T) {
+	gcpClient, err := gcp.NewClient(context.Background(), option.WithHTTPClient(hc))
+	assert.NilError(t, err)
+	assert.Assert(t, gcpClient != nil)
+}
+
 func TestBucketExists(t *testing.T) {
 	gcpClient := &gcp.GCPClient{
-		Client:     Client,
+		Client:     client,
 		StartRange: 0,
 		EndRange:   -1,
 	}
@@ -114,9 +127,21 @@ func TestBucketExists(t *testing.T) {
 	assert.Assert(t, exists)
 }
 
+func TestBucketNotExists(t *testing.T) {
+	bucket := "notexistsbucket"
+	gcpClient := &gcp.GCPClient{
+		Client:     client,
+		StartRange: 0,
+		EndRange:   -1,
+	}
+	exists, err := gcpClient.BucketExists(context.Background(), bucket)
+	assert.NilError(t, err)
+	assert.Assert(t, !exists)
+}
+
 func TestObjectAttributes(t *testing.T) {
 	gcpClient := &gcp.GCPClient{
-		Client:     Client,
+		Client:     client,
 		StartRange: 0,
 		EndRange:   -1,
 	}
@@ -131,7 +156,7 @@ func TestObjectAttributes(t *testing.T) {
 
 func TestListObjects(t *testing.T) {
 	gcpClient := &gcp.GCPClient{
-		Client:     Client,
+		Client:     client,
 		StartRange: 0,
 		EndRange:   -1,
 	}
@@ -151,7 +176,7 @@ func TestFGetObject(t *testing.T) {
 	assert.NilError(t, err)
 	defer os.RemoveAll(tempDir)
 	gcpClient := &gcp.GCPClient{
-		Client:     Client,
+		Client:     client,
 		StartRange: 0,
 		EndRange:   -1,
 	}
@@ -162,9 +187,41 @@ func TestFGetObject(t *testing.T) {
 	}
 }
 
+func TestFGetObjectNotExists(t *testing.T) {
+	object := "notexists.txt"
+	tempDir, err := os.MkdirTemp("", bucketName)
+	assert.NilError(t, err)
+	defer os.RemoveAll(tempDir)
+	gcpClient := &gcp.GCPClient{
+		Client:     client,
+		StartRange: 0,
+		EndRange:   -1,
+	}
+	localPath := filepath.Join(tempDir, object)
+	err = gcpClient.FGetObject(context.Background(), bucketName, object, localPath)
+	if err != io.EOF {
+		assert.Error(t, err, "storage: object doesn't exist")
+	}
+}
+
+func TestFGetObjectDirectoryIsFileName(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", bucketName)
+	defer os.RemoveAll(tempDir)
+	assert.NilError(t, err)
+	gcpClient := &gcp.GCPClient{
+		Client:     client,
+		StartRange: 0,
+		EndRange:   -1,
+	}
+	err = gcpClient.FGetObject(context.Background(), bucketName, objectName, tempDir)
+	if err != io.EOF {
+		assert.Error(t, err, "filename is a directory")
+	}
+}
+
 func TestSetRange(t *testing.T) {
 	gcpClient := &gcp.GCPClient{
-		Client:     Client,
+		Client:     client,
 		StartRange: 0,
 		EndRange:   -1,
 	}
@@ -173,6 +230,45 @@ func TestSetRange(t *testing.T) {
 	assert.Equal(t, gcpClient.EndRange, int64(5))
 }
 
+func TestValidateSecret(t *testing.T) {
+	t.Parallel()
+	testCases := []struct {
+		title  string
+		secret map[string][]byte
+		name   string
+		error  bool
+	}{
+		{
+			"Test Case 1",
+			map[string][]byte{
+				"serviceaccount": []byte("serviceaccount"),
+			},
+			"Service Account",
+			false,
+		},
+		{
+			"Test Case 2",
+			map[string][]byte{
+				"data": []byte("data"),
+			},
+			"Service Account",
+			true,
+		},
+	}
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.title, func(t *testing.T) {
+			t.Parallel()
+			err := gcp.ValidateSecret(testCase.secret, testCase.name)
+			if testCase.error {
+				assert.Error(t, err, fmt.Sprintf("invalid '%v' secret data: required fields 'serviceaccount'", testCase.name))
+			} else {
+				assert.NilError(t, err)
+			}
+		})
+	}
+}
+
 func newTestServer(handler func(w http.ResponseWriter, r *http.Request)) (*http.Client, func()) {
 	ts := httptest.NewTLSServer(http.HandlerFunc(handler))
 	tlsConf := &tls.Config{InsecureSkipVerify: true}