diff --git a/internal/mirror/modules/find_extra_images_test.go b/internal/mirror/modules/find_extra_images_test.go new file mode 100644 index 000000000..f7d9b8234 --- /dev/null +++ b/internal/mirror/modules/find_extra_images_test.go @@ -0,0 +1,264 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package modules + +import ( + "bytes" + "context" + "errors" + "io" + "sync/atomic" + "testing" + "testing/iotest" + "time" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + dkpreg "github.com/deckhouse/deckhouse/pkg/registry" + upfake "github.com/deckhouse/deckhouse/pkg/registry/fake" + + pkgclient "github.com/deckhouse/deckhouse-cli/pkg/registry/client" +) + +// ============================================================================= +// Tests: extra images discovery (findExtraImages) +// ============================================================================= + +const extraImagesTag = "v1.0.0" + +// A version either declares extra images or it doesn't - both are normal, and +// neither is an error. A missing version image is treated the same way. +func TestFindExtraImages_Discovery(t *testing.T) { + cases := []struct { + name string + image v1.Image // placed at modules/:; nil = don't add (GetImage -> not found) + wantRefs map[string][]string // extra-name -> tags + }{ + { + name: "version with extra_images.json yields extra images", + image: extraImagesImage(`{"scanner":"v1.2.3","enforcer":"v4.5.6"}`), + wantRefs: map[string][]string{ + "scanner": {"v1.2.3"}, + "enforcer": {"v4.5.6"}, + }, + }, + { + name: "version without extra_images.json is skipped", + image: versionImage(extraImagesTag), // carries only version.json + wantRefs: map[string][]string{}, + }, + { + name: "missing module version image is skipped", + image: nil, + wantRefs: map[string][]string{}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reg := upfake.NewRegistry(testHost) + if tc.image != nil { + reg.MustAddImage("modules/"+testModuleName, extraImagesTag, tc.image) + } + svc := newService(t, pkgclient.Adapt(upfake.NewClient(reg)), nil) + + got, err := svc.findExtraImages(context.Background(), testModuleName, []string{extraImagesTag}) + + require.NoError(t, err) + assertExtraImageRefs(t, tc.wantRefs, got) + }) + } +} + +// A transient registry error during discovery must be retried, not swallowed: +// the version's extra images still land once the registry recovers. +func TestFindExtraImages_RetriesTransientError(t *testing.T) { + setShortRetryDelay(t) + + reg := upfake.NewRegistry(testHost) + reg.MustAddImage("modules/"+testModuleName, extraImagesTag, extraImagesImage(`{"scanner":"v1.2.3"}`)) + + // Fail the first two GetImage calls, then let the third pass through. + client := newGetImageErrClient(upfake.NewClient(reg), errors.New("simulated registry 503"), 2) + svc := newService(t, pkgclient.Adapt(client), nil) + + got, err := svc.findExtraImages(context.Background(), testModuleName, []string{extraImagesTag}) + + require.NoError(t, err) + assertExtraImageRefs(t, map[string][]string{"scanner": {"v1.2.3"}}, got) + assert.Equal(t, int64(3), client.calls.Load(), "two failed attempts then one success") +} + +// A persistent registry error must fail the pull loudly instead of silently +// dropping the version's extra images from the bundle. +func TestFindExtraImages_FailsOnPersistentError(t *testing.T) { + setShortRetryDelay(t) + + transientErr := errors.New("simulated registry 503") + + reg := upfake.NewRegistry(testHost) + reg.MustAddImage("modules/"+testModuleName, extraImagesTag, extraImagesImage(`{"scanner":"v1.2.3"}`)) + + // Fail every attempt. + client := newGetImageErrClient(upfake.NewClient(reg), transientErr, int(extraImagesFetchRetries)) + svc := newService(t, pkgclient.Adapt(client), nil) + + _, err := svc.findExtraImages(context.Background(), testModuleName, []string{extraImagesTag}) + + require.Error(t, err) + assert.ErrorIs(t, err, transientErr) + assert.Equal(t, int64(extraImagesFetchRetries), client.calls.Load(), "all retry attempts must be spent") +} + +// ============================================================================= +// Tests: extractExtraImagesJSON reads layers directly +// ============================================================================= + +// A layer read failure (a network stream error) must surface as a real error, +// not collapse into errExtraImagesJSONNotFound. This is the regression that +// img.Extract() (mutate.Extract) hid by flushing a clean io.EOF on such errors. +func TestExtractExtraImagesJSON_LayerReadErrorIsNotSkipped(t *testing.T) { + img := layersImage{layers: []v1.Layer{failingLayer{}}} + + _, err := extractExtraImagesJSON(img) + + require.Error(t, err) + assert.False(t, errors.Is(err, errExtraImagesJSONNotFound), + "a layer read failure must not be classified as a clean 'not found' skip") + assert.ErrorIs(t, err, io.ErrUnexpectedEOF) +} + +// A version whose layer reads in full but has no extra_images.json is a clean +// skip. +func TestExtractExtraImagesJSON_AbsentFileIsNotFound(t *testing.T) { + _, err := extractExtraImagesJSON(versionImage("v1.0.0")) // carries only version.json + + assert.ErrorIs(t, err, errExtraImagesJSONNotFound) +} + +// The happy path still yields the declared extra images. +func TestExtractExtraImagesJSON_PresentFileIsParsed(t *testing.T) { + got, err := extractExtraImagesJSON(extraImagesImage(`{"scanner":"v1.2.3"}`)) + + require.NoError(t, err) + assert.Equal(t, "v1.2.3", got["scanner"]) +} + +// ============================================================================= +// Helpers +// ============================================================================= + +// extraImagesImage builds a v1.Image whose flattened tar carries extra_images.json. +func extraImagesImage(extraImagesJSON string) v1.Image { + return upfake.NewImageBuilder(). + WithFile("extra_images.json", extraImagesJSON). + MustBuild() +} + +// setShortRetryDelay shrinks the discovery retry delay for the duration of a +// test so the retry paths don't wait the production 10s between attempts. +func setShortRetryDelay(t *testing.T) { + t.Helper() + restore := extraImagesFetchRetryDelay + extraImagesFetchRetryDelay = time.Millisecond + t.Cleanup(func() { extraImagesFetchRetryDelay = restore }) +} + +// assertExtraImageRefs checks the discovered map against want (extra-name -> tags), +// and that each entry carries the expected Name and full registry ref. +func assertExtraImageRefs(t *testing.T, want map[string][]string, got map[string][]extraImageInfo) { + t.Helper() + + require.Len(t, got, len(want), "extra image group count mismatch: got %v", got) + + for name, tags := range want { + infos, ok := got[name] + require.True(t, ok, "missing extra image group %q", name) + + gotTags := make([]string, 0, len(infos)) + for _, info := range infos { + assert.Equal(t, name, info.Name) + wantRef := testHost + "/modules/" + testModuleName + "/extra/" + name + ":" + info.Tag + assert.Equal(t, wantRef, info.FullRef) + gotTags = append(gotTags, info.Tag) + } + + assert.ElementsMatch(t, tags, gotTags, "tags for extra image %q", name) + } +} + +// ============================================================================= +// Test doubles +// ============================================================================= + +// getImageErrClient returns a configured error from the first failFirst GetImage +// calls, then delegates. The counter is shared across the WithSegment chain so +// it tallies every GetImage regardless of the client's path. +type getImageErrClient struct { + dkpreg.Client + err error + failFirst int + calls *atomic.Int64 +} + +func newGetImageErrClient(c dkpreg.Client, err error, failFirst int) *getImageErrClient { + return &getImageErrClient{Client: c, err: err, failFirst: failFirst, calls: new(atomic.Int64)} +} + +func (c *getImageErrClient) WithSegment(segments ...string) dkpreg.Client { + return &getImageErrClient{ + Client: c.Client.WithSegment(segments...), + err: c.err, + failFirst: c.failFirst, + calls: c.calls, + } +} + +func (c *getImageErrClient) GetImage(ctx context.Context, tag string, opts ...dkpreg.ImageGetOption) (dkpreg.Image, error) { + if c.calls.Add(1) <= int64(c.failFirst) { + return nil, c.err + } + + return c.Client.GetImage(ctx, tag, opts...) +} + +// layersImage is a minimal stand-in for what extractExtraImagesJSON consumes: +// a value that yields image layers. +type layersImage struct { + layers []v1.Layer +} + +func (l layersImage) Layers() ([]v1.Layer, error) { return l.layers, nil } + +// failingLayer is a v1.Layer whose content read aborts mid-stream, standing in +// for a registry connection dropped while reading the layer. +type failingLayer struct{} + +func (failingLayer) Uncompressed() (io.ReadCloser, error) { + // A partial tar header, then an abrupt stream error. + r := io.MultiReader(bytes.NewReader([]byte("partial tar header")), iotest.ErrReader(io.ErrUnexpectedEOF)) + return io.NopCloser(r), nil +} + +func (failingLayer) Compressed() (io.ReadCloser, error) { return nil, nil } +func (failingLayer) Digest() (v1.Hash, error) { return v1.Hash{}, nil } +func (failingLayer) DiffID() (v1.Hash, error) { return v1.Hash{}, nil } +func (failingLayer) Size() (int64, error) { return 0, nil } +func (failingLayer) MediaType() (types.MediaType, error) { return types.DockerLayer, nil } diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index aa57ee2ce..577fef744 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -29,6 +29,7 @@ import ( "time" "github.com/Masterminds/semver/v3" + v1 "github.com/google/go-containerregistry/pkg/v1" dkplog "github.com/deckhouse/deckhouse/pkg/log" "github.com/deckhouse/deckhouse/pkg/registry/client" @@ -39,6 +40,8 @@ import ( "github.com/deckhouse/deckhouse-cli/pkg/libmirror/bundle" "github.com/deckhouse/deckhouse-cli/pkg/libmirror/layouts" "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/log" + "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/retry" + "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/retry/task" registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" ) @@ -690,7 +693,10 @@ func (svc *Service) pullInternalDigestImages(ctx context.Context, moduleName str // pullExtraImages discovers extra images declared by each module version and // pulls them into per-extra layouts (modules//extra//). func (svc *Service) pullExtraImages(ctx context.Context, moduleName string, versions []string, downloadList *ImageDownloadList) error { - extraImagesByName := svc.findExtraImages(ctx, moduleName, versions) + extraImagesByName, err := svc.findExtraImages(ctx, moduleName, versions) + if err != nil { + return fmt.Errorf("find extra images for module %s: %w", moduleName, err) + } for extraName, images := range extraImagesByName { if len(images) == 0 { @@ -875,10 +881,13 @@ type extraImageInfo struct { FullRef string } -// findExtraImages finds extra images from module images. +// findExtraImages finds extra images declared across the given module versions. // Returns a map where key is extra image name, value is list of image refs to pull. // Extra images are stored under: modules//extra/: -func (svc *Service) findExtraImages(ctx context.Context, moduleName string, versions []string) map[string][]extraImageInfo { +// +// A persistent registry error while reading a version is returned, so the pull +// fails loudly instead of producing a bundle silently missing extra images. +func (svc *Service) findExtraImages(ctx context.Context, moduleName string, versions []string) (map[string][]extraImageInfo, error) { // Map of extra-name -> list of images to pull extraImages := make(map[string][]extraImageInfo) @@ -894,16 +903,14 @@ func (svc *Service) findExtraImages(ctx context.Context, moduleName string, vers tag = parts[1] } - img, err := svc.modulesService.Module(moduleName).GetImage(ctx, tag) + extraImagesJSON, err := svc.getExtraImagesJSON(ctx, moduleName, tag) if err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to get module image %s:%s: %v", moduleName, tag, err)) - continue + return nil, err } - // Try to extract extra_images.json - extraImagesJSON, err := extractExtraImagesJSON(img) - if err != nil { - continue // No extra_images.json in this version + // No extra_images.json in this version + if extraImagesJSON == nil { + continue } for imageName, tagValue := range extraImagesJSON { @@ -931,33 +938,152 @@ func (svc *Service) findExtraImages(ctx context.Context, moduleName string, vers } } - return extraImages + return extraImages, nil } -// extractExtraImagesJSON extracts extra_images.json from an image -func extractExtraImagesJSON(img interface{ Extract() io.ReadCloser }) (map[string]interface{}, error) { - rc := img.Extract() +// Retry policy for reading extra_images.json during discovery, mirroring the +// puller's image-pull retry. Package vars so tests can shrink the delay. +var ( + extraImagesFetchRetries = uint(5) + extraImagesFetchRetryDelay = 10 * time.Second +) + +// getExtraImagesJSON reads and parses extra_images.json from a module version image. +// +// A version without extra images is not an error: returns (nil, nil) when the +// version image is absent or has no extra_images.json. Transient registry +// errors are retried; a persistent error is returned so the caller fails the +// pull rather than silently dropping extra images. +func (svc *Service) getExtraImagesJSON(ctx context.Context, moduleName, tag string) (map[string]interface{}, error) { + var extraImagesJSON map[string]interface{} + + // Set when the version legitimately has no extra images. Signaled out of + // band because the retry payload returns only an error: on a clean skip it + // returns nil so RunTask stops instead of retrying. + noExtraImages := false + + err := retry.RunTask( + ctx, + svc.userLogger, + fmt.Sprintf("Reading extra_images.json of %s:%s", moduleName, tag), + task.WithConstantRetries(extraImagesFetchRetries, extraImagesFetchRetryDelay, func(ctx context.Context) error { + // Fetch the module version image + img, err := svc.modulesService.Module(moduleName).GetImage(ctx, tag) + if err != nil { + // Image tag absent: nothing to read, skip without retrying + if errors.Is(err, client.ErrImageNotFound) { + noExtraImages = true + + return nil + } + + // Transient error: let RunTask retry + return err + } + + // Read extra_images.json out of the image + data, err := extractExtraImagesJSON(img) + if err != nil { + // This version declares no extra images, skip without retrying + if errors.Is(err, errExtraImagesJSONNotFound) { + noExtraImages = true + + return nil + } + + // Transient error: let RunTask retry + return err + } + + extraImagesJSON = data + + return nil + })) + + // If retry failed + if err != nil { + return nil, err + } + + // Version has no extra images + if noExtraImages { + return nil, nil + } + + // extra_images.json found and parsed + return extraImagesJSON, nil +} + +// errExtraImagesJSONNotFound marks that the image has no extra_images.json. +// This is a legitimate skip (not every module version declares extra images), +// distinct from a network/stream error while reading the layer. +var errExtraImagesJSONNotFound = errors.New("extra_images.json not found in image") + +// extractExtraImagesJSON reads extra_images.json from a module image by scanning +// its layers directly. +// +// Reading via img.Extract() (mutate.Extract) is deliberately avoided: on a +// mid-layer read failure it flushes an empty tar footer and hands the caller a +// clean io.EOF, hiding the network error behind the same signal as a genuinely +// absent file. A direct layer scan surfaces a truncated read as a real error, +// so the caller can retry a transient failure instead of dropping the version. +// +// Returns errExtraImagesJSONNotFound only when every layer is read in full and +// the file is absent. +func extractExtraImagesJSON(img interface{ Layers() ([]v1.Layer, error) }) (map[string]interface{}, error) { + layers, err := img.Layers() + if err != nil { + return nil, fmt.Errorf("list image layers: %w", err) + } + + // Top layer first, matching the flatten precedence of a layered image. + for i := len(layers) - 1; i >= 0; i-- { + extraImages, err := findExtraImagesJSONInLayer(layers[i]) + if errors.Is(err, errExtraImagesJSONNotFound) { + continue + } + + if err != nil { + return nil, err + } + + return extraImages, nil + } + + return nil, errExtraImagesJSONNotFound +} + +// findExtraImagesJSONInLayer scans one layer's tar for extra_images.json. +// It returns errExtraImagesJSONNotFound when the layer is read in full without +// the file, and a wrapped error when the layer read itself fails. +func findExtraImagesJSONInLayer(layer v1.Layer) (map[string]interface{}, error) { + rc, err := layer.Uncompressed() + if err != nil { + return nil, fmt.Errorf("read layer: %w", err) + } defer rc.Close() tr := tar.NewReader(rc) for { hdr, err := tr.Next() if err == io.EOF { - return nil, fmt.Errorf("extra_images.json not found in image") + return nil, errExtraImagesJSONNotFound } if err != nil { - return nil, err + return nil, fmt.Errorf("read layer: %w", err) } - if hdr.Name == "extra_images.json" { - var extraImages map[string]interface{} - if err := json.NewDecoder(tr).Decode(&extraImages); err != nil { - return nil, fmt.Errorf("parse extra_images.json: %w", err) - } + if filepath.Clean(hdr.Name) != "extra_images.json" { + continue + } - return extraImages, nil + var extraImages map[string]interface{} + if err := json.NewDecoder(tr).Decode(&extraImages); err != nil { + return nil, fmt.Errorf("parse extra_images.json: %w", err) } + + return extraImages, nil } } diff --git a/internal/mirror/packages/find_extra_images_test.go b/internal/mirror/packages/find_extra_images_test.go new file mode 100644 index 000000000..767dc3d5a --- /dev/null +++ b/internal/mirror/packages/find_extra_images_test.go @@ -0,0 +1,265 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package packages + +import ( + "bytes" + "context" + "errors" + "io" + "sync/atomic" + "testing" + "testing/iotest" + "time" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + dkpreg "github.com/deckhouse/deckhouse/pkg/registry" + upfake "github.com/deckhouse/deckhouse/pkg/registry/fake" + + "github.com/deckhouse/deckhouse-cli/internal" + pkgclient "github.com/deckhouse/deckhouse-cli/pkg/registry/client" +) + +// ============================================================================= +// Tests: extra images discovery (findExtraImages) +// ============================================================================= + +const extraImagesTag = "v1.0.0" + +// A version either declares extra images or it doesn't - both are normal, and +// neither is an error. A missing version image is treated the same way. +func TestFindExtraImages_Discovery(t *testing.T) { + cases := []struct { + name string + image v1.Image // placed at packages/:; nil = don't add (GetImage -> not found) + wantRefs map[string][]string // extra-name -> tags + }{ + { + name: "version with extra_images.json yields extra images", + image: extraImagesImage(`{"scanner":"v1.2.3","enforcer":"v4.5.6"}`), + wantRefs: map[string][]string{ + "scanner": {"v1.2.3"}, + "enforcer": {"v4.5.6"}, + }, + }, + { + name: "version without extra_images.json is skipped", + image: versionImage(extraImagesTag), // carries only version.json + wantRefs: map[string][]string{}, + }, + { + name: "missing package version image is skipped", + image: nil, + wantRefs: map[string][]string{}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reg := upfake.NewRegistry(testHost) + if tc.image != nil { + reg.MustAddImage(internal.PackagesSegment+"/"+testPackageName, extraImagesTag, tc.image) + } + svc := newService(t, pkgclient.Adapt(upfake.NewClient(reg)), nil) + + got, err := svc.findExtraImages(context.Background(), testPackageName, []string{extraImagesTag}) + + require.NoError(t, err) + assertExtraImageRefs(t, tc.wantRefs, got) + }) + } +} + +// A transient registry error during discovery must be retried, not swallowed: +// the version's extra images still land once the registry recovers. +func TestFindExtraImages_RetriesTransientError(t *testing.T) { + setShortRetryDelay(t) + + reg := upfake.NewRegistry(testHost) + reg.MustAddImage(internal.PackagesSegment+"/"+testPackageName, extraImagesTag, extraImagesImage(`{"scanner":"v1.2.3"}`)) + + // Fail the first two GetImage calls, then let the third pass through. + client := newGetImageErrClient(upfake.NewClient(reg), errors.New("simulated registry 503"), 2) + svc := newService(t, pkgclient.Adapt(client), nil) + + got, err := svc.findExtraImages(context.Background(), testPackageName, []string{extraImagesTag}) + + require.NoError(t, err) + assertExtraImageRefs(t, map[string][]string{"scanner": {"v1.2.3"}}, got) + assert.Equal(t, int64(3), client.calls.Load(), "two failed attempts then one success") +} + +// A persistent registry error must fail the pull loudly instead of silently +// dropping the version's extra images from the bundle. +func TestFindExtraImages_FailsOnPersistentError(t *testing.T) { + setShortRetryDelay(t) + + transientErr := errors.New("simulated registry 503") + + reg := upfake.NewRegistry(testHost) + reg.MustAddImage(internal.PackagesSegment+"/"+testPackageName, extraImagesTag, extraImagesImage(`{"scanner":"v1.2.3"}`)) + + // Fail every attempt. + client := newGetImageErrClient(upfake.NewClient(reg), transientErr, int(extraImagesFetchRetries)) + svc := newService(t, pkgclient.Adapt(client), nil) + + _, err := svc.findExtraImages(context.Background(), testPackageName, []string{extraImagesTag}) + + require.Error(t, err) + assert.ErrorIs(t, err, transientErr) + assert.Equal(t, int64(extraImagesFetchRetries), client.calls.Load(), "all retry attempts must be spent") +} + +// ============================================================================= +// Tests: extractExtraImagesJSON reads layers directly +// ============================================================================= + +// A layer read failure (a network stream error) must surface as a real error, +// not collapse into errExtraImagesJSONNotFound. This is the regression that +// img.Extract() (mutate.Extract) hid by flushing a clean io.EOF on such errors. +func TestExtractExtraImagesJSON_LayerReadErrorIsNotSkipped(t *testing.T) { + img := layersImage{layers: []v1.Layer{failingLayer{}}} + + _, err := extractExtraImagesJSON(img) + + require.Error(t, err) + assert.False(t, errors.Is(err, errExtraImagesJSONNotFound), + "a layer read failure must not be classified as a clean 'not found' skip") + assert.ErrorIs(t, err, io.ErrUnexpectedEOF) +} + +// A version whose layer reads in full but has no extra_images.json is a clean +// skip. +func TestExtractExtraImagesJSON_AbsentFileIsNotFound(t *testing.T) { + _, err := extractExtraImagesJSON(versionImage("v1.0.0")) // carries only version.json + + assert.ErrorIs(t, err, errExtraImagesJSONNotFound) +} + +// The happy path still yields the declared extra images. +func TestExtractExtraImagesJSON_PresentFileIsParsed(t *testing.T) { + got, err := extractExtraImagesJSON(extraImagesImage(`{"scanner":"v1.2.3"}`)) + + require.NoError(t, err) + assert.Equal(t, "v1.2.3", got["scanner"]) +} + +// ============================================================================= +// Helpers +// ============================================================================= + +// extraImagesImage builds a v1.Image whose flattened tar carries extra_images.json. +func extraImagesImage(extraImagesJSON string) v1.Image { + return upfake.NewImageBuilder(). + WithFile("extra_images.json", extraImagesJSON). + MustBuild() +} + +// setShortRetryDelay shrinks the discovery retry delay for the duration of a +// test so the retry paths don't wait the production 10s between attempts. +func setShortRetryDelay(t *testing.T) { + t.Helper() + restore := extraImagesFetchRetryDelay + extraImagesFetchRetryDelay = time.Millisecond + t.Cleanup(func() { extraImagesFetchRetryDelay = restore }) +} + +// assertExtraImageRefs checks the discovered map against want (extra-name -> tags), +// and that each entry carries the expected Name and full registry ref. +func assertExtraImageRefs(t *testing.T, want map[string][]string, got map[string][]extraImageInfo) { + t.Helper() + + require.Len(t, got, len(want), "extra image group count mismatch: got %v", got) + + for name, tags := range want { + infos, ok := got[name] + require.True(t, ok, "missing extra image group %q", name) + + gotTags := make([]string, 0, len(infos)) + for _, info := range infos { + assert.Equal(t, name, info.Name) + wantRef := testHost + "/" + internal.PackagesSegment + "/" + testPackageName + "/" + internal.PackagesExtraSegment + "/" + name + ":" + info.Tag + assert.Equal(t, wantRef, info.FullRef) + gotTags = append(gotTags, info.Tag) + } + + assert.ElementsMatch(t, tags, gotTags, "tags for extra image %q", name) + } +} + +// ============================================================================= +// Test doubles +// ============================================================================= + +// getImageErrClient returns a configured error from the first failFirst GetImage +// calls, then delegates. The counter is shared across the WithSegment chain so +// it tallies every GetImage regardless of the client's path. +type getImageErrClient struct { + dkpreg.Client + err error + failFirst int + calls *atomic.Int64 +} + +func newGetImageErrClient(c dkpreg.Client, err error, failFirst int) *getImageErrClient { + return &getImageErrClient{Client: c, err: err, failFirst: failFirst, calls: new(atomic.Int64)} +} + +func (c *getImageErrClient) WithSegment(segments ...string) dkpreg.Client { + return &getImageErrClient{ + Client: c.Client.WithSegment(segments...), + err: c.err, + failFirst: c.failFirst, + calls: c.calls, + } +} + +func (c *getImageErrClient) GetImage(ctx context.Context, tag string, opts ...dkpreg.ImageGetOption) (dkpreg.Image, error) { + if c.calls.Add(1) <= int64(c.failFirst) { + return nil, c.err + } + + return c.Client.GetImage(ctx, tag, opts...) +} + +// layersImage is a minimal stand-in for what extractExtraImagesJSON consumes: +// a value that yields image layers. +type layersImage struct { + layers []v1.Layer +} + +func (l layersImage) Layers() ([]v1.Layer, error) { return l.layers, nil } + +// failingLayer is a v1.Layer whose content read aborts mid-stream, standing in +// for a registry connection dropped while reading the layer. +type failingLayer struct{} + +func (failingLayer) Uncompressed() (io.ReadCloser, error) { + // A partial tar header, then an abrupt stream error. + r := io.MultiReader(bytes.NewReader([]byte("partial tar header")), iotest.ErrReader(io.ErrUnexpectedEOF)) + return io.NopCloser(r), nil +} + +func (failingLayer) Compressed() (io.ReadCloser, error) { return nil, nil } +func (failingLayer) Digest() (v1.Hash, error) { return v1.Hash{}, nil } +func (failingLayer) DiffID() (v1.Hash, error) { return v1.Hash{}, nil } +func (failingLayer) Size() (int64, error) { return 0, nil } +func (failingLayer) MediaType() (types.MediaType, error) { return types.DockerLayer, nil } diff --git a/internal/mirror/packages/packages.go b/internal/mirror/packages/packages.go index b7a8ac5fe..ea34675c4 100644 --- a/internal/mirror/packages/packages.go +++ b/internal/mirror/packages/packages.go @@ -41,6 +41,7 @@ import ( "time" "github.com/Masterminds/semver/v3" + v1 "github.com/google/go-containerregistry/pkg/v1" dkplog "github.com/deckhouse/deckhouse/pkg/log" "github.com/deckhouse/deckhouse/pkg/registry/client" @@ -53,6 +54,8 @@ import ( "github.com/deckhouse/deckhouse-cli/pkg/libmirror/bundle" "github.com/deckhouse/deckhouse-cli/pkg/libmirror/layouts" "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/log" + "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/retry" + "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/retry/task" regimage "github.com/deckhouse/deckhouse-cli/pkg/registry/image" registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" ) @@ -795,7 +798,10 @@ func (svc *Service) pullInternalDigestImages(ctx context.Context, packageName st // pullExtraImages discovers extra images declared by each package version and // pulls them into per-extra layouts (packages//extra//). func (svc *Service) pullExtraImages(ctx context.Context, packageName string, versions []string, downloadList *ImageDownloadList) error { - extraImagesByName := svc.findExtraImages(ctx, packageName, versions) + extraImagesByName, err := svc.findExtraImages(ctx, packageName, versions) + if err != nil { + return fmt.Errorf("find extra images for package %s: %w", packageName, err) + } for extraName, images := range extraImagesByName { if len(images) == 0 { @@ -969,9 +975,12 @@ type extraImageInfo struct { FullRef string } -// findExtraImages finds extra images from package images. +// findExtraImages finds extra images declared across the given package versions. // Extra images are stored under: packages//extra/: -func (svc *Service) findExtraImages(ctx context.Context, packageName string, versions []string) map[string][]extraImageInfo { +// +// A persistent registry error while reading a version is returned, so the pull +// fails loudly instead of producing a bundle silently missing extra images. +func (svc *Service) findExtraImages(ctx context.Context, packageName string, versions []string) (map[string][]extraImageInfo, error) { extraImages := make(map[string][]extraImageInfo) for _, version := range versions { @@ -985,15 +994,14 @@ func (svc *Service) findExtraImages(ctx context.Context, packageName string, ver tag = parts[1] } - img, err := svc.packagesService.Package(packageName).GetImage(ctx, tag) + extraImagesJSON, err := svc.getExtraImagesJSON(ctx, packageName, tag) if err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to get package image %s:%s: %v", packageName, tag, err)) - continue + return nil, err } - extraImagesJSON, err := extractExtraImagesJSON(img) - if err != nil { - continue // No extra_images.json in this version + // No extra_images.json in this version + if extraImagesJSON == nil { + continue } for imageName, tagValue := range extraImagesJSON { @@ -1020,33 +1028,152 @@ func (svc *Service) findExtraImages(ctx context.Context, packageName string, ver } } - return extraImages + return extraImages, nil } -// extractExtraImagesJSON extracts extra_images.json from an image. -func extractExtraImagesJSON(img interface{ Extract() io.ReadCloser }) (map[string]interface{}, error) { - rc := img.Extract() +// Retry policy for reading extra_images.json during discovery, mirroring the +// puller's image-pull retry. Package vars so tests can shrink the delay. +var ( + extraImagesFetchRetries = uint(5) + extraImagesFetchRetryDelay = 10 * time.Second +) + +// getExtraImagesJSON reads and parses extra_images.json from a package version image. +// +// A version without extra images is not an error: returns (nil, nil) when the +// version image is absent or has no extra_images.json. Transient registry +// errors are retried; a persistent error is returned so the caller fails the +// pull rather than silently dropping extra images. +func (svc *Service) getExtraImagesJSON(ctx context.Context, packageName, tag string) (map[string]interface{}, error) { + var extraImagesJSON map[string]interface{} + + // Set when the version legitimately has no extra images. Signaled out of + // band because the retry payload returns only an error: on a clean skip it + // returns nil so RunTask stops instead of retrying. + noExtraImages := false + + err := retry.RunTask( + ctx, + svc.userLogger, + fmt.Sprintf("Reading extra_images.json of %s:%s", packageName, tag), + task.WithConstantRetries(extraImagesFetchRetries, extraImagesFetchRetryDelay, func(ctx context.Context) error { + // Fetch the package version image + img, err := svc.packagesService.Package(packageName).GetImage(ctx, tag) + if err != nil { + // Image tag absent: nothing to read, skip without retrying + if errors.Is(err, client.ErrImageNotFound) { + noExtraImages = true + + return nil + } + + // Transient error: let RunTask retry + return err + } + + // Read extra_images.json out of the image + data, err := extractExtraImagesJSON(img) + if err != nil { + // This version declares no extra images, skip without retrying + if errors.Is(err, errExtraImagesJSONNotFound) { + noExtraImages = true + + return nil + } + + // Transient error: let RunTask retry + return err + } + + extraImagesJSON = data + + return nil + })) + + // If retry failed + if err != nil { + return nil, err + } + + // Version has no extra images + if noExtraImages { + return nil, nil + } + + // extra_images.json found and parsed + return extraImagesJSON, nil +} + +// errExtraImagesJSONNotFound marks that the image has no extra_images.json. +// This is a legitimate skip (not every package version declares extra images), +// distinct from a network/stream error while reading the layer. +var errExtraImagesJSONNotFound = errors.New("extra_images.json not found in image") + +// extractExtraImagesJSON reads extra_images.json from a package image by scanning +// its layers directly. +// +// Reading via img.Extract() (mutate.Extract) is deliberately avoided: on a +// mid-layer read failure it flushes an empty tar footer and hands the caller a +// clean io.EOF, hiding the network error behind the same signal as a genuinely +// absent file. A direct layer scan surfaces a truncated read as a real error, +// so the caller can retry a transient failure instead of dropping the version. +// +// Returns errExtraImagesJSONNotFound only when every layer is read in full and +// the file is absent. +func extractExtraImagesJSON(img interface{ Layers() ([]v1.Layer, error) }) (map[string]interface{}, error) { + layers, err := img.Layers() + if err != nil { + return nil, fmt.Errorf("list image layers: %w", err) + } + + // Top layer first, matching the flatten precedence of a layered image. + for i := len(layers) - 1; i >= 0; i-- { + extraImages, err := findExtraImagesJSONInLayer(layers[i]) + if errors.Is(err, errExtraImagesJSONNotFound) { + continue + } + + if err != nil { + return nil, err + } + + return extraImages, nil + } + + return nil, errExtraImagesJSONNotFound +} + +// findExtraImagesJSONInLayer scans one layer's tar for extra_images.json. +// It returns errExtraImagesJSONNotFound when the layer is read in full without +// the file, and a wrapped error when the layer read itself fails. +func findExtraImagesJSONInLayer(layer v1.Layer) (map[string]interface{}, error) { + rc, err := layer.Uncompressed() + if err != nil { + return nil, fmt.Errorf("read layer: %w", err) + } defer rc.Close() tr := tar.NewReader(rc) for { hdr, err := tr.Next() if err == io.EOF { - return nil, fmt.Errorf("extra_images.json not found in image") + return nil, errExtraImagesJSONNotFound } if err != nil { - return nil, err + return nil, fmt.Errorf("read layer: %w", err) } - if hdr.Name == "extra_images.json" { - var extraImages map[string]interface{} - if err := json.NewDecoder(tr).Decode(&extraImages); err != nil { - return nil, fmt.Errorf("parse extra_images.json: %w", err) - } + if filepath.Clean(hdr.Name) != "extra_images.json" { + continue + } - return extraImages, nil + var extraImages map[string]interface{} + if err := json.NewDecoder(tr).Decode(&extraImages); err != nil { + return nil, fmt.Errorf("parse extra_images.json: %w", err) } + + return extraImages, nil } }