Skip to content

Commit

Permalink
feat(bigquery): fetch labels for dataset and table resources (#9)
Browse files Browse the repository at this point in the history 
* feat(bigquery): fetch labels for dataset and table resources

* refactor: rename var

* fix: prevent user from updating Resource.Details["_metadata"]

* test: extend test for FetchResources

* refactor: simplify labels metadata assignment in dataset and table resource model

* refactor(bigquery): use bqApi to fetch datasets and tables to reduce calls to bigquery

* refactor(bigquery): fetch tables concurrently

* test(bigquery): add unit tests for GetDatasets and GetTables methods in client

* refactor: limit active goroutines when fetching datasets

* refactor: rename "_metadata" to "__metadata" and store it in a const

* refactor: declare var for an if block only
  • Loading branch information
@rahmatrhd
rahmatrhd authored Mar 27, 2023
1 parent efc92d3 commit d7e9b83
Show file tree
Hide file tree
Showing 9 changed files with 378 additions and 78 deletions.
7 changes: 3 additions & 4 deletions core/provider/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,16 +525,15 @@ func (s *Service) getResources(ctx context.Context, p *domain.Provider) ([]*doma
for _, r := range flattenedProviderResources {
for _, er := range existingGuardianResources {
if er.URN == r.URN {
existingMetadata := er.Details
if existingMetadata != nil {
if existingDetails := er.Details; existingDetails != nil {
if r.Details != nil {
for key, value := range existingMetadata {
for key, value := range existingDetails {
if _, ok := r.Details[key]; !ok {
r.Details[key] = value
}
}
} else {
r.Details = existingMetadata
r.Details = existingDetails
}
}

Expand Down
84 changes: 72 additions & 12 deletions core/provider/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ import (
"testing"

"github.com/go-playground/validator/v10"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/goto/guardian/core/provider"
providermocks "github.com/goto/guardian/core/provider/mocks"
"github.com/goto/guardian/core/resource"
"github.com/goto/guardian/domain"
"github.com/goto/salt/log"
"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -349,20 +352,77 @@ func (s *ServiceTestSuite) TestFetchResources() {
})

s.Run("should upsert all resources on success", func() {
s.mockProviderRepository.EXPECT().Find(mock.AnythingOfType("*context.emptyCtx")).Return(providers, nil).Once()
expectedResources := []*domain.Resource{}
for _, p := range providers {
resources := []*domain.Resource{
{
ProviderType: p.Type,
ProviderURN: p.URN,
existingResources := []*domain.Resource{
{
ID: "1",
ProviderType: mockProviderType,
ProviderURN: mockProvider,
Type: "test-resource-type",
URN: "test-resource-urn-1",
Details: map[string]interface{}{
"owner": "test-owner",
resource.ReservedDetailsKeyMetadata: map[string]interface{}{
"labels": map[string]string{
"foo": "bar",
"baz": "qux",
},
"x": "y",
},
},
}
s.mockProvider.On("GetResources", p.Config).Return(resources, nil).Once()
expectedResources = append(expectedResources, resources...)
},
}
s.mockResourceService.On("BulkUpsert", mock.Anything, expectedResources).Return(nil).Once()
s.mockResourceService.On("Find", mock.Anything, mock.Anything).Return([]*domain.Resource{}, nil).Once()
newResources := []*domain.Resource{
{
ProviderType: mockProviderType,
ProviderURN: mockProvider,
Type: "test-resource-type",
URN: "test-resource-urn-1",
Details: map[string]interface{}{
resource.ReservedDetailsKeyMetadata: map[string]interface{}{
"labels": map[string]string{
"new-key": "new-value",
},
},
},
},
{
ProviderType: mockProviderType,
ProviderURN: mockProvider,
Type: "test-resource-type",
URN: "test-resource-urn-2",
},
}
expectedResources := []*domain.Resource{
{
ProviderType: mockProviderType,
ProviderURN: mockProvider,
Type: "test-resource-type",
URN: "test-resource-urn-1",
Details: map[string]interface{}{
"owner": "test-owner", // owner not changed
resource.ReservedDetailsKeyMetadata: map[string]interface{}{ // metadata updated
"labels": map[string]string{
"new-key": "new-value",
},
},
},
},
{
ProviderType: mockProviderType,
ProviderURN: mockProvider,
Type: "test-resource-type",
URN: "test-resource-urn-2",
},
}

expectedProvider := providers[0]
s.mockProviderRepository.EXPECT().Find(mock.AnythingOfType("*context.emptyCtx")).Return([]*domain.Provider{expectedProvider}, nil).Once()
s.mockProvider.EXPECT().GetResources(expectedProvider.Config).Return(newResources, nil).Once()
s.mockResourceService.EXPECT().BulkUpsert(mock.Anything, mock.AnythingOfType("[]*domain.Resource")).
Run(func(_a0 context.Context, resources []*domain.Resource) {
s.Empty(cmp.Diff(expectedResources, resources, cmpopts.IgnoreFields(domain.Resource{}, "ID", "CreatedAt", "UpdatedAt")))
}).Return(nil).Once()
s.mockResourceService.EXPECT().Find(mock.Anything, mock.Anything).Return(existingResources, nil).Once()
actualError := s.service.FetchResources(context.Background())

s.Nil(actualError)
Expand Down
6 changes: 6 additions & 0 deletions core/resource/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const (
AuditKeyResourceUpdate = "resource.update"
AuditKeyResourceDelete = "resource.delete"
AuditKeyResourceBatchDelete = "resource.batchDelete"

ReservedDetailsKeyMetadata = "__metadata"
)

//go:generate mockery --name=repository --exported --with-expecter
Expand Down Expand Up @@ -89,6 +91,10 @@ func (s *Service) Update(ctx context.Context, r *domain.Resource) error {
return err
}

// Details[ReservedDetailsKeyMetadata] is not allowed to be updated by users
// value for this field should only set by the provider on FetchResources
delete(r.Details, ReservedDetailsKeyMetadata)

if err := mergo.Merge(r, existingResource); err != nil {
return err
}
Expand Down
59 changes: 51 additions & 8 deletions core/resource/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"errors"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/goto/guardian/core/resource"
"github.com/goto/guardian/core/resource/mocks"
"github.com/goto/guardian/domain"
Expand Down Expand Up @@ -115,11 +117,13 @@ func (s *ServiceTestSuite) TestUpdate() {

s.Run("should only allows details and labels to be edited", func() {
testCases := []struct {
name string
resourceUpdatePayload *domain.Resource
existingResource *domain.Resource
expectedUpdatedValues *domain.Resource
}{
{
name: "empty labels in existing resource",
resourceUpdatePayload: &domain.Resource{
ID: "1",
Labels: map[string]string{
Expand All @@ -137,6 +141,7 @@ func (s *ServiceTestSuite) TestUpdate() {
},
},
{
name: "empty details in existing resource",
resourceUpdatePayload: &domain.Resource{
ID: "2",
Details: map[string]interface{}{
Expand All @@ -154,6 +159,7 @@ func (s *ServiceTestSuite) TestUpdate() {
},
},
{
name: "trying to update resource type",
resourceUpdatePayload: &domain.Resource{
ID: "2",
Type: "test",
Expand All @@ -165,17 +171,54 @@ func (s *ServiceTestSuite) TestUpdate() {
ID: "2",
},
},
{
name: "should exclude __metadata from update payload",
resourceUpdatePayload: &domain.Resource{
ID: "2",
Details: map[string]interface{}{
"owner": "[email protected]",
resource.ReservedDetailsKeyMetadata: map[string]string{
"new-key": "new-value",
},
},
},
existingResource: &domain.Resource{
ID: "2",
Details: map[string]interface{}{
"owner": "[email protected]",
"foo": "bar",
resource.ReservedDetailsKeyMetadata: map[string]string{
"key": "value",
},
},
},
expectedUpdatedValues: &domain.Resource{
ID: "2",
Details: map[string]interface{}{
"owner": "[email protected]",
"foo": "bar",
resource.ReservedDetailsKeyMetadata: map[string]string{
"key": "value",
},
},
},
},
}

for _, tc := range testCases {
s.mockRepository.EXPECT().GetOne(mock.AnythingOfType("*context.emptyCtx"), tc.resourceUpdatePayload.ID).Return(tc.existingResource, nil).Once()
s.mockRepository.EXPECT().Update(mock.AnythingOfType("*context.emptyCtx"), tc.expectedUpdatedValues).Return(nil).Once()
s.mockAuditLogger.EXPECT().Log(mock.Anything, resource.AuditKeyResourceUpdate, mock.Anything).Return(nil)

actualError := s.service.Update(context.Background(), tc.resourceUpdatePayload)

s.Nil(actualError)
s.mockRepository.AssertExpectations(s.T())
s.Run(tc.name, func() {
s.mockRepository.EXPECT().GetOne(mock.AnythingOfType("*context.emptyCtx"), tc.resourceUpdatePayload.ID).Return(tc.existingResource, nil).Once()
s.mockRepository.EXPECT().Update(mock.AnythingOfType("*context.emptyCtx"), mock.AnythingOfType("*domain.Resource")).
Run(func(_a0 context.Context, updateResourcePayload *domain.Resource) {
s.Empty(cmp.Diff(tc.expectedUpdatedValues, updateResourcePayload, cmpopts.IgnoreFields(domain.Resource{}, "UpdatedAt", "CreatedAt")))
}).Return(nil).Once()
s.mockAuditLogger.EXPECT().Log(mock.Anything, resource.AuditKeyResourceUpdate, mock.Anything).Return(nil)

actualError := s.service.Update(context.Background(), tc.resourceUpdatePayload)

s.Nil(actualError)
s.mockRepository.AssertExpectations(s.T())
})
}
})
}
Expand Down
62 changes: 31 additions & 31 deletions plugins/providers/bigquery/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ type bigQueryClient struct {
apiClient *bqApi.Service
}

func newBigQueryClient(projectID string, credentialsJSON []byte) (*bigQueryClient, error) {
func NewBigQueryClient(projectID string, opts ...option.ClientOption) (*bigQueryClient, error) {
ctx := context.Background()
client, err := bq.NewClient(ctx, projectID, option.WithCredentialsJSON(credentialsJSON))
client, err := bq.NewClient(ctx, projectID, opts...)
if err != nil {
return nil, err
}

apiClient, err := bqApi.NewService(ctx, option.WithCredentialsJSON(credentialsJSON))
apiClient, err := bqApi.NewService(ctx, opts...)
if err != nil {
return nil, err
}

iamService, err := iam.NewService(ctx, option.WithCredentialsJSON(credentialsJSON))
iamService, err := iam.NewService(ctx, opts...)
if err != nil {
return nil, err
}
Expand All @@ -49,20 +49,20 @@ func newBigQueryClient(projectID string, credentialsJSON []byte) (*bigQueryClien
// GetDatasets returns all datasets within a project
func (c *bigQueryClient) GetDatasets(ctx context.Context) ([]*Dataset, error) {
var results []*Dataset
it := c.client.Datasets(ctx)
for {
dataset, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
return nil, err
}

results = append(results, &Dataset{
ProjectID: dataset.ProjectID,
DatasetID: dataset.DatasetID,
})
req := c.apiClient.Datasets.List(c.projectID)
if err := req.Pages(ctx, func(page *bqApi.DatasetList) error {
for _, dataset := range page.Datasets {
d := &Dataset{
ProjectID: dataset.DatasetReference.ProjectId,
DatasetID: dataset.DatasetReference.DatasetId,
Labels: dataset.Labels,
}
results = append(results, d)
}
return nil
}); err != nil {
return nil, err
}

return results, nil
Expand All @@ -71,21 +71,21 @@ func (c *bigQueryClient) GetDatasets(ctx context.Context) ([]*Dataset, error) {
// GetTables returns all tables within a dataset
func (c *bigQueryClient) GetTables(ctx context.Context, datasetID string) ([]*Table, error) {
var results []*Table
it := c.client.Dataset(datasetID).Tables(ctx)
for {
table, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
return nil, err
}

results = append(results, &Table{
ProjectID: table.ProjectID,
DatasetID: table.DatasetID,
TableID: table.TableID,
})
req := c.apiClient.Tables.List(c.projectID, datasetID)
if err := req.Pages(ctx, func(page *bqApi.TableList) error {
for _, table := range page.Tables {
t := &Table{
ProjectID: table.TableReference.ProjectId,
DatasetID: table.TableReference.DatasetId,
TableID: table.TableReference.TableId,
Labels: table.Labels,
}
results = append(results, t)
}
return nil
}); err != nil {
return nil, err
}

return results, nil
Expand Down
Loading

0 comments on commit d7e9b83

Please sign in to comment.