Add a pending remediation status and support tracking opened PRs by rdimitrov · Pull Request #2833 · mindersec/minder

rdimitrov · 2024-03-27T18:48:03Z

Summary

The following PR:

Adds a new metadata column in the rule_details_remediate table for storing the opened PR number
Refactors the way we process remediations through PRs - we no longer use magic comments to keep track of opened ones.
If the issue is fixed out of band (through another PR for example) we'll now automatically close the PR along with the opened alert too
Status is now pending when evaluation failed and remediation opened a PR
Status is skipped if we don't need remediation
Status is success if remediation was considered instant (i.e. rest) but we haven't re-evaluated yet (so rule is still failing).
Once we re-evaluate and the issue is fixed, the remediation status will become skipped.
Adds a new ClosePullRequest() handler
Removes the part where we were not updating the database for alerts with skipped status
Refactors the part where we make the decision of whether we should remediate or not to happen in one place in the actions engine
Fixes another bug where the remediation for pinned actions actually opens another PR after merging the first one

What's left (can be done in a follow-up PR):

The PR adds more log messages, but I will revisit/update their usefulness
Currently if there's nothing to do, we return the previous action state as we do upsert at the end. The good thing is it updates the timestamp, but not sure how wasteful it is. Perhaps it's better to return a custom ErrActionDidNothing and skip upserting if this is the case.
Test this more with other ruletypes that remediate through a PR
Add the pull request URL to be returned by the API
Support rebasing/updating the PR if main changes while the PR is opened

Supersedes #2530
Fixes #2526
Fixes #1207
Fixes #1201

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Tested locally with a demo repository using the dependabot_configured ruletype:

Start with a failing repo:

evaluation is failure
rem is pending
alert is on
We have an opened PR and alert

Case 1 - Merged the PR

eval is success
rem is skipped
alert is off
alert is closed automatically

Case 2 - Fixed the issue through another PR

Eval is ok
rem is skipped
alert is off
both PR and alert are closed automatically

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

jhrozek · 2024-03-27T21:32:14Z

+	// 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
+	}


aren't there legitimate cases for the PR evaluator where we want to push a new version of the PR even if the evaluation was failure before and is still failing?
e.g. I want frizbee to unroll tags to shas. The evaluation is failing and there is a PR. While the PR is open, someone pushes another workflow with more actions..

That's a good point, I haven't thought about the frizbee action as I assumed we are proposing only new files so far (dependabot, codeql, trivy, etc). I'll try to think about how to implement that 👍 Do you know if it's working like that right now and this will break it or?

with frizbee in particular I think the additional changes are the way it works now, yes, because unllike dependabot and codeql where we just put in the content, with frizbee you feed the content you get from evaluation to the frizbee library which modifies it and gives you back the modified content.

I'm fine fixing this in a follow-up, but could this be solved by:

if the old status is failure always defer to the remediator

in the PR remediator, check what HEAD the PR points to. If it is the same as the hash of the commit we created locally, shortcut, otherwise go forward and force-push

the other remediators we have at least for now are "atomic" so this should not happen in the first place and if it does, then we should have anotoher mechanism that prevents an infinite remediate loop anyway

I think so 👍 We can also filter this fallback by remediation type, i.e. pull_request to make sure we do that only for this remediation type and not for anything else in future 👍

coveralls · 2024-03-28T14:04:12Z

coverage: 47.037% (-0.5%) from 47.514%
when pulling ed5db4e on remediation
into 3e0583a on main.

rdimitrov · 2024-03-28T15:07:52Z

These are the rest of the changes I think we can do in a follow-up PR:

The PR adds more log messages, but I will revisit/update their usefulness
Add the pull request URL to be returned by the API
Support rebasing/updating the remediation PR if main changes while the PR is opened

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

jhrozek

Thank you the logic reads much better now. I'm fine merging this and following up on this work with figuring out how to update the pending remediations.

rdimitrov · 2024-03-28T16:50:10Z

Thanks!

For anyone else looking at the PR, let's wait a bit until we merge this so we have the smoke test updates in a PR ready for merging too 👍

…PRs (#2833)" This reverts commit f330d61.

…PRs" (#2862) Revert "Add a pending remediation status and support tracking opened PRs (#2833)" This reverts commit f330d61.

rdimitrov requested a review from a team March 27, 2024 18:48

rdimitrov self-assigned this Mar 27, 2024

jhrozek reviewed Mar 27, 2024

View reviewed changes

Comment thread internal/providers/github/common.go Outdated

jhrozek reviewed Mar 27, 2024

View reviewed changes

Comment thread database/query/profile_status.sql

jhrozek reviewed Mar 27, 2024

View reviewed changes

Comment thread internal/engine/actions/actions.go

evankanderson reviewed Mar 27, 2024

View reviewed changes

Comment thread database/query/profile_status.sql

Comment thread internal/engine/actions/actions.go Outdated

Comment thread internal/engine/actions/actions.go Outdated

Comment thread internal/engine/actions/actions.go Outdated

Comment thread internal/engine/actions/actions.go

rdimitrov force-pushed the remediation branch from 78086f4 to 098c027 Compare March 28, 2024 10:17

rdimitrov force-pushed the remediation branch from b022b99 to ed5db4e Compare March 28, 2024 14:07

rdimitrov changed the title ~~WIP: Add a pending remediation status and support tracking opened PRs~~ Add a pending remediation status and support tracking opened PRs Mar 28, 2024

jhrozek reviewed Mar 28, 2024

View reviewed changes

Comment thread database/migrations/000040_remediate_metadata.down.sql

rdimitrov added 10 commits March 28, 2024 18:21

Add metadata to remediate eval table

a4d3b56

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Refactor pull_request remediation to store it in the database

8a94785

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Use action commands for pull_request remediation

15de03d

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Remove relying on a magic comment

736874e

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Rename migration due to rebase

84ff16e

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Do not rely on skipped status for alerts and add more logs

3787bfb

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Use switch for going through the action use cases

a33f2f0

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Address linting errors

c77ef59

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Use PullRequest.Edit instead of a generic GitHub REST call

9ee06d0

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Move handling of ActionCmdDoNothing to happen in each action

dc68ba0

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

jhrozek reviewed Mar 28, 2024

View reviewed changes

Comment thread internal/engine/actions/actions.go

jhrozek reviewed Mar 28, 2024

View reviewed changes

Comment thread internal/engine/actions/actions.go Outdated

Update the pull_request unit tests to accomodate the new pending status

f713f62

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

rdimitrov force-pushed the remediation branch from ed5db4e to f713f62 Compare March 28, 2024 16:27

Fix RemStatus.Valid check in shouldRemediate

efcf63f

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

jhrozek approved these changes Mar 28, 2024

View reviewed changes

rdimitrov merged commit f330d61 into main Mar 29, 2024

rdimitrov deleted the remediation branch March 29, 2024 07:59

rdimitrov mentioned this pull request Mar 29, 2024

Add new pending remediation status and implement in pull_request #2530

Closed

10 tasks

rdimitrov added a commit that referenced this pull request Mar 29, 2024

Revert "Add a pending remediation status and support tracking opened …

527b8b7

…PRs (#2833)" This reverts commit f330d61.

rdimitrov mentioned this pull request Mar 29, 2024

Revert "Add a pending remediation status and support tracking opened PRs" #2862

Merged

rdimitrov added a commit that referenced this pull request Mar 29, 2024

Revert "Add a pending remediation status and support tracking opened …

be7bbf3

…PRs" (#2862) Revert "Add a pending remediation status and support tracking opened PRs (#2833)" This reverts commit f330d61.

evankanderson mentioned this pull request Apr 6, 2026

refactor: reduce db type coupling in engine actions using adapter helpers #6274

Open

Conversation

rdimitrov commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type

Testing

Review Checklist:

Uh oh!

Uh oh!

Uh oh!

jhrozek Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

rdimitrov Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

jhrozek Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

jhrozek Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

rdimitrov Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdimitrov commented Mar 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

rdimitrov commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rdimitrov commented Mar 27, 2024 •

edited

Loading

coveralls commented Mar 28, 2024 •

edited

Loading