OCPBUGS-88461: fix(helm): tolerate v-prefix version mismatch in disk-to-mirror#1430
Conversation
WalkthroughThis PR adds resolveChartPath to locate local Helm chart tarballs with optional leading "v" in versions, integrates it into the disk-to-mirror flow (errors are aggregated and processing continues), and adds tests covering matching, mismatching, missing, and an end-to-end DiskToMirror case. ChangesChart Version Prefix Resolution
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/pkg/helm/local_stored_collector_test.go (1)
771-777: ⚡ Quick winAssert the descriptive error contract in the not-found case.
Line 772 only checks that an error exists. Since this helper is expected to report both attempted candidates, assert those path fragments to prevent silent regressions in error quality.
✅ Suggested test addition
got, err := resolveChartPath(dir, tc.chartName, tc.version) if tc.wantErr { assert.Error(t, err) + assert.Contains(t, err.Error(), fmt.Sprintf("%s-%s.tgz", tc.chartName, tc.version)) + altVersion := "v" + tc.version + if strings.HasPrefix(tc.version, "v") { + altVersion = strings.TrimPrefix(tc.version, "v") + } + assert.Contains(t, err.Error(), fmt.Sprintf("%s-%s.tgz", tc.chartName, altVersion)) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/pkg/helm/local_stored_collector_test.go` around lines 771 - 777, When tc.wantErr is true, instead of only checking that err is non-nil, also assert the error string includes the two attempted path fragments so the descriptive "not found" contract doesn't regress; update the block inside the if tc.wantErr branch in local_stored_collector_test.go to call assert.Error(t, err) followed by assert.Contains(t, err.Error(), tc.candidateA) and assert.Contains(t, err.Error(), tc.candidateB) (use the actual test-case fields that hold the two candidate paths) so both attempted candidates are verified in the error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/pkg/helm/local_stored_collector.go`:
- Around line 363-381: The code builds primary and alt paths from untrusted
name/version (primary, alt) and can escape dir via path traversal; sanitize and
canonicalize: first reject or normalize name and version (remove path separators
or enforce a regex), then construct candidate paths and call filepath.Clean +
filepath.Abs on both the candidate and dir, and verify the candidate is inside
dir (e.g., use filepath.Rel or check that candidateAbs has dirAbs as a prefix)
before using them; apply these checks for both primary and alt before calling
os.Stat and before returning or logging (referencing variables primary, alt,
dir, name, version and the local_stored_collector logic).
- Around line 364-379: The code treats any os.Stat error as "file missing" for
primary and alt chart paths (variables primary and alt), which hides real
permission/I/O failures; update both os.Stat checks so that if err != nil and
!os.IsNotExist(err) you immediately return the wrapped error (include context
like the path and the original err) instead of falling through — keep the
existing not-found logic when os.IsNotExist(err) is true and retain the
lsc.Log.Debug call only when switching to the v-prefixed alt variant.
---
Nitpick comments:
In `@internal/pkg/helm/local_stored_collector_test.go`:
- Around line 771-777: When tc.wantErr is true, instead of only checking that
err is non-nil, also assert the error string includes the two attempted path
fragments so the descriptive "not found" contract doesn't regress; update the
block inside the if tc.wantErr branch in local_stored_collector_test.go to call
assert.Error(t, err) followed by assert.Contains(t, err.Error(), tc.candidateA)
and assert.Contains(t, err.Error(), tc.candidateB) (use the actual test-case
fields that hold the two candidate paths) so both attempted candidates are
verified in the error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35417b06-f3d1-49a0-9673-a498abbe2d25
📒 Files selected for processing (2)
internal/pkg/helm/local_stored_collector.gointernal/pkg/helm/local_stored_collector_test.go
During the disk-to-mirror phase oc-mirror reconstructed chart tarball
paths as {name}-{version}.tgz, but the Helm downloader saves files
using the URL basename from the repository index.yaml, which may embed
a "v" prefix (e.g. csi-driver-nfs-v4.9.0.tgz). This caused d2m to
fail with "file not found" even though the m2d download succeeded.
Introduce resolveChartPath which tries the primary path first and falls
back to toggling the "v" prefix on the version component:
- Path traversal is prevented by confining both candidates to the
charts directory via filepath.Rel.
- Non-ENOENT os.Stat errors (permission/I/O) are surfaced immediately
rather than being silently treated as "file missing".
- A descriptive error naming both attempted paths is returned when
neither candidate exists.
Add two new test functions:
- TestResolveChartPath: unit tests for the helper covering all four
cases (exact match, config omits v / disk has v, config has v / disk
has no v, both missing). The not-found case asserts both candidate
path fragments appear in the error message.
- TestHelmImageCollectorVPrefixDiskToMirror: end-to-end d2m collector
test that places a v-prefixed chart on disk while the config specifies
the bare version, and the reverse.
Co-authored-by: Cursor <cursoragent@cursor.com>
Sourav21jain
force-pushed
the
fix/helm-v-prefix-version-mismatch
branch
from
a539ead to
54ced19
Compare
|
/test lint |
|
Hi @Sourav21jain, Could you please have a look on the lint job failing and also provide the image set config you used to test the scenario with and without Thanks. |
|
Extract chartPathExists helper to handle os.Stat + error classification logic, dropping resolveChartPath's cyclomatic complexity from 11 to 6 to satisfy the cyclop linter's max-complexity limit of 10. Also promote buildChartCandidatePath and toggleVersionPrefix from inline closures to package-level functions for clarity. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@aguidirh I have updated the code as well as the file for testing. If you need anything else please let me know. |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for fixing it! I noticed we can simplify this implementation using the semver.NewVersion() function that we already use elsewhere in the codebase (e.g., internal/pkg/config/validate.go).
The semver.NewVersion() function from github.com/Masterminds/semver/v3 automatically handles the "v" prefix - it accepts both "v5.0.0" and "5.0.0" and normalizes them to "5.0.0".
I've tested this approach and it seems it works fine.
1. Add semver import:
import (
"bytes"
"context"
"errors"
"fmt"
"io/fs"
"net/http"
"os"
"path/filepath"
"strings"
"github.com/Masterminds/semver/v3" // Add this line
helmchart "helm.sh/helm/v3/pkg/chart"
// ... rest of imports2. Simplication of the resolveChartPath() function:
func resolveChartPath(dir, name, version string) (string, error) {
// Normalize version using semver (handles v-prefix automatically)
ver, err := semver.NewVersion(version)
if err != nil {
return "", fmt.Errorf("invalid chart version %q: %w", version, err)
}
canonical := ver.String() // always without "v"
baseDir := filepath.Clean(dir)
// Try both candidates: with and without v-prefix in filename
candidateVersions := []string{canonical, "v" + canonical}
var candidatePaths []string
for _, candidateVer := range candidateVersions {
path, err := buildChartCandidatePath(baseDir, name, candidateVer)
if err != nil {
return "", err
}
candidatePaths = append(candidatePaths, path)
exists, err := chartPathExists(path)
if err != nil {
return "", err
}
if exists {
return path, nil
}
}
return "", fmt.Errorf("chart file not found for %s version %s: tried %s and %s",
name, version, filepath.Base(candidatePaths[0]), filepath.Base(candidatePaths[1]))
}3. Removal the toggleVersionPrefix() function
|
Also, please create a bug on OCPBUGS and add it to the beginning of the PR title like |
Replace manual v-prefix toggling with semver.NewVersion() which already
exists in the codebase. semver.String() always returns the canonical
form without a "v" prefix, so we can deterministically probe both
"{name}-{canonical}.tgz" and "{name}-v{canonical}.tgz" in a loop,
removing the need for the toggleVersionPrefix() helper.
Co-authored-by: Cursor <cursoragent@cursor.com>
Sourav21jain
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@Sourav21jain: This pull request references Jira Issue OCPBUGS-88461, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@aguidirh I have updated both the things, Can you please do a validation? |
|
/test unit |
|
The ci/prow/unit failure is a pre-existing regression in main unrelated to this PR. The failing test is: --- FAIL: TestGetReleaseReferenceImages/TestGetReleaseReferenceImages_should_pass_(no_channels) This can be confirmed by checking out main directly (without this PR's changes) and running: go test -tags "json1 libdm_no_deferred_remove exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp" ./internal/pkg/release/... -run TestGetReleaseReferenceImages -v The regression was introduced by PR #1373 ("CLID-547: v2: add list releases command"), commit 3e0ec76, which moved OKD URL to cincinnati.OkdUpdateURL but omitted the UPDATE_URL_OVERRIDE support that exists for the OCP client. |
|
/test unit |
|
/retest |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@aguidirh: This pull request references Jira Issue OCPBUGS-88461, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
@r4f4 could you please have a look ? |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
@aguidirh All the checks are done can you review? |
openshift-ci
Bot
added
the
approved
|
Hello @aguidirh I believe only your approval is remaining for the merge. Thank you in advance. |
@Sourav21jain No, the PR is awaiting QE verification now. /cc @nidangavali |
|
/verified by @nidangavali |
openshift-ci-robot
added
the
verified
|
@nidangavali: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, aguidirh, nidangavali, Sourav21jain The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@Sourav21jain: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@Sourav21jain: Jira Issue Verification Checks: Jira Issue OCPBUGS-88461 Jira Issue OCPBUGS-88461 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.22 |
|
@aguidirh: new pull request created: #1448 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
7 participants
Description
During the disk-to-mirror (d2m) phase, oc-mirror constructs chart tarball
paths as
{name}-{version}.tgz. However, the Helm downloader saves filesusing the URL basename from the repository's
index.yaml, which may includea
vprefix (e.g.csi-driver-nfs-v4.9.0.tgz). This causes d2m to failwith file not found even though mirror-to-disk succeeded.
Root cause
filepath.Base(url)from indexfmt.Sprintf("%s-%s.tgz", name, version)When the index URL contained a
vprefix but the config did not (orvice-versa), the reconstructed path never matched the file on disk.
Github / Jira issue: https://redhat.atlassian.net/browse/RFE-8530
Type of change
How Has This Been Tested?
TestResolveChartPath— unit tests for the helper (exact match,config omits v / disk has v, config has v / disk has no v, both missing)
TestHelmImageCollectorVPrefixDiskToMirror— end-to-end d2m collectortest exercising both mismatch directions with a real chart tarball
oc-mirrorbinary using--dry-runwith apre-seeded working-dir containing
podinfo-v5.0.0.tgzand configspecifying
version: "5.0.0"Summary by CodeRabbit
Bug Fixes
Tests