Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move file patterns to a global level to be able to use it on any analyzer #2539

Merged

Conversation

jerbob92
Copy link
Contributor

@jerbob92 jerbob92 commented Jul 18, 2022

Description

This is the functionality of aquasecurity/fanal#372 moved into Trivy since Fanal was merged into it.

This PR moves the file patterns feature from the config analyzers to a global level so that we can use it on any analyzer.
In favor of aquasecurity/fanal#370, aquasecurity/fanal#357, aquasecurity/fanal#355 as this feature will allow you to set the needed file patterns yourself.

This also makes the config analyzers more like the other analyzers.

This can be enabled in Trivy without changing anything to the config scanner. The other commands will just get a --filter-patterns option.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@jerbob92 jerbob92 changed the title Move file patterns to a global level to be able to use it on any analyzer feat: move file patterns to a global level to be able to use it on any analyzer Jul 18, 2022
@jerbob92
Copy link
Contributor Author

jerbob92 commented Aug 9, 2022

@owenrumney @liamg @knqyf263
Is anyone going to look at this?

@jerbob92
Copy link
Contributor Author

@owenrumney @liamg @knqyf263 Are you guys straight up collectively ignoring me or is something else going on?

@liamg
Copy link
Contributor

liamg commented Aug 22, 2022

Hi @jerbob92 - sorry about this! Entirely my fault. It's been a busy couple of weeks and I totally missed this but that's no excuse, I apologise. Looking now 👀

@jerbob92
Copy link
Contributor Author

Hi @jerbob92 - sorry about this! Entirely my fault. It's been a busy couple of weeks and I totally missed this but that's no excuse, I apologise. Looking now eyes

Thanks! Just wondering whether I have to keep updating this MR 😅

Copy link
Contributor

@liamg liamg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

Sorry for the delay 🐌

@liamg
Copy link
Contributor

liamg commented Aug 22, 2022

If you could check the conflicts I'll make sure we merge this ASAP :)

@jerbob92
Copy link
Contributor Author

@liamg the current failure is this:

    docker_engine_test.go:275: 
        	Error Trace:	/home/runner/work/trivy/trivy/integration/docker_engine_test.go:275
        	Error:      	Received unexpected error:
        	            	Error: No such image: ghcr.io/aquasecurity/trivy-test-images:amazon-2
        	Test:       	TestDockerEngine/amazonlinux:2
        	Messages:   	amazonlinux:2

That doesn't seem related to my changes, do you have any idea?

@liamg
Copy link
Contributor

liamg commented Aug 23, 2022

@jerbob92 I haven't seen this failure before - I'll raise it internally and see if I can find a solution.

@liamg
Copy link
Contributor

liamg commented Aug 23, 2022

Apparently this is a fairly common issue with GHCR being unreliable 😬 - I've kicked off the integration tests again 🤞

@jerbob92 jerbob92 requested a review from liamg August 23, 2022 09:41
@liamg
Copy link
Contributor

liamg commented Aug 23, 2022

@knqyf263 What do you think? Can we merge this? 😃

@knqyf263
Copy link
Collaborator

@liamg Thanks for reviewing.
@jerbob92 I hope to take a look shortly.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update this doc?

# Same as '--file-patterns'
# Default is empty
file-patterns:

Comment on lines 351 to 357
filePatternMatch := false
for _, pattern := range ag.filePatterns[a.Type()] {
if pattern.MatchString(cleanPath) {
filePatternMatch = true
break
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having it in a dedicated method seems better.

func (ag AnalyzerGroup) filePatternMatch(analyzerType Type, filePath string) {
for _, pattern := range ag.filePatterns[analyzerType] {
if pattern.MatchString(cleanPath) {
return true
}
}
return false
}

@jerbob92
Copy link
Contributor Author

Could you also update this doc?

# Same as '--file-patterns'
# Default is empty
file-patterns:

Moving it to scan options?

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2022

CLA assistant check
All committers have signed the CLA.

@jerbob92
Copy link
Contributor Author

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Huh? Didn't I already sign this?

@afdesk
Copy link
Contributor

afdesk commented Aug 30, 2022

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Huh? Didn't I already sign this?

@jerbob92 it seems CLA was updated

@knqyf263
Copy link
Collaborator

Moving it to scan options?

Yes

@knqyf263 knqyf263 merged commit 5f0bf14 into aquasecurity:main Sep 1, 2022
@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 1, 2022

Thanks for your contribution and patience🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants