From f552b672305a420b54a725185f98e34e51fbd7ba Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Wed, 21 May 2025 21:30:05 +0200 Subject: [PATCH 1/2] Prevent failure when resolving version tags in oras memory store - The newReference() function transforms version tags by replacing + with _ for OCI compatibility - But the code was using the original ref (with +) for TagBytes() - Then it tries to find the tagged reference using parsedRef.String() (with _) - This mismatch causes the Resolve method to fail with "not found" - By using parsedRef.String() consistently in both places, the references will match and the lookup will succeed. I extracted the TagBytes function to improve testability. Push() includes several external calls that are hard to mock, so isolating this logic makes testing more manageable. Close: #30881 Signed-off-by: Benoit Tigeot --- pkg/registry/client.go | 40 ++++++++++++++++++++++++++----------- pkg/registry/client_test.go | 27 +++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 48adb42b4..00e369f86 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -711,19 +711,9 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu }) ociAnnotations := generateOCIAnnotations(meta, operation.creationTime) - manifest := ocispec.Manifest{ - Versioned: specs.Versioned{SchemaVersion: 2}, - Config: configDescriptor, - Layers: layers, - Annotations: ociAnnotations, - } - manifestData, err := json.Marshal(manifest) - if err != nil { - return nil, err - } - - manifestDescriptor, err := oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, ref) + manifestDescriptor, err := c.tagManifest(ctx, memoryStore, ref, configDescriptor, + layers, ociAnnotations) if err != nil { return nil, err } @@ -924,3 +914,29 @@ func (c *Client) ValidateReference(ref, version string, u *url.URL) (*url.URL, e return u, err } + +// tagManifest prepares and tags a manifest in memory storage +func (c *Client) tagManifest(ctx context.Context, memoryStore *memory.Store, + ref string, configDescriptor ocispec.Descriptor, layers []ocispec.Descriptor, + ociAnnotations map[string]string) (ocispec.Descriptor, error) { + + manifest := ocispec.Manifest{ + Versioned: specs.Versioned{SchemaVersion: 2}, + Config: configDescriptor, + Layers: layers, + Annotations: ociAnnotations, + } + + manifestData, err := json.Marshal(manifest) + if err != nil { + return ocispec.Descriptor{}, err + } + + parsedRef, err := newReference(ref) + if err != nil { + return ocispec.Descriptor{}, err + } + + return oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, + manifestData, parsedRef.String()) +} diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go index dcc54c0c4..d01f2fa6f 100644 --- a/pkg/registry/client_test.go +++ b/pkg/registry/client_test.go @@ -17,12 +17,15 @@ limitations under the License. package registry import ( + "context" "io" "testing" "github.com/containerd/containerd/remotes" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "oras.land/oras-go/v2/content/memory" ) func TestNewClientResolverNotSupported(t *testing.T) { @@ -52,3 +55,27 @@ func TestStripURL(t *testing.T) { assert.Equal(t, "127.0.0.1:15000", client.stripURL("127.0.0.1:15000/asdf/asdf")) assert.Equal(t, "127.0.0.1:15000", client.stripURL("127.0.0.1:15000")) } + +// Inspired by oras test +// https://github.com/oras-project/oras-go/blob/05a2b09cbf2eab1df691411884dc4df741ec56ab/content_test.go#L1802 +func TestTagManifestTransformsReferences(t *testing.T) { + memStore := memory.New() + client := &Client{out: io.Discard} + ctx := context.Background() + + refWithPlus := "test-registry.io/charts/test:1.0.0+metadata" + expectedRef := "test-registry.io/charts/test:1.0.0_metadata" // + becomes _ + + configDesc := ocispec.Descriptor{MediaType: ConfigMediaType, Digest: "sha256:config", Size: 100} + layers := []ocispec.Descriptor{{MediaType: ChartLayerMediaType, Digest: "sha256:layer", Size: 200}} + + desc, err := client.tagManifest(ctx, memStore, refWithPlus, configDesc, layers, nil) + require.NoError(t, err) + + transformedDesc, err := memStore.Resolve(ctx, expectedRef) + require.NoError(t, err, "Should find the reference with _ instead of +") + require.Equal(t, desc.Digest, transformedDesc.Digest) + + _, err = memStore.Resolve(ctx, refWithPlus) + require.Error(t, err, "Should NOT find the reference with the original +") +} From c33215d765e291bc9321984d4f60a0182c738938 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 23 May 2025 08:13:41 +0200 Subject: [PATCH 2/2] Prevent fetching newReference again as we have in calling method Signed-off-by: Benoit Tigeot --- pkg/registry/client.go | 13 ++++--------- pkg/registry/client_test.go | 5 ++++- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 00e369f86..ea197e6f7 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -712,8 +712,8 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu ociAnnotations := generateOCIAnnotations(meta, operation.creationTime) - manifestDescriptor, err := c.tagManifest(ctx, memoryStore, ref, configDescriptor, - layers, ociAnnotations) + manifestDescriptor, err := c.tagManifest(ctx, memoryStore, configDescriptor, + layers, ociAnnotations, parsedRef) if err != nil { return nil, err } @@ -917,8 +917,8 @@ func (c *Client) ValidateReference(ref, version string, u *url.URL) (*url.URL, e // tagManifest prepares and tags a manifest in memory storage func (c *Client) tagManifest(ctx context.Context, memoryStore *memory.Store, - ref string, configDescriptor ocispec.Descriptor, layers []ocispec.Descriptor, - ociAnnotations map[string]string) (ocispec.Descriptor, error) { + configDescriptor ocispec.Descriptor, layers []ocispec.Descriptor, + ociAnnotations map[string]string, parsedRef reference) (ocispec.Descriptor, error) { manifest := ocispec.Manifest{ Versioned: specs.Versioned{SchemaVersion: 2}, @@ -932,11 +932,6 @@ func (c *Client) tagManifest(ctx context.Context, memoryStore *memory.Store, return ocispec.Descriptor{}, err } - parsedRef, err := newReference(ref) - if err != nil { - return ocispec.Descriptor{}, err - } - return oras.TagBytes(ctx, memoryStore, ocispec.MediaTypeImageManifest, manifestData, parsedRef.String()) } diff --git a/pkg/registry/client_test.go b/pkg/registry/client_test.go index d01f2fa6f..f5ba41c21 100644 --- a/pkg/registry/client_test.go +++ b/pkg/registry/client_test.go @@ -69,7 +69,10 @@ func TestTagManifestTransformsReferences(t *testing.T) { configDesc := ocispec.Descriptor{MediaType: ConfigMediaType, Digest: "sha256:config", Size: 100} layers := []ocispec.Descriptor{{MediaType: ChartLayerMediaType, Digest: "sha256:layer", Size: 200}} - desc, err := client.tagManifest(ctx, memStore, refWithPlus, configDesc, layers, nil) + parsedRef, err := newReference(refWithPlus) + require.NoError(t, err) + + desc, err := client.tagManifest(ctx, memStore, configDesc, layers, nil, parsedRef) require.NoError(t, err) transformedDesc, err := memStore.Resolve(ctx, expectedRef)