From 41e48333e3c329cc02a6eb688bf0192dcbd77493 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 12 Jul 2024 14:00:30 +0100 Subject: [PATCH] Some prep work for further changes to the executor (#3874) These changes will simplify some upcoming PRs. They include: 1) Moving the rule type engine into its own sub package. 2) Removing the rule type PB struct from the evaluation parameters. The alerting code currently accesses the rule type information by multiple paths (some of it is passed to the constructor for the alert, and other bits from the evaluation parameters). By picking a single way to pass the rule type around, changes for caching are simplified. --- cmd/dev/app/rule_type/rttst.go | 8 ++++---- internal/engine/actions/alert/alert.go | 2 +- .../security_advisory/security_advisory.go | 15 ++++++++------- internal/engine/executor.go | 7 ++++--- internal/engine/interfaces/interface.go | 18 ++++++++++++------ .../engine.go} | 3 ++- internal/logger/telemetry_store.go | 7 +------ internal/logger/telemetry_store_test.go | 13 ++++--------- internal/profiles/rule_validator_test.go | 4 ++-- 9 files changed, 38 insertions(+), 39 deletions(-) rename internal/engine/{rule_type_engine.go => rtengine/engine.go} (98%) diff --git a/cmd/dev/app/rule_type/rttst.go b/cmd/dev/app/rule_type/rttst.go index 129bb8ab6e..f9d74ac67f 100644 --- a/cmd/dev/app/rule_type/rttst.go +++ b/cmd/dev/app/rule_type/rttst.go @@ -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/rtengine" "github.com/stacklok/minder/internal/logger" "github.com/stacklok/minder/internal/profiles" "github.com/stacklok/minder/internal/providers/credentials" @@ -159,7 +159,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error { off := "off" profile.Alert = &off - rules, err := engine.GetRulesFromProfileOfType(profile, ruletype) + rules, err := rtengine.GetRulesFromProfileOfType(profile, ruletype) if err != nil { return fmt.Errorf("error getting relevant fragment: %w", err) } @@ -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 := rtengine.NewRuleTypeEngine(ctx, ruletype, prov) if err != nil { return fmt.Errorf("cannot create rule type engine: %w", err) } @@ -198,7 +198,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error { func runEvaluationForRules( cmd *cobra.Command, - eng *engine.RuleTypeEngine, + eng *rtengine.RuleTypeEngine, inf *entities.EntityInfoWrapper, remediateStatus db.NullRemediationStatusTypes, remMetadata pqtype.NullRawMessage, diff --git a/internal/engine/actions/alert/alert.go b/internal/engine/actions/alert/alert.go index 8c42372858..93029d165d 100644 --- a/internal/engine/actions/alert/alert.go +++ b/internal/engine/actions/alert/alert.go @@ -56,7 +56,7 @@ 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()) diff --git a/internal/engine/actions/alert/security_advisory/security_advisory.go b/internal/engine/actions/alert/security_advisory/security_advisory.go index 5007ebf4e3..e0a7a0a48e 100644 --- a/internal/engine/actions/alert/security_advisory/security_advisory.go +++ b/internal/engine/actions/alert/security_advisory/security_advisory.go @@ -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 @@ -126,6 +126,7 @@ type templateParamsSA struct { RuleRemediation string Name string } + type alertMetadata struct { ID string `json:"ghsa_id,omitempty"` } @@ -133,7 +134,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) { @@ -160,7 +161,7 @@ func NewSecurityAdvisoryAlert( return &Alert{ actionType: actionType, cli: cli, - sev: sev, + ruleType: ruleType, saCfg: saCfg, summaryTmpl: sumT, descriptionTmpl: descT, @@ -349,16 +350,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" @@ -386,7 +387,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" } diff --git a/internal/engine/executor.go b/internal/engine/executor.go index 9e89448ff0..17812b26b2 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -30,6 +30,7 @@ import ( evalerrors "github.com/stacklok/minder/internal/engine/errors" "github.com/stacklok/minder/internal/engine/ingestcache" engif "github.com/stacklok/minder/internal/engine/interfaces" + "github.com/stacklok/minder/internal/engine/rtengine" "github.com/stacklok/minder/internal/history" minderlogger "github.com/stacklok/minder/internal/logger" "github.com/stacklok/minder/internal/profiles" @@ -190,7 +191,7 @@ func (e *executor) getEvaluator( rule *pb.Profile_Rule, hierarchy []uuid.UUID, ingestCache ingestcache.Cache, -) (*engif.EvalStatusParams, *RuleTypeEngine, *actions.RuleActionsEngine, error) { +) (*engif.EvalStatusParams, *rtengine.RuleTypeEngine, *actions.RuleActionsEngine, error) { // Create eval status params params, err := e.createEvalStatusParams(ctx, inf, profile, rule) if err != nil { @@ -220,10 +221,10 @@ func (e *executor) getEvaluator( return nil, nil, nil, fmt.Errorf("error parsing rule type ID: %w", err) } params.RuleTypeID = ruleTypeID - params.RuleType = ruleType + params.RuleTypeName = ruleType.Name // Create the rule type engine - rte, err := NewRuleTypeEngine(ctx, ruleType, provider) + rte, err := rtengine.NewRuleTypeEngine(ctx, ruleType, provider) if err != nil { return nil, nil, nil, fmt.Errorf("error creating rule type engine: %w", err) } diff --git a/internal/engine/interfaces/interface.go b/internal/engine/interfaces/interface.go index c1bfdc1edc..563c6db884 100644 --- a/internal/engine/interfaces/interface.go +++ b/internal/engine/interfaces/interface.go @@ -131,7 +131,7 @@ type EvalStatusParams struct { Result *Result Profile *pb.Profile Rule *pb.Profile_Rule - RuleType *pb.RuleType + RuleTypeName string ProfileID uuid.UUID RepoID uuid.NullUUID ArtifactID uuid.NullUUID @@ -208,14 +208,19 @@ func (e *EvalStatusParams) GetRule() *pb.Profile_Rule { return e.Rule } +// GetRuleTypeID returns the rule type ID +func (e *EvalStatusParams) GetRuleTypeID() uuid.UUID { + return e.RuleTypeID +} + // GetEvalStatusFromDb returns the evaluation status from the database func (e *EvalStatusParams) GetEvalStatusFromDb() *db.ListRuleEvaluationsByProfileIdRow { return e.EvalStatusFromDb } -// GetRuleType returns the rule type -func (e *EvalStatusParams) GetRuleType() *pb.RuleType { - return e.RuleType +// GetRuleTypeName returns the rule type name +func (e *EvalStatusParams) GetRuleTypeName() string { + return e.RuleTypeName } // GetProfile returns the profile @@ -240,7 +245,7 @@ func (e *EvalStatusParams) DecorateLogger(l zerolog.Logger) zerolog.Logger { Str("profile_id", e.ProfileID.String()). Str("rule_type", e.GetRule().GetType()). Str("rule_name", e.GetRule().GetName()). - Str("rule_type_id", e.GetRuleType().GetId()). + Str("rule_type_id", e.GetRuleTypeID().String()). Logger() if e.RepoID.Valid { outl = outl.With().Str("repository_id", e.RepoID.UUID.String()).Logger() @@ -275,6 +280,7 @@ type ActionsParams interface { GetActionsErr() evalerrors.ActionsError GetEvalErr() error GetEvalStatusFromDb() *db.ListRuleEvaluationsByProfileIdRow - GetRuleType() *pb.RuleType + GetRuleTypeName() string GetProfile() *pb.Profile + GetRuleTypeID() uuid.UUID } diff --git a/internal/engine/rule_type_engine.go b/internal/engine/rtengine/engine.go similarity index 98% rename from internal/engine/rule_type_engine.go rename to internal/engine/rtengine/engine.go index 60036aa3d7..647d2b5464 100644 --- a/internal/engine/rule_type_engine.go +++ b/internal/engine/rtengine/engine.go @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -package engine +// Package rtengine contains the rule type engine +package rtengine import ( "context" diff --git a/internal/logger/telemetry_store.go b/internal/logger/telemetry_store.go index e38eaf1f42..a910efedee 100644 --- a/internal/logger/telemetry_store.go +++ b/internal/logger/telemetry_store.go @@ -110,11 +110,6 @@ func (ts *TelemetryStore) AddRuleEval( return } - // Get rule type ID - ruleTypeID, err := uuid.Parse(evalInfo.GetRuleType().GetId()) - if err != nil { - return - } // Get profile ID profileID, err := uuid.Parse(evalInfo.GetProfile().GetId()) if err != nil { @@ -122,7 +117,7 @@ func (ts *TelemetryStore) AddRuleEval( } red := RuleEvalData{ - RuleType: RuleType{Name: evalInfo.GetRuleType().GetName(), ID: ruleTypeID}, + RuleType: RuleType{Name: evalInfo.GetRuleTypeName(), ID: evalInfo.GetRuleTypeID()}, Profile: Profile{Name: evalInfo.GetProfile().GetName(), ID: profileID}, EvalResult: errors.EvalErrorAsString(evalInfo.GetEvalErr()), Actions: map[interfaces.ActionType]ActionEvalData{ diff --git a/internal/logger/telemetry_store_test.go b/internal/logger/telemetry_store_test.go index 1af5e2e296..37a3a2151e 100644 --- a/internal/logger/telemetry_store_test.go +++ b/internal/logger/telemetry_store_test.go @@ -53,10 +53,7 @@ func TestTelemetryStore_Record(t *testing.T) { Name: "artifact_profile", Id: &testUUIDString, } - ep.RuleType = &minderv1.RuleType{ - Name: "artifact_signature", - Id: &testUUIDString, - } + ep.RuleTypeName = "artifact_signature" ep.SetEvalErr(enginerr.NewErrEvaluationFailed("evaluation failure reason")) ep.SetActionsOnOff(map[engif.ActionType]engif.ActionOpt{ alert.ActionType: engif.ActionOptOn, @@ -83,10 +80,8 @@ func TestTelemetryStore_Record(t *testing.T) { Name: "artifact_profile", Id: &testUUIDString, } - ep.RuleType = &minderv1.RuleType{ - Name: "artifact_signature", - Id: &testUUIDString, - } + ep.RuleTypeName = "artifact_signature" + ep.RuleTypeID = testUUID ep.SetEvalErr(enginerr.NewErrEvaluationFailed("evaluation failure reason")) ep.SetActionsOnOff(map[engif.ActionType]engif.ActionOpt{ alert.ActionType: engif.ActionOptOff, @@ -139,7 +134,7 @@ func TestTelemetryStore_Record(t *testing.T) { recordFunc: func(_ context.Context, _ engif.ActionsParams) { }, expected: `{"telemetry": "true"}`, - notPresent: []string{"project", "rules", "login_sha", "repository", "provider", "profile", "ruletype", "artifact", "pr"}, + notPresent: []string{"project", "rules", "login_sha", "repository", "provider", "profile", "ruletypes", "artifact", "pr"}, }} count := len(cases) diff --git a/internal/profiles/rule_validator_test.go b/internal/profiles/rule_validator_test.go index 099e919e43..d22969e40a 100644 --- a/internal/profiles/rule_validator_test.go +++ b/internal/profiles/rule_validator_test.go @@ -22,7 +22,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/stacklok/minder/internal/engine" + "github.com/stacklok/minder/internal/engine/rtengine" "github.com/stacklok/minder/internal/profiles" minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) @@ -67,7 +67,7 @@ func TestExampleRulesAreValidatedCorrectly(t *testing.T) { rval, err := profiles.NewRuleValidator(rt) require.NoError(t, err, "failed to create rule validator for rule type %s", path) - rules, err := engine.GetRulesFromProfileOfType(pol, rt) + rules, err := rtengine.GetRulesFromProfileOfType(pol, rt) require.NoError(t, err, "failed to get rules from profile for rule type %s", path) t.Log("validating rules")