Skip to content

Commit

Permalink
Surface selector check errors to handlers
Browse files Browse the repository at this point in the history 
This is a minimal patch to surface selector errors through profile
handlers to clients. It does not expose the structured data (e.g. which
line and which column did the error occur at). I started working on
that, but realized that that's a bigger endevor that ought to be
implemented separately.

Fixes: #3987
  • Loading branch information
@jhrozek
jhrozek committed Jul 27, 2024
1 parent 95535e5 commit 77cc1ef
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 24 deletions.
99 changes: 96 additions & 3 deletions internal/controlplane/handlers_profile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/db/embedded"
"github.com/stacklok/minder/internal/engine/engcontext"
"github.com/stacklok/minder/internal/engine/selectors"
stubeventer "github.com/stacklok/minder/internal/events/stubs"
"github.com/stacklok/minder/internal/profiles"
"github.com/stacklok/minder/internal/providers"
Expand Down Expand Up @@ -226,6 +227,96 @@ func TestCreateProfile(t *testing.T) {
},
},
},
{
name: "Selector with wrong entity",
profile: &minderv1.CreateProfileRequest{
Profile: &minderv1.Profile{
Name: "test_selectors_bad_entity",
Alert: proto.String("off"),
Remediate: proto.String("off"),
Repository: []*minderv1.Profile_Rule{{
Type: "rule_type_1",
Def: &structpb.Struct{},
}},
Selection: []*minderv1.Profile_Selector{
{
Entity: "no_such_entity",
Selector: "repository.name != stacklok/demo-repo-go",
Description: "Exclude stacklok/demo-repo-go",
},
},
},
},
wantErr: `unsupported entity type: invalid entity type no_such_entity: unsupported entity type`,
},
{
name: "Selector with valid but unsupported entity",
profile: &minderv1.CreateProfileRequest{
Profile: &minderv1.Profile{
Name: "test_selectors_bad_entity",
Alert: proto.String("off"),
Remediate: proto.String("off"),
Repository: []*minderv1.Profile_Rule{{
Type: "rule_type_1",
Def: &structpb.Struct{},
}},
Selection: []*minderv1.Profile_Selector{
{
// this entity is valid in the sense that it converts to an entity type but
// the current selelectors implementation does not support it
Entity: "build_environment",
Selector: "repository.name != stacklok/demo-repo-go",
Description: "Exclude stacklok/demo-repo-go",
},
},
},
},
wantErr: `unsupported entity type: no environment for entity ENTITY_BUILD_ENVIRONMENTS: unsupported entity type`,
},
{
name: "Selector does not parse",
profile: &minderv1.CreateProfileRequest{
Profile: &minderv1.Profile{
Name: "test_selectors_syntax_error",
Alert: proto.String("off"),
Remediate: proto.String("off"),
Repository: []*minderv1.Profile_Rule{{
Type: "rule_type_1",
Def: &structpb.Struct{},
}},
Selection: []*minderv1.Profile_Selector{
{
Entity: "repository",
Selector: "repository.name != ",
Description: "Exclude stacklok/demo-repo-go",
},
},
},
},
wantErr: `selector failed to parse: Syntax error: mismatched input '<EOF>' expecting {'[', '{', '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, NUM_UINT, STRING, BYTES, IDENTIFIER}`,
},
{
name: "Selector uses wrong attribute",
profile: &minderv1.CreateProfileRequest{
Profile: &minderv1.Profile{
Name: "test_selectors_attr_error",
Alert: proto.String("off"),
Remediate: proto.String("off"),
Repository: []*minderv1.Profile_Rule{{
Type: "rule_type_1",
Def: &structpb.Struct{},
}},
Selection: []*minderv1.Profile_Selector{
{
Entity: "repository",
Selector: "repository.foo != 'stacklok/demo-repo-go'",
Description: "Exclude stacklok/demo-repo-go",
},
},
},
},
wantErr: `selector is invalid: undefined field 'foo'`,
},
}

for _, tc := range tests {
Expand All @@ -246,11 +337,12 @@ func TestCreateProfile(t *testing.T) {
ctx := engcontext.WithEntityContext(context.Background(), &engcontext.EntityContext{
Project: engcontext.Project{ID: dbproj.ID},
})

evts := &stubeventer.StubEventer{}
s := &Server{
store: dbStore,
// Do not replace this with a mock - these tests are used to test ProfileService as well
profiles: profiles.NewProfileService(evts),
profiles: profiles.NewProfileService(evts, selectors.NewEnv()),
providerStore: providers.NewProviderStore(dbStore),
evt: evts,
}
Expand Down Expand Up @@ -965,7 +1057,7 @@ func TestPatchProfile(t *testing.T) {
s := &Server{
store: dbStore,
// Do not replace this with a mock - these tests are used to test ProfileService as well
profiles: profiles.NewProfileService(evts),
profiles: profiles.NewProfileService(evts, selectors.NewEnv()),
providerStore: providers.NewProviderStore(dbStore),
evt: evts,
}
Expand Down Expand Up @@ -1069,11 +1161,12 @@ func TestPatchManagedProfile(t *testing.T) {
ctx = engcontext.WithEntityContext(context.Background(), &engcontext.EntityContext{
Project: engcontext.Project{ID: dbproj.ID},
})

evts := &stubeventer.StubEventer{}
s := &Server{
store: dbStore,
// Do not replace this with a mock - these tests are used to test ProfileService as well
profiles: profiles.NewProfileService(evts),
profiles: profiles.NewProfileService(evts, selectors.NewEnv()),
providerStore: providers.NewProviderStore(dbStore),
evt: evts,
}
Expand Down
59 changes: 40 additions & 19 deletions internal/profiles/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (

"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine/entities"
"github.com/stacklok/minder/internal/engine/selectors"
"github.com/stacklok/minder/internal/events"
"github.com/stacklok/minder/internal/logger"
"github.com/stacklok/minder/internal/marketplaces/namespaces"
Expand Down Expand Up @@ -89,10 +90,13 @@ type profileService struct {
}

// NewProfileService creates an instance of ProfileService
func NewProfileService(publisher events.Publisher) ProfileService {
func NewProfileService(
publisher events.Publisher,
selChecker selectors.SelectionChecker,
) ProfileService {
return &profileService{
publisher: publisher,
validator: &Validator{},
validator: NewValidator(selChecker),
}
}

Expand Down Expand Up @@ -168,7 +172,7 @@ func (p *profileService) CreateProfile(
}
}

if err := createSelectors(ctx, newProfile.ID, qtx, profile.GetSelection()); err != nil {
if err := p.createSelectors(ctx, newProfile.ID, qtx, profile.GetSelection()); err != nil {
return nil, err
}

Expand Down Expand Up @@ -307,7 +311,7 @@ func (p *profileService) UpdateProfile(
return nil, status.Errorf(codes.Internal, "error updating profile: %v", err)
}

if err := updateSelectors(ctx, updatedProfile.ID, qtx, profile.GetSelection()); err != nil {
if err := p.updateSelectors(ctx, updatedProfile.ID, qtx, profile.GetSelection()); err != nil {
return nil, status.Errorf(codes.Internal, "error updating profile: %v", err)
}

Expand Down Expand Up @@ -712,7 +716,38 @@ func upsertRuleInstances(
return updatedIDs, nil
}

func createSelectors(
func (p *profileService) createSelectors(
ctx context.Context,
profID uuid.UUID,
qtx db.Querier,
selection []*minderv1.Profile_Selector,
) error {
if err := p.validator.ValidateSelection(selection); err != nil {
return err
}

return createSelectorDbRecords(ctx, profID, qtx, selection)
}

func (p *profileService) updateSelectors(
ctx context.Context,
profID uuid.UUID,
qtx db.Querier,
selection []*minderv1.Profile_Selector,
) error {
if err := p.validator.ValidateSelection(selection); err != nil {
return err
}

err := qtx.DeleteSelectorsByProfileID(ctx, profID)
if err != nil {
return fmt.Errorf("error deleting selectors: %w", err)
}

return createSelectorDbRecords(ctx, profID, qtx, selection)
}

func createSelectorDbRecords(
ctx context.Context,
profID uuid.UUID,
qtx db.Querier,
Expand All @@ -737,17 +772,3 @@ func createSelectors(

return nil
}

func updateSelectors(
ctx context.Context,
profID uuid.UUID,
qtx db.Querier,
selection []*minderv1.Profile_Selector,
) error {
err := qtx.DeleteSelectorsByProfileID(ctx, profID)
if err != nil {
return fmt.Errorf("error deleting selectors: %w", err)
}

return createSelectors(ctx, profID, qtx, selection)
}
55 changes: 54 additions & 1 deletion internal/profiles/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,21 @@ import (
"k8s.io/apimachinery/pkg/util/sets"

"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine/selectors"
"github.com/stacklok/minder/internal/ruletypes"
"github.com/stacklok/minder/internal/util"
minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
)

// Validator encapsulates the logic for validating profiles
type Validator struct{}
type Validator struct {
selBld selectors.SelectionChecker
}

// NewValidator creates a new profile validator
func NewValidator(selBld selectors.SelectionChecker) *Validator {
return &Validator{selBld: selBld}
}

// ValidateAndExtractRules validates a profile to ensure it is well-formed
// it also returns information about the rules in the profile
Expand Down Expand Up @@ -323,3 +331,48 @@ func validateEntities(

return nil
}

// ValidateSelection validates the selectors in a profile
func (v *Validator) ValidateSelection(
selection []*minderv1.Profile_Selector,
) error {
for _, sel := range selection {
if err := v.selBld.CheckSelector(sel); err != nil {
return handleSelectorError(err)
}
}

return nil
}

func handleSelectorError(err error) error {
if errors.Is(err, selectors.ErrUnsupported) {
return util.UserVisibleError(codes.InvalidArgument, "unsupported entity type: %v", err)
} else if errors.Is(err, selectors.ErrSelectorCheck) {
return handleErrSelectorCheck(err)
}

// fallback, just return the error as-is
return err
}

func handleErrSelectorCheck(err error) error {
var selParseErr *selectors.ParseError
var selCheckErr *selectors.CheckError

if errors.As(err, &selParseErr) {
msg := "No error message found"
if len(selParseErr.Errors) > 0 {
msg = selParseErr.Errors[0].Msg
}
return util.UserVisibleError(codes.InvalidArgument, "selector failed to parse: %s", msg)
} else if errors.As(err, &selCheckErr) {
msg := "No error message found"
if len(selCheckErr.Errors) > 0 {
msg = selCheckErr.Errors[0].Msg
}
return util.UserVisibleError(codes.InvalidArgument, "selector is invalid: %s", msg)
}

return err
}
3 changes: 2 additions & 1 deletion internal/service/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ func AllInOneServerService(

historySvc := history.NewEvaluationHistoryService()
inviteSvc := invites.NewInviteService()
profileSvc := profiles.NewProfileService(evt)
selChecker := selectors.NewEnv()
profileSvc := profiles.NewProfileService(evt, selChecker)
ruleSvc := ruletypes.NewRuleTypeService()
roleScv := roles.NewRoleService()
marketplace, err := marketplaces.NewMarketplaceFromServiceConfig(cfg.Marketplace, profileSvc, ruleSvc)
Expand Down

0 comments on commit 77cc1ef

Please sign in to comment.