Skip to content

[ROX-12625 + ROX-13032] : Add GetCollectionCount and UpdateCollection endpoints and services #3749

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

Merged
keyallis merged 7 commits into from
Nov 10, 2022
Merged

[ROX-12625 + ROX-13032] : Add GetCollectionCount and UpdateCollection endpoints and services #3749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5956e79
Add GetCollectionCount and UpdateCollection services
charmik-redhat Nov 8, 2022
0e12814
Added more tests
charmik-redhat Nov 8, 2022
3ed0d27
Fix error log typo
charmik-redhat Nov 8, 2022
2ab50e8
Preserve createdAt and createdBy on updates
charmik-redhat Nov 8, 2022
7c1402b
Add permissions and update error msg
charmik-redhat Nov 8, 2022
385fcf0
Add comment to datastore interface
charmik-redhat Nov 8, 2022
88fa5a4
Address comments
charmik-redhat Nov 9, 2022
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
1 change: 1 addition & 0 deletions central/resourcecollection/datastore/datastore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type DataStore interface {
AddCollection(ctx context.Context, collection *storage.ResourceCollection) error
DeleteCollection(ctx context.Context, id string) error
DryRunAddCollection(ctx context.Context, collection *storage.ResourceCollection) error
// UpdateCollection updates the given collection object, and preserves createdAt and createdBy fields from stored collection
UpdateCollection(ctx context.Context, collection *storage.ResourceCollection) error
// autocomplete workflow, maybe SearchResults? TODO ROX-12616
}
Expand Down
3 changes: 3 additions & 0 deletions central/resourcecollection/datastore/datastore_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ func (ds *datastoreImpl) updateCollectionWorkflow(ctx context.Context, collectio
return err
}

collection.CreatedBy = storedCollection.GetCreatedBy()
collection.CreatedAt = storedCollection.GetCreatedAt()

// if this is a dryrun we don't want to make changes to the datastore or tracking objects
if dryrun {
return nil
Expand Down
20 changes: 20 additions & 0 deletions central/resourcecollection/datastore/datastore_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"context"
"fmt"
"testing"
"time"

"github.com/heimdalr/dag"
"github.com/jackc/pgx/v4/pgxpool"
Expand All @@ -14,6 +15,7 @@ import (
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/postgres/pgtest"
"github.com/stackrox/rox/pkg/protoconv"
"github.com/stackrox/rox/pkg/sac"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -270,10 +272,28 @@ func (s *CollectionPostgresDataStoreTestSuite) TestCollectionWorkflows() {
assert.True(s.T(), ok)
assert.Equal(s.T(), objB, obj)

// successful updates preserve createdAt and createdBy
objC := getTestCollection("c", nil)
objC.CreatedAt = protoconv.ConvertTimeToTimestamp(time.Now())
objC.CreatedBy = &storage.SlimUser{
Id: "uid",
Name: "uname",
}
err = s.datastore.AddCollection(ctx, objC)
assert.NoError(s.T(), err)
objC.Name = "C"
err = s.datastore.UpdateCollection(ctx, objC)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objC.GetId())
assert.NoError(s.T(), err)
assert.True(s.T(), ok)
assert.Equal(s.T(), objC, obj)

// clean up testing data and verify the datastore is empty
assert.NoError(s.T(), s.datastore.DeleteCollection(ctx, objB.GetId()))
assert.NoError(s.T(), s.datastore.DeleteCollection(ctx, objE.GetId()))
assert.NoError(s.T(), s.datastore.DeleteCollection(ctx, objA.GetId()))
assert.NoError(s.T(), s.datastore.DeleteCollection(ctx, objC.GetId()))
count, err = s.datastore.Count(ctx, nil)
assert.NoError(s.T(), err)
assert.Equal(s.T(), 0, count)
Expand Down
61 changes: 52 additions & 9 deletions central/resourcecollection/service/service_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var (
authorizer = or.SensorOrAuthorizer(perrpc.FromMap(map[authz.Authorizer][]string{
user.With(permissions.View(resources.WorkflowAdministration)): {
"/v1.CollectionService/GetCollection",
// "/v1.CollectionService/GetCollectionCount", TODO ROX-12625
"/v1.CollectionService/GetCollectionCount",
"/v1.CollectionService/ListCollections",
// "/v1.CollectionService/ListCollectionSelectors", TODO ROX-12612
},
Expand All @@ -41,7 +41,7 @@ var (
"/v1.CollectionService/CreateCollection",
"/v1.CollectionService/DeleteCollection",
// "/v1.CollectionService/DryRunCollection", TODO ROX-13031
// "/v1.CollectionService/UpdateCollection", TODO ROX-13032
"/v1.CollectionService/UpdateCollection",
},
}))
)
Expand Down Expand Up @@ -78,7 +78,7 @@ func (s *serviceImpl) AuthFuncOverride(ctx context.Context, fullMethodName strin
// GetCollection returns a collection for the given request
func (s *serviceImpl) GetCollection(ctx context.Context, request *v1.GetCollectionRequest) (*v1.GetCollectionResponse, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.New("Resource collections is not enabled")
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}
if request.GetId() == "" {
return nil, errors.Wrap(errox.InvalidArgs, "Id field should be set when requesting a collection")
Expand All @@ -101,10 +101,29 @@ func (s *serviceImpl) getCollection(ctx context.Context, id string) (*v1.GetColl
}, nil
}

// GetCollectionCount returns count of collections matching the query in the request
func (s *serviceImpl) GetCollectionCount(ctx context.Context, request *v1.GetCollectionCountRequest) (*v1.GetCollectionCountResponse, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}

// parse query
parsedQuery, err := search.ParseQuery(request.GetQuery().GetQuery(), search.MatchAllIfEmpty())
if err != nil {
return nil, errors.Wrap(errox.InvalidArgs, err.Error())
}

count, err := s.datastore.Count(ctx, parsedQuery)
if err != nil {
return nil, err
}
return &v1.GetCollectionCountResponse{Count: int32(count)}, nil
}

// DeleteCollection deletes the collection with the given ID
func (s *serviceImpl) DeleteCollection(ctx context.Context, request *v1.ResourceByID) (*v1.Empty, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.New("Support for resource collections is not enabled")
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}
if request.GetId() == "" {
return nil, errors.Wrap(errox.InvalidArgs, "Non empty collection id must be specified to delete a collection")
Expand All @@ -118,10 +137,10 @@ func (s *serviceImpl) DeleteCollection(ctx context.Context, request *v1.Resource
// CreateCollection creates a new collection from the given request
func (s *serviceImpl) CreateCollection(ctx context.Context, request *v1.CreateCollectionRequest) (*v1.CreateCollectionResponse, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.New("Support for resource collections is not enabled")
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}

collection, err := collectionRequestToCollection(ctx, request, true)
collection, err := collectionRequestToCollection(ctx, request, "")
if err != nil {
return nil, err
}
Expand All @@ -134,7 +153,29 @@ func (s *serviceImpl) CreateCollection(ctx context.Context, request *v1.CreateCo
return &v1.CreateCollectionResponse{Collection: collection}, nil
}

func collectionRequestToCollection(ctx context.Context, request collectionRequest, isCreate bool) (*storage.ResourceCollection, error) {
func (s *serviceImpl) UpdateCollection(ctx context.Context, request *v1.UpdateCollectionRequest) (*v1.UpdateCollectionResponse, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}

if request.GetId() == "" {
Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Nov 8, 2022

Choose a reason for hiding this comment

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

Thought this is checked by the datastore, I added this check here to avoid looping through embedded collections. I do not feel strongly about it though and can remove it if someone feels that way.

Copy link
Copy Markdown
Contributor

@keyallis keyallis Nov 8, 2022

Choose a reason for hiding this comment

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

What do you mean by avoiding looping through embedded collections?

Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Nov 8, 2022

Choose a reason for hiding this comment

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

I mean the loop at the end of collectionRequestToCollection func.

if len(request.GetEmbeddedCollectionIds()) > 0 {
		embeddedCollections := make([]*storage.ResourceCollection_EmbeddedResourceCollection, 0, len(request.GetEmbeddedCollectionIds()))
		for _, id := range request.GetEmbeddedCollectionIds() {
			embeddedCollections = append(embeddedCollections, &storage.ResourceCollection_EmbeddedResourceCollection{Id: id})
		}
		collection.EmbeddedCollections = embeddedCollections
	}

return nil, errors.Wrap(errox.InvalidArgs, "Non empty collection id must be specified to update a collection")
}

collection, err := collectionRequestToCollection(ctx, request, request.GetId())
if err != nil {
return nil, err
}

err = s.datastore.UpdateCollection(ctx, collection)
Copy link
Copy Markdown
Contributor

@md2119 md2119 Nov 8, 2022

Choose a reason for hiding this comment

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

You need to fetch the stored collection object and reconcile the updated fields with it i.e. the createdAt and createdBy. Update the UpdateCollection func and add a unit test.

Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Nov 8, 2022

Choose a reason for hiding this comment

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

Good catch, made that change on UpdateCollection implementation on datastore instead as datastore fetches the storedCollection anyways.

if err != nil {
return nil, err
}

return &v1.UpdateCollectionResponse{Collection: collection}, nil
}

func collectionRequestToCollection(ctx context.Context, request collectionRequest, id string) (*storage.ResourceCollection, error) {
if request.GetName() == "" {
return nil, errors.Wrap(errox.InvalidArgs, "Collection name should not be empty")
}
Expand All @@ -151,14 +192,16 @@ func collectionRequestToCollection(ctx context.Context, request collectionReques
timeNow := protoconv.ConvertTimeToTimestamp(time.Now())

collection := &storage.ResourceCollection{
Id: id,
Name: request.GetName(),
Description: request.GetDescription(),
LastUpdated: timeNow,
UpdatedBy: slimUser,
ResourceSelectors: request.GetResourceSelectors(),
}

if isCreate {
if id == "" {
// new collection
collection.CreatedBy = slimUser
collection.CreatedAt = timeNow
}
Expand All @@ -176,7 +219,7 @@ func collectionRequestToCollection(ctx context.Context, request collectionReques

func (s *serviceImpl) ListCollections(ctx context.Context, request *v1.ListCollectionsRequest) (*v1.ListCollectionsResponse, error) {
if !features.ObjectCollections.Enabled() {
return nil, errors.New("Resource collections is not enabled")
return nil, errors.Errorf("%s env var is not enabled", features.ObjectCollections.EnvVar())
}

// parse query
Expand Down
114 changes: 114 additions & 0 deletions central/resourcecollection/service/service_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,32 @@ func (suite *CollectionServiceTestSuite) TestGetCollection() {
suite.Nil(result)
}

func (suite *CollectionServiceTestSuite) TestGetCollectionCount() {
if !features.ObjectCollections.Enabled() {
suite.T().Skip("skipping because env var is not set")
}

allAccessCtx := sac.WithAllAccess(context.Background())
request := &v1.GetCollectionCountRequest{
Query: &v1.RawQuery{},
}

parsedQuery, err := search.ParseQuery(request.GetQuery().GetQuery(), search.MatchAllIfEmpty())
suite.NoError(err)

suite.dataStore.EXPECT().Count(allAccessCtx, parsedQuery).Times(1).Return(10, nil)
resp, err := suite.collectionService.GetCollectionCount(allAccessCtx, request)
suite.NoError(err)
suite.NotNil(resp)
suite.Equal(int32(10), resp.GetCount())

// test error
suite.dataStore.EXPECT().Count(allAccessCtx, parsedQuery).Times(1).Return(0, errors.New("test error"))
resp, err = suite.collectionService.GetCollectionCount(allAccessCtx, request)
suite.Error(err)
suite.Nil(resp)
}

func (suite *CollectionServiceTestSuite) TestCreateCollection() {
if !features.ObjectCollections.Enabled() {
suite.T().Skip("skipping because env var is not set")
Expand Down Expand Up @@ -164,6 +190,94 @@ func (suite *CollectionServiceTestSuite) TestCreateCollection() {
suite.NotNil(resp.GetCollection().GetLastUpdated())
}

func (suite *CollectionServiceTestSuite) TestUpdateCollection() {
if !features.ObjectCollections.Enabled() {
suite.T().Skip("skipping because env var is not set")
}
allAccessCtx := sac.WithAllAccess(context.Background())

// test error when collection Id is empty
request := &v1.UpdateCollectionRequest{
Id: "",
}
resp, err := suite.collectionService.UpdateCollection(allAccessCtx, request)
suite.NotNil(err)
suite.Nil(resp)

// test error when collection name is empty
request = &v1.UpdateCollectionRequest{
Id: "id1",
Name: "",
}
resp, err = suite.collectionService.UpdateCollection(allAccessCtx, request)
suite.NotNil(err)
suite.Nil(resp)

// test error on context without identity
request = &v1.UpdateCollectionRequest{
Id: "id2",
Name: "b",
}
resp, err = suite.collectionService.UpdateCollection(allAccessCtx, request)
suite.NotNil(err)
suite.Nil(resp)

// test error on empty/nil resource selectors
request = &v1.UpdateCollectionRequest{
Id: "id3",
Name: "c",
}
mockID := mockIdentity.NewMockIdentity(suite.mockCtrl)
mockID.EXPECT().UID().Return("uid").Times(1)
mockID.EXPECT().FullName().Return("name").Times(1)
mockID.EXPECT().FriendlyName().Return("name").Times(1)
ctx := authn.ContextWithIdentity(allAccessCtx, mockID, suite.T())
resp, err = suite.collectionService.UpdateCollection(ctx, request)
suite.NotNil(err)
suite.Nil(resp)

// test successful update
request = &v1.UpdateCollectionRequest{
Id: "id4",
Name: "d",
Description: "description",
ResourceSelectors: []*storage.ResourceSelector{
{
Rules: []*storage.SelectorRule{
{
FieldName: search.DeploymentName.String(),
Operator: storage.BooleanOperator_OR,
Values: []*storage.RuleValue{
{
Value: "deployment",
},
},
},
},
},
},
EmbeddedCollectionIds: []string{"id1", "id2"},
}

mockID.EXPECT().UID().Return("uid").Times(1)
mockID.EXPECT().FullName().Return("name").Times(1)
mockID.EXPECT().FriendlyName().Return("name").Times(1)
ctx = authn.ContextWithIdentity(allAccessCtx, mockID, suite.T())

suite.dataStore.EXPECT().UpdateCollection(ctx, gomock.Any()).Times(1).Return(nil)
resp, err = suite.collectionService.UpdateCollection(ctx, request)
suite.NoError(err)
suite.NotNil(resp.GetCollection())
suite.Equal(request.GetId(), resp.GetCollection().GetId())
suite.Equal(request.Name, resp.GetCollection().GetName())
suite.Equal(request.GetDescription(), resp.GetCollection().GetDescription())
suite.Equal(request.GetResourceSelectors(), resp.GetCollection().GetResourceSelectors())
suite.Equal(request.GetEmbeddedCollectionIds(), suite.embeddedCollectionsToIds(resp.GetCollection().GetEmbeddedCollections()))
suite.Equal("uid", resp.GetCollection().GetUpdatedBy().GetId())
suite.Equal("name", resp.GetCollection().GetUpdatedBy().GetName())
suite.NotNil(resp.GetCollection().GetLastUpdated())
}

func (suite *CollectionServiceTestSuite) TestDeleteCollection() {
if !features.ObjectCollections.Enabled() {
suite.T().Skip("skipping because env var is not set")
Expand Down
Loading