From be7bbf378ffb83be956ebc1314dde838e5f2ad0e Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Fri, 29 Mar 2024 11:22:46 +0200 Subject: [PATCH] Revert "Add a pending remediation status and support tracking opened PRs" (#2862) Revert "Add a pending remediation status and support tracking opened PRs (#2833)" This reverts commit f330d61d382855dfe80a66ed4dde9c239c751560. --- cmd/cli/app/profile/table_render.go | 4 +- .../000041_remediate_metadata.down.sql | 17 - .../000041_remediate_metadata.up.sql | 17 - database/query/profile_status.sql | 12 +- internal/db/models.go | 2 - internal/db/profile_status.sql.go | 22 +- internal/db/profiles_test.go | 20 +- internal/engine/actions/actions.go | 109 ++---- .../security_advisory/security_advisory.go | 43 +-- .../remediate/pull_request/pull_request.go | 334 ++++++++---------- .../pull_request/pull_request_test.go | 175 ++++----- internal/engine/errors/errors.go | 50 +-- internal/engine/eval_status.go | 4 +- internal/engine/executor.go | 32 +- internal/engine/executor_test.go | 1 - internal/engine/rule_type_engine.go | 18 +- internal/providers/github/common.go | 11 - internal/providers/github/mock/github.go | 15 - pkg/providers/v1/providers.go | 1 - 19 files changed, 313 insertions(+), 574 deletions(-) delete mode 100644 database/migrations/000041_remediate_metadata.down.sql delete mode 100644 database/migrations/000041_remediate_metadata.up.sql diff --git a/cmd/cli/app/profile/table_render.go b/cmd/cli/app/profile/table_render.go index 99aa37b3d8..fa72afa948 100644 --- a/cmd/cli/app/profile/table_render.go +++ b/cmd/cli/app/profile/table_render.go @@ -189,7 +189,7 @@ func getEvalStatusText(status string) string { // Gets a friendly status text with an emoji func getRemediationStatusText(status string) string { - // remediation statuses can be 'success', 'failure', 'error', 'skipped', 'pending' or 'not supported' + // remediation statuses can be 'success', 'failure', 'error', 'skipped', 'not supported' switch strings.ToLower(status) { case successStatus: return "Success" @@ -199,8 +199,6 @@ func getRemediationStatusText(status string) string { return "Error" case skippedStatus: return "Skipped" // visually empty as we didn't have to remediate - case pendingStatus: - return "Pending" case notAvailableStatus: return "Not Available" default: diff --git a/database/migrations/000041_remediate_metadata.down.sql b/database/migrations/000041_remediate_metadata.down.sql deleted file mode 100644 index ba30fe753d..0000000000 --- a/database/migrations/000041_remediate_metadata.down.sql +++ /dev/null @@ -1,17 +0,0 @@ --- Copyright 2024 Stacklok, Inc --- --- Licensed under the Apache License, Version 2.0 (the "License"); --- you may not use this file except in compliance with the License. --- You may obtain a copy of the License at --- --- http://www.apache.org/licenses/LICENSE-2.0 --- --- Unless required by applicable law or agreed to in writing, software --- distributed under the License is distributed on an "AS IS" BASIS, --- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. --- See the License for the specific language governing permissions and --- limitations under the License. - -ALTER TABLE rule_details_remediate DROP COLUMN metadata; - --- It is not possible to drop added values from enums, ref. `pending` for remediation_status_types \ No newline at end of file diff --git a/database/migrations/000041_remediate_metadata.up.sql b/database/migrations/000041_remediate_metadata.up.sql deleted file mode 100644 index f6bf07826d..0000000000 --- a/database/migrations/000041_remediate_metadata.up.sql +++ /dev/null @@ -1,17 +0,0 @@ --- Copyright 2024 Stacklok, Inc --- --- Licensed under the Apache License, Version 2.0 (the "License"); --- you may not use this file except in compliance with the License. --- You may obtain a copy of the License at --- --- http://www.apache.org/licenses/LICENSE-2.0 --- --- Unless required by applicable law or agreed to in writing, software --- distributed under the License is distributed on an "AS IS" BASIS, --- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. --- See the License for the specific language governing permissions and --- limitations under the License. - -ALTER TABLE rule_details_remediate ADD COLUMN metadata JSONB NOT NULL DEFAULT '{}'; - -ALTER TYPE remediation_status_types add value 'pending'; \ No newline at end of file diff --git a/database/query/profile_status.sql b/database/query/profile_status.sql index 108709a352..2985202e47 100644 --- a/database/query/profile_status.sql +++ b/database/query/profile_status.sql @@ -28,15 +28,13 @@ INSERT INTO rule_details_remediate ( rule_eval_id, status, details, - metadata, last_updated ) -VALUES ($1, $2, $3, sqlc.arg(metadata)::jsonb, NOW()) +VALUES ($1, $2, $3, NOW()) ON CONFLICT(rule_eval_id) DO UPDATE SET status = $2, details = $3, - metadata = sqlc.arg(metadata)::jsonb, last_updated = NOW() WHERE rule_details_remediate.rule_eval_id = $1 RETURNING id; @@ -52,9 +50,9 @@ INSERT INTO rule_details_alert ( VALUES ($1, $2, $3, sqlc.arg(metadata)::jsonb, NOW()) ON CONFLICT(rule_eval_id) DO UPDATE SET - status = $2, - details = $3, - metadata = sqlc.arg(metadata)::jsonb, + status = CASE WHEN $2 != 'skipped' THEN $2 ELSE rule_details_alert.status END, + details = CASE WHEN $2 != 'skipped' THEN $3 ELSE rule_details_alert.details END, + metadata = CASE WHEN $2 != 'skipped' THEN sqlc.arg(metadata)::jsonb ELSE rule_details_alert.metadata END, last_updated = NOW() WHERE rule_details_alert.rule_eval_id = $1 RETURNING id; @@ -98,7 +96,6 @@ WITH rule_eval_id, status AS rem_status, details AS rem_details, - metadata AS rem_metadata, last_updated AS rem_last_updated FROM rule_details_remediate ), @@ -118,7 +115,6 @@ SELECT ed.eval_details, rd.rem_status, rd.rem_details, - rd.rem_metadata, rd.rem_last_updated, ad.alert_status, ad.alert_details, diff --git a/internal/db/models.go b/internal/db/models.go index c1f367d88a..72ff73e9a9 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -330,7 +330,6 @@ const ( RemediationStatusTypesError RemediationStatusTypes = "error" RemediationStatusTypesSkipped RemediationStatusTypes = "skipped" RemediationStatusTypesNotAvailable RemediationStatusTypes = "not_available" - RemediationStatusTypesPending RemediationStatusTypes = "pending" ) func (e *RemediationStatusTypes) Scan(src interface{}) error { @@ -599,7 +598,6 @@ type RuleDetailsRemediate struct { Status RemediationStatusTypes `json:"status"` Details string `json:"details"` LastUpdated time.Time `json:"last_updated"` - Metadata json.RawMessage `json:"metadata"` } type RuleEvaluation struct { diff --git a/internal/db/profile_status.sql.go b/internal/db/profile_status.sql.go index bf0a0cbb93..227feb03a2 100644 --- a/internal/db/profile_status.sql.go +++ b/internal/db/profile_status.sql.go @@ -152,7 +152,6 @@ WITH rule_eval_id, status AS rem_status, details AS rem_details, - metadata AS rem_metadata, last_updated AS rem_last_updated FROM rule_details_remediate ), @@ -172,7 +171,6 @@ SELECT ed.eval_details, rd.rem_status, rd.rem_details, - rd.rem_metadata, rd.rem_last_updated, ad.alert_status, ad.alert_details, @@ -223,7 +221,6 @@ type ListRuleEvaluationsByProfileIdRow struct { EvalDetails sql.NullString `json:"eval_details"` RemStatus NullRemediationStatusTypes `json:"rem_status"` RemDetails sql.NullString `json:"rem_details"` - RemMetadata pqtype.NullRawMessage `json:"rem_metadata"` RemLastUpdated sql.NullTime `json:"rem_last_updated"` AlertStatus NullAlertStatusTypes `json:"alert_status"` AlertDetails sql.NullString `json:"alert_details"` @@ -263,7 +260,6 @@ func (q *Queries) ListRuleEvaluationsByProfileId(ctx context.Context, arg ListRu &i.EvalDetails, &i.RemStatus, &i.RemDetails, - &i.RemMetadata, &i.RemLastUpdated, &i.AlertStatus, &i.AlertDetails, @@ -305,9 +301,9 @@ INSERT INTO rule_details_alert ( VALUES ($1, $2, $3, $4::jsonb, NOW()) ON CONFLICT(rule_eval_id) DO UPDATE SET - status = $2, - details = $3, - metadata = $4::jsonb, + status = CASE WHEN $2 != 'skipped' THEN $2 ELSE rule_details_alert.status END, + details = CASE WHEN $2 != 'skipped' THEN $3 ELSE rule_details_alert.details END, + metadata = CASE WHEN $2 != 'skipped' THEN $4::jsonb ELSE rule_details_alert.metadata END, last_updated = NOW() WHERE rule_details_alert.rule_eval_id = $1 RETURNING id @@ -367,15 +363,13 @@ INSERT INTO rule_details_remediate ( rule_eval_id, status, details, - metadata, last_updated ) -VALUES ($1, $2, $3, $4::jsonb, NOW()) +VALUES ($1, $2, $3, NOW()) ON CONFLICT(rule_eval_id) DO UPDATE SET status = $2, details = $3, - metadata = $4::jsonb, last_updated = NOW() WHERE rule_details_remediate.rule_eval_id = $1 RETURNING id @@ -385,16 +379,10 @@ type UpsertRuleDetailsRemediateParams struct { RuleEvalID uuid.UUID `json:"rule_eval_id"` Status RemediationStatusTypes `json:"status"` Details string `json:"details"` - Metadata json.RawMessage `json:"metadata"` } func (q *Queries) UpsertRuleDetailsRemediate(ctx context.Context, arg UpsertRuleDetailsRemediateParams) (uuid.UUID, error) { - row := q.db.QueryRowContext(ctx, upsertRuleDetailsRemediate, - arg.RuleEvalID, - arg.Status, - arg.Details, - arg.Metadata, - ) + row := q.db.QueryRowContext(ctx, upsertRuleDetailsRemediate, arg.RuleEvalID, arg.Status, arg.Details) var id uuid.UUID err := row.Scan(&id) return id, err diff --git a/internal/db/profiles_test.go b/internal/db/profiles_test.go index 69d2e55fb5..a5dc091031 100644 --- a/internal/db/profiles_test.go +++ b/internal/db/profiles_test.go @@ -9,7 +9,6 @@ import ( "time" "github.com/google/uuid" - "github.com/sqlc-dev/pqtype" "github.com/stretchr/testify/require" "github.com/stacklok/minder/internal/util/rand" @@ -29,10 +28,6 @@ func createRandomProfile(t *testing.T, prov Provider, projectID uuid.UUID, label ActionType: "on", Valid: true, }, - Alert: NullActionType{ - ActionType: "on", - Valid: true, - }, Labels: labels, } @@ -114,7 +109,7 @@ func upsertEvalStatus( func upsertRemediationStatus( t *testing.T, profileID uuid.UUID, repoID uuid.UUID, ruleTypeID uuid.UUID, - remStatus RemediationStatusTypes, details string, metadata json.RawMessage, + remStatus RemediationStatusTypes, details string, ) { t.Helper() @@ -134,7 +129,6 @@ func upsertRemediationStatus( RuleEvalID: id, Status: remStatus, Details: details, - Metadata: metadata, }) require.NoError(t, err) } @@ -651,10 +645,8 @@ func compareRows(t *testing.T, a, b *ListRuleEvaluationsByProfileIdRow) { require.Equal(t, a.EvalDetails, b.EvalDetails) require.Equal(t, a.RemStatus, b.RemStatus) require.Equal(t, a.RemDetails, b.RemDetails) - require.Equal(t, a.RemMetadata, b.RemMetadata) require.Equal(t, a.AlertStatus, b.AlertStatus) require.Equal(t, a.AlertDetails, b.AlertDetails) - require.Equal(t, a.AlertMetadata, b.AlertMetadata) require.Equal(t, a.Entity, b.Entity) } @@ -788,7 +780,7 @@ func TestListRuleEvaluations(t *testing.T) { EvalStatusTypesFailure, "this rule failed") upsertRemediationStatus( t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, - RemediationStatusTypesSuccess, "this rule was remediated", json.RawMessage(`{"pr_number": "56"}`)) + RemediationStatusTypesSuccess, "this rule was remediated") upsertAlertStatus( t, profile.ID, randomEntities.repo.ID, randomEntities.ruleType1.ID, AlertStatusTypesOn, "we alerted about this rule", json.RawMessage(`{"ghsa_id": "GHSA-xxxx-xxxx-xxxx"}`)) @@ -814,10 +806,6 @@ func TestListRuleEvaluations(t *testing.T) { String: "this rule was remediated", Valid: true, }, - RemMetadata: pqtype.NullRawMessage{ - RawMessage: json.RawMessage(`{"pr_number": "56"}`), - Valid: true, - }, AlertStatus: NullAlertStatusTypes{ AlertStatusTypes: AlertStatusTypesOn, Valid: true, @@ -826,10 +814,6 @@ func TestListRuleEvaluations(t *testing.T) { String: "we alerted about this rule", Valid: true, }, - AlertMetadata: pqtype.NullRawMessage{ - RawMessage: json.RawMessage(`{"ghsa_id": "GHSA-xxxx-xxxx-xxxx"}`), - Valid: true, - }, Entity: EntitiesRepository, }, }, diff --git a/internal/engine/actions/actions.go b/internal/engine/actions/actions.go index 558354728b..895ec06d3a 100644 --- a/internal/engine/actions/actions.go +++ b/internal/engine/actions/actions.go @@ -115,7 +115,7 @@ func (rae *RuleActionsEngine) DoActions( cmd := shouldRemediate(params.GetEvalStatusFromDb(), params.GetEvalErr()) // Run remediation result.RemediateMeta, result.RemediateErr = rae.processAction(ctx, remediate.ActionType, cmd, ent, params, - getMeta(params.GetEvalStatusFromDb().RemMetadata)) + nil) } // Try alerting @@ -138,64 +138,22 @@ func (rae *RuleActionsEngine) processAction( params engif.ActionsParams, metadata *json.RawMessage, ) (json.RawMessage, error) { - zerolog.Ctx(ctx).Debug().Str("action", string(actionType)).Str("cmd", string(cmd)).Msg("invoking action") // Get action engine action := rae.actions[actionType] // Return the result of the action + logger := zerolog.Ctx(ctx) + logger.Debug(). + Str("action", string(actionType)). + Str("cmd", string(cmd)). + Msg("invoking action") return action.Do(ctx, cmd, rae.actionsOnOff[actionType], ent, params, metadata) } // shouldRemediate returns the action command for remediation taking into account previous evaluations func shouldRemediate(prevEvalFromDb *db.ListRuleEvaluationsByProfileIdRow, evalErr error) engif.ActionCmd { - // Get current evaluation status - newEval := enginerr.ErrorAsEvalStatus(evalErr) - - // Get previous evaluation status - prevEval := db.EvalStatusTypesPending - if prevEvalFromDb.EvalStatus.Valid { - prevEval = prevEvalFromDb.EvalStatus.EvalStatusTypes - } - - // Get previous Remediation status - prevRemediation := db.RemediationStatusTypesSkipped - if prevEvalFromDb.RemStatus.Valid { - prevRemediation = prevEvalFromDb.RemStatus.RemediationStatusTypes - } - - // Start evaluation scenarios - - // Case 1 - Do nothing if the evaluation status has not changed and remediation did not errored-out - if newEval == prevEval && prevRemediation != db.RemediationStatusTypesError { - return engif.ActionCmdDoNothing - } - - // Proceed with use cases where the evaluation changed - switch newEval { - case db.EvalStatusTypesError: - case db.EvalStatusTypesSuccess: - // Case 2 - Evaluation changed from something else to ERROR -> Remediation should be OFF - // Case 3 - Evaluation changed from something else to PASSING -> Remediation should be OFF - // The Remediation should be OFF (if it wasn't already) - if db.RemediationStatusTypesSkipped != prevRemediation { - return engif.ActionCmdOff - } - // We should do nothing if remediation was already skipped - return engif.ActionCmdDoNothing - case db.EvalStatusTypesFailure: - // Case 4 - Evaluation has changed from something else to FAILED -> Remediation should be ON - // We should do nothing if the Remediation is already pending or successful - if db.RemediationStatusTypesPending == prevRemediation || db.RemediationStatusTypesSuccess == prevRemediation { - return engif.ActionCmdDoNothing - } - // The Remediation should be turned ON (if it wasn't already) - return engif.ActionCmdOn - case db.EvalStatusTypesSkipped: - case db.EvalStatusTypesPending: - return engif.ActionCmdDoNothing - } - - // Default to do nothing - return engif.ActionCmdDoNothing + _ = prevEvalFromDb + _ = evalErr + return engif.ActionCmdOn } // shouldAlert returns the action command for alerting taking into account previous evaluations @@ -237,70 +195,65 @@ func shouldAlert( } // Proceed with use cases where the evaluation changed - switch newEval { - case db.EvalStatusTypesError: - case db.EvalStatusTypesFailure: - // Case 3 - Evaluation changed from something else to ERROR -> Alert should be ON - // Case 4 - Evaluation has changed from something else to FAILED -> Alert should be ON - // The Alert should be on (if it wasn't already) - if db.AlertStatusTypesOn != prevAlert { - return engif.ActionCmdOn - } - // We should do nothing if alert was already turned on - return engif.ActionCmdDoNothing - case db.EvalStatusTypesSuccess: - // Case 5 - Evaluation changed from something else to PASSING -> Alert should be OFF + + // Case 3 - Evaluation changed from something else to PASSING -> Alarm should be OFF + if db.EvalStatusTypesSuccess == newEval { // The Alert should be turned OFF (if it wasn't already) if db.AlertStatusTypesOff != prevAlert { return engif.ActionCmdOff } // We should do nothing if the Alert is already OFF return engif.ActionCmdDoNothing - case db.EvalStatusTypesSkipped: - case db.EvalStatusTypesPending: - return engif.ActionCmdDoNothing } + // Case 4 - Evaluation has changed from something else to FAILED -> Alarm should be ON + if db.EvalStatusTypesFailure == newEval { + // The Alert should be turned ON (if it wasn't already) + if db.AlertStatusTypesOn != prevAlert { + return engif.ActionCmdOn + } + // We should do nothing if the Alert is already ON + return engif.ActionCmdDoNothing + } // Default to do nothing return engif.ActionCmdDoNothing } // isSkippable returns true if the action should be skipped func (rae *RuleActionsEngine) isSkippable(ctx context.Context, actionType engif.ActionType, evalErr error) bool { - var skipAction bool + var skipRemediation bool - logger := zerolog.Ctx(ctx).Info(). - Str("eval_status", string(enginerr.ErrorAsEvalStatus(evalErr))). - Str("action", string(actionType)) + logger := zerolog.Ctx(ctx) // Get the profile option set for this action type actionOnOff, ok := rae.actionsOnOff[actionType] if !ok { // If the action is not found, definitely skip it - logger.Msg("action type not found, skipping") return true } // Check the action option switch actionOnOff { case engif.ActionOptOff: // Action is off, skip - logger.Msg("action is off, skipping") return true case engif.ActionOptUnknown: // Action is unknown, skip - logger.Msg("unknown action option, skipping") + logger.Info().Msg("unknown action option, check your profile definition") return true case engif.ActionOptDryRun, engif.ActionOptOn: // Action is on or dry-run, do not skip yet. Check the evaluation error - skipAction = + skipRemediation = // rule evaluation was skipped, skip action too errors.Is(evalErr, enginerr.ErrEvaluationSkipped) || // rule evaluation was skipped silently, skip action - errors.Is(evalErr, enginerr.ErrEvaluationSkipSilently) + errors.Is(evalErr, enginerr.ErrEvaluationSkipSilently) || + // skip if the error is not a failure and the action type is remediate + (!errors.Is(evalErr, enginerr.ErrEvaluationFailed) && actionType == remediate.ActionType) || + // rule evaluation had no error, skip action if actionType IS NOT alert + (evalErr == nil && actionType != alert.ActionType) } - logger.Bool("skip_action", skipAction).Msg("action skip decision") // Everything else, do not skip - return skipAction + return skipRemediation } // getMeta returns the json.RawMessage from the database type, empty if not valid diff --git a/internal/engine/actions/alert/security_advisory/security_advisory.go b/internal/engine/actions/alert/security_advisory/security_advisory.go index a4d4ffa456..c723b06fbb 100644 --- a/internal/engine/actions/alert/security_advisory/security_advisory.go +++ b/internal/engine/actions/alert/security_advisory/security_advisory.go @@ -22,7 +22,6 @@ import ( "encoding/json" "errors" "fmt" - "github.com/stacklok/minder/internal/db" htmltemplate "html/template" "strings" @@ -114,7 +113,6 @@ type paramsSA struct { Description string Vulnerabilities []*github.AdvisoryVulnerability Metadata *alertMetadata - prevStatus *db.ListRuleEvaluationsByProfileIdRow } type templateParamsSA struct { @@ -207,7 +205,7 @@ func (alert *Alert) Do( case interfaces.ActionOptOn: return alert.run(ctx, p, cmd) case interfaces.ActionOptDryRun: - return alert.runDry(ctx, p, cmd) + return nil, alert.runDry(ctx, p, cmd) case interfaces.ActionOptOff, interfaces.ActionOptUnknown: return nil, fmt.Errorf("unexpected action setting: %w", enginerr.ErrActionFailed) } @@ -258,14 +256,13 @@ func (alert *Alert) run(ctx context.Context, params *paramsSA, cmd interfaces.Ac // Success - return ErrActionTurnedOff to indicate the action was successful return nil, fmt.Errorf("%s : %w", alert.Class(), enginerr.ErrActionTurnedOff) case interfaces.ActionCmdDoNothing: - // Return the previous alert status. - return alert.runDoNothing(ctx, params) + return nil, enginerr.ErrActionSkipped } return nil, enginerr.ErrActionSkipped } // runDry runs the security advisory action in dry run mode -func (alert *Alert) runDry(ctx context.Context, params *paramsSA, cmd interfaces.ActionCmd) (json.RawMessage, error) { +func (alert *Alert) runDry(ctx context.Context, params *paramsSA, cmd interfaces.ActionCmd) error { logger := zerolog.Ctx(ctx) // Process the command @@ -276,30 +273,28 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsSA, cmd interfaces body := "" curlCmd, err := util.GenerateCurlCommand("POST", alert.cli.GetBaseURL(), endpoint, body) if err != nil { - return nil, fmt.Errorf("cannot generate curl command: %w", err) + return fmt.Errorf("cannot generate curl command: %w", err) } logger.Info().Msgf("run the following curl command to open a security-advisory: \n%s\n", curlCmd) - return nil, nil + return nil // Close a security advisory case interfaces.ActionCmdOff: if params.Metadata == nil || params.Metadata.ID == "" { // We cannot do anything without the GHSA_ID, so we assume that closing this is a success - return nil, fmt.Errorf("no security advisory GHSA_ID provided: %w", enginerr.ErrActionTurnedOff) + return fmt.Errorf("no security advisory GHSA_ID provided: %w", enginerr.ErrActionTurnedOff) } endpoint := fmt.Sprintf("repos/%v/%v/security-advisories/%v", params.Owner, params.Repo, params.Metadata.ID) body := "{\"state\": \"closed\"}" curlCmd, err := util.GenerateCurlCommand("PATCH", alert.cli.GetBaseURL(), endpoint, body) if err != nil { - return nil, fmt.Errorf("cannot generate curl command to close a security-adivsory: %w", err) + return fmt.Errorf("cannot generate curl command to close a security-adivsory: %w", err) } logger.Info().Msgf("run the following curl command: \n%s\n", curlCmd) case interfaces.ActionCmdDoNothing: - // Return the previous alert status. - return alert.runDoNothing(ctx, params) - + return enginerr.ErrActionSkipped } - return nil, enginerr.ErrActionSkipped + return enginerr.ErrActionSkipped } // getParamsForSecurityAdvisory extracts the details from the entity @@ -310,9 +305,7 @@ func (alert *Alert) getParamsForSecurityAdvisory( metadata *json.RawMessage, ) (*paramsSA, error) { logger := zerolog.Ctx(ctx) - result := ¶msSA{ - prevStatus: params.GetEvalStatusFromDb(), - } + result := ¶msSA{} // Get the owner and repo from the entity switch entity := entity.(type) { @@ -399,19 +392,3 @@ func (alert *Alert) getSeverityString() string { return alert.saCfg.Severity } - -// runDoNothing returns the previous alert status -func (_ *Alert) runDoNothing(ctx context.Context, params *paramsSA) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).With().Str("repo", params.Repo).Logger() - - logger.Debug().Msg("Running do nothing") - - // Return the previous alert status. - err := enginerr.AlertStatusAsError(params.prevStatus.AlertStatus.AlertStatusTypes) - // If there is a valid alert metadata, return it too - if params.prevStatus.AlertMetadata.Valid { - return params.prevStatus.AlertMetadata.RawMessage, err - } - // If there is no alert metadata, return nil as the metadata and the error - return nil, err -} diff --git a/internal/engine/actions/remediate/pull_request/pull_request.go b/internal/engine/actions/remediate/pull_request/pull_request.go index 14c39423e8..1cfe563ea2 100644 --- a/internal/engine/actions/remediate/pull_request/pull_request.go +++ b/internal/engine/actions/remediate/pull_request/pull_request.go @@ -27,15 +27,16 @@ import ( "text/template" "time" + "github.com/go-git/go-billy/v5" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/go-git/go-git/v5/storage" + "github.com/google/go-github/v60/github" "github.com/rs/zerolog" "google.golang.org/protobuf/reflect/protoreflect" - "github.com/stacklok/minder/internal/db" - enginerr "github.com/stacklok/minder/internal/engine/errors" "github.com/stacklok/minder/internal/engine/interfaces" "github.com/stacklok/minder/internal/providers" "github.com/stacklok/minder/internal/util" @@ -56,14 +57,13 @@ const ( ) const ( + prMagicTemplateName = "prMagicComment" + prBodyMagicTemplate = `` + prTemplateName = "prBody" - prBodyTmplStr = "{{.PrText}}" + prBodyTmplStr = "{{.MagicComment}}\n\n{{.PrText}}" ) -type pullRequestMetadata struct { - Number int `json:"pr_number,omitempty"` -} - // Remediator is the remediation engine for the Pull Request remediation type type Remediator struct { ghCli provifv1.GitHub @@ -76,16 +76,6 @@ type Remediator struct { bodyTemplate *htmltemplate.Template } -type paramsPR struct { - ingested *interfaces.Result - repo *pb.Repository - title string - modifier fsModifier - body string - metadata *pullRequestMetadata - prevStatus *db.ListRuleEvaluationsByProfileIdRow -} - // NewPullRequestRemediate creates a new PR remediation engine func NewPullRequestRemediate( actionType interfaces.ActionType, @@ -151,39 +141,15 @@ func (_ *Remediator) GetOnOffState(p *pb.Profile) interfaces.ActionOpt { return interfaces.ActionOptFromString(p.Remediate, interfaces.ActionOptOff) } -// Do perform the remediation +// Do performs the remediation func (r *Remediator) Do( ctx context.Context, - cmd interfaces.ActionCmd, - setting interfaces.ActionOpt, + _ interfaces.ActionCmd, + remAction interfaces.ActionOpt, ent protoreflect.ProtoMessage, params interfaces.ActionsParams, - metadata *json.RawMessage, + _ *json.RawMessage, ) (json.RawMessage, error) { - p, err := r.getParamsForPRRemediation(ctx, ent, params, metadata) - if err != nil { - return nil, fmt.Errorf("cannot get PR remediation params: %w", err) - } - var remErr error - switch setting { - case interfaces.ActionOptOn: - return r.run(ctx, cmd, p) - case interfaces.ActionOptDryRun: - return r.dryRun(ctx, cmd, p) - case interfaces.ActionOptOff, interfaces.ActionOptUnknown: - remErr = errors.New("unexpected action") - } - return nil, remErr -} - -func (r *Remediator) getParamsForPRRemediation( - ctx context.Context, - ent protoreflect.ProtoMessage, - params interfaces.ActionsParams, - metadata *json.RawMessage, -) (*paramsPR, error) { - logger := zerolog.Ctx(ctx) - repo, ok := ent.(*pb.Repository) if !ok { return nil, fmt.Errorf("expected repository, got %T", ent) @@ -220,91 +186,77 @@ func (r *Remediator) getParamsForPRRemediation( return nil, fmt.Errorf("cannot create PR entries: %w", err) } - prFullBodyText, err := r.getPrBodyText(tmplParams) + magicComment, err := r.prMagicComment(modification) + if err != nil { + return nil, fmt.Errorf("cannot create PR magic comment: %w", err) + } + + prFullBodyText, err := r.getPrBodyText(tmplParams, magicComment) if err != nil { return nil, fmt.Errorf("cannot create PR full body text: %w", err) } - // Unmarshal the existing remediation metadata, if any - meta := &pullRequestMetadata{} - if metadata != nil { - err := json.Unmarshal(*metadata, meta) + var remErr error + switch remAction { + case interfaces.ActionOptOn: + alreadyExists, err := prWithContentAlreadyExists(ctx, r.ghCli, repo, magicComment) if err != nil { - // There's nothing saved apparently, so no need to fail here, but do log the error - logger.Debug().Msgf("error unmarshalling remediation metadata: %v", err) + return nil, fmt.Errorf("cannot check if PR already exists: %w", err) } + if alreadyExists { + zerolog.Ctx(ctx).Info().Msg("PR already exists, won't create a new one") + return nil, nil + } + remErr = r.runGit(ctx, ingested.Fs, ingested.Storer, modification, repo, title.String(), prFullBodyText) + case interfaces.ActionOptDryRun: + r.dryRun(modification, title.String(), prFullBodyText) + remErr = nil + case interfaces.ActionOptOff, interfaces.ActionOptUnknown: + remErr = errors.New("unexpected action") } - return ¶msPR{ - ingested: ingested, - repo: repo, - title: title.String(), - modifier: modification, - body: prFullBodyText, - metadata: meta, - prevStatus: params.GetEvalStatusFromDb(), - }, nil + return nil, remErr } -func (r *Remediator) dryRun( - ctx context.Context, - cmd interfaces.ActionCmd, - p *paramsPR, -) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).Info().Str("repo", p.repo.String()) - // Process the command - switch cmd { - case interfaces.ActionCmdOn: - // TODO: jsonize too - logger.Msgf("title:\n%s\n", p.title) - logger.Msgf("body:\n%s\n", p.body) - - err := p.modifier.writeSummary(os.Stdout) - if err != nil { - logger.Msgf("cannot write summary: %s\n", err) - } - return nil, nil - case interfaces.ActionCmdOff: - if p.metadata == nil || p.metadata.Number == 0 { - // We cannot do anything without a PR number, so we assume that closing this is a success - return nil, fmt.Errorf("no pull request number provided: %w", enginerr.ErrActionSkipped) - } - endpoint := fmt.Sprintf("repos/%v/%v/pulls/%d", p.repo.GetOwner(), p.repo.GetName(), p.metadata.Number) - body := "{\"state\": \"closed\"}" - curlCmd, err := util.GenerateCurlCommand("PATCH", r.ghCli.GetBaseURL(), endpoint, body) - if err != nil { - return nil, fmt.Errorf("cannot generate curl command to close a pull request: %w", err) - } - logger.Msgf("run the following curl command: \n%s\n", curlCmd) - return nil, nil - case interfaces.ActionCmdDoNothing: - return r.runDoNothing(ctx, p) +func (_ *Remediator) dryRun(modifier fsModifier, title, body string) { + // TODO: jsonize too + fmt.Printf("title:\n%s\n", title) + fmt.Printf("body:\n%s\n", body) + + err := modifier.writeSummary(os.Stdout) + if err != nil { + fmt.Printf("cannot write summary: %s\n", err) } - return nil, nil } -func (r *Remediator) runOn( + +func (r *Remediator) runGit( ctx context.Context, - p *paramsPR, -) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).With().Str("repo", p.repo.String()).Logger() - repo, err := git.Open(p.ingested.Storer, p.ingested.Fs) + fs billy.Filesystem, + storer storage.Storer, + modifier fsModifier, + pbRepo *pb.Repository, + title, body string, +) error { + logger := zerolog.Ctx(ctx).With().Str("repo", pbRepo.String()).Logger() + + repo, err := git.Open(storer, fs) if err != nil { - return nil, fmt.Errorf("cannot open git repo: %w", err) + return fmt.Errorf("cannot open git repo: %w", err) } wt, err := repo.Worktree() if err != nil { - return nil, fmt.Errorf("cannot get worktree: %w", err) + return fmt.Errorf("cannot get worktree: %w", err) } logger.Debug().Msg("Getting authenticated user details") email, err := r.ghCli.GetPrimaryEmail(ctx) if err != nil { - return nil, fmt.Errorf("cannot get primary email: %w", err) + return fmt.Errorf("cannot get primary email: %w", err) } currentHeadReference, err := repo.Head() if err != nil { - return nil, fmt.Errorf("cannot get current HEAD: %w", err) + return fmt.Errorf("cannot get current HEAD: %w", err) } currHeadName := currentHeadReference.Name() @@ -312,30 +264,30 @@ func (r *Remediator) runOn( // This also makes sure, all new remediations check out from main branch rather than prev remediation branch. defer checkoutToOriginallyFetchedBranch(&logger, wt, currHeadName) - logger.Debug().Str("branch", branchBaseName(p.title)).Msg("Checking out branch") + logger.Debug().Str("branch", branchBaseName(title)).Msg("Checking out branch") err = wt.Checkout(&git.CheckoutOptions{ - Branch: plumbing.NewBranchReferenceName(branchBaseName(p.title)), + Branch: plumbing.NewBranchReferenceName(branchBaseName(title)), Create: true, }) if err != nil { - return nil, fmt.Errorf("cannot checkout branch: %w", err) + return fmt.Errorf("cannot checkout branch: %w", err) } logger.Debug().Msg("Creating file entries") - changeEntries, err := p.modifier.modifyFs() + changeEntries, err := modifier.modifyFs() if err != nil { - return nil, fmt.Errorf("cannot modifyFs: %w", err) + return fmt.Errorf("cannot modifyFs: %w", err) } logger.Debug().Msg("Staging changes") for _, entry := range changeEntries { if _, err := wt.Add(entry.Path); err != nil { - return nil, fmt.Errorf("cannot add file %s: %w", entry.Path, err) + return fmt.Errorf("cannot add file %s: %w", entry.Path, err) } } logger.Debug().Msg("Committing changes") - _, err = wt.Commit(p.title, &git.CommitOptions{ + _, err = wt.Commit(title, &git.CommitOptions{ Author: &object.Signature{ Name: userNameForCommit(ctx, r.ghCli), Email: email, @@ -343,68 +295,41 @@ func (r *Remediator) runOn( }, }) if err != nil { - return nil, fmt.Errorf("cannot commit: %w", err) + return fmt.Errorf("cannot commit: %w", err) } - refspec := refFromBranch(branchBaseName(p.title)) + refspec := refFromBranch(branchBaseName(title)) err = pushBranch(ctx, repo, refspec, r.ghCli) if err != nil { - return nil, fmt.Errorf("cannot push branch: %w", err) + return fmt.Errorf("cannot push branch: %w", err) } - pr, err := r.ghCli.CreatePullRequest( - ctx, p.repo.GetOwner(), p.repo.GetName(), - p.title, p.body, - refspec, - dflBranchTo, - ) - if err != nil { - return nil, fmt.Errorf("cannot create pull request: %w, %w", err, enginerr.ErrActionFailed) - } - newMeta, err := json.Marshal(pullRequestMetadata{Number: pr.GetNumber()}) + // if a PR from this branch already exists, don't create a new one + // this handles the case where the content changed (e.g. profile changed) + // but the PR was not closed + prAlreadyExists, err := prFromBranchAlreadyExists(ctx, r.ghCli, pbRepo, branchBaseName(title)) if err != nil { - return nil, fmt.Errorf("error marshalling pull request remediation metadata json: %w", err) + return fmt.Errorf("cannot check if PR from branch already exists: %w", err) } - // Success - return the new metadata for storing the pull request number - logger.Info().Int("pr_number", pr.GetNumber()).Msg("pull request created") - return newMeta, enginerr.ErrActionPending -} - -func (r *Remediator) runOff( - ctx context.Context, - p *paramsPR, -) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).With().Str("repo", p.repo.String()).Logger() - if p.metadata == nil || p.metadata.Number == 0 { - // We cannot do anything without a PR number, so we assume that closing this is a success - return nil, fmt.Errorf("no pull request number provided: %w", enginerr.ErrActionSkipped) + if prAlreadyExists { + zerolog.Ctx(ctx).Info().Msg("PR from branch already exists, won't create a new one") + return nil } - pr, err := r.ghCli.ClosePullRequest(ctx, p.repo.GetOwner(), p.repo.GetName(), p.metadata.Number) + _, err = r.ghCli.CreatePullRequest( + ctx, pbRepo.GetOwner(), pbRepo.GetName(), + title, body, + refspec, + dflBranchTo, + ) if err != nil { - return nil, fmt.Errorf("error closing pull request %d: %w, %w", p.metadata.Number, err, enginerr.ErrActionFailed) + return fmt.Errorf("cannot create pull request: %w", err) } - logger.Info().Int("pr_number", pr.GetNumber()).Msg("pull request closed") - return nil, enginerr.ErrActionSkipped -} -func (r *Remediator) run( - ctx context.Context, - cmd interfaces.ActionCmd, - p *paramsPR, -) (json.RawMessage, error) { - // Process the command - switch cmd { - case interfaces.ActionCmdOn: - return r.runOn(ctx, p) - case interfaces.ActionCmdOff: - return r.runOff(ctx, p) - case interfaces.ActionCmdDoNothing: - return r.runDoNothing(ctx, p) - } - return nil, enginerr.ErrActionSkipped + zerolog.Ctx(ctx).Info().Msg("Pull request created") + return nil } func pushBranch(ctx context.Context, repo *git.Repository, refspec string, gh provifv1.GitHub) error { @@ -473,13 +398,39 @@ func userNameForCommit(ctx context.Context, gh provifv1.GitHub) string { return name } -func (r *Remediator) getPrBodyText(tmplParams *PrTemplateParams) (string, error) { +func (_ *Remediator) prMagicComment(modifier fsModifier) (string, error) { + tmpl, err := template.New(prMagicTemplateName).Option("missingkey=error").Parse(prBodyMagicTemplate) + if err != nil { + return "", err + } + + contentSha, err := modifier.hash() + if err != nil { + return "", fmt.Errorf("cannot get content sha1: %w", err) + } + + data := struct { + ContentSha string + }{ + ContentSha: contentSha, + } + + var buf bytes.Buffer + err = tmpl.Execute(&buf, data) + if err != nil { + return "", err + } + + return buf.String(), nil +} + +func (r *Remediator) getPrBodyText(tmplParams *PrTemplateParams, magicComment string) (string, error) { body := new(bytes.Buffer) if err := r.bodyTemplate.Execute(body, tmplParams); err != nil { return "", fmt.Errorf("cannot execute body template: %w", err) } - prFullBodyText, err := createReviewBody(body.String()) + prFullBodyText, err := createReviewBody(body.String(), magicComment) if err != nil { return "", fmt.Errorf("cannot create PR full body text: %w", err) } @@ -495,16 +446,18 @@ func getMethod(prCfg *pb.RuleType_Definition_Remediate_PullRequestRemediation) s return prCfg.Method } -func createReviewBody(prText string) (string, error) { +func createReviewBody(prText, magicComment string) (string, error) { tmpl, err := template.New(prTemplateName).Option("missingkey=error").Parse(prBodyTmplStr) if err != nil { return "", err } data := struct { - PrText string + MagicComment string + PrText string }{ - PrText: prText, + MagicComment: magicComment, + PrText: prText, } // Execute the template @@ -516,6 +469,45 @@ func createReviewBody(prText string) (string, error) { return buf.String(), nil } +// returns true if an open PR with the magic comment already exists +func prWithContentAlreadyExists( + ctx context.Context, + cli provifv1.GitHub, + repo *pb.Repository, + magicComment string, +) (bool, error) { + openPrs, err := cli.ListPullRequests(ctx, repo.GetOwner(), repo.GetName(), &github.PullRequestListOptions{}) + if err != nil { + return false, fmt.Errorf("cannot list pull requests: %w", err) + } + + for _, pr := range openPrs { + if strings.Contains(pr.GetBody(), magicComment) { + return true, nil + } + } + return false, nil +} + +func prFromBranchAlreadyExists( + ctx context.Context, + cli provifv1.GitHub, + repo *pb.Repository, + branchName string, +) (bool, error) { + // TODO(jakub): pagination + opts := &github.PullRequestListOptions{ + Head: fmt.Sprintf("%s:%s", repo.GetOwner(), branchName), + } + + openPrs, err := cli.ListPullRequests(ctx, repo.GetOwner(), repo.GetName(), opts) + if err != nil { + return false, fmt.Errorf("cannot list pull requests: %w", err) + } + + return len(openPrs) > 0, nil +} + func checkoutToOriginallyFetchedBranch( logger *zerolog.Logger, wt *git.Worktree, @@ -532,19 +524,3 @@ func checkoutToOriginallyFetchedBranch( logger.Info().Msg(fmt.Sprintf("checked out back to %s branch", originallyFetchedBranch)) } } - -// runDoNothing returns the previous remediation status -func (_ *Remediator) runDoNothing(ctx context.Context, p *paramsPR) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).With().Str("repo", p.repo.String()).Logger() - - logger.Debug().Msg("Running do nothing") - - // Return the previous remediation status. - err := enginerr.RemediationStatusAsError(p.prevStatus.RemStatus.RemediationStatusTypes) - // If there is a valid remediation metadata, return it too - if p.prevStatus.RemMetadata.Valid { - return p.prevStatus.RemMetadata.RawMessage, err - } - // If there is no remediation metadata, return nil as the metadata and the error - return nil, err -} diff --git a/internal/engine/actions/remediate/pull_request/pull_request_test.go b/internal/engine/actions/remediate/pull_request/pull_request_test.go index 7beb46faf6..c05bb04162 100644 --- a/internal/engine/actions/remediate/pull_request/pull_request_test.go +++ b/internal/engine/actions/remediate/pull_request/pull_request_test.go @@ -41,7 +41,6 @@ import ( serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/db" - "github.com/stacklok/minder/internal/engine/errors" "github.com/stacklok/minder/internal/engine/interfaces" "github.com/stacklok/minder/internal/providers" "github.com/stacklok/minder/internal/providers/credentials" @@ -57,14 +56,20 @@ const ( repoName = "minder" commitTitle = "Add Dependabot configuration for gomod" - prBody = `Adds Dependabot configuration for gomod` + prBody = ` + +Adds Dependabot configuration for gomod` authorLogin = "stacklok-bot" authorEmail = "bot@stacklok.com" - frizbeeCommitTitle = "Replace tags with sha" - frizbeePrBody = `This PR replaces tags with sha` - frizbeePrBodyWithExcludes = `This PR replaces tags with sha` + frizbeeCommitTitle = "Replace tags with sha" + frizbeePrBody = ` + +This PR replaces tags with sha` + frizbeePrBodyWithExcludes = ` + +This PR replaces tags with sha` actionWithTags = ` on: @@ -191,15 +196,17 @@ func createTestRemArgsWithExcludes() *remediateArgs { } func happyPathMockSetup(mockGitHub *mock_ghclient.MockGitHub) { - // no pull request so far - //mockGitHub.EXPECT(). - // ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{}, nil) + // no pull requst so far + mockGitHub.EXPECT(). + ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{}, nil) mockGitHub.EXPECT(). GetName(gomock.Any()).Return("stacklok-bot", nil) mockGitHub.EXPECT(). GetPrimaryEmail(gomock.Any()).Return("test@stacklok.com", nil) mockGitHub.EXPECT(). AddAuthToPushOptions(gomock.Any(), gomock.Any()).Return(nil) + mockGitHub.EXPECT(). + ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{}, nil) } func resolveActionMockSetup(t *testing.T, mockGitHub *mock_ghclient.MockGitHub, url, ref string) { @@ -372,14 +379,13 @@ func TestPullRequestRemediate(t *testing.T) { } tests := []struct { - name string - newRemArgs *newPullRequestRemediateArgs - remArgs *remediateArgs - repoSetup func(*testing.T) (*git.Repository, error) - mockSetup func(*testing.T, *mock_ghclient.MockGitHub) - expectedErr error - wantInitErr bool - expectedMetadata json.RawMessage + name string + newRemArgs *newPullRequestRemediateArgs + remArgs *remediateArgs + repoSetup func(*testing.T) (*git.Repository, error) + mockSetup func(*testing.T, *mock_ghclient.MockGitHub) + wantErr bool + wantInitErr bool }{ { name: "open a PR", @@ -399,20 +405,18 @@ func TestPullRequestRemediate(t *testing.T) { repoOwner, repoName, commitTitle, prBody, refFromBranch(branchBaseName(commitTitle)), dflBranchTo). - Return(&github.PullRequest{Number: github.Int(42)}, nil) + Return(nil, nil) }, - expectedErr: errors.ErrActionPending, - expectedMetadata: json.RawMessage(`{"pr_number":42}`), }, { - name: "fail to open a PR", + name: "update an existing PR branch with a force-push", newRemArgs: &newPullRequestRemediateArgs{ prRem: dependabotPrRem(), pbuild: testGithubProviderBuilder(), actionType: TestActionTypeValid, }, remArgs: createTestRemArgs(), - repoSetup: defaultMockRepoSetup, + repoSetup: mockRepoSetupWithBranch, mockSetup: func(_ *testing.T, mockGitHub *mock_ghclient.MockGitHub) { happyPathMockSetup(mockGitHub) @@ -422,86 +426,60 @@ func TestPullRequestRemediate(t *testing.T) { repoOwner, repoName, commitTitle, prBody, refFromBranch(branchBaseName(commitTitle)), dflBranchTo). - Return(nil, fmt.Errorf("failed to create PR")) + Return(nil, nil) }, - expectedErr: errors.ErrActionFailed, - expectedMetadata: json.RawMessage(nil), }, { - name: "update an existing PR branch with a force-push", + name: "A PR with the same content already exists", newRemArgs: &newPullRequestRemediateArgs{ prRem: dependabotPrRem(), pbuild: testGithubProviderBuilder(), actionType: TestActionTypeValid, }, remArgs: createTestRemArgs(), - repoSetup: mockRepoSetupWithBranch, + repoSetup: defaultMockRepoSetup, mockSetup: func(_ *testing.T, mockGitHub *mock_ghclient.MockGitHub) { - happyPathMockSetup(mockGitHub) - mockGitHub.EXPECT(). - CreatePullRequest( - gomock.Any(), - repoOwner, repoName, - commitTitle, prBody, - refFromBranch(branchBaseName(commitTitle)), dflBranchTo). - Return(&github.PullRequest{Number: github.Int(41)}, nil) + ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()). + Return([]*github.PullRequest{ + { + Body: github.String(prBody), + }, + }, nil) + }, + }, + { + name: "A branch for this PR already exists, shouldn't open a new PR, but only update the branch", + newRemArgs: &newPullRequestRemediateArgs{ + prRem: dependabotPrRem(), + pbuild: testGithubProviderBuilder(), + actionType: TestActionTypeValid, + }, + remArgs: createTestRemArgs(), + repoSetup: defaultMockRepoSetup, + mockSetup: func(_ *testing.T, mockGitHub *mock_ghclient.MockGitHub) { + // no pull requst so far + mockGitHub.EXPECT(). + ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{}, nil) + // we need to get the user information and update the branch + mockGitHub.EXPECT(). + GetName(gomock.Any()).Return("stacklok-bot", nil) + // likewise we need to update the branch with a valid e-mail + mockGitHub.EXPECT(). + GetPrimaryEmail(gomock.Any()).Return("test@stacklok.com", nil) + mockGitHub.EXPECT(). + AddAuthToPushOptions(gomock.Any(), gomock.Any()).Return(nil) + // this is the last call we expect to make. It returns existing PRs from this branch, so we + // stop after having updated the branch + mockGitHub.EXPECT(). + ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{ + // it doesn't matter what we return here, we just need to return a non-empty list + { + Number: github.Int(1), + }, + }, nil) }, - expectedErr: errors.ErrActionPending, - expectedMetadata: json.RawMessage(`{"pr_number":41}`), }, - //{ - // name: "A PR with the same content already exists", - // newRemArgs: &newPullRequestRemediateArgs{ - // prRem: dependabotPrRem(), - // pbuild: testGithubProviderBuilder(), - // actionType: TestActionTypeValid, - // }, - // remArgs: createTestRemArgs(), - // repoSetup: defaultMockRepoSetup, - // mockSetup: func(_ *testing.T, mockGitHub *mock_ghclient.MockGitHub) { - // mockGitHub.EXPECT(). - // ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()). - // Return([]*github.PullRequest{ - // { - // Body: github.String(prBody), - // }, - // }, nil) - // }, - //}, - // - //{ - // name: "A branch for this PR already exists, shouldn't open a new PR, but only update the branch", - // newRemArgs: &newPullRequestRemediateArgs{ - // prRem: dependabotPrRem(), - // pbuild: testGithubProviderBuilder(), - // actionType: TestActionTypeValid, - // }, - // remArgs: createTestRemArgs(), - // repoSetup: defaultMockRepoSetup, - // mockSetup: func(_ *testing.T, mockGitHub *mock_ghclient.MockGitHub) { - // // no pull requst so far - // mockGitHub.EXPECT(). - // ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{}, nil) - // // we need to get the user information and update the branch - // mockGitHub.EXPECT(). - // GetName(gomock.Any()).Return("stacklok-bot", nil) - // // likewise we need to update the branch with a valid e-mail - // mockGitHub.EXPECT(). - // GetPrimaryEmail(gomock.Any()).Return("test@stacklok.com", nil) - // mockGitHub.EXPECT(). - // AddAuthToPushOptions(gomock.Any(), gomock.Any()).Return(nil) - // // this is the last call we expect to make. It returns existing PRs from this branch, so we - // // stop after having updated the branch - // mockGitHub.EXPECT(). - // ListPullRequests(gomock.Any(), repoOwner, repoName, gomock.Any()).Return([]*github.PullRequest{ - // // it doesn't matter what we return here, we just need to return a non-empty list - // { - // Number: github.Int(1), - // }, - // }, nil) - // }, - //}, { name: "resolve tags using frizbee", newRemArgs: &newPullRequestRemediateArgs{ @@ -525,10 +503,8 @@ func TestPullRequestRemediate(t *testing.T) { repoOwner, repoName, frizbeeCommitTitle, frizbeePrBody, refFromBranch(branchBaseName(frizbeeCommitTitle)), dflBranchTo). - Return(&github.PullRequest{Number: github.Int(40)}, nil) + Return(nil, nil) }, - expectedErr: errors.ErrActionPending, - expectedMetadata: json.RawMessage(`{"pr_number":40}`), }, { name: "resolve tags using frizbee with excludes", @@ -545,16 +521,15 @@ func TestPullRequestRemediate(t *testing.T) { happyPathMockSetup(mockGitHub) resolveActionMockSetup(t, mockGitHub, "repos/actions/checkout/git/refs/tags/v4", checkoutV4Ref) + mockGitHub.EXPECT(). CreatePullRequest( gomock.Any(), repoOwner, repoName, frizbeeCommitTitle, frizbeePrBodyWithExcludes, refFromBranch(branchBaseName(frizbeeCommitTitle)), dflBranchTo). - Return(&github.PullRequest{Number: github.Int(43)}, nil) + Return(nil, nil) }, - expectedErr: errors.ErrActionPending, - expectedMetadata: json.RawMessage(`{"pr_number":43}`), }, { name: "resolve tags using frizbee with excludes from rule", @@ -578,10 +553,8 @@ func TestPullRequestRemediate(t *testing.T) { repoOwner, repoName, frizbeeCommitTitle, frizbeePrBodyWithExcludes, refFromBranch(branchBaseName(frizbeeCommitTitle)), dflBranchTo). - Return(&github.PullRequest{Number: github.Int(44)}, nil) + Return(nil, nil) }, - expectedErr: errors.ErrActionPending, - expectedMetadata: json.RawMessage(`{"pr_number":44}`), }, } @@ -646,9 +619,13 @@ func TestPullRequestRemediate(t *testing.T) { tt.remArgs.ent, evalParams, nil) + if tt.wantErr { + require.Error(t, err, "expected error") + require.Nil(t, retMeta, "expected nil metadata") + return + } - require.ErrorIs(t, err, tt.expectedErr, "expected error") - require.Equal(t, tt.expectedMetadata, retMeta) + require.NoError(t, err, "unexpected error running remediate engine") }) } } diff --git a/internal/engine/errors/errors.go b/internal/engine/errors/errors.go index 94b6b72a38..d89fd9e055 100644 --- a/internal/engine/errors/errors.go +++ b/internal/engine/errors/errors.go @@ -71,18 +71,12 @@ func NewErrEvaluationSkipSilently(sfmt string, args ...any) error { } // ErrActionSkipped is an error code that indicates that the action was not performed at all because -// the evaluation passed and the action was not needed -var ErrActionSkipped = errors.New("action skipped") - -// ErrActionPending is an error code that indicates that the action was performed but is pending, i.e., opened a PR. -var ErrActionPending = errors.New("action pending") +// the evaluation passed and the action was not needed. +var ErrActionSkipped = errors.New("action not performed") // IsActionInformativeError returns true if the error is an informative error that should not be reported to the user func IsActionInformativeError(err error) bool { - return errors.Is(err, ErrActionSkipped) || - errors.Is(err, ErrActionNotAvailable) || - errors.Is(err, ErrActionTurnedOff) || - errors.Is(err, ErrActionPending) + return errors.Is(err, ErrActionSkipped) || errors.Is(err, ErrActionNotAvailable) || errors.Is(err, ErrActionTurnedOff) } // IsActionFatalError returns true if the error is a fatal error that should stop be reported to the user @@ -150,31 +144,10 @@ func ErrorAsRemediationStatus(err error) db.RemediationStatusTypes { return db.RemediationStatusTypesSkipped case errors.Is(err, ErrActionNotAvailable): return db.RemediationStatusTypesNotAvailable - case errors.Is(err, ErrActionPending): - return db.RemediationStatusTypesPending } return db.RemediationStatusTypesError } -// RemediationStatusAsError returns the remediation status for a given error -func RemediationStatusAsError(s db.RemediationStatusTypes) error { - switch s { - case db.RemediationStatusTypesSuccess: - return nil - case db.RemediationStatusTypesFailure: - return ErrActionFailed - case db.RemediationStatusTypesSkipped: - return ErrActionSkipped - case db.RemediationStatusTypesNotAvailable: - return ErrActionNotAvailable - case db.RemediationStatusTypesPending: - return ErrActionPending - case db.RemediationStatusTypesError: - return fmt.Errorf("generic remediation error status: %s", s) - } - return fmt.Errorf("generic remediation error status: %s", s) -} - // ErrorAsAlertStatus returns the alert status for a given error func ErrorAsAlertStatus(err error) db.AlertStatusTypes { if err == nil { @@ -194,23 +167,6 @@ func ErrorAsAlertStatus(err error) db.AlertStatusTypes { return db.AlertStatusTypesError } -// AlertStatusAsError returns the error for a given alert status -func AlertStatusAsError(s db.AlertStatusTypes) error { - switch s { - case db.AlertStatusTypesOn: - return nil - case db.AlertStatusTypesOff: - return ErrActionTurnedOff - case db.AlertStatusTypesError: - return ErrActionFailed - case db.AlertStatusTypesSkipped: - return ErrActionSkipped - case db.AlertStatusTypesNotAvailable: - return ErrActionNotAvailable - } - return fmt.Errorf("unknown alert status: %s", s) -} - var ( // ErrUnauthorized is returned when a request is unauthorized ErrUnauthorized = errors.New("unauthorized") diff --git a/internal/engine/eval_status.go b/internal/engine/eval_status.go index 50b9c633e1..191e98a61c 100644 --- a/internal/engine/eval_status.go +++ b/internal/engine/eval_status.go @@ -113,7 +113,7 @@ func (e *Executor) createOrUpdateEvalStatus( // Check if we should skip silently if errors.Is(params.GetEvalErr(), evalerrors.ErrEvaluationSkipSilently) { - logger.Info().Msg("rule evaluation skipped silently - skip updating the database") + logger.Debug().Msg("rule evaluation skipped silently") return nil } @@ -135,7 +135,6 @@ func (e *Executor) createOrUpdateEvalStatus( logger.Err(err).Msg("error upserting rule evaluation") return err } - // Upsert evaluation details _, err = e.querier.UpsertRuleDetailsEval(ctx, db.UpsertRuleDetailsEvalParams{ RuleEvalID: id, @@ -152,7 +151,6 @@ func (e *Executor) createOrUpdateEvalStatus( RuleEvalID: id, Status: evalerrors.ErrorAsRemediationStatus(params.GetActionsErr().RemediateErr), Details: errorAsActionDetails(params.GetActionsErr().RemediateErr), - Metadata: params.GetActionsErr().RemediateMeta, }) if err != nil { logger.Err(err).Msg("error upserting rule remediation details") diff --git a/internal/engine/executor.go b/internal/engine/executor.go index 2ceb718fce..d4fcfc5cfb 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -27,8 +27,6 @@ import ( serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/crypto" "github.com/stacklok/minder/internal/db" - "github.com/stacklok/minder/internal/engine/actions/alert" - "github.com/stacklok/minder/internal/engine/actions/remediate" "github.com/stacklok/minder/internal/engine/entities" evalerrors "github.com/stacklok/minder/internal/engine/errors" "github.com/stacklok/minder/internal/engine/ingestcache" @@ -228,10 +226,6 @@ func (e *Executor) evalEntityEvent( ectx *EntityContext, cli *providers.ProviderBuilder, ) error { - logger := zerolog.Ctx(ctx).Info(). - Str("entity_type", inf.Type.ToString()). - Str("execution_id", inf.ExecutionID.String()) - logger.Msg("entity evaluation - started") // this is a cache so we can avoid querying the ingester upstream // for every rule. We use a sync.Map because it's safe for concurrent // access. @@ -292,7 +286,6 @@ func (e *Executor) getEvaluator( ctx context.Context, inf *entities.EntityInfoWrapper, ectx *EntityContext, - cli *providers.ProviderBuilder, profile *pb.Profile, rule *pb.Profile_Rule, @@ -415,14 +408,29 @@ func logEval( Str("project_id", inf.ProjectID.String()). Logger()) - // log evaluation result and actions status - evalLog.Info(). - Str("action", string(remediate.ActionType)). + // log evaluation + evalLog.Err(params.GetEvalErr()).Msg("result - evaluation") + + // log remediation + evalLog.Err(filterActionErrorForLogging(params.GetActionsErr().RemediateErr)). + Str("action", "remediate"). Str("action_status", string(evalerrors.ErrorAsRemediationStatus(params.GetActionsErr().RemediateErr))). - Str("action", string(alert.ActionType)). + Msg("result - action") + + // log alert + evalLog.Err(filterActionErrorForLogging(params.GetActionsErr().AlertErr)). + Str("action", "alert"). Str("action_status", string(evalerrors.ErrorAsAlertStatus(params.GetActionsErr().AlertErr))). - Msg("entity evaluation - completed") + Msg("result - action") // log business logic minderlogger.BusinessRecord(ctx).AddRuleEval(params) } + +func filterActionErrorForLogging(err error) error { + if evalerrors.IsActionFatalError(err) { + return err + } + + return nil +} diff --git a/internal/engine/executor_test.go b/internal/engine/executor_test.go index bfad9dd7c7..aa5014e580 100644 --- a/internal/engine/executor_test.go +++ b/internal/engine/executor_test.go @@ -227,7 +227,6 @@ default allow = true`, RuleEvalID: ruleEvalId, Status: db.RemediationStatusTypesSkipped, Details: "", - Metadata: json.RawMessage("{}"), }).Return(ruleEvalRemediationId, nil) // Empty metadata meta, _ := json.Marshal(map[string]any{}) diff --git a/internal/engine/rule_type_engine.go b/internal/engine/rule_type_engine.go index ed729bc14c..6d6102a240 100644 --- a/internal/engine/rule_type_engine.go +++ b/internal/engine/rule_type_engine.go @@ -20,7 +20,7 @@ import ( "fmt" "strings" - "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "google.golang.org/protobuf/encoding/protojson" "github.com/stacklok/minder/internal/db" @@ -148,11 +148,6 @@ func (r *RuleTypeEngine) GetRuleInstanceValidator() *RuleValidator { // Eval runs the rule type engine against the given entity func (r *RuleTypeEngine) Eval(ctx context.Context, inf *entities.EntityInfoWrapper, params engif.EvalParamsReadWriter) error { - logger := zerolog.Ctx(ctx).Info(). - Str("entity_type", inf.Type.ToString()). - Str("execution_id", inf.ExecutionID.String()) - - logger.Msg("entity evaluation - ingest started") // Try looking at the ingesting cache first result, ok := r.ingestCache.Get(r.rdi, inf.Entity, params.GetRule().Params) if !ok { @@ -164,18 +159,15 @@ func (r *RuleTypeEngine) Eval(ctx context.Context, inf *entities.EntityInfoWrapp // Note that for some types of ingesting the evalErr can already be set from the ingester. return fmt.Errorf("error ingesting data: %w", err) } + r.ingestCache.Set(r.rdi, inf.Entity, params.GetRule().Params, result) } else { - logger.Str("id", r.GetID()).Msg("entity evaluation - ingest using cache") + log.Printf("Using cached result for %s", r.GetID()) } - logger.Msg("entity evaluation - ingest completed") - params.SetIngestResult(result) + params.SetIngestResult(result) // Process evaluation - logger.Msg("entity evaluation - evaluation started") - err := r.reval.Eval(ctx, params.GetRule().Def.AsMap(), result) - logger.Msg("entity evaluation - evaluation completed") - return err + return r.reval.Eval(ctx, params.GetRule().Def.AsMap(), result) } // Actions runs all actions for the rule type engine against the given entity diff --git a/internal/providers/github/common.go b/internal/providers/github/common.go index 49a7a52cee..f4514e5461 100644 --- a/internal/providers/github/common.go +++ b/internal/providers/github/common.go @@ -564,17 +564,6 @@ func (c *GitHub) CreatePullRequest( return pr, nil } -// ClosePullRequest closes a pull request in a repository. -func (c *GitHub) ClosePullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { - pr, _, err := c.client.PullRequests.Edit(ctx, owner, repo, number, &github.PullRequest{ - State: github.String("closed"), - }) - if err != nil { - return nil, err - } - return pr, nil -} - // ListPullRequests lists all pull requests in a repository. func (c *GitHub) ListPullRequests( ctx context.Context, diff --git a/internal/providers/github/mock/github.go b/internal/providers/github/mock/github.go index a954f6d370..e547bd7d56 100644 --- a/internal/providers/github/mock/github.go +++ b/internal/providers/github/mock/github.go @@ -239,21 +239,6 @@ func (mr *MockGitHubMockRecorder) Clone(ctx, url, branch any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Clone", reflect.TypeOf((*MockGitHub)(nil).Clone), ctx, url, branch) } -// ClosePullRequest mocks base method. -func (m *MockGitHub) ClosePullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ClosePullRequest", ctx, owner, repo, number) - ret0, _ := ret[0].(*github.PullRequest) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ClosePullRequest indicates an expected call of ClosePullRequest. -func (mr *MockGitHubMockRecorder) ClosePullRequest(ctx, owner, repo, number any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClosePullRequest", reflect.TypeOf((*MockGitHub)(nil).ClosePullRequest), ctx, owner, repo, number) -} - // CloseSecurityAdvisory mocks base method. func (m *MockGitHub) CloseSecurityAdvisory(ctx context.Context, owner, repo, id string) error { m.ctrl.T.Helper() diff --git a/pkg/providers/v1/providers.go b/pkg/providers/v1/providers.go index 89ae6a081c..6bf1c89878 100644 --- a/pkg/providers/v1/providers.go +++ b/pkg/providers/v1/providers.go @@ -103,7 +103,6 @@ type GitHub interface { v []*github.AdvisoryVulnerability) (string, error) CloseSecurityAdvisory(ctx context.Context, owner, repo, id string) error CreatePullRequest(ctx context.Context, owner, repo, title, body, head, base string) (*github.PullRequest, error) - ClosePullRequest(ctx context.Context, owner, repo string, number int) (*github.PullRequest, error) ListPullRequests(ctx context.Context, owner, repo string, opt *github.PullRequestListOptions) ([]*github.PullRequest, error) GetUserId(ctx context.Context) (int64, error) GetName(ctx context.Context) (string, error)