Skip to content

[ROX-12622] : Add create collection service #3600

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
charmik-redhat merged 3 commits into from
Oct 31, 2022
Merged

[ROX-12622] : Add create collection service #3600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fade6b
Add create collection service and tests
charmik-redhat Oct 27, 2022
b622f79
Address PR comments
charmik-redhat Oct 28, 2022
d1cf445
Address PR comments
charmik-redhat Oct 31, 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
74 changes: 73 additions & 1 deletion central/resourcecollection/service/service_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,23 @@ package service

import (
"context"
"time"

"github.com/grpc-ecosystem/grpc-gateway/runtime"
"github.com/pkg/errors"
"github.com/stackrox/rox/central/resourcecollection/datastore"
"github.com/stackrox/rox/central/role/resources"
"github.com/stackrox/rox/central/vulnerabilityrequest/utils"
v1 "github.com/stackrox/rox/generated/api/v1"
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/auth/permissions"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/grpc/authz"
"github.com/stackrox/rox/pkg/grpc/authz/or"
"github.com/stackrox/rox/pkg/grpc/authz/perrpc"
"github.com/stackrox/rox/pkg/grpc/authz/user"
"github.com/stackrox/rox/pkg/protoconv"
"google.golang.org/grpc"
)

Expand All @@ -28,14 +32,21 @@ var (
},
user.With(permissions.Modify(resources.WorkflowAdministration)): {
// "/v1.CollectionService/AutoCompleteCollection", TODO ROX-12616
// "/v1.CollectionService/CreateCollection", TODO ROX-12622
"/v1.CollectionService/CreateCollection",
// "/v1.CollectionService/DeleteCollection", TODO ROX-13030
// "/v1.CollectionService/DryRunCollection", TODO ROX-13031
// "/v1.CollectionService/UpdateCollection", TODO ROX-13032
},
}))
)

type collectionRequest interface {
GetName() string
GetDescription() string
GetResourceSelectors() []*storage.ResourceSelector
GetEmbeddedCollectionIds() []string
}

// serviceImpl is the struct that manages the collection API
type serviceImpl struct {
v1.UnimplementedCollectionServiceServer
Expand Down Expand Up @@ -83,3 +94,64 @@ func (s *serviceImpl) getCollection(ctx context.Context, id string) (*v1.GetColl
Deployments: nil,
}, nil
}

// 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("Resource collections is not enabled")
}

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

err = s.datastore.AddCollection(ctx, collection)
if err != nil {
return nil, err
}

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

func collectionRequestToCollection(ctx context.Context, request collectionRequest, isCreate bool, getID func() string) (*storage.ResourceCollection, error) {
if request.GetName() == "" {
return nil, errors.Wrap(errox.InvalidArgs, "Collection name should not be empty")
}

slimUser := utils.UserFromContext(ctx)
if slimUser == nil {
return nil, errors.New("Could not determine user identity from provided context")
}

if len(request.GetResourceSelectors()) == 0 {
return nil, errors.Wrap(errox.InvalidArgs, "No resource selectors were provided")
}

timeNow := protoconv.ConvertTimeToTimestamp(time.Now())

collection := &storage.ResourceCollection{
Name: request.GetName(),
Description: request.GetDescription(),
LastUpdated: timeNow,
UpdatedBy: slimUser,
ResourceSelectors: request.GetResourceSelectors(),
Copy link
Copy Markdown
Contributor

@md2119 md2119 Oct 28, 2022

Choose a reason for hiding this comment

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

valid resource selectors are non-empty

Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Oct 31, 2022

Choose a reason for hiding this comment

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

Added an error on empty resource selectors.

Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Nov 9, 2022

Choose a reason for hiding this comment

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

@keyallis

}

if isCreate {
collection.CreatedBy = slimUser
collection.CreatedAt = timeNow
} else if getID != nil {
collection.Id = getID()
}
Comment on lines +144 to +146
Copy link
Copy Markdown
Contributor

@md2119 md2119 Oct 31, 2022

Choose a reason for hiding this comment

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

implying getID wrt isCreate is confusing. Strongly recommend dropping this check and explicitly basing the condition on create and update workflow.

Copy link
Copy Markdown
Contributor Author

@charmik-redhat charmik-redhat Nov 1, 2022

Choose a reason for hiding this comment

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

Ah, I merged this PR before noticing the comment. I will add it in my Delete collections service PR. Should I change to something like below?

if isCreate {...}
else {
    if getID == nil {
        return nil, err
    }
    collection.Id = getID()
}


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 collection, nil
}
101 changes: 94 additions & 7 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 @@ -11,6 +11,10 @@ import (
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/buildinfo/testbuildinfo"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/grpc/authn"
mockIdentity "github.com/stackrox/rox/pkg/grpc/authn/mocks"
"github.com/stackrox/rox/pkg/sac"
"github.com/stackrox/rox/pkg/search"
"github.com/stackrox/rox/pkg/version/testutils"
"github.com/stretchr/testify/suite"
)
Expand All @@ -23,13 +27,15 @@ type CollectionServiceTestSuite struct {
suite.Suite
mockCtrl *gomock.Controller

dataStore *datastoreMocks.MockDataStore
dataStore *datastoreMocks.MockDataStore
collectionService Service
}

func (suite *CollectionServiceTestSuite) SetupSuite() {
suite.mockCtrl = gomock.NewController(suite.T())
suite.dataStore = datastoreMocks.NewMockDataStore(suite.mockCtrl)
suite.T().Setenv(features.ObjectCollections.EnvVar(), "true")
suite.collectionService = New(suite.dataStore)

testbuildinfo.SetForTest(suite.T())
testutils.SetExampleVersion(suite.T())
Expand All @@ -56,30 +62,111 @@ func (suite *CollectionServiceTestSuite) TestGetCollection() {

// successful get
suite.dataStore.EXPECT().Get(gomock.Any(), request.Id).Times(1).Return(collection, true, nil)
collectionService := New(suite.dataStore)

expected := &v1.GetCollectionResponse{
Collection: collection,
Deployments: nil,
}

result, err := collectionService.GetCollection(context.Background(), request)
result, err := suite.collectionService.GetCollection(context.Background(), request)
suite.NoError(err)
suite.Equal(expected, result)

// collection not present
suite.dataStore.EXPECT().Get(gomock.Any(), request.Id).Times(1).Return(nil, false, nil)
collectionService = New(suite.dataStore)

result, err = collectionService.GetCollection(context.Background(), request)
result, err = suite.collectionService.GetCollection(context.Background(), request)
suite.NotNil(err)
suite.Nil(result)

// error
suite.dataStore.EXPECT().Get(gomock.Any(), request.Id).Times(1).Return(nil, false, errors.New("test error"))
collectionService = New(suite.dataStore)

result, err = collectionService.GetCollection(context.Background(), request)
result, err = suite.collectionService.GetCollection(context.Background(), request)
suite.NotNil(err)
suite.Nil(result)
}

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

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

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

// test error on empty/nil resource selectors
request = &v1.CreateCollectionRequest{
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.CreateCollection(ctx, request)
suite.NotNil(err)
suite.Nil(resp)

// test successful collection creation
request = &v1.CreateCollectionRequest{
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().AddCollection(gomock.Any(), gomock.Any()).Times(1).Return(nil)
resp, err = suite.collectionService.CreateCollection(ctx, request)
suite.NoError(err)
suite.NotNil(resp.GetCollection())
suite.Equal(request.Name, resp.GetCollection().GetName())
suite.Equal(request.GetDescription(), resp.GetCollection().GetDescription())
suite.Equal(request.GetResourceSelectors(), resp.GetCollection().GetResourceSelectors())
suite.NotNil(resp.GetCollection().GetEmbeddedCollections())
suite.Equal(request.GetEmbeddedCollectionIds(), suite.embeddedCollectionToIds(resp.GetCollection().GetEmbeddedCollections()))
}

func (suite *CollectionServiceTestSuite) embeddedCollectionToIds(embeddedCollections []*storage.ResourceCollection_EmbeddedResourceCollection) []string {
if len(embeddedCollections) == 0 {
return nil
}
ids := make([]string, 0, len(embeddedCollections))
for _, c := range embeddedCollections {
ids = append(ids, c.GetId())
}
return ids
}
Loading