Fix: Handle OCI artifact versions with '+' correctly #30
Conversation
This change addresses an issue where Helm's automatic replacement of '+'
with '_' in version strings causes problems with OCI artifact registries
that do not support '+' in tags.
The following improvements have been made:
- Modified `internal/helper/helper.go`:
- Added comprehensive documentation to the `SemVerReplace` function.
- Clarified that `SemVerReplace` focuses on underscore-to-plus
conversion and does not perform full semver validation.
- Simplified the `SemVerReplace` function logic.
- Added extensive unit tests for `SemVerReplace` covering various
scenarios, including empty strings, multiple underscores,
leading/trailing underscores, and mixed-case strings.
- Updated `internal/manifest/charts.go`:
- Added debug logging before attempting underscore-to-plus conversion
for chart versions.
- Enhanced error messages to be more specific when a chart version is
not found, indicating that lookups were attempted with both the
original reference and the converted reference.
- Updated `internal/manifest/manifest.go`:
- Ensured consistent handling of underscore-to-plus conversion for
manifest targets in both GET and HEAD request methods.
- Added debug logging before attempting underscore-to-plus conversion
for manifest targets in both GET and HEAD methods.
- Enhanced error messages to be more specific when a manifest is not
found, indicating that lookups were attempted with both the
original target and the converted target.
These changes ensure that versions containing '+' are correctly processed
by first attempting the lookup with the original reference (which might
contain '_') and then, if that fails, by attempting the lookup again
after converting underscores to plus signs. This provides more robust
handling for OCI artifacts and improves debuggability.
📝 WalkthroughWalkthroughA new helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManifestHandler
participant Helper
Client->>ManifestHandler: GET/HEAD manifest (target)
ManifestHandler->>ManifestHandler: Lookup manifest (target)
alt Found
ManifestHandler-->>Client: Return manifest
else Not Found
ManifestHandler->>Helper: SemVerReplace(target)
ManifestHandler->>ManifestHandler: Lookup manifest (transformed target)
alt Found
ManifestHandler-->>Client: Return manifest
else Not Found
ManifestHandler-->>Client: Return NOT FOUND (both targets attempted)
end
end
sequenceDiagram
participant ChartPreparer
participant Helper
ChartPreparer->>ChartPreparer: Lookup chart version (ref)
alt Found
ChartPreparer-->>Caller: Return chart
else Not Found
ChartPreparer->>Helper: SemVerReplace(ref)
ChartPreparer->>ChartPreparer: Lookup chart version (transformed ref)
alt Found
ChartPreparer-->>Caller: Return chart
else Not Found
ChartPreparer-->>Caller: Return error (both refs attempted)
end
end
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with OCI artifact version handling by ensuring that version strings containing '+' are correctly processed. Key changes include:
- Updating manifest handlers (GET and HEAD) to retry lookups after converting underscores to plus signs.
- Enhancing chart lookup error messages and adding debug logging.
- Extending documentation in the helper function along with comprehensive unit tests for SemVerReplace.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/manifest/manifest.go | Updated lookup logic and error messages for version conversion handling. |
| internal/manifest/charts.go | Improved chart lookup with debug logging and detailed error messages. |
| internal/helper/helper.go | Enhanced SemVerReplace documentation and simplification of logic. |
| internal/helper/helper_test.go | Added extensive unit tests covering multiple SemVerReplace scenarios. |
| originalTarget := target // Store original target | ||
| if m.config.Debug { | ||
| m.log.Printf("GET: Chart lookup failed for repo %s with target %s after prepareChart. Attempting again after converting underscores to plus signs.", repo, originalTarget) | ||
| } | ||
| target = helper.SemVerReplace(originalTarget) // Use originalTarget for conversion | ||
| ma, ok = c[target] // Attempt lookup with the new target | ||
| if !ok { | ||
| // we failed again | ||
| return &errors.RegError{ | ||
| Status: http.StatusNotFound, | ||
| Code: "NOT FOUND", | ||
| Message: fmt.Sprintf("GET: Chart prepare's result not found for repo %s. Tried target '%s' and after underscore conversion '%s'.", repo, originalTarget, target), | ||
| } | ||
| } |
There was a problem hiding this comment.
The conversion logic for a failed lookup is similar in both GET and HEAD handlers. Consider extracting this functionality into a dedicated helper function to improve maintainability and reduce duplication.
Copilot uses AI. Check for mistakes.
| if m.config.Debug { | ||
| m.log.Printf("Chart lookup for '%s' with reference '%s' failed. Attempting again after converting underscores to plus signs in reference.", chart, originalReference) | ||
| } | ||
| reference = helper.SemVerReplace(originalReference) // Use originalReference for conversion |
There was a problem hiding this comment.
This conversion step closely mirrors the logic in manifest.go. Consider consolidating the underscore-to-plus conversion into a shared helper to ensure consistency and ease future updates.
| reference = helper.SemVerReplace(originalReference) // Use originalReference for conversion | |
| reference = helper.SemVerReplace(originalReference) // Centralized underscore-to-plus conversion |
Copilot uses AI. Check for mistakes.
![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
🔭 Outside diff range comments (1)
internal/manifest/manifest.go (1)
18-19:⚠️ Potential issueMissing helper package import
You're using the helper package in this file but it's not imported. Add the import statement.
import ( "bytes" "context" "crypto/sha256" "encoding/hex" "encoding/json" "fmt" "github.com/container-registry/helm-charts-oci-proxy/internal/blobs/handler" "github.com/container-registry/helm-charts-oci-proxy/internal/errors" + "github.com/container-registry/helm-charts-oci-proxy/internal/helper" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/sirupsen/logrus" "io" "net/http" "sort" "strconv" "strings" "sync" "time" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/helper/helper.go(2 hunks)internal/helper/helper_test.go(1 hunks)internal/manifest/charts.go(2 hunks)internal/manifest/manifest.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/helper/helper_test.go (1)
internal/helper/helper.go (1)
SemVerReplace(123-131)
internal/manifest/charts.go (2)
internal/helper/helper.go (1)
SemVerReplace(123-131)internal/errors/error.go (1)
RegError(23-27)
🪛 golangci-lint (1.64.8)
internal/helper/helper.go
24-24: expected declaration, found eading
(typecheck)
internal/manifest/charts.go
9-9: could not import github.com/container-registry/helm-charts-oci-proxy/internal/helper (-: # github.com/container-registry/helm-charts-oci-proxy/internal/helper
internal/helper/helper.go:24:1: syntax error: non-declaration statement outside function body)
(typecheck)
63-63: undefined: originalReference
(typecheck)
internal/manifest/manifest.go
167-167: undefined: helper
(typecheck)
216-216: undefined: helper
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (6)
internal/helper/helper.go (1)
78-131: Well-documented helper function implementationThe
SemVerReplacefunction is well-documented with clear explanations of its purpose, usage scenarios, transformation examples, and edge cases. The implementation is simple and effective, usingstrings.ReplaceAllwhich handles all the edge cases gracefully.internal/helper/helper_test.go (1)
7-73: Comprehensive test coverage for SemVerReplaceGreat job with the test cases! They cover a wide range of scenarios including empty strings, strings without underscores, single/multiple underscores, leading/trailing underscores, mixed case strings, and combinations with plus signs. This thorough testing helps ensure the function behaves correctly in all edge cases.
internal/manifest/charts.go (2)
9-9: Add import for the helper packageThe helper package is imported correctly, but note that this import will fail until the syntax error in helper.go is fixed.
🧰 Tools
🪛 golangci-lint (1.64.8)
9-9: could not import github.com/container-registry/helm-charts-oci-proxy/internal/helper (-: # github.com/container-registry/helm-charts-oci-proxy/internal/helper
internal/helper/helper.go:24:1: syntax error: non-declaration statement outside function body)(typecheck)
51-57: Good fallback strategy for chart version lookupsThe implementation correctly stores the original reference and attempts a retry with underscores converted to plus signs. The debug logging is helpful for troubleshooting.
internal/manifest/manifest.go (2)
163-175: Good fallback strategy for GET handlerThe implementation correctly stores the original target and attempts a retry with underscores converted to plus signs. The debug logging provides useful context for troubleshooting.
🧰 Tools
🪛 golangci-lint (1.64.8)
167-167: undefined: helper
(typecheck)
212-224: Good fallback strategy for HEAD handlerSimilar to the GET handler, the HEAD handler implementation correctly stores the original target and attempts a retry with underscores converted to plus signs. The comments provide clarity about the multiple lookup attempts.
🧰 Tools
🪛 golangci-lint (1.64.8)
216-216: undefined: helper
(typecheck)
| // This is complicated because Blob is indicated by the trailing path, not the l | ||
| eading path. |
There was a problem hiding this comment.
Fix line wrapping issue in comment
There's a syntax error in the comment where "leading" is split across two lines. This breaks the code and causes compilation errors that affect other files.
-// This is complicated because Blob is indicated by the trailing path, not the l
-eading path.
+// This is complicated because Blob is indicated by the trailing path, not the leading path.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This is complicated because Blob is indicated by the trailing path, not the l | |
| eading path. | |
| // This is complicated because Blob is indicated by the trailing path, not the leading path. |
🧰 Tools
🪛 golangci-lint (1.64.8)
24-24: expected declaration, found eading
(typecheck)
🤖 Prompt for AI Agents
In internal/helper/helper.go at lines 23 to 24, the comment has a line break
splitting the word "leading" into two parts, causing a syntax error. Fix this by
merging the split word into a single line without breaking it, ensuring the
entire comment is properly formatted and does not cause compilation errors.
| return &errors.RegError{ | ||
| Status: http.StatusNotFound, | ||
| Code: "NOT FOUND", | ||
| Message: fmt.Sprintf("Chart: %s version: %s not found: %v", chart, reference, err), | ||
| Message: fmt.Sprintf("Chart: %s version: %s not found. Attempted lookup with original reference and after converting underscores to plus signs. Last error: %v", chart, originalReference, err), | ||
| } |
There was a problem hiding this comment.
Fix potential undefined variable in error message
The variable originalReference might be undefined if the initial lookup doesn't fail but the retry fails. Ensure it's always defined before the error message.
if err != nil {
+ originalReference := reference // Ensure originalReference is always defined
return &errors.RegError{
Status: http.StatusNotFound,
Code: "NOT FOUND",
Message: fmt.Sprintf("Chart: %s version: %s not found. Attempted lookup with original reference and after converting underscores to plus signs. Last error: %v", chart, originalReference, err),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return &errors.RegError{ | |
| Status: http.StatusNotFound, | |
| Code: "NOT FOUND", | |
| Message: fmt.Sprintf("Chart: %s version: %s not found: %v", chart, reference, err), | |
| Message: fmt.Sprintf("Chart: %s version: %s not found. Attempted lookup with original reference and after converting underscores to plus signs. Last error: %v", chart, originalReference, err), | |
| } | |
| if err != nil { | |
| // Ensure originalReference is always defined before using it in the error message | |
| originalReference := reference | |
| return &errors.RegError{ | |
| Status: http.StatusNotFound, | |
| Code: "NOT FOUND", | |
| Message: fmt.Sprintf( | |
| "Chart: %s version: %s not found. Attempted lookup with original reference and after converting underscores to plus signs. Last error: %v", | |
| chart, originalReference, err, | |
| ), | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
63-63: undefined: originalReference
(typecheck)
🤖 Prompt for AI Agents
In internal/manifest/charts.go around lines 60 to 64, the variable
originalReference used in the error message may be undefined if the initial
lookup succeeds but the retry fails. To fix this, ensure originalReference is
always assigned a value before this error return statement, either by
initializing it earlier or by capturing it from the first lookup attempt, so it
is defined regardless of which lookup fails.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 3 issues across 4 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| Status: http.StatusNotFound, | ||
| Code: "NOT FOUND", | ||
| Message: fmt.Sprintf("Chart: %s version: %s not found: %v", chart, reference, err), | ||
| Message: fmt.Sprintf("Chart: %s version: %s not found. Attempted lookup with original reference and after converting underscores to plus signs. Last error: %v", chart, originalReference, err), |
There was a problem hiding this comment.
originalReference is undefined here because it was declared inside the previous if-block and is out of scope, causing a compilation error.
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { |
There was a problem hiding this comment.
The loop variable tc is captured by reference inside the sub-test closure. If t.Parallel is added later, sub-tests could run concurrently and observe the mutated tc value, leading to flaky tests. Create a new local variable (tc := tc) before calling t.Run to capture the current iteration value.
| // https://github.com/opencontainers/distribution-spec/blob/master/spec.md#pulling-a-layer | ||
| // https://github.com/opencontainers/distribution-spec/blob/master/spec.md#pushing-a-layer | ||
| // This is complicated because Blob is indicated by the trailing path, not the l | ||
| eading path. |
There was a problem hiding this comment.
Line is not commented, resulting in an identifier at package scope and causing a syntax error that breaks compilation.
This change addresses an issue where Helm's automatic replacement of '+' with '_' in version strings causes problems with OCI artifact registries that do not support '+' in tags.
The following improvements have been made:
Modified
internal/helper/helper.go:SemVerReplacefunction.SemVerReplacefocuses on underscore-to-plus conversion and does not perform full semver validation.SemVerReplacefunction logic.SemVerReplacecovering various scenarios, including empty strings, multiple underscores, leading/trailing underscores, and mixed-case strings.Updated
internal/manifest/charts.go:Updated
internal/manifest/manifest.go:These changes ensure that versions containing '+' are correctly processed by first attempting the lookup with the original reference (which might contain '_') and then, if that fails, by attempting the lookup again after converting underscores to plus signs. This provides more robust handling for OCI artifacts and improves debuggability.
Summary by cubic
Fixed handling of OCI artifact versions with '+' by converting underscores to plus signs when needed, ensuring correct lookups and clearer error messages.
Summary by CodeRabbit