Migrating vulnerabilities pkgs to slog. - #40106
Conversation
GitHub Actions
Failure
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request implements a systematic migration of the vulnerability scanning codebase from go-kit logging to Go's standard library slog. The changes include replacing all Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/vulnerabilities/nvd/sync/cve_syncer_test.go (1)
60-61: Minor: hoist the discard logger out of the loop.
slog.New(slog.DiscardHandler)is allocated on every iteration. Since the logger is stateless, it can be created once before the loop.♻️ Suggested change
Add before the loop (e.g., around line 55):
discardLogger := slog.New(slog.DiscardHandler)Then on line 61:
- convertedLegacyVuln := convertAPI20CVEToLegacy(t.Context(), api20Vuln.CVE, slog.New(slog.DiscardHandler)) + convertedLegacyVuln := convertAPI20CVEToLegacy(t.Context(), api20Vuln.CVE, discardLogger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/vulnerabilities/nvd/sync/cve_syncer_test.go` around lines 60 - 61, Hoist the stateless discard logger out of the loop so you don't allocate it on every iteration: create a single discardLogger using slog.New(slog.DiscardHandler) before iterating over api20CVEs, then pass that discardLogger into convertAPI20CVEToLegacy inside the loop instead of calling slog.New(slog.DiscardHandler) each time.tools/nvd/nvdvuln/nvdvuln.go (1)
296-306: Optional: simplifyvulnDBSyncbody.The error-forwarding dance (
if err != nil { return err }; return nil) can be reduced to a direct return; the function is already a thin wrapper.♻️ Proposed simplification
func vulnDBSync(ctx context.Context, vulnDBDir string, debug bool, logger *slog.Logger) error { opts := nvd.SyncOptions{ VulnPath: vulnDBDir, Debug: debug, } - err := nvd.Sync(ctx, opts, logger) - if err != nil { - return err - } - return nil + return nvd.Sync(ctx, opts, logger) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/nvd/nvdvuln/nvdvuln.go` around lines 296 - 306, The vulnDBSync function body contains an unnecessary error check; replace the current pattern in vulnDBSync that assigns err := nvd.Sync(...), checks if err != nil then returns err, and finally returns nil, with a direct return of the call to nvd.Sync(ctx, opts, logger). Keep construction of opts (nvd.SyncOptions with VulnPath and Debug) as-is and then simply return nvd.Sync(ctx, opts, logger) to simplify the wrapper.cmd/fleet/cron.go (1)
455-473: Consider: repeatedlogger.SlogLogger()calls could be hoisted.Both
goval_dictionary.Refresh(Line 458) andgoval_dictionary.Analyze(Line 473) calllogger.SlogLogger()separately. You could hoist a singleslogLogger := logger.SlogLogger()at the top ofcheckGovalDictionaryVulnerabilitiesand reuse it. Same applies tocheckNVDVulnerabilitieswhich has four such calls. This is purely a readability nit — no functional concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleet/cron.go` around lines 455 - 473, Multiple repeated calls to logger.SlogLogger() reduce readability; hoist a single slogLogger := logger.SlogLogger() at the top of checkGovalDictionaryVulnerabilities and replace the calls to goval_dictionary.Refresh(..., logger.SlogLogger()) and goval_dictionary.Analyze(..., logger.SlogLogger()) to use slogLogger instead, and do the same in checkNVDVulnerabilities (replace its four logger.SlogLogger() calls with the single slogLogger variable) so the same slog instance is reused throughout each function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/fleet/cron.go`:
- Around line 455-473: Multiple repeated calls to logger.SlogLogger() reduce
readability; hoist a single slogLogger := logger.SlogLogger() at the top of
checkGovalDictionaryVulnerabilities and replace the calls to
goval_dictionary.Refresh(..., logger.SlogLogger()) and
goval_dictionary.Analyze(..., logger.SlogLogger()) to use slogLogger instead,
and do the same in checkNVDVulnerabilities (replace its four logger.SlogLogger()
calls with the single slogLogger variable) so the same slog instance is reused
throughout each function.
In `@server/vulnerabilities/nvd/sync/cve_syncer_test.go`:
- Around line 60-61: Hoist the stateless discard logger out of the loop so you
don't allocate it on every iteration: create a single discardLogger using
slog.New(slog.DiscardHandler) before iterating over api20CVEs, then pass that
discardLogger into convertAPI20CVEToLegacy inside the loop instead of calling
slog.New(slog.DiscardHandler) each time.
In `@tools/nvd/nvdvuln/nvdvuln.go`:
- Around line 296-306: The vulnDBSync function body contains an unnecessary
error check; replace the current pattern in vulnDBSync that assigns err :=
nvd.Sync(...), checks if err != nil then returns err, and finally returns nil,
with a direct return of the call to nvd.Sync(ctx, opts, logger). Keep
construction of opts (nvd.SyncOptions with VulnPath and Debug) as-is and then
simply return nvd.Sync(ctx, opts, logger) to simplify the wrapper.
There was a problem hiding this comment.
Pull request overview
This PR migrates the vulnerabilities packages (nvd, msrc, goval_dictionary, customcve) from go-kit/log to the standard library's log/slog as part of the ongoing slog migration effort tracked in issue #40054.
Changes:
- Migrated all vulnerability processing packages from
go-kit/logto*slog.Logger - Added
context.Contextparameters to functions that use logging for proper context-aware logging - Updated test files to use
slog.New(slog.DiscardHandler)andt.Context() - Updated integration points (cron jobs, CLI tools) to work with new logger signatures
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/nvd/nvdvuln/nvdvuln.go | Migrated CLI tool to create slog logger directly, added context to vulnDBSync |
| server/vulnerabilities/nvd/sync_test.go | Updated tests to use slog.DiscardHandler |
| server/vulnerabilities/nvd/sync/cve_syncer_test.go | Updated tests to use t.Context() and slog.DiscardHandler |
| server/vulnerabilities/nvd/sync/cve_syncer.go | Migrated CVE syncer to *slog.Logger, added context to functions, updated logging calls |
| server/vulnerabilities/nvd/sync.go | Migrated sync functions to *slog.Logger with context |
| server/vulnerabilities/nvd/db.go | Updated sqliteDBReadOnly to use *slog.Logger |
| server/vulnerabilities/nvd/cve_test.go | Updated tests to use slog.DiscardHandler |
| server/vulnerabilities/nvd/cve.go | Migrated CVE processing functions to *slog.Logger with context |
| server/vulnerabilities/nvd/cpe_test.go | Updated tests to use t.Context() and slog.DiscardHandler |
| server/vulnerabilities/nvd/cpe.go | Migrated CPE processing to *slog.Logger with context, updated software transformers |
| server/vulnerabilities/msrc/analyzer_test.go | Updated tests to use t.Context() and slog.DiscardHandler |
| server/vulnerabilities/msrc/analyzer.go | Migrated Windows vulnerability analyzer to *slog.Logger with context |
| server/vulnerabilities/goval_dictionary/sync.go | Migrated sync function to *slog.Logger with context |
| server/vulnerabilities/goval_dictionary/database_test.go | Updated tests to use t.Context() and slog.DiscardHandler |
| server/vulnerabilities/goval_dictionary/database.go | Migrated database evaluation to *slog.Logger with context |
| server/vulnerabilities/goval_dictionary/analyzer.go | Updated analyzer function to use *slog.Logger |
| server/vulnerabilities/customcve/matching_rules_test.go | Updated tests to use slog.DiscardHandler |
| server/vulnerabilities/customcve/matching_rules.go | Migrated custom CVE matching to *slog.Logger |
| cmd/fleet/cron.go | Updated cron jobs to call logger.SlogLogger() when passing to vulnerability functions |
| cmd/cve/validate/main.go | Migrated validation CLI tool to create slog logger directly, added context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40106 +/- ##
==========================================
+ Coverage 66.28% 66.30% +0.01%
==========================================
Files 2445 2446 +1
Lines 195899 196001 +102
Branches 8676 8676
==========================================
+ Hits 129847 129949 +102
Misses 54291 54291
Partials 11761 11761
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mostlikelee
left a comment
mostlikelee
left a comment
There was a problem hiding this comment.
LGTM, agree with the warning levels on many of the retryable failures
Related issue: Resolves #40054
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit