Skip to content

ROX-34024: Use name-based matching in populateImageMetadata#20021

Open
AlexVulaj wants to merge 1 commit intomasterfrom
AlexVulaj/ROX-34024-name-based-image-metadata-matching
Open

ROX-34024: Use name-based matching in populateImageMetadata#20021
AlexVulaj wants to merge 1 commit intomasterfrom
AlexVulaj/ROX-34024-name-based-image-metadata-matching

Conversation

@AlexVulaj
Copy link
Copy Markdown
Contributor

Description

Changes populateImageMetadata in 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.Containers is a unified list containing both init and regular containers, while pod.Status.ContainerStatuses only 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

  • 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

Relying on CI + no regressions in existing tests. TestPopulateImageMetadata and TestPopulateImageMetadataWithUnqualified were updated to assign consistent container names across deployment containers, pod spec containers, and pod container statuses.

@AlexVulaj AlexVulaj requested a review from a team as a code owner April 15, 2026 14:14
@AlexVulaj AlexVulaj self-assigned this Apr 15, 2026
@AlexVulaj AlexVulaj requested review from dcaravel and removed request for a team April 15, 2026 14:14
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread sensor/kubernetes/listener/resources/convert.go
@AlexVulaj AlexVulaj force-pushed the AlexVulaj/ROX-34024-name-based-image-metadata-matching branch from 995c863 to c6698a9 Compare April 15, 2026 14:20
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 15, 2026

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes

  • Enhanced container status detection in Kubernetes deployments by using container names for matching instead of positional ordering, improving accuracy when containers are not consistently aligned.

Walkthrough

The 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

Cohort / File(s) Summary
Core Container Matching Logic
sensor/kubernetes/listener/resources/convert.go
Refactored container alignment from index-based sorting to name-based map lookup. Precomputes maps of container names to deployment containers and pod spec containers. Skips processing when containers lack matching entries instead of using bounds checks. Removed stable sorting of container slices.
Test Fixtures
sensor/kubernetes/listener/resources/convert_test.go
Updated test cases to assign deterministic container names (e.g., container-0, container-1) via fmt.Sprintf in storage.Container, v1.Container, and v1.ContainerStatus entries during fixture construction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: switching from index-based to name-based matching in populateImageMetadata, with the ticket reference for traceability.
Description check ✅ Passed The description comprehensively covers the problem (index-based matching breaking with init containers), the solution (name-based map lookups), testing approach (modified existing tests), and all required template sections are addressed with appropriate checkmarks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch AlexVulaj/ROX-34024-name-based-image-metadata-matching

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

Add 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.Containers includes an init container but pod.Status.ContainerStatuses only 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

📥 Commits

Reviewing files that changed from the base of the PR and between d721274 and c6698a9.

📒 Files selected for processing (2)
  • sensor/kubernetes/listener/resources/convert.go
  • sensor/kubernetes/listener/resources/convert_test.go

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 15, 2026

🚀 Build Images Ready

Images are ready for commit 5371377. To use with deploy scripts:

export MAIN_IMAGE_TAG=4.11.x-659-g53713779a1

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2026

Codecov Report

❌ Patch coverage is 77.27273% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.61%. Comparing base (d721274) to head (5371377).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
sensor/kubernetes/listener/resources/convert.go 77.27% 3 Missing and 2 partials ⚠️
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     
Flag Coverage Δ
go-unit-tests 49.61% <77.27%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@AlexVulaj AlexVulaj force-pushed the AlexVulaj/ROX-34024-name-based-image-metadata-matching branch from c6698a9 to 5371377 Compare April 15, 2026 14:42
@AlexVulaj
Copy link
Copy Markdown
Contributor Author

/test gke-qa-e2e-tests

@AlexVulaj
Copy link
Copy Markdown
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant