From eda584f97df7a23d054d70aced5e12a773cd8bb7 Mon Sep 17 00:00:00 2001
From: "google-labs-jules[bot]"
<161369871+google-labs-jules[bot]@users.noreply.github.com>
Date: Thu, 22 May 2025 14:05:14 +0000
Subject: [PATCH] Fix: Handle OCI artifact versions with '+' correctly
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.
---
internal/helper/helper.go | 64 +++++++++++++++++++++++++++--
internal/helper/helper_test.go | 73 ++++++++++++++++++++++++++++++++++
internal/manifest/charts.go | 12 +++++-
internal/manifest/manifest.go | 41 ++++++++++++++-----
4 files changed, 175 insertions(+), 15 deletions(-)
create mode 100644 internal/helper/helper_test.go
diff --git a/internal/helper/helper.go b/internal/helper/helper.go
index 328eeaf..74d8eed 100644
--- a/internal/helper/helper.go
+++ b/internal/helper/helper.go
@@ -20,9 +20,12 @@ import (
)
// Returns whether this url should be handled by the Blob handler
-// This is complicated because Blob is indicated by the trailing path, not the leading path.
-// 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.
+// https://github.com/opencontainers/distribution-spec/blob/master/spec.md#pulli
+ng-a-layer
+// https://github.com/opencontainers/distribution-spec/blob/master/spec.md#pushi
+ng-a-layer
func IsBlob(req *http.Request) bool {
elem := strings.Split(req.URL.Path, "/")
elem = elem[1:]
@@ -71,3 +74,58 @@ func IsV2(req *http.Request) bool {
}
return elems[len(elems)-1] == "v2"
}
+
+// SemVerReplace replaces all underscores in a given string with plus signs.
+// This function is primarily intended for use with semantic version strings
+// where underscores might have been used in place of plus signs (e.g., for
+// build metadata or pre-release identifiers). Standard Semantic Versioning
+// (semver.org) specifies plus signs for build metadata (e.g., "1.0.0+20130313144700")
+// and hyphens for pre-release identifiers (e.g., "1.0.0-alpha").
+//
+// Purpose:
+// The main purpose of this function is to normalize version strings by converting
+// any underscores to plus signs. This can be particularly useful when dealing with
+// version strings from systems or sources that use underscores due to constraints
+// (e.g., where '+' is a special character) or by convention for information that
+// semantically aligns with build metadata.
+//
+// When to use it:
+// Use this function when you encounter version strings like "v1.2.3_build456" or
+// "2.0.0_rc_1" and need to transform them into a format like "v1.2.3+build456" or
+// "2.0.0+rc+1". This transformation is often a preparatory step before parsing
+// the string with a semantic versioning library that strictly expects '+' for
+// build metadata, or when aiming for a consistent display format for version information.
+//
+// Transformation Examples:
+// - Input: "1.0.0_alpha"
+// Output: "1.0.0+alpha"
+// - Input: "v2.1.3_beta_build123" (handles multiple underscores)
+// Output: "v2.1.3+beta+build123"
+// - Input: "1.2.3" (string with no underscores)
+// Output: "1.2.3" (string remains unchanged)
+// - Input: "" (empty string)
+// Output: "" (empty string remains unchanged)
+//
+// Semver Validation:
+// This function does NOT perform validation of the overall semantic version string structure.
+// For example, it does not check if the version string conforms to the MAJOR.MINOR.PATCH
+// numerical format or other specific semver rules. Its sole responsibility is to
+// replace every occurrence of the underscore character '_' with a plus sign '+'.
+// For comprehensive semver parsing and validation, it is recommended to use a
+// dedicated semver library on the string after this transformation, if necessary.
+//
+// Edge Cases Handled:
+// - Multiple underscores: All occurrences of underscores are replaced.
+// For instance, "1.0.0_alpha_snapshot" becomes "1.0.0+alpha+snapshot".
+// - Empty string: If an empty string is provided, an empty string is returned.
+// - String without underscores: If the string does not contain any underscores,
+// it is returned as is.
+func SemVerReplace(semver string) string {
+ // strings.ReplaceAll is efficient and handles edge cases gracefully:
+ // - If `semver` is an empty string, it returns an empty string.
+ // - If `semver` does not contain "_", it returns `semver` unchanged.
+ // - It replaces all occurrences of "_" with "+".
+ // Therefore, the original conditional check (if semver != "" && strings.Contains(semver, "_"))
+ // is not strictly necessary for correctness.
+ return strings.ReplaceAll(semver, "_", "+")
+}
diff --git a/internal/helper/helper_test.go b/internal/helper/helper_test.go
new file mode 100644
index 0000000..65e1876
--- /dev/null
+++ b/internal/helper/helper_test.go
@@ -0,0 +1,73 @@
+package helper
+
+import (
+ "testing"
+)
+
+func TestSemVerReplace(t *testing.T) {
+ testCases := []struct {
+ name string
+ input string
+ expected string
+ }{
+ {
+ name: "empty string",
+ input: "",
+ expected: "",
+ },
+ {
+ name: "no underscores",
+ input: "1.0.0",
+ expected: "1.0.0",
+ },
+ {
+ name: "one underscore",
+ input: "1.0.0_alpha",
+ expected: "1.0.0+alpha",
+ },
+ {
+ name: "multiple underscores",
+ input: "1.0.0_alpha_build123",
+ expected: "1.0.0+alpha+build123",
+ },
+ {
+ name: "leading and trailing underscores",
+ input: "_1.0.0_",
+ expected: "+1.0.0+",
+ },
+ {
+ name: "only underscores",
+ input: "___",
+ expected: "+++",
+ },
+ {
+ name: "mixed case and underscores",
+ input: "v1.2.3_Rc1_candidate",
+ expected: "v1.2.3+Rc1+candidate",
+ },
+ {
+ name: "no underscores with v prefix",
+ input: "v2.3.4",
+ expected: "v2.3.4",
+ },
+ {
+ name: "already has plus (should not change)",
+ input: "1.0.0+beta",
+ expected: "1.0.0+beta",
+ },
+ {
+ name: "mixed plus and underscore",
+ input: "1.0.0_alpha+beta_rc1",
+ expected: "1.0.0+alpha+beta+rc1",
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ actual := SemVerReplace(tc.input)
+ if actual != tc.expected {
+ t.Errorf("SemVerReplace(%q) = %q; want %q", tc.input, actual, tc.expected)
+ }
+ })
+ }
+}
diff --git a/internal/manifest/charts.go b/internal/manifest/charts.go
index 45d37c8..8727dc4 100644
--- a/internal/manifest/charts.go
+++ b/internal/manifest/charts.go
@@ -6,6 +6,7 @@ import (
"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"
"github.com/opencontainers/go-digest"
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
"helm.sh/helm/v3/pkg/chart"
@@ -47,10 +48,19 @@ func (m *Manifests) prepareChart(ctx context.Context, repo string, reference str
m.log.Printf("searching index for %s with reference %s\n", chart, reference)
chartVer, err := index.Get(chart, reference)
if err != nil {
+ originalReference := reference // Store Original Reference
+ 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
+ chartVer, err = index.Get(chart, reference)
+ }
+ if err != nil {
+
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),
}
}
diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go
index c3981b3..8f0a539 100644
--- a/internal/manifest/manifest.go
+++ b/internal/manifest/manifest.go
@@ -160,11 +160,19 @@ func (m *Manifests) Handle(resp http.ResponseWriter, req *http.Request) error {
ma, ok = c[target]
if !ok {
- // we failed
- return &errors.RegError{
- Status: http.StatusNotFound,
- Code: "NOT FOUND",
- Message: fmt.Sprintf("Chart prepare's result not found: %v, %v", repo, target),
+ 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),
+ }
}
}
}
@@ -192,17 +200,28 @@ func (m *Manifests) Handle(resp http.ResponseWriter, req *http.Request) error {
}
ma, ok := m.manifests[repo][target]
if !ok {
+ // First lookup failed, try preparing chart (which might involve its own SemVerReplace)
err := m.prepareChart(req.Context(), repo, target)
if err != nil {
- return err
+ return err // Error from prepareChart
}
+ // After prepareChart, try lookup again with the potentially modified target from prepareChart
+ // and then with SemVerReplace if that also fails.
ma, ok = m.manifests[repo][target]
if !ok {
- // we failed
- return &errors.RegError{
- Status: http.StatusNotFound,
- Code: "NOT FOUND",
- Message: "Chart prepare error",
+ originalTarget := target // Store target before SemVerReplace
+ if m.config.Debug {
+ m.log.Printf("HEAD: Manifest not found for repo %s with target %s even after prepareChart. Attempting again after converting underscores to plus signs.", repo, originalTarget)
+ }
+ target = helper.SemVerReplace(originalTarget) // Convert underscores
+ ma, ok = m.manifests[repo][target] // Final attempt
+ if !ok {
+ // All attempts failed
+ return &errors.RegError{
+ Status: http.StatusNotFound,
+ Code: "NOT FOUND",
+ Message: fmt.Sprintf("HEAD: Chart manifest not found for repo %s. Tried target '%s' and after underscore conversion '%s'.", repo, originalTarget, target),
+ }
}
}
}
--
GitLab