Repository navigation
Conversation
…image The chart applies the images.* repository path rule to the Collector image: uppercase is allowed only in the registry host and the tag. Its looser shape rule stays, so an IPv6 registry host still works. Profiles take no Collector image and leave the chart default.
…rcase path in the Collector guide
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Codex review: needs maintainer review before merge. What this changesThe Helm chart rejects uppercase repository paths in enabled Collector images and documents the restriction. Example: Enable the Collector with docker.io/Otel/collector@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it closes a verified Collector image-validation gap on main, and no introduced correctness or security defect was found. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe production Helm chart consumes operator values and emits Collector workload, identity, and network-policy manifests; image validation runs before those manifests are rendered. flowchart TD
A[Operator values] --> B[Collector chart validation]
B --> C[Invalid image refusal]
B --> D[Collector DaemonSet]
D --> E[Kubernetes image pull]
Technical reviewBest possible solution: Reject unusable Collector repository paths at the existing Helm validation boundary while preserving valid image spellings. Do we have a high-confidence way to reproduce the issue? Main's Collector guard accepts docker.io/Otel/collector with a lowercase SHA-256 digest; the PR reports failing-before and passing-after Helm cases, but this review executed no target code. Is this the best way to solve the issue? The patch fixes the omitted consumer at its existing validation boundary, preserves the intentionally broader Collector image syntax, and adds relevant positive and negative integration coverage. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e4aae3e111c7. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 13, 2026, 5:07 PM ET / 21:07 UTC. |
Problem
#2358 and #2362 made profile preflight and the chart refuse an uppercase letter in an image repository path, which Kubernetes refuses to pull (
InvalidImageName).logging.collector.imagewas left out: the chart checks it with its own looser rule (^[^[:space:]@]+@sha256:[a-f0-9]{64}$), which acceptsdocker.io/Otel/collector@sha256:..., so the Collector DaemonSet renders and then never pulls.Change
logging.collector.imagehas an uppercase repository path fails withlogging.collector.image repository path must be lowercase; Kubernetes refuses to pull an uppercase one, using theimages.*rule (uppercase only in the registry host, the first component when a/follows it, and in the tag).images.*shape comes fromprepare-bootstrap-volume, which never sees the Collector image, and the looser rule lets the Collector use an IPv6 registry host ([fd00::1]:5000/otel/collector@sha256:...), asruntime.imagecan. Other spellings the looser rule still accepts (a tag with no repository,+or?in the path) are unchanged.controlPlane.loggingCollectoraccepts onlyenabledandexporter, so preflight refuses aloggingCollector.imagefield and writes none, and the chart renders its default (lowercase) image. The new tests pin both.upgrade-production-imagesis unchanged: its Collector drift guard compares only the config Secret, never the image.logging.collector.imageamong the chart fields; the observability guide asks for a lowercase repository path.Verification
profile-preflight-chart-parity.test.mjs: the chart refusesregistry.example.invalid/Otel/collector,...:5000/otel/Collector:release_1,Registry.Example.invalid/otel/Collector,[fd00::1]:5000/otel/CollectorandCollector, and accepts an uppercase registry host and tag and an IPv6 registry host (also with uppercase hex). A profile withloggingCollector.imagefails preflight; an enabled Collector profile writes no image and renders the chart default.logging-packaging.test.mjs: an uppercase Collector repository path fails with the new message.profile-renderer.test.mjs: acontrolPlane.loggingCollector.imagerow in the Collector refusal table (passes on main too: it pins existing behaviour).taskset -c 0,1): the chart parity Collector and uppercase tests, the renderer Collector refusal table and the logging-packaging refusal test, each 20/20 (and 20/20 again after the review nits).Risks