Skip to content

Commit

Permalink
Cache RuleTypeEngine instances in Executor
Browse files Browse the repository at this point in the history
This simplifies an upcoming PR, and also addresses a comment in the code
about avoiding continuously re-querying for rule types. This PR changes
the executor to load all relevant rule types before evaluation and
construct RuleTypeEngine instances for each. These are then stored in a
cache and used during rule evaluation.

Certain aspects of this change (specifically, the need to query for the
rule type ID) are a bit clunky, but will be simplified in an upcoming
PR.
  • Loading branch information
dmjb committed Jun 28, 2024
1 parent 83ae180 commit b1fc1a4
Show file tree
Hide file tree
Showing 17 changed files with 307 additions and 87 deletions.
12 changes: 6 additions & 6 deletions cmd/dev/app/rule_type/rttst.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ import (

serverconfig "github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine"
"github.com/stacklok/minder/internal/engine/actions"
"github.com/stacklok/minder/internal/engine/entities"
"github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/eval/rego"
engif "github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/engine/ruleengine"
"github.com/stacklok/minder/internal/logger"
"github.com/stacklok/minder/internal/profiles"
"github.com/stacklok/minder/internal/providers/credentials"
Expand Down Expand Up @@ -159,7 +159,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
off := "off"
profile.Alert = &off

rules, err := engine.GetRulesFromProfileOfType(profile, ruletype)
ruleInstances, err := ruleengine.GetRulesFromProfileOfType(profile, ruletype)
if err != nil {
return fmt.Errorf("error getting relevant fragment: %w", err)
}
Expand All @@ -172,7 +172,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {

// TODO: use cobra context here
ctx := context.Background()
eng, err := engine.NewRuleTypeEngine(ctx, ruletype, prov)
eng, err := ruleengine.NewRuleTypeEngine(ctx, ruletype, prov)
if err != nil {
return fmt.Errorf("cannot create rule type engine: %w", err)
}
Expand All @@ -189,16 +189,16 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
return fmt.Errorf("error creating rule type engine: %w", err)
}

if len(rules) == 0 {
if len(ruleInstances) == 0 {
return fmt.Errorf("no rules found with type %s", ruletype.Name)
}

return runEvaluationForRules(cmd, eng, inf, remediateStatus, remMetadata, rules, actionEngine)
return runEvaluationForRules(cmd, eng, inf, remediateStatus, remMetadata, ruleInstances, actionEngine)
}

func runEvaluationForRules(
cmd *cobra.Command,
eng *engine.RuleTypeEngine,
eng *ruleengine.RuleTypeEngine,
inf *entities.EntityInfoWrapper,
remediateStatus db.NullRemediationStatusTypes,
remMetadata pqtype.NullRawMessage,
Expand Down
30 changes: 30 additions & 0 deletions database/mock/store.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion database/query/rule_instances.sql
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,12 @@ AND NOT id = ANY(sqlc.arg(updated_ids)::UUID[]);
SELECT id FROM rule_instances
WHERE profile_id = $1
AND entity_type = $2
AND name = $3;
AND name = $3;

-- intended as a temporary transition query
-- this will be removed once rule_instances is used consistently in the engine
-- name: GetRuleTypeIDByRuleNameEntityProfile :one
SELECT rule_type_id FROM rule_instances
WHERE name = $1
AND entity_type = $2
AND profile_id = $3;
6 changes: 6 additions & 0 deletions database/query/rule_types.sql
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,9 @@ UPDATE rule_type
SET description = $2, definition = sqlc.arg(definition)::jsonb, severity_value = sqlc.arg(severity_value), display_name = sqlc.arg(display_name)
WHERE id = $1
RETURNING *;

-- name: GetRuleTypesByEntityInHierarchy :many
SELECT rt.* FROM rule_type AS rt
JOIN rule_instances AS ri ON ri.rule_type_id = rt.id
WHERE ri.entity_type = $1
AND ri.project_id = ANY(sqlc.arg(projects)::uuid[]);
4 changes: 4 additions & 0 deletions internal/db/querier.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions internal/db/rule_instances.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 49 additions & 0 deletions internal/db/rule_types.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion internal/engine/actions/alert/alert.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,12 @@ func NewRuleAlert(
Msg("provider is not a GitHub provider. Silently skipping alerts.")
return noop.NewNoopAlert(ActionType)
}
return security_advisory.NewSecurityAdvisoryAlert(ActionType, ruletype.GetSeverity(), alertCfg.GetSecurityAdvisory(), client)
return security_advisory.NewSecurityAdvisoryAlert(
ActionType,
ruletype,
alertCfg.GetSecurityAdvisory(),
client,
)
}

return nil, fmt.Errorf("unknown alert type: %s", alertCfg.GetType())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ If you have any questions or believe that this evaluation is incorrect, please d
type Alert struct {
actionType interfaces.ActionType
cli provifv1.GitHub
sev *pb.Severity
ruleType *pb.RuleType
saCfg *pb.RuleType_Definition_Alert_AlertTypeSA
summaryTmpl *htmltemplate.Template
descriptionTmpl *htmltemplate.Template
Expand Down Expand Up @@ -133,7 +133,7 @@ type alertMetadata struct {
// NewSecurityAdvisoryAlert creates a new security-advisory alert action
func NewSecurityAdvisoryAlert(
actionType interfaces.ActionType,
sev *pb.Severity,
ruleType *pb.RuleType,
saCfg *pb.RuleType_Definition_Alert_AlertTypeSA,
cli provifv1.GitHub,
) (*Alert, error) {
Expand All @@ -160,7 +160,7 @@ func NewSecurityAdvisoryAlert(
return &Alert{
actionType: actionType,
cli: cli,
sev: sev,
ruleType: ruleType,
saCfg: saCfg,
summaryTmpl: sumT,
descriptionTmpl: descT,
Expand Down Expand Up @@ -349,16 +349,16 @@ func (alert *Alert) getParamsForSecurityAdvisory(
// Get the severity
result.Template.Severity = alert.getSeverityString()
// Get the guidance
result.Template.Guidance = params.GetRuleType().Guidance
result.Template.Guidance = alert.ruleType.Guidance
// Get the rule type name
result.Template.Rule = params.GetRuleType().Name
result.Template.Rule = alert.ruleType.Name
// Get the profile name
result.Template.Profile = params.GetProfile().Name
// Get the rule name
result.Template.Name = params.GetRule().Name

// Check if remediation is available for the rule type
if params.GetRuleType().Def.Remediate != nil {
if alert.ruleType.Def.Remediate != nil {
result.Template.RuleRemediation = "already available"
} else {
result.Template.RuleRemediation = "not available yet"
Expand Down Expand Up @@ -386,7 +386,7 @@ func (alert *Alert) getParamsForSecurityAdvisory(

func (alert *Alert) getSeverityString() string {
if alert.saCfg.Severity == "" {
ruleSev := alert.sev.GetValue().Enum().AsString()
ruleSev := alert.ruleType.Severity.GetValue().Enum().AsString()
if ruleSev == "info" || ruleSev == "unknown" {
return "low"
}
Expand Down
Loading

0 comments on commit b1fc1a4

Please sign in to comment.