Trivy scan meddling #482
Trivy scan meddling #482
Conversation
…. Opt in feature for only creating tickets based on max lts
securityscanutils/securityscan.go
Outdated
|
|
||
| maxPatchForMinor := map[string]struct{}{} |
There was a problem hiding this comment.
We have a Predicate that is responsible for defining the logic for filtering releases that we want to process:
go-utils/securityscanutils/securityscan.go
Line 341 in a7cea00
There was a problem hiding this comment.
Alright it made it a bit larger of a change set but this is now supported
We probably can also do a better job of cutting this into tests but putting back up for review for general approach.
Its kinda nasty and I am not super happy with it as is. Thoughts?
…. Opt in feature for only creating tickets based on max lts
|
Issues linked to changelog: |
|
@sam-heilbron does this general approach fit with what you were hoping |
There was a problem hiding this comment.
This looks good and I think will be a nice addition to reduce the noise of our GitHub issues.
I was imagining a slightly different approach, which I'll outline below. Certainly not required, just wanted to share my thoughts. Feel free to ignore if you prefer your approach and I'm happy to review/approve as is:
In trivy scanning we perform 2 types of filters:
- We filter certain github releases from being scanned
- We filter certain scans from being uploaded to github
We support various configuration to control the behaviors of these.
I imagined we could have our scanner construct the two instances of these filters (or predicates) at runtime based on the injected settings.
Then the scanner, unaware of how these operate performs:
- Load releases
- Filter releases that we don't care about
- Run scans
- If scan passes filter that defines whether or not to create an issue, create one. Else, skip it.
In this scenario, I was hoping we could not have any conditional checks around if this setting is set, do this and instead, just run the RepoFilter and let the complexity of what the RepoFilter does and how it is configured be isolated from the runtime code
#485 Is a quick change outlining my proposal. Again, this is just how I would have approached it, but certainly not the right or only way.
| @@ -145,17 +145,100 @@ func GetRawGitFile(ctx context.Context, client *github.Client, content *github.R | |||
| return byt, err | |||
| } | |||
|
|
|||
| // A RepoFilter can check if a release shold be filtered out based on a condition. | |||
| // In general filters may be stateful and be a StatefulRepoFilter | |||
| type RepoFilter interface { | |||
There was a problem hiding this comment.
How is this different that a RepositoryReleasePredicate? It has the same interface, so I don't quite follow how each are intended to be used
There was a problem hiding this comment.
You had stated that you wanted the LTS determining filter to exist in the same place as our existing predicates.
Predicate becomes a bad name for a repofiltering mechanism once you are no longer filtering purely on predicates.
|
|
||
| // A StatefulRepoFilter has a stateful prerun check and along with a way to determine | ||
| // if a release conforms to our desired filter. | ||
| type StatefulRepoFilter interface { |
There was a problem hiding this comment.
I don't fully understand how a StatefulFilter works. To me it seems like the stateful filter PreFilterCheck is a way of defining the state on a Filter (to be used during the filter logic).
I wonder if instead, we just stateful component should just be handled at construction time. That way, if we create a RepoFilter (Or RepositoryReleasePredicate), we can instantiate it to behave a certain way (ie filter only latest release), and then the code doesn't need to be aware that it has any internal state.
There was a problem hiding this comment.
So this is the crux of the issue (other than the related issue where in issue updates we retrun too early which breaks things)
Namely we only get releases in go-utils today.
So if we want to only upload issues for the most recent LTS branch then we need to be able to determine that either here or leak repository retrieval info to another library. This is true because one can only know what an LTS branch is by inspection of the other existing releases.
| @@ -157,14 +162,28 @@ func (s *SecurityScanner) GenerateSecurityScans(ctx context.Context) error { | |||
| return eris.Wrapf(err, "error fetching all issues from github.com/%s/%s", repo.Owner, repo.Repo) | |||
| } | |||
| } | |||
|
|
|||
| if repo.Opts.CreateGithubIssuePerLTSVersion && len(repoLTSFilter.MaxPatchForMinor) == 0 { | |||
There was a problem hiding this comment.
This seems like our implementation code is overly aware of our interface. Namely, we read properties off a specific type and then use the interface method. If we change our implementation of the repoLTSFilter, we would have to change this code as well. Ideally, it would be great if we could change the implementation of an interface instance, and not have any other dependent code change.
There was a problem hiding this comment.
I agree this isnt the cleanest part but I was to lazy to add in an additional set of post filters at this point in time and wanted to see opinions on the predicate overhaul that you had proposed earlier.
In this we revert the default behavior that was erroneously added to only scan the latest patch for cves.
Older patch versions may still get new cves which we should report in docs or other reporting structures even if none of them will be fixed in that old patch version.
In order to implement some minor noise reduction strategies this PR instead introduces a new option for security scans to only create issues for the max lts branches.
The ability to only check latest branches (such as when running locally) is now supported as an additional repo option.
This PR does not introduce an overload level optionset but this does present itself as a possible future extension.