ROX-34024: Use name-based matching in populateImageMetadata#20021
ROX-34024: Use name-based matching in populateImageMetadata#20021
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When a container name from
ContainerStatusesor the pod spec is not found incontainersByName/specContainersByNamethe code silently continues; consider at least a debug log in these branches to aid diagnosing unexpected name mismatches between deployment and pods. - The comment above
containersByNamehas a small formatting issue (//present); fixing this improves readability and keeps comments consistent with the rest of the file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When a container name from `ContainerStatuses` or the pod spec is not found in `containersByName`/`specContainersByName` the code silently continues; consider at least a debug log in these branches to aid diagnosing unexpected name mismatches between deployment and pods.
- The comment above `containersByName` has a small formatting issue (`//present`); fixing this improves readability and keeps comments consistent with the rest of the file.
## Individual Comments
### Comment 1
<location path="sensor/kubernetes/listener/resources/convert.go" line_range="334" />
<code_context>
+ specContainersByName[sc.Name] = sc
+ }
+
+ for _, c := range p.Status.ContainerStatuses {
+ deployContainer, found := containersByName[c.Name]
+ if !found {
</code_context>
<issue_to_address>
**suggestion:** Consider surfacing mismatches between deployment/spec containers and status containers instead of silently skipping.
Right now, missing entries in `containersByName` or `specContainersByName` are just skipped with `continue`, which hides cases where the deployment spec and pod status diverge (e.g., renamed, init, ephemeral, or partially failed containers). Consider at least a debug log in these branches, and ideally treat known container types (init/ephemeral) differently from truly unexpected container names so mismatches are visible during diagnosis.
Suggested implementation:
```golang
// Build a map from container name to pod spec container for name-based lookup.
specContainersByName := make(map[string]v1.Container, len(p.Spec.Containers))
for _, sc := range p.Spec.Containers {
specContainersByName[sc.Name] = sc
}
for _, c := range p.Status.ContainerStatuses {
deployContainer, found := containersByName[c.Name]
if !found {
// Surface mismatches between deployment containers and pod status containers.
// These can indicate renamed or unexpected containers and are useful for diagnosis.
log.Debugf("Pod %s/%s: skipping status container %q with no matching container in deployment spec; this may indicate a renamed or unexpected container",
p.GetNamespace(), p.GetName(), c.Name)
continue
}
image := deployContainer.GetImage()
var runtimeImageName *storage.ImageName
if features.UnqualifiedSearchRegistries.Enabled() && c.ImageID != "" {
continue
}
```
1. Ensure this file has access to a `log` logger consistent with the rest of the package, for example:
```go
var (
log = logging.LoggerForModule()
)
```
and that `github.com/stackrox/rox/pkg/logging` (or the appropriate logging package used elsewhere in this directory) is imported.
2. In the corresponding code paths where `specContainersByName` is used and a status container is not found for a spec container (likely another `if !found { continue }` branch when iterating spec containers), add a similar `Debugf` log. For spec containers, the message should emphasize that a container defined in the pod spec has no corresponding status entry, and explicitly mention if it is an init or ephemeral container when you have that context (e.g. when iterating over `p.Spec.InitContainers` or `p.Spec.EphemeralContainers` and their respective status slices).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
995c863 to
c6698a9
Compare
📝 WalkthroughSummary by CodeRabbitBug Fixes
WalkthroughThe changes replace index-based container matching with name-based lookup using maps in the Kubernetes listener's container conversion logic. Tests are updated to assign deterministic container names to align with the refactored matching approach. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sensor/kubernetes/listener/resources/convert_test.go (1)
394-418:⚠️ Potential issue | 🟠 MajorAdd a regression case with init containers.
Lines 394-418 and 553-577 only give the existing fixtures stable names; they still keep a 1:1 regular-container mapping, so the previous sort+index implementation would also pass. Please add a case where
wrap.Containersincludes an init container butpod.Status.ContainerStatusesonly includes the regular container, otherwise the bug this PR fixes is not actually locked in by tests.Also applies to: 553-577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sensor/kubernetes/listener/resources/convert_test.go` around lines 394 - 418, Add a regression fixture that includes an init container in wrap.Containers but does not include a corresponding init entry in pod.Status.ContainerStatuses so the test verifies mapping by name (not by 1:1 index). Concretely, when building wrap.Containers (the loop that appends to wrap.Containers) add at least one container representing an init container (unique Name like "init-container-0" or with an Init flag in your fixture data) while constructing the k8s Pod(s) for pods and pod.Status.ContainerStatuses only append the regular container statuses (no status for the init container). Repeat the same change in the other fixture section referenced (the block around lines 553-577) so both test cases include this mismatch and guard against the old sort+index bug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sensor/kubernetes/listener/resources/convert_test.go`:
- Around line 394-418: Add a regression fixture that includes an init container
in wrap.Containers but does not include a corresponding init entry in
pod.Status.ContainerStatuses so the test verifies mapping by name (not by 1:1
index). Concretely, when building wrap.Containers (the loop that appends to
wrap.Containers) add at least one container representing an init container
(unique Name like "init-container-0" or with an Init flag in your fixture data)
while constructing the k8s Pod(s) for pods and pod.Status.ContainerStatuses only
append the regular container statuses (no status for the init container). Repeat
the same change in the other fixture section referenced (the block around lines
553-577) so both test cases include this mismatch and guard against the old
sort+index bug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f72ec729-50d4-443f-8720-a7ced6ed55a1
📒 Files selected for processing (2)
sensor/kubernetes/listener/resources/convert.gosensor/kubernetes/listener/resources/convert_test.go
🚀 Build Images ReadyImages are ready for commit 5371377. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-659-g53713779a1 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20021 +/- ##
=======================================
Coverage 49.61% 49.61%
=======================================
Files 2765 2765
Lines 208649 208654 +5
=======================================
+ Hits 103523 103526 +3
- Misses 97471 97472 +1
- Partials 7655 7656 +1
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:
|
c6698a9 to
5371377
Compare
|
/test gke-qa-e2e-tests |
|
/retest |
Description
Changes
populateImageMetadatain Sensor to use name-based container matching instead of index-based matching when correlating pod container statuses with deployment containers.The current implementation sorts deployment containers and pod container statuses by name, then aligns them by index. This will break when init containers are present because
deployment.Containersis a unified list containing both init and regular containers, whilepod.Status.ContainerStatusesonly contains regular containers. Index-based alignment then assigns the wrong image digests to the wrong containers.The fix replaces index-based alignment with name-based map lookups: deployment containers and pod spec containers are indexed by name, and each pod container status is matched to its corresponding deployment container via
containersByName[c.Name]. This correctly handles deployments with init containers, regular containers, or both. Existing unit tests are updated to include container names so name-based matching is exercised.User-facing documentation
Testing and quality
Automated testing
How I validated my change
Relying on CI + no regressions in existing tests.
TestPopulateImageMetadataandTestPopulateImageMetadataWithUnqualifiedwere updated to assign consistent container names across deployment containers, pod spec containers, and pod container statuses.