fix: create ignore rule patterns optionally #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It looks like only the linux kernel headers ignore rules make use of pattern matching. Create and use regex patterns for these cases only to reduce memory allocations from pattern.compile and improve FindMatches performance. In future if we wish to support regular expressions more broadly perhaps then perhaps ignore rules should have some way to define whether they should be treated as a regular expression or not. Signed-off-by: Adam McClenaghan <[email protected]>
Signed-off-by: Adam McClenaghan <[email protected]>
grype/match/ignore.go
Outdated
Comment on lines
209
to
230
| pattern, err := packageNameRegex(name) | ||
| if err != nil { | ||
| return func(Match) bool { return false } | ||
| var ( | ||
| pattern *regexp.Regexp | ||
| err error | ||
| ) | ||
| if strings.HasPrefix(name, "linux") && strings.Contains(name, "-headers-") { | ||
| pattern, err = packageNameRegex(name) | ||
| } | ||
|
|
||
| return func(match Match) bool { | ||
| return pattern.MatchString(match.Package.Name) | ||
| if err != nil { | ||
| // Error compiling pattern | ||
| return false | ||
| } | ||
|
|
||
| if pattern != nil { | ||
| // Do regex for linux-headers | ||
| return pattern.MatchString(match.Package.Name) | ||
| } | ||
|
|
||
| // Exact string match for everything else | ||
| return name == match.Package.Name |
There was a problem hiding this comment.
nit but this feels cleaner if we just invert the strings match logic:
func ifPackageNameApplies(name string) ignoreCondition {
if !(strings.HasPrefix(name, "linux") && strings.Contains(name, "-headers-")) {
return func(match Match) bool {
return name == match.Package.Name
}
}
pattern, err := packageNameRegex(name)
if err != nil {
return func(Match) bool { return false }
}
return func(match Match) bool {
return pattern.MatchString(match.Package.Name)
}
}
grype/match/ignore.go
Outdated
| pattern *regexp.Regexp | ||
| err error | ||
| ) | ||
| if strings.HasPrefix(name, "linux") { |
There was a problem hiding this comment.
how come we dont care about -headers- here?
There was a problem hiding this comment.
https://github.com/anchore/grype/blob/main/cmd/grype/cli/commands/root.go#L106C1-L106C161
{Package: match.IgnoreRulePackage{Name: "linux(-.*)?-headers-.*", [ Type: string(syftPkg.DebPkg)}, MatchType: match.ExactIndirectMatch},
Upstream cares about this part:
UpstreamName: "linux.*",
| @@ -677,33 +677,6 @@ func TestApplyIgnoreRules(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
the reasoning for removing these are fine, but perhaps thats a separate PR?
|
LGTM 👍 |
Signed-off-by: Adam McClenaghan <[email protected]>
|
Opened against upstream: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In anchore#1809 both
ifPackageNameAppliesandifUpstreamPackageNameApplieswere updated to use regular expressions for all rule names/upstream names. This was recently updated in anchore#2320 too.So far as I can see from the codebase only the linux kernel headers ignore rules require regular expression matching.
In my testing I've found that this is significantly slowing down the time
FindMatchestakes to complete. For one of my test scans with about ~60k packages, the scan is 2x faster with this change and there is ~8GB less cumulative allocations during the scanBefore
After
For now, I've modified
ifPackageNameAppliesandifUpstreamPackageNameAppliesto handle the linux-headers as special cases.In future if we wish to support regular expressions more broadly perhaps there should be a nicer interface passed into the likes of
ifPackageNameAppliesandifUpstreamPackageNameAppliesrather than a string, and this interface could define whether or not to treat the match type as a regex etc...I've had to make some changes to the unit tests.
I've removed two tests that don't seem to be testing behaviour that Grype currently doesn't have configuration for, namely:
kernel-headers.*so I've removed the testignore on name regexignore on name regex, line termination test matchI've also modified some of the tests to match the actual patterns set out in Grype, namely:
--
linux-.*-headers-.*is now declaredlinux(-.*)?-headers-.*--
linux-.*is now defined aslinux.*.. as a result of fixing this I've also updated the upstream name fromlinuxtonotalinuxfor the test case. The ignore rules defined today would actually ignorelinux, the only reason the test was passing before is because it defined the regex aslinux-.*In lieue of a larger refactor to have a nicer interface for
ifPackageNameApplies/ifUpstreamPackageNameApplieswhich I've previously mentioned, it would at least perhaps be nice to declare thelinux(-.*)?-headers-.*andlinux.*as a const in some shared place. If the maintainers can suggest somewhere that would be a good fit, I'd be happy to move it.