ROX-29921: Add deployment containers migration #17096
Draft
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
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 9e5a9ee. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17096 +/- ##
==========================================
- Coverage 48.79% 48.79% -0.01%
==========================================
Files 2712 2712
Lines 202332 202341 +9
==========================================
- Hits 98732 98727 -5
- Misses 95816 95827 +11
- Partials 7784 7787 +3
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:
|
ajheflin
force-pushed
the
ajheflin/deployment-container-migration
branch
from
fe36783 to
82782f5
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The PR description still contains placeholder 'change me!'—please provide a clear summary of the changes and their rationale.
- This PR touches many modules with near-identical v1/v2 branches; consider refactoring shared logic or introducing helper abstractions to reduce duplication.
- There are several TODOs in the migration and schema code—please address or remove these before merging, and ensure the migration test suite is fully implemented.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR description still contains placeholder 'change me!'—please provide a clear summary of the changes and their rationale.
- This PR touches many modules with near-identical v1/v2 branches; consider refactoring shared logic or introducing helper abstractions to reduce duplication.
- There are several TODOs in the migration and schema code—please address or remove these before merging, and ensure the migration test suite is fully implemented.
## Individual Comments
### Comment 1
<location> `migrator/migrations/m_213_to_m_214_populate_deployment_containers_imageidv2/migration_impl.go:47-56` </location>
<code_context>
+
+ db := database.PostgresDB
+
+ getStmt := `SELECT idx, image_name_fullname, image_id FROM deployments_containers WHERE image_id is not null AND image_id != '' AND image_name_fullname is not null AND image_name_fullname != ''`
+ rows, err := db.Query(database.DBCtx, getStmt)
+ if err != nil {
+ return err
+ }
+
+ containers, err := readRows(rows)
+ if err != nil {
+ return err
+ }
+ for _, container := range containers {
+ updateStmt := `UPDATE deployments_containers SET image_idv2 = $1 WHERE idx = $2`
+ _, err = db.Exec(database.DBCtx, updateStmt, uuid.NewV5FromNonUUIDs(container.ImageNameFullName, container.ImageID).String(), container.Idx)
</code_context>
<issue_to_address>
**issue (bug_risk):** Update statement does not scope by deployment, risking incorrect updates for containers with duplicate idx.
Include deployments_id in the WHERE clause of the update statement to ensure only the intended container row is updated.
</issue_to_address>
### Comment 2
<location> `central/enrichment/enricher_impl.go:69-71` </location>
<code_context>
+func (e *enricherImpl) EnrichDeploymentV2(ctx context.Context, enrichCtx enricher.EnrichmentContext, deployment *storage.Deployment) (images []*storage.ImageV2, updatedIndices []int, pendingEnrichment bool, err error) {
+ for i, c := range deployment.GetContainers() {
+ var imgToProcess *storage.ImageV2
+ if (enrichCtx.FetchOnlyIfMetadataEmpty() || enrichCtx.FetchOnlyIfScanEmpty()) && c.GetImage().GetIdV2() != "" {
+ var img *storage.ImageV2
+ img, _, err = e.imagesV2.GetImage(getImageContext, c.GetImage().GetId())
+ if err != nil {
+ return
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential mismatch between IdV2 and Id usage for image lookup.
If Id and IdV2 differ, using Id for the lookup may cause incorrect or missed image enrichment. Use IdV2 for lookups when available to ensure accuracy.
</issue_to_address>
### Comment 3
<location> `pkg/images/utils/utils.go:334-348` </location>
<code_context>
}
}
+// FilterSuppressedCVEsNoCloneV2 removes the vulns from the image that are currently suppressed
+func FilterSuppressedCVEsNoCloneV2(img *storage.ImageV2) {
+ cveSet := set.NewStringSet()
+ for _, c := range img.GetScan().GetComponents() {
+ filteredVulns := make([]*storage.EmbeddedVulnerability, 0, len(c.GetVulns()))
+ for _, vuln := range c.GetVulns() {
+ if !cve.IsCVESnoozed(vuln) {
+ cveSet.Add(vuln.GetCve())
+ filteredVulns = append(filteredVulns, vuln)
+ }
+ }
+ c.Vulns = filteredVulns
+ }
+ enricher.FillScanStatsV2(img)
+}
+
</code_context>
<issue_to_address>
**suggestion:** Suppressed CVEs filtering for V2 images may not handle edge cases with nil or missing fields.
Add nil checks for img.Scan and its components to prevent potential panics.
```suggestion
// FilterSuppressedCVEsNoCloneV2 removes the vulns from the image that are currently suppressed
func FilterSuppressedCVEsNoCloneV2(img *storage.ImageV2) {
if img == nil {
return
}
scan := img.GetScan()
if scan == nil {
return
}
components := scan.GetComponents()
if components == nil {
return
}
cveSet := set.NewStringSet()
for _, c := range components {
if c == nil {
continue
}
filteredVulns := make([]*storage.EmbeddedVulnerability, 0, len(c.GetVulns()))
for _, vuln := range c.GetVulns() {
if vuln == nil {
continue
}
if !cve.IsCVESnoozed(vuln) {
cveSet.Add(vuln.GetCve())
filteredVulns = append(filteredVulns, vuln)
}
}
c.Vulns = filteredVulns
}
enricher.FillScanStatsV2(img)
}
```
</issue_to_address>
### Comment 4
<location> `migrator/migrations/m_213_to_m_214_populate_deployment_containers_imageidv2/migration_test.go:36` </location>
<code_context>
+
+
+
+func (s *migrationTestSuite) TestMigration() {
+ // TODO(dont-merge): instantiate any store required for the pre-migration dataset push to DB
+
</code_context>
<issue_to_address>
**issue (testing):** Migration test contains only TODOs and lacks actual test logic.
Please add test logic to cover pre-migration setup, migration execution, and post-migration validation.
</issue_to_address>
### Comment 5
<location> `central/reprocessor/reprocessor_unit_test.go:528-154` </location>
<code_context>
+func TestReprocessImagesV2AndResyncDeployments_SkipBrokenSensor(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Tests for reprocessImagesV2AndResyncDeployments cover healthy and broken cluster scenarios.
Please add a test for the scenario where no images are found to verify early exit behavior.
Suggested implementation:
```golang
func TestReprocessImagesV2AndResyncDeployments_SkipBrokenSensor(t *testing.T) {
testutils.MustUpdateFeature(t, features.FlattenImageData, true)
imgs := []*storage.ImageV2{}
for _, cluster := range []string{"a", "b"} { // two clusters
// Create at least one more image than max semaphore size to ensure skip logic is executed.
for i := range imageReprocessorSemaphoreSize + 1 {
imgs = append(imgs, &storage.ImageV2{Id: fmt.Sprintf("img%d-%s", i, cluster)})
}
}
results := []search.Result{}
}
// Test for early exit when no images are found
func TestReprocessImagesV2AndResyncDeployments_NoImagesEarlyExit(t *testing.T) {
testutils.MustUpdateFeature(t, features.FlattenImageData, true)
imgs := []*storage.ImageV2{} // No images
// You may need to mock dependencies here as in other tests
// For example, mock the deployment resyncer, logger, etc.
// Call the function under test
// Replace the following with the actual function call and assertions
// For example:
// err := reprocessImagesV2AndResyncDeployments(imgs, ...)
// require.NoError(t, err)
// require.NoProcessingOccurred(t) // Replace with actual assertion
// For demonstration, we'll just assert that the slice is empty and function returns early
if len(imgs) != 0 {
t.Errorf("Expected no images, got %d", len(imgs))
}
// TODO: Add assertion to verify early exit (e.g., no deployments resynced, no errors, etc.)
}
```
You may need to:
1. Add the actual call to `reprocessImagesV2AndResyncDeployments` with appropriate mocked dependencies.
2. Add assertions to verify that no processing or resyncing occurred (e.g., using mocks or checking side effects).
3. Adjust the test to fit your test setup and dependency injection patterns.
</issue_to_address>
### Comment 6
<location> `central/reprocessor/reprocessor_test.go:80-77` </location>
<code_context>
+func TestImagesWithSignaturesQueryV2(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Test for ImagesWithSignaturesQueryV2 mirrors v1 logic and covers basic cases.
Please add a test case for when no images have signatures to verify the query returns an empty result set.
</issue_to_address>
### Comment 7
<location> `central/reprocessor/reprocessor_unit_test.go:157-154` </location>
<code_context>
})
}
+func TestReprocessWatchedImageV2Delegation(t *testing.T) {
+ testutils.MustUpdateFeature(t, features.FlattenImageData, true)
+ t.Run("delegation disabled", func(t *testing.T) {
+ testutils.MustUpdateFeature(t, features.DelegateWatchedImageReprocessing, false)
+
+ enrichmentCtx := gomock.Cond(func(ctxRaw any) bool {
+ // Ensure that the enrichment isn't delegable.
+ ectx := ctxRaw.(imageEnricher.EnrichmentContext)
+ return !ectx.Delegable
+ })
+
+ ctrl := gomock.NewController(t)
+ enricher := mocks.NewMockImageEnricherV2(ctrl)
+ enricher.EXPECT().EnrichImage(emptyCtx, enrichmentCtx, gomock.Any())
+
+ loop := &loopImpl{imageEnricherV2: enricher}
+ loop.reprocessWatchedImageV2("example.com/repo/path:tag")
+ })
+
+ t.Run("delegation enabled", func(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Tests for reprocessWatchedImageV2Delegation cover delegation enabled/disabled.
Please add a test case for error handling in the enrichment process, such as when EnrichImage returns an error, to verify graceful failure handling.
Suggested implementation:
```golang
t.Run("delegation enabled", func(t *testing.T) {
testutils.MustUpdateFeature(t, features.DelegateWatchedImageReprocessing, true)
ctx := gomock.Cond(func(ctxRaw any) bool {
// Delegation will fail if context does not have image read access.
ctx := ctxRaw.(context.Context)
scopeChecker := sac.GlobalAccessScopeChecker(ctx).
AccessMode(storage.Access_READ_ACCESS).
Resource(resources.Image)
return scopeChecker.IsAllowed()
})
})
t.Run("enrichment error handling", func(t *testing.T) {
testutils.MustUpdateFeature(t, features.DelegateWatchedImageReprocessing, false)
enrichmentCtx := gomock.Cond(func(ctxRaw any) bool {
ectx := ctxRaw.(imageEnricher.EnrichmentContext)
return !ectx.Delegable
})
ctrl := gomock.NewController(t)
enricher := mocks.NewMockImageEnricherV2(ctrl)
expectedErr := errors.New("enrichment failed")
enricher.EXPECT().EnrichImage(emptyCtx, enrichmentCtx, gomock.Any()).Return(nil, expectedErr)
loop := &loopImpl{imageEnricherV2: enricher}
// The function under test should handle the error gracefully.
// If it returns an error, you can check it here. If it logs, you may want to capture logs.
// For now, we just ensure it does not panic.
defer func() {
if r := recover(); r != nil {
t.Errorf("reprocessWatchedImageV2 panicked on enrichment error: %v", r)
}
}()
loop.reprocessWatchedImageV2("example.com/repo/path:tag")
```
If `reprocessWatchedImageV2` returns an error, you should capture and assert it in the test. If it logs errors, consider using a log capturing utility to assert the log output. Adjust the test as needed to match your error handling conventions.
</issue_to_address>
### Comment 8
<location> `central/enrichment/enricher_impl.go:66` </location>
<code_context>
return
}
+
+func (e *enricherImpl) EnrichDeploymentV2(ctx context.Context, enrichCtx enricher.EnrichmentContext, deployment *storage.Deployment) (images []*storage.ImageV2, updatedIndices []int, pendingEnrichment bool, err error) {
+ for i, c := range deployment.GetContainers() {
+ var imgToProcess *storage.ImageV2
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the duplicated enrichment logic into a single generic helper function using Go generics.
```suggestion
You can collapse both `EnrichDeployment` and `EnrichDeploymentV2` into a single generic helper that takes the type‐specific bits as arguments. Here’s a minimal sketch using Go 1.18+ generics:
```go
// shared.go
package enrichment
import (
"context"
"github.com/stackrox/rox/pkg/images/enricher"
"github.com/stackrox/rox/pkg/sac"
)
// result holds the common return values
type enrichResult[T any] struct {
images []T
updatedIndices []int
pendingEnrichment bool
}
// enrichGeneric runs the loop, calling type‐specific functions as parameters.
func enrichGeneric[T any](
ctx context.Context,
enrichCtx enricher.EnrichmentContext,
containers []*storage.Container,
getID func(*storage.Container) string,
fetch func(context.Context, string) (T, bool, error),
toImage func(*storage.Image) T,
isNotPullable func(T) bool,
doEnrich func(context.Context, enricher.EnrichmentContext, T) (enricher.EnrichmentResult, error),
) (enrichResult[T], error) {
var res enrichResult[T]
for i, c := range containers {
var img T
id := getID(c)
if (enrichCtx.FetchOnlyIfMetadataEmpty() || enrichCtx.FetchOnlyIfScanEmpty()) && id != "" {
var found bool
img, found, err = fetch(sac.WithGlobalAccessScopeChecker(context.Background(), /*…*/), id)
if err != nil || !found {
return res, err
}
}
// default
if reflect.ValueOf(img).IsZero() {
img = toImage(c.GetImage())
}
res.images = append(res.images, img)
if id != "" && isNotPullable(img) {
continue
}
er, err := doEnrich(ctx, enrichCtx, img)
if err != nil {
log.Error(err)
}
if er.ImageUpdated {
res.updatedIndices = append(res.updatedIndices, i)
}
if er.ScanResult == enricher.ScanTriggered {
res.pendingEnrichment = true
}
}
return res, nil
}
```
```go
// v1_and_v2.go
package enrichment
func (e *enricherImpl) EnrichDeployment(
ctx context.Context,
enrichCtx enricher.EnrichmentContext,
deployment *storage.Deployment,
) ([]*storage.Image, []int, bool, error) {
out, err := enrichGeneric(
ctx, enrichCtx, deployment.GetContainers(),
func(c *storage.Container) string { return c.GetImage().GetId() },
func(c context.Context, id string) (*storage.Image, bool, error) {
return e.images.GetImage(c, id)
},
func(i *storage.Image) *storage.Image { return i },
func(i *storage.Image) bool { return i.GetNotPullable() },
e.imageEnricher.EnrichImage,
)
return out.images, out.updatedIndices, out.pendingEnrichment, err
}
func (e *enricherImpl) EnrichDeploymentV2(
ctx context.Context,
enrichCtx enricher.EnrichmentContext,
deployment *storage.Deployment,
) ([]*storage.ImageV2, []int, bool, error) {
out, err := enrichGeneric(
ctx, enrichCtx, deployment.GetContainers(),
func(c *storage.Container) string { return c.GetImage().GetIdV2() },
func(c context.Context, id string) (*storage.ImageV2, bool, error) {
return e.imagesV2.GetImage(c, id)
},
types.ToImageV2,
func(i *storage.ImageV2) bool { return i.GetNotPullable() },
e.imageEnricherV2.EnrichImage,
)
return out.images, out.updatedIndices, out.pendingEnrichment, err
}
```
Steps:
1. Copy the common loop into `enrichGeneric` with a type parameter `T any`.
2. Parameterize everything that differs (ID extractor, store fetcher, converter, pullable check, enricher).
3. Make `EnrichDeployment` / `EnrichDeploymentV2` trivial callers of `enrichGeneric`.
This removes line‐for‐line duplication while preserving both versions’ behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+47
to
+56
| getStmt := `SELECT idx, image_name_fullname, image_id FROM deployments_containers WHERE image_id is not null AND image_id != '' AND image_name_fullname is not null AND image_name_fullname != ''` | ||
| rows, err := db.Query(database.DBCtx, getStmt) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| containers, err := readRows(rows) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Update statement does not scope by deployment, risking incorrect updates for containers with duplicate idx.
Include deployments_id in the WHERE clause of the update statement to ensure only the intended container row is updated.
Comment on lines
69
to
71
| if (enrichCtx.FetchOnlyIfMetadataEmpty() || enrichCtx.FetchOnlyIfScanEmpty()) && c.GetImage().GetIdV2() != "" { | ||
| var img *storage.ImageV2 | ||
| img, _, err = e.imagesV2.GetImage(getImageContext, c.GetImage().GetId()) |
There was a problem hiding this comment.
issue (bug_risk): Potential mismatch between IdV2 and Id usage for image lookup.
If Id and IdV2 differ, using Id for the lookup may cause incorrect or missed image enrichment. Use IdV2 for lookups when available to ensure accuracy.
|
|
||
|
|
||
|
|
||
| func (s *migrationTestSuite) TestMigration() { |
There was a problem hiding this comment.
issue (testing): Migration test contains only TODOs and lacks actual test logic.
Please add test logic to cover pre-migration setup, migration execution, and post-migration validation.
ajheflin
force-pushed
the
ajheflin/deployment-container-migration
branch
from
82782f5 to
19befcf
Compare
ajheflin
force-pushed
the
ajheflin/deployment-container-migration
branch
from
19befcf to
9e5a9ee
Compare
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
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!