Skip to content

libs: Add envoy.dependency.check[cves]#186

Merged
htuch merged 1 commit intoenvoyproxy:mainfrom
phlax:envoy.dependency.check-rename_cve-scanner
Jan 26, 2022
Merged

libs: Add envoy.dependency.check[cves]#186
htuch merged 1 commit intoenvoyproxy:mainfrom
phlax:envoy.dependency.check-rename_cve-scanner

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Jan 19, 2022

Signed-off-by: Ryan Northey ryan@synca.io

This PR:

  • renames envoy.dependency.cve_scan -> envoy.dependency.check
  • refactors the package to be a more generic dependency checker
  • switches the default action error -> warn on finding a bad CVE (this provides more flexibility)

@netlify
Copy link
Copy Markdown

netlify bot commented Jan 19, 2022

✔️ Deploy Preview for nifty-bassi-e26446 ready!

🔨 Explore the source changes: 763cbb3

🔍 Inspect the deploy log: https://app.netlify.com/sites/nifty-bassi-e26446/deploys/61ec7a03ab6f36000832adb8

😎 Browse the preview: https://deploy-preview-186--nifty-bassi-e26446.netlify.app

@phlax phlax changed the title libs: Add envoy.dependency.check[cves] [WIP] libs: Add envoy.dependency.check[cves] Jan 19, 2022
@phlax phlax marked this pull request as draft January 19, 2022 22:12
@phlax phlax force-pushed the envoy.dependency.check-rename_cve-scanner branch 2 times, most recently from a8b8824 to 26d9bba Compare January 20, 2022 10:39
@phlax phlax changed the title [WIP] libs: Add envoy.dependency.check[cves] libs: Add envoy.dependency.check[cves] Jan 20, 2022
@phlax phlax marked this pull request as ready for review January 20, 2022 10:46
[
[
"CVE data downloaded from: NIST_URL"
"No CPE listed for: bazel_compdb"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this correct?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hmm, not sure, good spot, ill look further

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so the removal is correct - it doesnt print that anymore in the info log (it has a debug log instead)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

and yep, its not an addition, its just the diff from removing the first log - so i think it is correct

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

since the name is no_fail.json and the log does not match with that, I was confused

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i think i called it that because it wasnt a total failure - but the paths where this is the outcome have that log.info in the output

its kinda no_fail_but_no_anything_else_either

catches=[exceptions.CVECheckError])
async def preload_cves(self) -> None:
async for download in self.cves.downloads:
self.log.debug(f"Preloaded cve data: {download}")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is the debug log that was previously a log.info

self,
dep: "abstract.ADependency") -> None:
if not dep.cpe:
self.log.info(f"No CPE listed for: {dep.id}")
Copy link
Copy Markdown
Member Author

@phlax phlax Jan 20, 2022

Choose a reason for hiding this comment

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

this is the new log.info saying that the dep has no cve - im just wondering what this did before...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@kfaseela
Copy link
Copy Markdown
Collaborator

LGTM. would wait for others to review

@phlax phlax requested a review from htuch January 21, 2022 16:12
@phlax phlax force-pushed the envoy.dependency.check-rename_cve-scanner branch 5 times, most recently from bf8d1b8 to 1c0d36d Compare January 22, 2022 21:37
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the envoy.dependency.check-rename_cve-scanner branch from 1c0d36d to 763cbb3 Compare January 22, 2022 21:41
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 1650ad7 into envoyproxy:main Jan 26, 2022
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.

3 participants