stackrox icon indicating copy to clipboard operation
stackrox copied to clipboard

ROX-27404: Check if new CVE data model was activated during upgrade

Open ajheflin opened this issue 9 months ago • 5 comments

Description

change me!

User-facing documentation

  • [ ] CHANGELOG is updated OR update is not needed
  • [ ] documentation PR is created and is linked above OR is not needed

Testing and quality

  • [ ] the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
  • [ ] CI results are inspected

Automated testing

  • [ ] added unit tests
  • [ ] added e2e tests
  • [ ] added regression tests
  • [ ] added compatibility tests
  • [ ] modified existing tests

How I validated my change

change me!

Summary by Sourcery

Add a check to detect and handle the first startup of a new CVE data model during system upgrade

New Features:

  • Implement a mechanism to detect when the new CVE data model is first activated during system upgrade

Enhancements:

  • Add logic to trigger reprocessing of image integrations when transitioning to a new CVE data model

Chores:

  • Update image integration registration process to support data model migration

ajheflin avatar Apr 08 '25 17:04 ajheflin

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

openshift-ci[bot] avatar Apr 08 '25 17:04 openshift-ci[bot]

Reviewer's Guide by Sourcery

This pull request adds a check to determine if the new CVE data model was activated during the upgrade process. If the new data model was activated and the V1 component data store has data while the V2 component data store is empty, it will reprocess all image integrations.

Sequence diagram for reprocessing image integrations on new CVE data model activation

sequenceDiagram
  participant main.go
  participant imageComponentDataStore
  participant imageComponentV2DataStore
  participant reprocessor

  main.go->>imageComponentDataStore: Count(v1)
  activate imageComponentDataStore
  imageComponentDataStore-->>main.go: count
  deactivate imageComponentDataStore
  main.go->>imageComponentV2DataStore: Count(v2)
  activate imageComponentV2DataStore
  imageComponentV2DataStore-->>main.go: count
  deactivate imageComponentV2DataStore
  alt v1Count > 0 && v2Count == 0
    main.go->>reprocessor: ShortCircuit()
    activate reprocessor
    reprocessor-->>main.go: 
    deactivate reprocessor
  end

File-Level Changes

Change Details Files
Added a check to determine if the new CVE data model was activated during the upgrade process.
  • Added a check to see if the FlattenCVEData feature flag is enabled.
  • If the feature flag is enabled, it checks if the V1 component data store has data while the V2 component data store is empty.
  • If the condition is met, it marks a flag to reprocess all image integrations.
  • The image integrations will be reprocessed if the newCVEDataModelFirstStart flag is true.
central/main.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an issue from a review comment by replying to it. You can also reply to a review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in the pull request body to generate a PR summary at any time exactly where you want it. You can also comment @sourcery-ai summary on the pull request to (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the pull request to resolve all Sourcery comments. Useful if you've already addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull request to dismiss all existing Sourcery reviews. Especially useful if you want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

  • Contact our support team for questions or feedback.
  • Visit our documentation for detailed guides and information.
  • Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai[bot] avatar Apr 08 '25 17:04 sourcery-ai[bot]

Images are ready for the commit at ce79fdc.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-604-gce79fdca78.

rhacs-bot avatar Apr 08 '25 18:04 rhacs-bot

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 49.07%. Comparing base (517b2b5) to head (ce79fdc). Report is 68 commits behind head on master.

Files with missing lines Patch % Lines
central/image/datastore/store/v2/postgres/store.go 53.84% 4 Missing and 2 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14924      +/-   ##
==========================================
+ Coverage   49.02%   49.07%   +0.04%     
==========================================
  Files        2550     2557       +7     
  Lines      187296   187839     +543     
==========================================
+ Hits        91827    92183     +356     
- Misses      88201    88380     +179     
- Partials     7268     7276       +8     
Flag Coverage Δ
go-unit-tests 49.07% <53.84%> (+0.04%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Apr 08 '25 20:04 codecov[bot]

This PR aims to trigger a re-scan of images on upgrade if the new CVE data model feature flag was set for the first time during the upgrade. If so, this should populate all of the new tables with CVE data for the images that exist in the images table.

I think I may be missing something - Central's reprocessor will re-scan all images on startup (existing functionality).

The effective change here appears to be a new log line and an overwrite of the default scanner integration? (which I believe will erase user specific settings for that integration)

I may be missing something.

So, from what I could tell, and from my testing that I did, the scan only occurs on a first start, since if the image integration is registered in the DB, it skips actually processing that image integration in the startup process. This just bypasses that skipping logic if this startup is the first startup since we enabled the feature flag for the new CVE data model.

ajheflin avatar Apr 19 '25 00:04 ajheflin

So, from what I could tell, and from my testing that I did, the scan only occurs on a first start, since if the image integration is registered in the DB, it skips actually processing that image integration in the startup process. This just bypasses that skipping logic if this startup is the first startup since we enabled the feature flag for the new CVE data model.

Think we should revisit, I'm happy to chat about options.

User customizations to the StackRox Scanner integration will be overwritten (which we shouldn't be doing). Also the only delayed integration is the StackRox Scanner which is deprecated and will be removed in a future release (Scanner V4 does not use a delayed integration by design).

The reprocessors short circuit will only trigger a re-scan for images that have no pre-existing scans - so this may not fully achieve the desired goal of ensuring everything is re-scanned / re-saved.

the scan only occurs on a first start,

A full scan should occur on first start and every 4 hours after that (by default). Am interested to see what you're seeing, perhaps we have a bug.

dcaravel avatar Apr 21 '25 14:04 dcaravel

After quite a few commits, this is ready to go

ajheflin avatar Apr 29 '25 17:04 ajheflin

So, from what I could tell, and from my testing that I did, the scan only occurs on a first start, since if the image integration is registered in the DB, it skips actually processing that image integration in the startup process. This just bypasses that skipping logic if this startup is the first startup since we enabled the feature flag for the new CVE data model.

Think we should revisit, I'm happy to chat about options.

User customizations to the StackRox Scanner integration will be overwritten (which we shouldn't be doing). Also the only delayed integration is the StackRox Scanner which is deprecated and will be removed in a future release (Scanner V4 does not use a delayed integration by design).

The reprocessors short circuit will only trigger a re-scan for images that have no pre-existing scans - so this may not fully achieve the desired goal of ensuring everything is re-scanned / re-saved.

the scan only occurs on a first start,

A full scan should occur on first start and every 4 hours after that (by default). Am interested to see what you're seeing, perhaps we have a bug.

Turns out what I was experiencing was caused by something on the store layer and the results I was seeing initially were misleading, so, barring feedback that says otherwise, I think scanner should be in the clear here

ajheflin avatar Apr 29 '25 17:04 ajheflin