From 82e2905795aece1680b80384900274d3d48dca39 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 01:36:56 +0300 Subject: [PATCH 01/15] Better doc comments to clarify general situation Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index cd4e9b35..7d2241b0 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -180,6 +180,8 @@ type moduleData struct { func (svc *Service) pullModules(ctx context.Context) error { logger := svc.userLogger + // Temporary workspace for module OCI layouts: + // - stores intermediate pulled images; final bundle is packed later. tmpDir := filepath.Join(svc.workingDir, "modules") // List all available modules @@ -193,7 +195,7 @@ func (svc *Service) pullModules(ctx context.Context) error { return nil } - // Filter modules according to whitelist/blacklist + // Filter-out modules that are not allowed by the filter (blacklist or whitelist) filteredModules := make([]moduleData, 0) for _, moduleName := range moduleNames { mod := &Module{ From 3e345f6306f1f0965746a6c991df4a56b678c2ab Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 01:42:12 +0300 Subject: [PATCH 02/15] Make "--include/exclude modules" mutually exclusive It's obvious from the flags logic itself, and it simplifies the following code. Signed-off-by: Roman Berezkin --- internal/mirror/cmd/pull/pull.go | 15 ++++++++++----- internal/mirror/cmd/pull/pull_test.go | 12 ++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/internal/mirror/cmd/pull/pull.go b/internal/mirror/cmd/pull/pull.go index f18a123d..714d88ee 100644 --- a/internal/mirror/cmd/pull/pull.go +++ b/internal/mirror/cmd/pull/pull.go @@ -98,6 +98,7 @@ func NewCommand() *cobra.Command { } pullflags.AddFlags(pullCmd.Flags()) + pullCmd.MarkFlagsMutuallyExclusive("include-module", "exclude-module") pullflags.ParseEnvironmentVariables() return pullCmd @@ -375,14 +376,18 @@ func (p *Puller) validateModulesAccess() error { // createModuleFilter creates the appropriate module filter based on whitelist/blacklist func (p *Puller) createModuleFilter() (*modules.Filter, error) { - filterExpressions := pullflags.ModulesBlacklist - filterType := modules.FilterTypeBlacklist + // Flags are mutually exclusive: + // - --include-module: whitelist mode (mirror only listed modules); + // - otherwise: blacklist mode via --exclude-module (skip listed modules). if pullflags.ModulesWhitelist != nil { - filterExpressions = pullflags.ModulesWhitelist - filterType = modules.FilterTypeWhitelist + filter, err := modules.NewFilter(pullflags.ModulesWhitelist, modules.FilterTypeWhitelist) + if err != nil { + return nil, fmt.Errorf("Prepare module filter: %w", err) + } + return filter, nil } - filter, err := modules.NewFilter(filterExpressions, filterType) + filter, err := modules.NewFilter(pullflags.ModulesBlacklist, modules.FilterTypeBlacklist) if err != nil { return nil, fmt.Errorf("Prepare module filter: %w", err) } diff --git a/internal/mirror/cmd/pull/pull_test.go b/internal/mirror/cmd/pull/pull_test.go index 4bc488ce..95fd2910 100644 --- a/internal/mirror/cmd/pull/pull_test.go +++ b/internal/mirror/cmd/pull/pull_test.go @@ -54,6 +54,18 @@ func TestNewCommand(t *testing.T) { assert.NotNil(t, cmd.Flags()) } +func TestNewCommandMutuallyExclusiveModuleFlags(t *testing.T) { + cmd := NewCommand() + cmd.PreRunE = nil + cmd.RunE = func(_ *cobra.Command, _ []string) error { return nil } + cmd.SetArgs([]string{"--include-module", "module-a", "--exclude-module", "module-b", "bundle-path"}) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "include-module") + assert.Contains(t, err.Error(), "exclude-module") +} + func TestSetupLogger(t *testing.T) { tests := []struct { name string From fc2e6eb1087a036300e2fe55f34d48c9e653bd6c Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 01:46:59 +0300 Subject: [PATCH 03/15] Added doc comment Signed-off-by: Roman Berezkin --- internal/mirror/modules/filter.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/mirror/modules/filter.go b/internal/mirror/modules/filter.go index 7582e230..bd8f7d11 100644 --- a/internal/mirror/modules/filter.go +++ b/internal/mirror/modules/filter.go @@ -152,6 +152,8 @@ func (f *Filter) ShouldMirrorReleaseChannels(moduleName string) bool { return true } +// VersionsToMirror resolves module constraints from --include-module into concrete tags to pull. +// Returns nil when no explicit version tags should be added for this module. func (f *Filter) VersionsToMirror(mod *Module) []string { c, ok := f.modules[mod.Name] if !ok { From 9c0400c3f9103220bf50b32f4f91832af4d48f67 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 01:57:15 +0300 Subject: [PATCH 04/15] FIX THE BUG: List tags for filter candidates instead of using default zero value (nil) + test for the behavior + names refactor Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 9 + internal/mirror/modules/pull_versions_test.go | 158 ++++++++++++++++++ 2 files changed, 167 insertions(+) create mode 100644 internal/mirror/modules/pull_versions_test.go diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index 7d2241b0..13adaeb0 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -319,10 +319,19 @@ func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) err moduleVersions = svc.extractVersionsFromReleaseChannels(ctx, module.name) } + // Fetch the full list of module tags from the registry so semver constraints + // in the filter can match against actual published versions, not just the + // versions currently advertised on release channels. + tags, err := svc.modulesService.Module(module.name).ListTags(ctx) + if err != nil { + return fmt.Errorf("list tags for module %s: %w", module.name, err) + } + // Check for explicit version constraints from filter mod := &Module{ Name: module.name, RegistryPath: module.registryPath, + Releases: tags, } // Get specific versions to mirror from filter (for whitelist with version constraints) diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go new file mode 100644 index 00000000..7a8d7819 --- /dev/null +++ b/internal/mirror/modules/pull_versions_test.go @@ -0,0 +1,158 @@ +/* +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 ( + "context" + "log/slog" + "strings" + "testing" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + dkplog "github.com/deckhouse/deckhouse/pkg/log" + upfake "github.com/deckhouse/deckhouse/pkg/registry/fake" + + "github.com/deckhouse/deckhouse-cli/pkg" + "github.com/deckhouse/deckhouse-cli/pkg/libmirror/util/log" + pkgclient "github.com/deckhouse/deckhouse-cli/pkg/registry/client" + registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" +) + +const ( + pullVersionsTestHost = "fake.registry" + pullVersionsTestModule = "console" +) + +// TestPullSingleModule_SemverConstraintExpandsToRegistryTags is a regression test +// for the bug where `--include-module @` ignored the registry's +// list of module version tags and produced a download set containing only the +// versions currently advertised on release channels. +// +// Before the fix, pullSingleModule built `&Module{Name, RegistryPath}` with +// Releases == nil and passed it to Filter.VersionsToMirror. The semver branch +// iterates mod.Releases, which was empty, so VersionsToMirror returned nil and +// only the channel version made it into downloadList.Module. +// +// After the fix, pullSingleModule fetches the full tag list via +// modulesService.Module(name).ListTags(ctx) and stores it in Module.Releases, +// so the semver constraint matches every version in the registry that falls in +// range. +func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { + const channelVersion = "v1.45.2" + + // Versions present in the fake registry. v1.39.0 is intentionally below the + // semver lower bound (^1.40.0 → >=1.40.0) and must be excluded by the filter. + allVersions := []string{ + "v1.39.0", + "v1.40.0", + "v1.40.1", + "v1.41.0", + "v1.42.0", + "v1.43.0", + "v1.44.0", + "v1.45.2", + } + + reg := upfake.NewRegistry(pullVersionsTestHost) + + // Top-level "modules" repo: tags are module names. + // modulesService.ListTags returns ["console"]. + reg.MustAddImage("modules", pullVersionsTestModule, pullVersionsImage(channelVersion)) + + // Per-module repo: tags are the versions available in the registry. + // modulesService.Module("console").ListTags returns the full version list. + for _, v := range allVersions { + reg.MustAddImage("modules/"+pullVersionsTestModule, v, pullVersionsImage(v)) + } + + // Release-channel repo: 5 channels, each pointing at the same channel version. + // extractVersionsFromReleaseChannels reads version.json from each. + for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { + reg.MustAddImage("modules/"+pullVersionsTestModule+"/release", ch, pullVersionsImage(channelVersion)) + } + + stubClient := pkgclient.Adapt(upfake.NewClient(reg)) + + // Whitelist filter: console@1.40.0 → semver ^1.40.0 (>=1.40.0 <2.0.0). + filter, err := NewFilter([]string{pullVersionsTestModule + "@1.40.0"}, FilterTypeWhitelist) + require.NoError(t, err) + + logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) + userLogger := log.NewSLogger(slog.LevelWarn) + + regSvc := registryservice.NewService(stubClient, pkg.NoEdition, logger) + + svc := NewService( + regSvc, + t.TempDir(), + &Options{ + BundleDir: t.TempDir(), + Filter: filter, + SkipVexImages: true, // VEX discovery is unrelated to this test. + }, + logger, + userLogger, + ) + + require.NoError(t, svc.PullModules(context.Background())) + + // After PullModules the per-module download list must contain all versions + // that match the filter, not only the channel version. + moduleDL := svc.modulesDownloadList.list[pullVersionsTestModule] + require.NotNil(t, moduleDL, "expected download list entry for module %q", pullVersionsTestModule) + + got := make([]string, 0, len(moduleDL.Module)) + for ref := range moduleDL.Module { + // extractInternalDigestImages can add @sha256: refs - we only care about + // version-tagged refs in this assertion. + if strings.Contains(ref, "@sha256:") { + continue + } + got = append(got, ref) + } + + want := []string{ + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.40.0", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.40.1", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.41.0", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.42.0", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.43.0", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.44.0", + pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.45.2", + } + + assert.ElementsMatch(t, want, got, + "semver ^1.40.0 must expand to every matching tag in the registry, not only the channel version") + + // v1.39.0 is below the lower bound; it must not be pulled. + assert.NotContains(t, got, pullVersionsTestHost+"/modules/"+pullVersionsTestModule+":v1.39.0", + "v1.39.0 is below ^1.40.0 lower bound and must be excluded") +} + +// pullVersionsImage builds a minimal v1.Image with version.json so that +// extractVersionsFromReleaseChannels and extractVersionJSON have something to +// read. images_digests.json and extra_images.json are intentionally omitted - +// downstream code tolerates missing files (debug-log + skip). +func pullVersionsImage(version string) v1.Image { + return upfake.NewImageBuilder(). + WithFile("version.json", `{"version":"`+version+`"}`). + WithLabel("org.opencontainers.image.version", version). + MustBuild() +} From a672550bcaf9021985ec8cf05ecc3d28082bb3e7 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 01:57:33 +0300 Subject: [PATCH 05/15] Names refactor Signed-off-by: Roman Berezkin --- internal/mirror/modules/filter.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/mirror/modules/filter.go b/internal/mirror/modules/filter.go index bd8f7d11..606af5a3 100644 --- a/internal/mirror/modules/filter.go +++ b/internal/mirror/modules/filter.go @@ -146,7 +146,8 @@ func parseSemver(v string) (VersionConstraint, error) { } func (f *Filter) ShouldMirrorReleaseChannels(moduleName string) bool { - if c, ok := f.modules[moduleName]; ok && c.IsExact() { + constraint, hasConstraint := f.modules[moduleName] + if hasConstraint && constraint.IsExact() { return false } return true @@ -155,26 +156,27 @@ func (f *Filter) ShouldMirrorReleaseChannels(moduleName string) bool { // VersionsToMirror resolves module constraints from --include-module into concrete tags to pull. // Returns nil when no explicit version tags should be added for this module. func (f *Filter) VersionsToMirror(mod *Module) []string { - c, ok := f.modules[mod.Name] - if !ok { + constraint, hasConstraint := f.modules[mod.Name] + if !hasConstraint { return nil } - if c.IsExact() { - if e, ok := c.(*ExactTagConstraint); ok { - return []string{e.Tag()} + if constraint.IsExact() { + exact, isExactTag := constraint.(*ExactTagConstraint) + if !isExactTag { + return nil } - return nil + return []string{exact.Tag()} } - sc, ok := c.(*SemanticVersionConstraint) - if !ok { + semver, isSemver := constraint.(*SemanticVersionConstraint) + if !isSemver { return nil } var tags []string for _, v := range mod.Versions() { - if sc.Match(v) { + if semver.Match(v) { tags = append(tags, "v"+v.String()) } } From fc04748624993016545176ea14c0744b8352bfd8 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 03:54:02 +0300 Subject: [PATCH 06/15] Skip per-module ListTags when filter doesn't need Releases - `ListTags` for a module's tag list now runs only when the constraint is semver. Exact-tag and blacklist-mode modules take filter branches that never read `Module.Releases`, so the call was wasted work. - For a default `d8 mirror pull` (no `--include-module`), per-module `ListTags` drops to zero across the registry. - Adds tests verifying the skip for exact-tag and blacklist cases, plus a counter-check that semver constraint still triggers the call. Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 19 +- internal/mirror/modules/pull_versions_test.go | 188 ++++++++++++------ 2 files changed, 142 insertions(+), 65 deletions(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index 13adaeb0..0c12827f 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -319,12 +319,19 @@ func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) err moduleVersions = svc.extractVersionsFromReleaseChannels(ctx, module.name) } - // Fetch the full list of module tags from the registry so semver constraints - // in the filter can match against actual published versions, not just the - // versions currently advertised on release channels. - tags, err := svc.modulesService.Module(module.name).ListTags(ctx) - if err != nil { - return fmt.Errorf("list tags for module %s: %w", module.name, err) + // Fetch the full list of module tags from the registry only when the filter + // constraint actually needs them: + // - a semver constraint iterates Releases to pick matching versions. + // - Exact-tag constraints and modules without a constraint + // (blacklist mode for non-excluded modules) never read Releases + // so the network call is skipped to avoid wasted requests. + var tags []string + if constraint, hasConstraint := svc.options.Filter.GetConstraint(module.name); hasConstraint && !constraint.IsExact() { + var err error + tags, err = svc.modulesService.Module(module.name).ListTags(ctx) + if err != nil { + return fmt.Errorf("list tags for module %s: %w", module.name, err) + } } // Check for explicit version constraints from filter diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go index 7a8d7819..41635307 100644 --- a/internal/mirror/modules/pull_versions_test.go +++ b/internal/mirror/modules/pull_versions_test.go @@ -20,6 +20,7 @@ import ( "context" "log/slog" "strings" + "sync/atomic" "testing" v1 "github.com/google/go-containerregistry/pkg/v1" @@ -27,6 +28,7 @@ import ( "github.com/stretchr/testify/require" dkplog "github.com/deckhouse/deckhouse/pkg/log" + dkpreg "github.com/deckhouse/deckhouse/pkg/registry" upfake "github.com/deckhouse/deckhouse/pkg/registry/fake" "github.com/deckhouse/deckhouse-cli/pkg" @@ -40,25 +42,12 @@ const ( pullVersionsTestModule = "console" ) -// TestPullSingleModule_SemverConstraintExpandsToRegistryTags is a regression test -// for the bug where `--include-module @` ignored the registry's -// list of module version tags and produced a download set containing only the -// versions currently advertised on release channels. -// -// Before the fix, pullSingleModule built `&Module{Name, RegistryPath}` with -// Releases == nil and passed it to Filter.VersionsToMirror. The semver branch -// iterates mod.Releases, which was empty, so VersionsToMirror returned nil and -// only the channel version made it into downloadList.Module. -// -// After the fix, pullSingleModule fetches the full tag list via -// modulesService.Module(name).ListTags(ctx) and stores it in Module.Releases, -// so the semver constraint matches every version in the registry that falls in -// range. +// Regression: --include-module @ must pull every matching tag +// in the registry, not only the version currently on a release channel. func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { const channelVersion = "v1.45.2" - // Versions present in the fake registry. v1.39.0 is intentionally below the - // semver lower bound (^1.40.0 → >=1.40.0) and must be excluded by the filter. + // v1.39.0 is below the ^1.40.0 lower bound and must be excluded by the filter. allVersions := []string{ "v1.39.0", "v1.40.0", @@ -71,57 +60,26 @@ func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { } reg := upfake.NewRegistry(pullVersionsTestHost) - - // Top-level "modules" repo: tags are module names. - // modulesService.ListTags returns ["console"]. reg.MustAddImage("modules", pullVersionsTestModule, pullVersionsImage(channelVersion)) - - // Per-module repo: tags are the versions available in the registry. - // modulesService.Module("console").ListTags returns the full version list. for _, v := range allVersions { reg.MustAddImage("modules/"+pullVersionsTestModule, v, pullVersionsImage(v)) } - - // Release-channel repo: 5 channels, each pointing at the same channel version. - // extractVersionsFromReleaseChannels reads version.json from each. for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { reg.MustAddImage("modules/"+pullVersionsTestModule+"/release", ch, pullVersionsImage(channelVersion)) } - stubClient := pkgclient.Adapt(upfake.NewClient(reg)) - - // Whitelist filter: console@1.40.0 → semver ^1.40.0 (>=1.40.0 <2.0.0). filter, err := NewFilter([]string{pullVersionsTestModule + "@1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) - logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) - userLogger := log.NewSLogger(slog.LevelWarn) - - regSvc := registryservice.NewService(stubClient, pkg.NoEdition, logger) - - svc := NewService( - regSvc, - t.TempDir(), - &Options{ - BundleDir: t.TempDir(), - Filter: filter, - SkipVexImages: true, // VEX discovery is unrelated to this test. - }, - logger, - userLogger, - ) - + svc := newPullVersionsService(t, pkgclient.Adapt(upfake.NewClient(reg)), filter) require.NoError(t, svc.PullModules(context.Background())) - // After PullModules the per-module download list must contain all versions - // that match the filter, not only the channel version. moduleDL := svc.modulesDownloadList.list[pullVersionsTestModule] - require.NotNil(t, moduleDL, "expected download list entry for module %q", pullVersionsTestModule) + require.NotNil(t, moduleDL) got := make([]string, 0, len(moduleDL.Module)) for ref := range moduleDL.Module { - // extractInternalDigestImages can add @sha256: refs - we only care about - // version-tagged refs in this assertion. + // Skip @sha256: refs added by extractInternalDigestImages - the assertion is about version-tagged refs. if strings.Contains(ref, "@sha256:") { continue } @@ -138,21 +96,133 @@ func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.45.2", } - assert.ElementsMatch(t, want, got, - "semver ^1.40.0 must expand to every matching tag in the registry, not only the channel version") + assert.ElementsMatch(t, want, got) + assert.NotContains(t, got, pullVersionsTestHost+"/modules/"+pullVersionsTestModule+":v1.39.0") +} - // v1.39.0 is below the lower bound; it must not be pulled. - assert.NotContains(t, got, pullVersionsTestHost+"/modules/"+pullVersionsTestModule+":v1.39.0", - "v1.39.0 is below ^1.40.0 lower bound and must be excluded") +// Exact-tag constraint takes a Filter branch that never reads Module.Releases, +// so per-module ListTags must be skipped. Baseline = 2 (validateModulesAccess +// + pullModules root listing). +func TestPullSingleModule_ExactTagConstraintSkipsPerModuleListTags(t *testing.T) { + filter, err := NewFilter([]string{pullVersionsTestModule + "@=v1.40.0"}, FilterTypeWhitelist) + require.NoError(t, err) + + counter := runPullModulesWithCounter(t, filter) + + assert.Equal(t, int64(2), counter.calls.Load(), + "exact-tag constraint must skip per-module ListTags") +} + +// In blacklist mode (no --include-module) modules without a filter entry +// short-circuit in Filter.VersionsToMirror, so per-module ListTags must be +// skipped. Saves N requests for N non-excluded modules. +func TestPullSingleModule_BlacklistModeSkipsPerModuleListTags(t *testing.T) { + filter, err := NewFilter(nil, FilterTypeBlacklist) + require.NoError(t, err) + + counter := runPullModulesWithCounter(t, filter) + + assert.Equal(t, int64(2), counter.calls.Load(), + "blacklist mode must skip per-module ListTags for non-excluded modules") } -// pullVersionsImage builds a minimal v1.Image with version.json so that -// extractVersionsFromReleaseChannels and extractVersionJSON have something to -// read. images_digests.json and extra_images.json are intentionally omitted - -// downstream code tolerates missing files (debug-log + skip). +// Counterpart to the two skip tests: semver constraint MUST trigger per-module +// ListTags, otherwise the original bug resurfaces. +func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T) { + filter, err := NewFilter([]string{pullVersionsTestModule + "@1.40.0"}, FilterTypeWhitelist) + require.NoError(t, err) + + counter := runPullModulesWithCounter(t, filter) + + assert.Equal(t, int64(3), counter.calls.Load(), + "semver constraint must trigger per-module ListTags exactly once") +} + +// pullVersionsImage builds a v1.Image with only version.json. +// images_digests.json and extra_images.json are intentionally omitted - +// downstream code tolerates missing files. func pullVersionsImage(version string) v1.Image { return upfake.NewImageBuilder(). WithFile("version.json", `{"version":"`+version+`"}`). WithLabel("org.opencontainers.image.version", version). MustBuild() } + +// listTagsCounter wraps a Client and counts ListTags calls. The counter is +// shared across all sub-clients produced by WithSegment, so scoped calls +// increment the same counter as the parent. +type listTagsCounter struct { + dkpreg.Client + calls *atomic.Int64 +} + +func newListTagsCounter(c dkpreg.Client) *listTagsCounter { + return &listTagsCounter{Client: c, calls: new(atomic.Int64)} +} + +func (c *listTagsCounter) WithSegment(segments ...string) dkpreg.Client { + return &listTagsCounter{ + Client: c.Client.WithSegment(segments...), + calls: c.calls, + } +} + +func (c *listTagsCounter) ListTags(ctx context.Context, opts ...dkpreg.ListTagsOption) ([]string, error) { + c.calls.Add(1) + return c.Client.ListTags(ctx, opts...) +} + +// pullVersionsBuildRegistry mirrors the production modules layout: top-level +// "modules" repo, per-module repo with version tags, per-module "release" repo +// with channel tags. +func pullVersionsBuildRegistry() *upfake.Registry { + const channelVersion = "v1.45.2" + + reg := upfake.NewRegistry(pullVersionsTestHost) + + reg.MustAddImage("modules", pullVersionsTestModule, pullVersionsImage(channelVersion)) + + for _, v := range []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} { + reg.MustAddImage("modules/"+pullVersionsTestModule, v, pullVersionsImage(v)) + } + + for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { + reg.MustAddImage("modules/"+pullVersionsTestModule+"/release", ch, pullVersionsImage(channelVersion)) + } + + return reg +} + +// newPullVersionsService wires a Service against the given registry client and +// filter with logging muted to warn-level. Used by tests that inspect the +// resulting download list. +func newPullVersionsService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service { + t.Helper() + + logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) + userLogger := log.NewSLogger(slog.LevelWarn) + regSvc := registryservice.NewService(stubClient, pkg.NoEdition, logger) + + return NewService( + regSvc, + t.TempDir(), + &Options{ + BundleDir: t.TempDir(), + Filter: filter, + SkipVexImages: true, + }, + logger, + userLogger, + ) +} + +// runPullModulesWithCounter runs PullModules against a fresh fake registry +// and returns the counter for ListTags assertions. +func runPullModulesWithCounter(t *testing.T, filter *Filter) *listTagsCounter { + t.Helper() + + counter := newListTagsCounter(upfake.NewClient(pullVersionsBuildRegistry())) + svc := newPullVersionsService(t, pkgclient.Adapt(counter), filter) + require.NoError(t, svc.PullModules(context.Background())) + return counter +} From ada8961d0eb75a9e02960a070318595aed2a53f4 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 18:25:11 +0300 Subject: [PATCH 07/15] Fix typo in the doc comment Signed-off-by: Roman Berezkin --- internal/mirror/modules/filter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mirror/modules/filter.go b/internal/mirror/modules/filter.go index 606af5a3..b66fdf52 100644 --- a/internal/mirror/modules/filter.go +++ b/internal/mirror/modules/filter.go @@ -123,7 +123,7 @@ func parseVersionConstraint(v string) (VersionConstraint, error) { } func parseExact(body string) (VersionConstraint, error) { - // exac match, console@=1.38.1 = registry.deckhouse.io/deckhouse/ce/modules/console:v1.38.1 + // exact match, console@=v1.38.1 -> registry.deckhouse.io/deckhouse/ce/modules/console:v1.38.1 tag, ch, _ := strings.Cut(body, "+") if tag == "" { return nil, fmt.Errorf("empty tag in %q", body) From cc220723d984fb228c319f34573687b95a1e7f2d Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 18:45:59 +0300 Subject: [PATCH 08/15] Refactor tests Signed-off-by: Roman Berezkin --- internal/mirror/modules/pull_versions_test.go | 189 ++++++++++-------- 1 file changed, 103 insertions(+), 86 deletions(-) diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go index 41635307..0eae04df 100644 --- a/internal/mirror/modules/pull_versions_test.go +++ b/internal/mirror/modules/pull_versions_test.go @@ -37,79 +37,68 @@ import ( registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" ) +// Test fixture identifiers and shared registry contents. Tests that need +// extra version tags pass their own list to buildTestRegistry; otherwise +// defaultModuleVersions is used. const ( - pullVersionsTestHost = "fake.registry" - pullVersionsTestModule = "console" + testHost = "fake.registry" + testModule = "console" + channelVersion = "v1.45.2" +) + +var defaultModuleVersions = []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} + +// ListTags is called twice on the root level for every pullModules run: +// - once by validateModulesAccess to smoke-check registry credentials, +// - once by pullModules itself to enumerate all modules in the registry. +// On top of this baseline, pullSingleModule calls ListTags once per module +// that has a non-exact (semver) constraint - exact-tag and blacklist modules +// short-circuit before this call. +const ( + baselineListTagsCalls int64 = 2 + perModuleListTagsCall int64 = 1 ) // Regression: --include-module @ must pull every matching tag // in the registry, not only the version currently on a release channel. func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { - const channelVersion = "v1.45.2" - // v1.39.0 is below the ^1.40.0 lower bound and must be excluded by the filter. - allVersions := []string{ + registryVersions := []string{ "v1.39.0", - "v1.40.0", - "v1.40.1", - "v1.41.0", - "v1.42.0", - "v1.43.0", - "v1.44.0", + "v1.40.0", "v1.40.1", + "v1.41.0", "v1.42.0", "v1.43.0", "v1.44.0", "v1.45.2", } - - reg := upfake.NewRegistry(pullVersionsTestHost) - reg.MustAddImage("modules", pullVersionsTestModule, pullVersionsImage(channelVersion)) - for _, v := range allVersions { - reg.MustAddImage("modules/"+pullVersionsTestModule, v, pullVersionsImage(v)) - } - for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { - reg.MustAddImage("modules/"+pullVersionsTestModule+"/release", ch, pullVersionsImage(channelVersion)) + expectedVersions := []string{ + "v1.40.0", "v1.40.1", + "v1.41.0", "v1.42.0", "v1.43.0", "v1.44.0", + "v1.45.2", } - filter, err := NewFilter([]string{pullVersionsTestModule + "@1.40.0"}, FilterTypeWhitelist) + filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) - svc := newPullVersionsService(t, pkgclient.Adapt(upfake.NewClient(reg)), filter) + svc := newService(t, pkgclient.Adapt(upfake.NewClient(buildTestRegistry(registryVersions))), filter) require.NoError(t, svc.PullModules(context.Background())) - moduleDL := svc.modulesDownloadList.list[pullVersionsTestModule] - require.NotNil(t, moduleDL) - - got := make([]string, 0, len(moduleDL.Module)) - for ref := range moduleDL.Module { - // Skip @sha256: refs added by extractInternalDigestImages - the assertion is about version-tagged refs. - if strings.Contains(ref, "@sha256:") { - continue - } - got = append(got, ref) - } - - want := []string{ - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.40.0", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.40.1", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.41.0", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.42.0", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.43.0", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.44.0", - pullVersionsTestHost + "/modules/" + pullVersionsTestModule + ":v1.45.2", - } + dl := svc.modulesDownloadList.list[testModule] + require.NotNil(t, dl) - assert.ElementsMatch(t, want, got) - assert.NotContains(t, got, pullVersionsTestHost+"/modules/"+pullVersionsTestModule+":v1.39.0") + got := extractTaggedRefs(dl) + assert.ElementsMatch(t, taggedModuleRefs(expectedVersions), got) + assert.NotContains(t, got, taggedRef("v1.39.0"), + "v1.39.0 is below ^1.40.0 lower bound and must be excluded") } // Exact-tag constraint takes a Filter branch that never reads Module.Releases, -// so per-module ListTags must be skipped. Baseline = 2 (validateModulesAccess -// + pullModules root listing). +// so per-module ListTags must be skipped - only the baseline calls happen. func TestPullSingleModule_ExactTagConstraintSkipsPerModuleListTags(t *testing.T) { - filter, err := NewFilter([]string{pullVersionsTestModule + "@=v1.40.0"}, FilterTypeWhitelist) + filter, err := NewFilter([]string{testModule + "@=v1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) - counter := runPullModulesWithCounter(t, filter) + counter := runPullWithCounter(t, filter) - assert.Equal(t, int64(2), counter.calls.Load(), + assert.Equal(t, baselineListTagsCalls, counter.calls.Load(), "exact-tag constraint must skip per-module ListTags") } @@ -120,34 +109,83 @@ func TestPullSingleModule_BlacklistModeSkipsPerModuleListTags(t *testing.T) { filter, err := NewFilter(nil, FilterTypeBlacklist) require.NoError(t, err) - counter := runPullModulesWithCounter(t, filter) + counter := runPullWithCounter(t, filter) - assert.Equal(t, int64(2), counter.calls.Load(), + assert.Equal(t, baselineListTagsCalls, counter.calls.Load(), "blacklist mode must skip per-module ListTags for non-excluded modules") } // Counterpart to the two skip tests: semver constraint MUST trigger per-module // ListTags, otherwise the original bug resurfaces. func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T) { - filter, err := NewFilter([]string{pullVersionsTestModule + "@1.40.0"}, FilterTypeWhitelist) + filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) - counter := runPullModulesWithCounter(t, filter) + counter := runPullWithCounter(t, filter) - assert.Equal(t, int64(3), counter.calls.Load(), + assert.Equal(t, baselineListTagsCalls+perModuleListTagsCall, counter.calls.Load(), "semver constraint must trigger per-module ListTags exactly once") } -// pullVersionsImage builds a v1.Image with only version.json. +// buildTestRegistry returns a fake registry that mirrors the production +// "modules/" + "modules//release/" layout. +// Pass moduleVersions to control the set of version-tagged module images. +func buildTestRegistry(moduleVersions []string) *upfake.Registry { + reg := upfake.NewRegistry(testHost) + + reg.MustAddImage("modules", testModule, makeVersionImage(channelVersion)) + + for _, v := range moduleVersions { + reg.MustAddImage("modules/"+testModule, v, makeVersionImage(v)) + } + + for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { + reg.MustAddImage("modules/"+testModule+"/release", ch, makeVersionImage(channelVersion)) + } + + return reg +} + +// makeVersionImage builds a v1.Image with only version.json populated. // images_digests.json and extra_images.json are intentionally omitted - // downstream code tolerates missing files. -func pullVersionsImage(version string) v1.Image { +func makeVersionImage(version string) v1.Image { return upfake.NewImageBuilder(). WithFile("version.json", `{"version":"`+version+`"}`). WithLabel("org.opencontainers.image.version", version). MustBuild() } +// taggedRef returns the registry URL the production code uses for a single +// version-tagged module image. +func taggedRef(version string) string { + return testHost + "/modules/" + testModule + ":" + version +} + +// taggedModuleRefs converts a list of versions into the registry URLs the +// regression assertions expect to see in the download list. +func taggedModuleRefs(versions []string) []string { + refs := make([]string, 0, len(versions)) + for _, v := range versions { + refs = append(refs, taggedRef(v)) + } + return refs +} + +// extractTaggedRefs returns version-tagged module image refs from the +// download list, filtering out @sha256: refs added by extra-image +// resolution (those are not relevant to constraint tests). +func extractTaggedRefs(dl *ImageDownloadList) []string { + refs := make([]string, 0, len(dl.Module)) + for ref := range dl.Module { + if strings.Contains(ref, "@sha256:") { + continue + } + refs = append(refs, ref) + } + return refs +} + // listTagsCounter wraps a Client and counts ListTags calls. The counter is // shared across all sub-clients produced by WithSegment, so scoped calls // increment the same counter as the parent. @@ -172,31 +210,10 @@ func (c *listTagsCounter) ListTags(ctx context.Context, opts ...dkpreg.ListTagsO return c.Client.ListTags(ctx, opts...) } -// pullVersionsBuildRegistry mirrors the production modules layout: top-level -// "modules" repo, per-module repo with version tags, per-module "release" repo -// with channel tags. -func pullVersionsBuildRegistry() *upfake.Registry { - const channelVersion = "v1.45.2" - - reg := upfake.NewRegistry(pullVersionsTestHost) - - reg.MustAddImage("modules", pullVersionsTestModule, pullVersionsImage(channelVersion)) - - for _, v := range []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} { - reg.MustAddImage("modules/"+pullVersionsTestModule, v, pullVersionsImage(v)) - } - - for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { - reg.MustAddImage("modules/"+pullVersionsTestModule+"/release", ch, pullVersionsImage(channelVersion)) - } - - return reg -} - -// newPullVersionsService wires a Service against the given registry client and -// filter with logging muted to warn-level. Used by tests that inspect the -// resulting download list. -func newPullVersionsService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service { +// newService wires a Service against the given registry client and filter +// with logging muted to warn-level. Used by tests that inspect either the +// download list or the ListTags call counter. +func newService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service { t.Helper() logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) @@ -216,13 +233,13 @@ func newPullVersionsService(t *testing.T, stubClient dkpreg.Client, filter *Filt ) } -// runPullModulesWithCounter runs PullModules against a fresh fake registry -// and returns the counter for ListTags assertions. -func runPullModulesWithCounter(t *testing.T, filter *Filter) *listTagsCounter { +// runPullWithCounter runs PullModules against a fresh fake registry built +// with defaultModuleVersions and returns the ListTags call counter. +func runPullWithCounter(t *testing.T, filter *Filter) *listTagsCounter { t.Helper() - counter := newListTagsCounter(upfake.NewClient(pullVersionsBuildRegistry())) - svc := newPullVersionsService(t, pkgclient.Adapt(counter), filter) + counter := newListTagsCounter(upfake.NewClient(buildTestRegistry(defaultModuleVersions))) + svc := newService(t, pkgclient.Adapt(counter), filter) require.NoError(t, svc.PullModules(context.Background())) return counter } From c017b96e8c3273ac18d0a6b457bacac699580da3 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 18:50:50 +0300 Subject: [PATCH 09/15] Expand pull versions tests (to check different semver constraint filtration) Signed-off-by: Roman Berezkin --- internal/mirror/modules/pull_versions_test.go | 158 ++++++++++++++---- 1 file changed, 125 insertions(+), 33 deletions(-) diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go index 0eae04df..4bcffcff 100644 --- a/internal/mirror/modules/pull_versions_test.go +++ b/internal/mirror/modules/pull_versions_test.go @@ -61,33 +61,119 @@ const ( // Regression: --include-module @ must pull every matching tag // in the registry, not only the version currently on a release channel. -func TestPullSingleModule_SemverConstraintExpandsToRegistryTags(t *testing.T) { - // v1.39.0 is below the ^1.40.0 lower bound and must be excluded by the filter. +// Each subtest uses the same registry layout but a different constraint; +// the spread of registry versions is wide enough to make the expected sets +// of caret / tilde / range distinguishable. +func TestPullSingleModule_SemverConstraintMatchesRegistryTags(t *testing.T) { registryVersions := []string{ "v1.39.0", "v1.40.0", "v1.40.1", - "v1.41.0", "v1.42.0", "v1.43.0", "v1.44.0", - "v1.45.2", + "v1.41.0", + "v1.42.0", + "v1.43.0", + channelVersion, // v1.45.2 - the version every release channel points at } - expectedVersions := []string{ - "v1.40.0", "v1.40.1", - "v1.41.0", "v1.42.0", "v1.43.0", "v1.44.0", - "v1.45.2", + + cases := []struct { + name string + constraint string // value placed after "@" in the filter + expected []string // module versions expected in the download list + mustNotContain string // a version that MUST be excluded (sanity check) + }{ + { + // Implicit caret: bare version is expanded to ^X.Y.Z for legacy + // backward compatibility (see filter.go:parseVersionConstraint). + name: "implicit_caret", + constraint: "1.40.0", + expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, + mustNotContain: "v1.39.0", + }, + { + // Explicit caret: same range as implicit. Verifies the parser + // treats both forms identically. + name: "explicit_caret", + constraint: "^1.40.0", + expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, + mustNotContain: "v1.39.0", + }, + { + // Tilde: pinned to the same minor (>=1.40.0 <1.41.0). + // channelVersion is unrelated to the constraint but is always + // added to the download list because release channels are still + // pulled for any non-exact constraint. + name: "tilde", + constraint: "~1.40.0", + expected: []string{"v1.40.0", "v1.40.1", channelVersion}, + mustNotContain: "v1.41.0", + }, + { + // Explicit range. The upper bound is exclusive, so v1.43.0 + // must not appear in the result. + name: "explicit_range", + constraint: ">=1.40.0 <1.43.0", + expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", channelVersion}, + mustNotContain: "v1.43.0", + }, } - filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + filter, err := NewFilter([]string{testModule + "@" + tc.constraint}, FilterTypeWhitelist) + require.NoError(t, err) + + svc := newService(t, pkgclient.Adapt(upfake.NewClient(buildTestRegistry(registryVersions))), filter) + require.NoError(t, svc.PullModules(context.Background())) + + dl := svc.modulesDownloadList.list[testModule] + require.NotNil(t, dl) + + got := extractTaggedRefs(dl) + assert.ElementsMatch(t, taggedModuleRefs(testModule, tc.expected), got) + assert.NotContains(t, got, taggedRef(testModule, tc.mustNotContain), + "constraint %q must exclude %s", tc.constraint, tc.mustNotContain) + }) + } +} + +// Multiple --include-module flags must each apply their own constraint +// independently; constraints on one module must not leak into another. +// This case mixes a semver-constraint module with an exact-tag module to +// also verify the two filter branches coexist correctly in one pull run. +func TestPullSingleModule_MultipleModulesIndependentConstraints(t *testing.T) { + const ( + consoleName = "console" + commanderName = "commander" + ) + + reg := upfake.NewRegistry(testHost) + addModule(reg, consoleName, "v1.40.1", []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"}) + addModule(reg, commanderName, "v0.5.1", []string{"v0.5.0", "v0.5.1", "v0.6.0"}) + + filter, err := NewFilter( + []string{ + consoleName + "@~1.40.0", // tilde: only v1.40.x + channel snapshot + commanderName + "@=v0.6.0", // exact: only v0.6.0, no channels + }, + FilterTypeWhitelist, + ) require.NoError(t, err) - svc := newService(t, pkgclient.Adapt(upfake.NewClient(buildTestRegistry(registryVersions))), filter) + svc := newService(t, pkgclient.Adapt(upfake.NewClient(reg)), filter) require.NoError(t, svc.PullModules(context.Background())) - dl := svc.modulesDownloadList.list[testModule] - require.NotNil(t, dl) - - got := extractTaggedRefs(dl) - assert.ElementsMatch(t, taggedModuleRefs(expectedVersions), got) - assert.NotContains(t, got, taggedRef("v1.39.0"), - "v1.39.0 is below ^1.40.0 lower bound and must be excluded") + consoleDL := svc.modulesDownloadList.list[consoleName] + require.NotNil(t, consoleDL) + assert.ElementsMatch(t, + taggedModuleRefs(consoleName, []string{"v1.40.0", "v1.40.1"}), + extractTaggedRefs(consoleDL), + "tilde constraint on console must match only v1.40.x (channel v1.40.1 dedups)") + + commanderDL := svc.modulesDownloadList.list[commanderName] + require.NotNil(t, commanderDL) + assert.ElementsMatch(t, + taggedModuleRefs(commanderName, []string{"v0.6.0"}), + extractTaggedRefs(commanderDL), + "exact constraint on commander must match only v0.6.0 and skip channels") } // Exact-tag constraint takes a Filter branch that never reads Module.Releases, @@ -127,22 +213,28 @@ func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T "semver constraint must trigger per-module ListTags exactly once") } -// buildTestRegistry returns a fake registry that mirrors the production -// "modules/" + "modules//release/" layout. -// Pass moduleVersions to control the set of version-tagged module images. -func buildTestRegistry(moduleVersions []string) *upfake.Registry { - reg := upfake.NewRegistry(testHost) - - reg.MustAddImage("modules", testModule, makeVersionImage(channelVersion)) +// addModule populates a single module's images in the given registry, +// mirroring the production "modules/" + "modules//release/" +// layout. All 5 release channels point at channelVer. +func addModule(reg *upfake.Registry, name, channelVer string, versions []string) { + reg.MustAddImage("modules", name, makeVersionImage(channelVer)) - for _, v := range moduleVersions { - reg.MustAddImage("modules/"+testModule, v, makeVersionImage(v)) + for _, v := range versions { + reg.MustAddImage("modules/"+name, v, makeVersionImage(v)) } for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { - reg.MustAddImage("modules/"+testModule+"/release", ch, makeVersionImage(channelVersion)) + reg.MustAddImage("modules/"+name+"/release", ch, makeVersionImage(channelVer)) } +} +// buildTestRegistry returns a fake registry containing a single module +// (testModule) with the given versions and channelVersion on every channel. +// For multi-module fixtures, build the registry by hand and call addModule +// for each module. +func buildTestRegistry(moduleVersions []string) *upfake.Registry { + reg := upfake.NewRegistry(testHost) + addModule(reg, testModule, channelVersion, moduleVersions) return reg } @@ -158,16 +250,16 @@ func makeVersionImage(version string) v1.Image { // taggedRef returns the registry URL the production code uses for a single // version-tagged module image. -func taggedRef(version string) string { - return testHost + "/modules/" + testModule + ":" + version +func taggedRef(moduleName, version string) string { + return testHost + "/modules/" + moduleName + ":" + version } -// taggedModuleRefs converts a list of versions into the registry URLs the -// regression assertions expect to see in the download list. -func taggedModuleRefs(versions []string) []string { +// taggedModuleRefs converts a list of versions for a given module into the +// registry URLs the regression assertions expect to see in the download list. +func taggedModuleRefs(moduleName string, versions []string) []string { refs := make([]string, 0, len(versions)) for _, v := range versions { - refs = append(refs, taggedRef(v)) + refs = append(refs, taggedRef(moduleName, v)) } return refs } From 28c284edb1b933d96941556cdeee90070ee23f52 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 20:58:08 +0300 Subject: [PATCH 10/15] Skip per-module ListTags when registry reports module repo missing - `Module(name).ListTags` now warns and falls back to channel-only versions on `client.ErrImageNotFound`, matching the policy of `validateModulesAccess`. Other errors keep the existing fail-fast behavior. Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 8 +- internal/mirror/modules/pull_versions_test.go | 177 ++++++++++++------ 2 files changed, 126 insertions(+), 59 deletions(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index 0c12827f..a4c02cf0 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -329,7 +329,13 @@ func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) err if constraint, hasConstraint := svc.options.Filter.GetConstraint(module.name); hasConstraint && !constraint.IsExact() { var err error tags, err = svc.modulesService.Module(module.name).ListTags(ctx) - if err != nil { + switch { + case errors.Is(err, client.ErrImageNotFound): + // Module repository is missing or empty - registry inconsistency we can't + // act on. Continue with channel-only versions instead of failing the whole + // pull (same policy as validateModulesAccess). + svc.userLogger.Warnf("Skipping tag list for module %s: %v", module.name, err) + case err != nil: return fmt.Errorf("list tags for module %s: %w", module.name, err) } } diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go index 4bcffcff..c388eb6b 100644 --- a/internal/mirror/modules/pull_versions_test.go +++ b/internal/mirror/modules/pull_versions_test.go @@ -18,6 +18,7 @@ package modules import ( "context" + "errors" "log/slog" "strings" "sync/atomic" @@ -37,9 +38,7 @@ import ( registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" ) -// Test fixture identifiers and shared registry contents. Tests that need -// extra version tags pass their own list to buildTestRegistry; otherwise -// defaultModuleVersions is used. +// Shared test-fixture identifiers. const ( testHost = "fake.registry" testModule = "console" @@ -48,22 +47,16 @@ const ( var defaultModuleVersions = []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} -// ListTags is called twice on the root level for every pullModules run: -// - once by validateModulesAccess to smoke-check registry credentials, -// - once by pullModules itself to enumerate all modules in the registry. -// On top of this baseline, pullSingleModule calls ListTags once per module -// that has a non-exact (semver) constraint - exact-tag and blacklist modules -// short-circuit before this call. +// Each pullModules run calls ListTags twice at the root (validateModulesAccess +// + module enumeration). Per-module ListTags adds one more call, but only for +// non-exact (semver) constraints - exact and blacklist short-circuit. const ( baselineListTagsCalls int64 = 2 perModuleListTagsCall int64 = 1 ) // Regression: --include-module @ must pull every matching tag -// in the registry, not only the version currently on a release channel. -// Each subtest uses the same registry layout but a different constraint; -// the spread of registry versions is wide enough to make the expected sets -// of caret / tilde / range distinguishable. +// from the registry, not only the version currently on a release channel. func TestPullSingleModule_SemverConstraintMatchesRegistryTags(t *testing.T) { registryVersions := []string{ "v1.39.0", @@ -81,34 +74,28 @@ func TestPullSingleModule_SemverConstraintMatchesRegistryTags(t *testing.T) { mustNotContain string // a version that MUST be excluded (sanity check) }{ { - // Implicit caret: bare version is expanded to ^X.Y.Z for legacy - // backward compatibility (see filter.go:parseVersionConstraint). + // Bare version expands to ^X.Y.Z (legacy backward compat in parseVersionConstraint). name: "implicit_caret", constraint: "1.40.0", expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, mustNotContain: "v1.39.0", }, { - // Explicit caret: same range as implicit. Verifies the parser - // treats both forms identically. + // Explicit caret must produce the same range as implicit. name: "explicit_caret", constraint: "^1.40.0", expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, mustNotContain: "v1.39.0", }, { - // Tilde: pinned to the same minor (>=1.40.0 <1.41.0). - // channelVersion is unrelated to the constraint but is always - // added to the download list because release channels are still - // pulled for any non-exact constraint. + // Tilde pins to the same minor; channelVersion still arrives via release channels. name: "tilde", constraint: "~1.40.0", expected: []string{"v1.40.0", "v1.40.1", channelVersion}, mustNotContain: "v1.41.0", }, { - // Explicit range. The upper bound is exclusive, so v1.43.0 - // must not appear in the result. + // Explicit range with exclusive upper bound. name: "explicit_range", constraint: ">=1.40.0 <1.43.0", expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", channelVersion}, @@ -135,10 +122,8 @@ func TestPullSingleModule_SemverConstraintMatchesRegistryTags(t *testing.T) { } } -// Multiple --include-module flags must each apply their own constraint -// independently; constraints on one module must not leak into another. -// This case mixes a semver-constraint module with an exact-tag module to -// also verify the two filter branches coexist correctly in one pull run. +// Constraints on different --include-module flags must not leak across modules. +// Mixes semver and exact constraints to also exercise both filter branches in one pull. func TestPullSingleModule_MultipleModulesIndependentConstraints(t *testing.T) { const ( consoleName = "console" @@ -176,8 +161,7 @@ func TestPullSingleModule_MultipleModulesIndependentConstraints(t *testing.T) { "exact constraint on commander must match only v0.6.0 and skip channels") } -// Exact-tag constraint takes a Filter branch that never reads Module.Releases, -// so per-module ListTags must be skipped - only the baseline calls happen. +// Exact-tag branch never reads Module.Releases, so per-module ListTags is skipped. func TestPullSingleModule_ExactTagConstraintSkipsPerModuleListTags(t *testing.T) { filter, err := NewFilter([]string{testModule + "@=v1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) @@ -188,9 +172,7 @@ func TestPullSingleModule_ExactTagConstraintSkipsPerModuleListTags(t *testing.T) "exact-tag constraint must skip per-module ListTags") } -// In blacklist mode (no --include-module) modules without a filter entry -// short-circuit in Filter.VersionsToMirror, so per-module ListTags must be -// skipped. Saves N requests for N non-excluded modules. +// Blacklist mode short-circuits VersionsToMirror, so per-module ListTags is skipped. func TestPullSingleModule_BlacklistModeSkipsPerModuleListTags(t *testing.T) { filter, err := NewFilter(nil, FilterTypeBlacklist) require.NoError(t, err) @@ -201,8 +183,7 @@ func TestPullSingleModule_BlacklistModeSkipsPerModuleListTags(t *testing.T) { "blacklist mode must skip per-module ListTags for non-excluded modules") } -// Counterpart to the two skip tests: semver constraint MUST trigger per-module -// ListTags, otherwise the original bug resurfaces. +// Counter-check: semver constraint MUST trigger per-module ListTags, or the original bug returns. func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T) { filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) require.NoError(t, err) @@ -213,8 +194,54 @@ func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T "semver constraint must trigger per-module ListTags exactly once") } -// addModule populates a single module's images in the given registry, -// mirroring the production "modules/" + "modules//release/" +// Missing module repo is warned and skipped (same policy as validateModulesAccess), +// not propagated as a fatal error. +func TestPullSingleModule_PerModuleListTagsImageNotFoundIsSkipped(t *testing.T) { + failingClient := newFailingListTagsClient( + upfake.NewClient(buildTestRegistry(defaultModuleVersions)), + []string{"modules", testModule}, + dkpreg.ErrImageNotFound, + ) + + filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) + require.NoError(t, err) + + svc := newService(t, pkgclient.Adapt(failingClient), filter) + require.NoError(t, svc.PullModules(context.Background()), + "pull must not fail when per-module ListTags reports the module repo is missing") + + dl := svc.modulesDownloadList.list[testModule] + require.NotNil(t, dl) + + // With per-module tag list skipped, only the channel snapshot contributes. + assert.ElementsMatch(t, + taggedModuleRefs(testModule, []string{channelVersion}), + extractTaggedRefs(dl), + "bundle should contain only the channel snapshot") +} + +// Anything other than ErrImageNotFound is fail-fast: we cannot verify the +// constraint and refuse to produce a silently-wrong partial bundle. +func TestPullSingleModule_PerModuleListTagsTransientErrorFailsFast(t *testing.T) { + transientErr := errors.New("simulated registry 503") + failingClient := newFailingListTagsClient( + upfake.NewClient(buildTestRegistry(defaultModuleVersions)), + []string{"modules", testModule}, + transientErr, + ) + + filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) + require.NoError(t, err) + + svc := newService(t, pkgclient.Adapt(failingClient), filter) + pullErr := svc.PullModules(context.Background()) + + require.Error(t, pullErr, "non-NotFound errors from per-module ListTags must propagate") + assert.ErrorIs(t, pullErr, transientErr, "wrapped error must preserve the underlying cause") + assert.Contains(t, pullErr.Error(), "list tags for module "+testModule) +} + +// addModule mirrors the production "modules/" + "modules//release/" // layout. All 5 release channels point at channelVer. func addModule(reg *upfake.Registry, name, channelVer string, versions []string) { reg.MustAddImage("modules", name, makeVersionImage(channelVer)) @@ -228,19 +255,16 @@ func addModule(reg *upfake.Registry, name, channelVer string, versions []string) } } -// buildTestRegistry returns a fake registry containing a single module -// (testModule) with the given versions and channelVersion on every channel. -// For multi-module fixtures, build the registry by hand and call addModule -// for each module. +// buildTestRegistry builds a single-module fixture. Multi-module tests should +// call addModule directly on a fresh upfake.NewRegistry. func buildTestRegistry(moduleVersions []string) *upfake.Registry { reg := upfake.NewRegistry(testHost) addModule(reg, testModule, channelVersion, moduleVersions) return reg } -// makeVersionImage builds a v1.Image with only version.json populated. -// images_digests.json and extra_images.json are intentionally omitted - -// downstream code tolerates missing files. +// makeVersionImage builds a v1.Image carrying only version.json. Missing +// images_digests.json / extra_images.json is tolerated downstream. func makeVersionImage(version string) v1.Image { return upfake.NewImageBuilder(). WithFile("version.json", `{"version":"`+version+`"}`). @@ -248,14 +272,12 @@ func makeVersionImage(version string) v1.Image { MustBuild() } -// taggedRef returns the registry URL the production code uses for a single -// version-tagged module image. +// taggedRef builds the registry URL production code uses for a single version-tagged image. func taggedRef(moduleName, version string) string { return testHost + "/modules/" + moduleName + ":" + version } -// taggedModuleRefs converts a list of versions for a given module into the -// registry URLs the regression assertions expect to see in the download list. +// taggedModuleRefs is taggedRef applied to a slice; convenient for ElementsMatch assertions. func taggedModuleRefs(moduleName string, versions []string) []string { refs := make([]string, 0, len(versions)) for _, v := range versions { @@ -264,9 +286,8 @@ func taggedModuleRefs(moduleName string, versions []string) []string { return refs } -// extractTaggedRefs returns version-tagged module image refs from the -// download list, filtering out @sha256: refs added by extra-image -// resolution (those are not relevant to constraint tests). +// extractTaggedRefs returns tagged refs from the download list, dropping +// @sha256: refs added by extra-image resolution (irrelevant to constraint tests). func extractTaggedRefs(dl *ImageDownloadList) []string { refs := make([]string, 0, len(dl.Module)) for ref := range dl.Module { @@ -278,9 +299,8 @@ func extractTaggedRefs(dl *ImageDownloadList) []string { return refs } -// listTagsCounter wraps a Client and counts ListTags calls. The counter is -// shared across all sub-clients produced by WithSegment, so scoped calls -// increment the same counter as the parent. +// listTagsCounter counts ListTags calls. The counter is shared across +// sub-clients spawned by WithSegment, so all scoped calls increment one counter. type listTagsCounter struct { dkpreg.Client calls *atomic.Int64 @@ -302,9 +322,51 @@ func (c *listTagsCounter) ListTags(ctx context.Context, opts ...dkpreg.ListTagsO return c.Client.ListTags(ctx, opts...) } -// newService wires a Service against the given registry client and filter -// with logging muted to warn-level. Used by tests that inspect either the -// download list or the ListTags call counter. +// failingListTagsClient replaces ListTags with failErr only when the accumulated +// WithSegment chain matches failOn exactly. Other paths pass through unchanged. +type failingListTagsClient struct { + dkpreg.Client + segments []string + failOn []string + failErr error +} + +func newFailingListTagsClient(c dkpreg.Client, failOn []string, failErr error) *failingListTagsClient { + return &failingListTagsClient{Client: c, failOn: failOn, failErr: failErr} +} + +func (c *failingListTagsClient) WithSegment(segments ...string) dkpreg.Client { + next := make([]string, 0, len(c.segments)+len(segments)) + next = append(next, c.segments...) + next = append(next, segments...) + return &failingListTagsClient{ + Client: c.Client.WithSegment(segments...), + segments: next, + failOn: c.failOn, + failErr: c.failErr, + } +} + +func (c *failingListTagsClient) ListTags(ctx context.Context, opts ...dkpreg.ListTagsOption) ([]string, error) { + if segmentsEqual(c.segments, c.failOn) { + return nil, c.failErr + } + return c.Client.ListTags(ctx, opts...) +} + +func segmentsEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// newService wires a Service against the given client and filter, muting logs. func newService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service { t.Helper() @@ -325,8 +387,7 @@ func newService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service ) } -// runPullWithCounter runs PullModules against a fresh fake registry built -// with defaultModuleVersions and returns the ListTags call counter. +// runPullWithCounter runs PullModules against the default fixture, returning the call counter. func runPullWithCounter(t *testing.T, filter *Filter) *listTagsCounter { t.Helper() From 01fbe218bdf06fd807a47c9a6470cb7e707d9004 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 21:13:38 +0300 Subject: [PATCH 11/15] Simplify comment Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index a4c02cf0..2c7a4648 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -319,12 +319,8 @@ func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) err moduleVersions = svc.extractVersionsFromReleaseChannels(ctx, module.name) } - // Fetch the full list of module tags from the registry only when the filter - // constraint actually needs them: - // - a semver constraint iterates Releases to pick matching versions. - // - Exact-tag constraints and modules without a constraint - // (blacklist mode for non-excluded modules) never read Releases - // so the network call is skipped to avoid wasted requests. + // ListTags is only needed for semver constraints - they iterate Releases. + // Exact-tag and no-constraint paths don't read Releases, so skip the call. var tags []string if constraint, hasConstraint := svc.options.Filter.GetConstraint(module.name); hasConstraint && !constraint.IsExact() { var err error From 7d75dbc7a6e197c0aaeb428c49e888ee4e27df85 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 29 Apr 2026 22:33:32 +0300 Subject: [PATCH 12/15] Refactor pullSingleModule into a thin orchestrator - 8 new phase methods make the orchestrator read top-to-bottom as a plan. - Each phase preserves its error policy: fail-fast for required pulls, debug-log-and-continue for release-version and internal-digest pulls where missing artifacts are expected. - All 10 phases live right after the orchestrator (`pullVexImages` and `findVexImage` moved into the cluster), so the call order is visible in file order. Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 482 ++++++++++++++++------------- 1 file changed, 259 insertions(+), 223 deletions(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index 2c7a4648..35c5a173 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -282,178 +282,220 @@ func (svc *Service) pullModules(ctx context.Context) error { } func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) error { - // Initialize download list for this module downloadList := NewImageDownloadList(filepath.Join(svc.rootURL, "modules", module.name)) svc.modulesDownloadList.list[module.name] = downloadList - // Determine which release channels to pull based on filter - shouldPullReleaseChannels := svc.options.Filter.ShouldMirrorReleaseChannels(module.name) + channelVersions, err := svc.discoverChannelVersions(ctx, module.name, downloadList) + if err != nil { + return err + } - // Get module release channel versions for image discovery - moduleVersions := make([]string, 0) + tags, err := svc.listTagsIfConstrained(ctx, module.name) + if err != nil { + return err + } - if shouldPullReleaseChannels && !svc.options.OnlyExtraImages { - // Fill release channels - for _, channel := range internal.GetAllDefaultReleaseChannels() { - downloadList.ModuleReleaseChannels[svc.rootURL+"/modules/"+module.name+"/release:"+channel] = nil - } + moduleVersions := svc.mergeAndDedupeVersions(module.name, module.registryPath, channelVersions, tags) - if !svc.options.DryRun { - // Pull release channels first to get version information - config := puller.PullConfig{ - Name: module.name + " release channels", - ImageSet: downloadList.ModuleReleaseChannels, - Layout: svc.layout.Module(module.name).ModulesReleaseChannels, - AllowMissingTags: true, - GetterService: svc.modulesService.Module(module.name).ReleaseChannels(), - } + if svc.options.DryRun { + svc.printDryRunPlan(module.name, downloadList, moduleVersions) + return nil + } - if err := svc.pullerService.PullImages(ctx, config); err != nil { - return fmt.Errorf("pull release channels: %w", err) - } + if !svc.options.OnlyExtraImages { + if err := svc.pullModuleImages(ctx, module.name, moduleVersions, downloadList); err != nil { + return err } + svc.pullReleaseVersionImages(ctx, module.name, moduleVersions, downloadList) + svc.pullInternalDigestImages(ctx, module.name, moduleVersions, downloadList) + } + + if err := svc.pullExtraImages(ctx, module.name, moduleVersions, downloadList); err != nil { + return err + } + + if !svc.options.SkipVexImages { + svc.pullVexImages(ctx, module.name, downloadList) + } + + return nil +} + +// discoverChannelVersions enqueues release-channel refs into downloadList, +// optionally pulls them (skipped on DryRun), and returns versions extracted +// from those channels. +func (svc *Service) discoverChannelVersions(ctx context.Context, moduleName string, downloadList *ImageDownloadList) ([]string, error) { + if !svc.options.Filter.ShouldMirrorReleaseChannels(moduleName) || svc.options.OnlyExtraImages { + return nil, nil + } - // Extract versions from release channels. - // Does not depend on PullImages above - calls GetImage() directly - // against the remote registry, not from the local OCI layout. - moduleVersions = svc.extractVersionsFromReleaseChannels(ctx, module.name) + for _, channel := range internal.GetAllDefaultReleaseChannels() { + downloadList.ModuleReleaseChannels[svc.rootURL+"/modules/"+moduleName+"/release:"+channel] = nil } - // ListTags is only needed for semver constraints - they iterate Releases. - // Exact-tag and no-constraint paths don't read Releases, so skip the call. - var tags []string - if constraint, hasConstraint := svc.options.Filter.GetConstraint(module.name); hasConstraint && !constraint.IsExact() { - var err error - tags, err = svc.modulesService.Module(module.name).ListTags(ctx) - switch { - case errors.Is(err, client.ErrImageNotFound): - // Module repository is missing or empty - registry inconsistency we can't - // act on. Continue with channel-only versions instead of failing the whole - // pull (same policy as validateModulesAccess). - svc.userLogger.Warnf("Skipping tag list for module %s: %v", module.name, err) - case err != nil: - return fmt.Errorf("list tags for module %s: %w", module.name, err) + if !svc.options.DryRun { + config := puller.PullConfig{ + Name: moduleName + " release channels", + ImageSet: downloadList.ModuleReleaseChannels, + Layout: svc.layout.Module(moduleName).ModulesReleaseChannels, + AllowMissingTags: true, + GetterService: svc.modulesService.Module(moduleName).ReleaseChannels(), + } + + if err := svc.pullerService.PullImages(ctx, config); err != nil { + return nil, fmt.Errorf("pull release channels: %w", err) } } - // Check for explicit version constraints from filter + // Calls GetImage() directly against the remote registry, not from the local + // OCI layout, so it works in DryRun too. + return svc.extractVersionsFromReleaseChannels(ctx, moduleName), nil +} + +// listTagsIfConstrained returns the module's tag list, but only when the filter +// has a non-exact (semver) constraint - exact-tag and no-constraint paths don't +// read Releases. ErrImageNotFound is logged and treated as no tags (same policy +// as validateModulesAccess for missing module repos). +func (svc *Service) listTagsIfConstrained(ctx context.Context, moduleName string) ([]string, error) { + constraint, hasConstraint := svc.options.Filter.GetConstraint(moduleName) + if !hasConstraint || constraint.IsExact() { + return nil, nil + } + + tags, err := svc.modulesService.Module(moduleName).ListTags(ctx) + switch { + case errors.Is(err, client.ErrImageNotFound): + svc.userLogger.Warnf("Skipping tag list for module %s: %v", moduleName, err) + return nil, nil + case err != nil: + return nil, fmt.Errorf("list tags for module %s: %w", moduleName, err) + } + return tags, nil +} + +// mergeAndDedupeVersions merges channel-derived versions with versions resolved +// from filter constraints over the given tags, then deduplicates. +func (svc *Service) mergeAndDedupeVersions(moduleName, registryPath string, channelVersions, tags []string) []string { + versions := append([]string(nil), channelVersions...) + mod := &Module{ - Name: module.name, - RegistryPath: module.registryPath, + Name: moduleName, + RegistryPath: registryPath, Releases: tags, } + versions = append(versions, svc.options.Filter.VersionsToMirror(mod)...) + + return deduplicateStrings(versions) +} - // Get specific versions to mirror from filter (for whitelist with version constraints) - filterVersions := svc.options.Filter.VersionsToMirror(mod) - if len(filterVersions) > 0 { - moduleVersions = append(moduleVersions, filterVersions...) +// printDryRunPlan prints the set of refs that would be pulled, without downloading. +func (svc *Service) printDryRunPlan(moduleName string, downloadList *ImageDownloadList, versions []string) { + svc.userLogger.InfoLn("[dry-run] Module '" + moduleName + "' images that would be pulled:") + for ref := range downloadList.ModuleReleaseChannels { + svc.userLogger.InfoLn(" " + ref) } + for _, version := range versions { + svc.userLogger.InfoLn(" " + svc.rootURL + "/modules/" + moduleName + ":" + version) + } + if len(versions) > 0 { + svc.userLogger.InfoLn(" (extra images discovery requires a real pull)") + } +} - // Deduplicate versions - moduleVersions = deduplicateStrings(moduleVersions) +// pullModuleImages pulls module images for the given versions (modules/:vX.Y.Z). +func (svc *Service) pullModuleImages(ctx context.Context, moduleName string, versions []string, downloadList *ImageDownloadList) error { + for _, version := range versions { + downloadList.Module[svc.rootURL+"/modules/"+moduleName+":"+version] = nil + } - // In dry-run mode: print what would be pulled and return without downloading blobs - if svc.options.DryRun { - svc.userLogger.InfoLn("[dry-run] Module '" + module.name + "' images that would be pulled:") - for ref := range downloadList.ModuleReleaseChannels { - svc.userLogger.InfoLn(" " + ref) - } - for _, version := range moduleVersions { - svc.userLogger.InfoLn(" " + svc.rootURL + "/modules/" + module.name + ":" + version) - } - if len(moduleVersions) > 0 { - svc.userLogger.InfoLn(" (extra images discovery requires a real pull)") - } + if len(downloadList.Module) == 0 { return nil } - // Skip main module images if only pulling extra images - if !svc.options.OnlyExtraImages { - // Fill module images for each version - for _, version := range moduleVersions { - downloadList.Module[svc.rootURL+"/modules/"+module.name+":"+version] = nil - } - - // Pull module images - if len(downloadList.Module) > 0 { - config := puller.PullConfig{ - Name: module.name + " images", - ImageSet: downloadList.Module, - Layout: svc.layout.Module(module.name).Modules, - AllowMissingTags: true, - GetterService: svc.modulesService.Module(module.name), - } + config := puller.PullConfig{ + Name: moduleName + " images", + ImageSet: downloadList.Module, + Layout: svc.layout.Module(moduleName).Modules, + AllowMissingTags: true, + GetterService: svc.modulesService.Module(moduleName), + } - if err := svc.pullerService.PullImages(ctx, config); err != nil { - return fmt.Errorf("pull module images: %w", err) - } - } + if err := svc.pullerService.PullImages(ctx, config); err != nil { + return fmt.Errorf("pull module images: %w", err) + } + return nil +} - // Also pull release images with version tags (modules//release:v1.x.x) - // These are in addition to channel tags (alpha, beta, etc.) - if len(moduleVersions) > 0 { - releaseVersionSet := make(map[string]*puller.ImageMeta) - for _, version := range moduleVersions { - releaseVersionSet[svc.rootURL+"/modules/"+module.name+"/release:"+version] = nil - downloadList.ModuleReleaseChannels[svc.rootURL+"/modules/"+module.name+"/release:"+version] = nil - } +// pullReleaseVersionImages pulls modules//release:vX.Y.Z tags in addition +// to channel tags (alpha, beta, ...). These may not exist for every version, so +// errors are logged at Debug and not propagated. +func (svc *Service) pullReleaseVersionImages(ctx context.Context, moduleName string, versions []string, downloadList *ImageDownloadList) { + if len(versions) == 0 { + return + } - config := puller.PullConfig{ - Name: module.name + " release versions", - ImageSet: releaseVersionSet, - Layout: svc.layout.Module(module.name).ModulesReleaseChannels, - AllowMissingTags: true, - GetterService: svc.modulesService.Module(module.name).ReleaseChannels(), - } + releaseVersionSet := make(map[string]*puller.ImageMeta) + for _, version := range versions { + releaseVersionSet[svc.rootURL+"/modules/"+moduleName+"/release:"+version] = nil + downloadList.ModuleReleaseChannels[svc.rootURL+"/modules/"+moduleName+"/release:"+version] = nil + } - if err := svc.pullerService.PullImages(ctx, config); err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to pull release version images for %s: %v", module.name, err)) - // Don't fail - version release images may not exist for all versions - } - } + config := puller.PullConfig{ + Name: moduleName + " release versions", + ImageSet: releaseVersionSet, + Layout: svc.layout.Module(moduleName).ModulesReleaseChannels, + AllowMissingTags: true, + GetterService: svc.modulesService.Module(moduleName).ReleaseChannels(), + } - // Extract and pull internal digest images from module versions (images_digests.json) - // These are internal images that module uses at runtime - digestImages := svc.extractInternalDigestImages(ctx, module.name, moduleVersions) - if len(digestImages) > 0 { - // Add digest images to download list - digestImageSet := make(map[string]*puller.ImageMeta) - for _, digestRef := range digestImages { - digestImageSet[digestRef] = nil - downloadList.Module[digestRef] = nil - } + if err := svc.pullerService.PullImages(ctx, config); err != nil { + svc.logger.Debug(fmt.Sprintf("Failed to pull release version images for %s: %v", moduleName, err)) + } +} - config := puller.PullConfig{ - Name: module.name + " internal images", - ImageSet: digestImageSet, - Layout: svc.layout.Module(module.name).Modules, - AllowMissingTags: true, - GetterService: svc.modulesService.Module(module.name), - } +// pullInternalDigestImages discovers and pulls images referenced by +// images_digests.json inside each module version. Errors are logged at Debug and +// not propagated - missing internal images do not fail the whole module pull. +func (svc *Service) pullInternalDigestImages(ctx context.Context, moduleName string, versions []string, downloadList *ImageDownloadList) { + digestImages := svc.extractInternalDigestImages(ctx, moduleName, versions) + if len(digestImages) == 0 { + return + } - if err := svc.pullerService.PullImages(ctx, config); err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to pull internal digest images for %s: %v", module.name, err)) - // Don't fail on missing internal images, just log warning - } - } + digestImageSet := make(map[string]*puller.ImageMeta) + for _, digestRef := range digestImages { + digestImageSet[digestRef] = nil + downloadList.Module[digestRef] = nil + } + + config := puller.PullConfig{ + Name: moduleName + " internal images", + ImageSet: digestImageSet, + Layout: svc.layout.Module(moduleName).Modules, + AllowMissingTags: true, + GetterService: svc.modulesService.Module(moduleName), + } + + if err := svc.pullerService.PullImages(ctx, config); err != nil { + svc.logger.Debug(fmt.Sprintf("Failed to pull internal digest images for %s: %v", moduleName, err)) } +} - // Extract and pull extra images from module versions - // Each extra image gets its own layout: modules//extra// - extraImagesByName := svc.findExtraImages(ctx, module.name, moduleVersions) +// 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) for extraName, images := range extraImagesByName { if len(images) == 0 { continue } - // Get or create layout for this extra image - extraLayout, err := svc.layout.Module(module.name).GetOrCreateExtraLayout(extraName) + extraLayout, err := svc.layout.Module(moduleName).GetOrCreateExtraLayout(extraName) if err != nil { return fmt.Errorf("create layout for extra image %s: %w", extraName, err) } - // Build image set for this extra imageSet := make(map[string]*puller.ImageMeta) for _, img := range images { imageSet[img.FullRef] = nil @@ -461,24 +503,84 @@ func (svc *Service) pullSingleModule(ctx context.Context, module moduleData) err } config := puller.PullConfig{ - Name: module.name + "/" + extraName, + Name: moduleName + "/" + extraName, ImageSet: imageSet, Layout: extraLayout, AllowMissingTags: true, - GetterService: svc.modulesService.Module(module.name).ExtraImage(extraName), + GetterService: svc.modulesService.Module(moduleName).ExtraImage(extraName), } if err := svc.pullerService.PullImages(ctx, config); err != nil { return fmt.Errorf("pull extra image %s: %w", extraName, err) } } + return nil +} - if !svc.options.SkipVexImages { - // Find and pull VEX images for all module images - svc.pullVexImages(ctx, module.name, downloadList) +// pullVexImages finds and pulls VEX attestation images for module images +func (svc *Service) pullVexImages(ctx context.Context, moduleName string, downloadList *ImageDownloadList) { + allImages := make([]string, 0, len(downloadList.Module)+len(downloadList.ModuleExtra)) + + for img := range downloadList.Module { + allImages = append(allImages, img) + } + for img := range downloadList.ModuleExtra { + allImages = append(allImages, img) } - return nil + // Find VEX images and add to a separate set for pulling + vexImageSet := make(map[string]*puller.ImageMeta) + for _, img := range allImages { + vexImageName, err := svc.findVexImage(ctx, moduleName, img) + if err != nil { + svc.logger.Debug(fmt.Sprintf("Failed to find VEX image for %s: %v", img, err)) + continue + } + if vexImageName != "" { + svc.logger.Debug(fmt.Sprintf("Found VEX image: %s", vexImageName)) + vexImageSet[vexImageName] = nil + downloadList.Module[vexImageName] = nil + } + } + + // Pull VEX images if any found + if len(vexImageSet) > 0 { + config := puller.PullConfig{ + Name: moduleName + " VEX images", + ImageSet: vexImageSet, + Layout: svc.layout.Module(moduleName).Modules, + AllowMissingTags: true, // VEX images may not exist + GetterService: svc.modulesService.Module(moduleName), + } + + if err := svc.pullerService.PullImages(ctx, config); err != nil { + svc.logger.Debug(fmt.Sprintf("Failed to pull VEX images for %s: %v", moduleName, err)) + // Don't fail on VEX image pull errors + } + } +} + +// findVexImage checks if a VEX attestation image exists for the given image +func (svc *Service) findVexImage(ctx context.Context, moduleName string, imageRef string) (string, error) { + // VEX image reference format: sha256-xxx.att + vexImageName := strings.Replace(strings.Replace(imageRef, "@sha256:", "@sha256-", 1), "@sha256", ":sha256", 1) + ".att" + + // Extract tag from vex image name + splitIndex := strings.LastIndex(vexImageName, ":") + if splitIndex == -1 { + return "", nil + } + tag := vexImageName[splitIndex+1:] + + err := svc.modulesService.Module(moduleName).CheckImageExists(ctx, tag) + if errors.Is(err, client.ErrImageNotFound) { + return "", nil + } + if err != nil { + return "", err + } + + return vexImageName, nil } // extractVersionsFromReleaseChannels extracts version tags from pulled release channel images @@ -633,37 +735,6 @@ func extractExtraImagesJSON(img interface{ Extract() io.ReadCloser }) (map[strin } } -// digestRegex matches sha256 digests in images_digests.json -var digestRegex = regexp.MustCompile(`sha256:[a-f0-9]{64}`) - -// extractImagesDigestsJSON extracts images_digests.json from module image -// and returns list of sha256 digests. These are internal images that module uses at runtime. -func extractImagesDigestsJSON(img interface{ Extract() io.ReadCloser }) ([]string, error) { - rc := img.Extract() - defer rc.Close() - - tr := tar.NewReader(rc) - for { - hdr, err := tr.Next() - if err == io.EOF { - return nil, fmt.Errorf("images_digests.json not found in image") - } - if err != nil { - return nil, err - } - - if hdr.Name == "images_digests.json" { - data, err := io.ReadAll(tr) - if err != nil { - return nil, fmt.Errorf("read images_digests.json: %w", err) - } - // Extract all sha256:... digests from JSON file - digests := digestRegex.FindAllString(string(data), -1) - return digests, nil - } - } -} - // extractInternalDigestImages extracts internal digest images from module versions. // It reads images_digests.json from each module version image and returns // list of image references in format "repo@sha256:..." which will be pulled @@ -717,72 +788,37 @@ func (svc *Service) extractInternalDigestImages(ctx context.Context, moduleName return digestRefs } -// pullVexImages finds and pulls VEX attestation images for module images -func (svc *Service) pullVexImages(ctx context.Context, moduleName string, downloadList *ImageDownloadList) { - allImages := make([]string, 0, len(downloadList.Module)+len(downloadList.ModuleExtra)) +// digestRegex matches sha256 digests in images_digests.json +var digestRegex = regexp.MustCompile(`sha256:[a-f0-9]{64}`) - for img := range downloadList.Module { - allImages = append(allImages, img) - } - for img := range downloadList.ModuleExtra { - allImages = append(allImages, img) - } +// extractImagesDigestsJSON extracts images_digests.json from module image +// and returns list of sha256 digests. These are internal images that module uses at runtime. +func extractImagesDigestsJSON(img interface{ Extract() io.ReadCloser }) ([]string, error) { + rc := img.Extract() + defer rc.Close() - // Find VEX images and add to a separate set for pulling - vexImageSet := make(map[string]*puller.ImageMeta) - for _, img := range allImages { - vexImageName, err := svc.findVexImage(ctx, moduleName, img) - if err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to find VEX image for %s: %v", img, err)) - continue - } - if vexImageName != "" { - svc.logger.Debug(fmt.Sprintf("Found VEX image: %s", vexImageName)) - vexImageSet[vexImageName] = nil - downloadList.Module[vexImageName] = nil + tr := tar.NewReader(rc) + for { + hdr, err := tr.Next() + if err == io.EOF { + return nil, fmt.Errorf("images_digests.json not found in image") } - } - - // Pull VEX images if any found - if len(vexImageSet) > 0 { - config := puller.PullConfig{ - Name: moduleName + " VEX images", - ImageSet: vexImageSet, - Layout: svc.layout.Module(moduleName).Modules, - AllowMissingTags: true, // VEX images may not exist - GetterService: svc.modulesService.Module(moduleName), + if err != nil { + return nil, err } - if err := svc.pullerService.PullImages(ctx, config); err != nil { - svc.logger.Debug(fmt.Sprintf("Failed to pull VEX images for %s: %v", moduleName, err)) - // Don't fail on VEX image pull errors + if hdr.Name == "images_digests.json" { + data, err := io.ReadAll(tr) + if err != nil { + return nil, fmt.Errorf("read images_digests.json: %w", err) + } + // Extract all sha256:... digests from JSON file + digests := digestRegex.FindAllString(string(data), -1) + return digests, nil } } } -// findVexImage checks if a VEX attestation image exists for the given image -func (svc *Service) findVexImage(ctx context.Context, moduleName string, imageRef string) (string, error) { - // VEX image reference format: sha256-xxx.att - vexImageName := strings.Replace(strings.Replace(imageRef, "@sha256:", "@sha256-", 1), "@sha256", ":sha256", 1) + ".att" - - // Extract tag from vex image name - splitIndex := strings.LastIndex(vexImageName, ":") - if splitIndex == -1 { - return "", nil - } - tag := vexImageName[splitIndex+1:] - - err := svc.modulesService.Module(moduleName).CheckImageExists(ctx, tag) - if errors.Is(err, client.ErrImageNotFound) { - return "", nil - } - if err != nil { - return "", err - } - - return vexImageName, nil -} - // applyChannelAliases applies release channel tags to images with exact tag constraints func (svc *Service) applyChannelAliases(moduleName string) error { constraint, ok := svc.options.Filter.GetConstraint(moduleName) From 46ca02eddf89430d264555e11fe7c0999e42010a Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Thu, 30 Apr 2026 00:38:52 +0300 Subject: [PATCH 13/15] Better guard for the pullModuleImages Signed-off-by: Roman Berezkin --- internal/mirror/modules/modules.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/mirror/modules/modules.go b/internal/mirror/modules/modules.go index 35c5a173..189ade7b 100644 --- a/internal/mirror/modules/modules.go +++ b/internal/mirror/modules/modules.go @@ -404,12 +404,14 @@ func (svc *Service) printDryRunPlan(moduleName string, downloadList *ImageDownlo // pullModuleImages pulls module images for the given versions (modules/:vX.Y.Z). func (svc *Service) pullModuleImages(ctx context.Context, moduleName string, versions []string, downloadList *ImageDownloadList) error { - for _, version := range versions { - downloadList.Module[svc.rootURL+"/modules/"+moduleName+":"+version] = nil + // Guard against future call-order changes: other helpers also write to + // downloadList.Module, so checking versions directly is the only invariant. + if len(versions) == 0 { + return nil } - if len(downloadList.Module) == 0 { - return nil + for _, version := range versions { + downloadList.Module[svc.rootURL+"/modules/"+moduleName+":"+version] = nil } config := puller.PullConfig{ From bba1306827123a9936cb34fd9f4ba25b400f0969 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Thu, 30 Apr 2026 00:50:31 +0300 Subject: [PATCH 14/15] Restructure pull-versions tests for readability - `TestPullModules_*` names match the public entry point each test exercises. - The per-module `ListTags` policy is covered by two table-driven tests: call count and error handling. - Assertions read the download list through the public `Module(name)` accessor. - File is split into named sections - fixtures, builders, assertions, test doubles - with consistent helper names. Signed-off-by: Roman Berezkin --- internal/mirror/modules/pull_versions_test.go | 457 +++++++++--------- 1 file changed, 234 insertions(+), 223 deletions(-) diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_versions_test.go index c388eb6b..47bffcab 100644 --- a/internal/mirror/modules/pull_versions_test.go +++ b/internal/mirror/modules/pull_versions_test.go @@ -38,257 +38,310 @@ import ( registryservice "github.com/deckhouse/deckhouse-cli/pkg/registry/service" ) -// Shared test-fixture identifiers. +// ============================================================================= +// Shared fixtures +// ============================================================================= + const ( testHost = "fake.registry" - testModule = "console" - channelVersion = "v1.45.2" + testModuleName = "console" + channelVersion = "v1.45.2" // version every release channel points at ) -var defaultModuleVersions = []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} +// defaultRegistryVersions is a small but representative tag set for tests +// that don't care about the exact list - a few patches and a few minors. +var defaultRegistryVersions = []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"} -// Each pullModules run calls ListTags twice at the root (validateModulesAccess -// + module enumeration). Per-module ListTags adds one more call, but only for -// non-exact (semver) constraints - exact and blacklist short-circuit. -const ( - baselineListTagsCalls int64 = 2 - perModuleListTagsCall int64 = 1 -) +// ============================================================================= +// Tests: which versions get pulled +// ============================================================================= -// Regression: --include-module @ must pull every matching tag -// from the registry, not only the version currently on a release channel. -func TestPullSingleModule_SemverConstraintMatchesRegistryTags(t *testing.T) { +// Regression for --include-module @: every tag in the registry +// that satisfies the semver constraint must end up in the download list, +// not only the tag pinned by the release channel snapshot. +func TestPullModules_SemverConstraintPullsAllMatchingTags(t *testing.T) { registryVersions := []string{ "v1.39.0", "v1.40.0", "v1.40.1", - "v1.41.0", - "v1.42.0", - "v1.43.0", - channelVersion, // v1.45.2 - the version every release channel points at + "v1.41.0", "v1.42.0", "v1.43.0", + channelVersion, } cases := []struct { - name string - constraint string // value placed after "@" in the filter - expected []string // module versions expected in the download list - mustNotContain string // a version that MUST be excluded (sanity check) + name string + constraint string // text after "@" in --include-module + wantTags []string // tags expected to land in the download list + rejectTag string // tag present in the registry that the constraint must reject }{ { - // Bare version expands to ^X.Y.Z (legacy backward compat in parseVersionConstraint). - name: "implicit_caret", - constraint: "1.40.0", - expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, - mustNotContain: "v1.39.0", + name: "implicit caret (1.40.0 -> ^1.40.0)", + constraint: "1.40.0", + wantTags: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, + rejectTag: "v1.39.0", }, { - // Explicit caret must produce the same range as implicit. - name: "explicit_caret", - constraint: "^1.40.0", - expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, - mustNotContain: "v1.39.0", + name: "explicit caret (^1.40.0)", + constraint: "^1.40.0", + wantTags: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", "v1.43.0", channelVersion}, + rejectTag: "v1.39.0", }, { - // Tilde pins to the same minor; channelVersion still arrives via release channels. - name: "tilde", - constraint: "~1.40.0", - expected: []string{"v1.40.0", "v1.40.1", channelVersion}, - mustNotContain: "v1.41.0", + name: "tilde (~1.40.0 - patch only)", + constraint: "~1.40.0", + wantTags: []string{"v1.40.0", "v1.40.1", channelVersion}, + rejectTag: "v1.41.0", }, { - // Explicit range with exclusive upper bound. - name: "explicit_range", - constraint: ">=1.40.0 <1.43.0", - expected: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", channelVersion}, - mustNotContain: "v1.43.0", + name: "explicit range (>=1.40.0 <1.43.0)", + constraint: ">=1.40.0 <1.43.0", + wantTags: []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.42.0", channelVersion}, + rejectTag: "v1.43.0", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - filter, err := NewFilter([]string{testModule + "@" + tc.constraint}, FilterTypeWhitelist) - require.NoError(t, err) + reg := singleModuleRegistry(testModuleName, channelVersion, registryVersions) + filter := mustNewFilter(t, FilterTypeWhitelist, testModuleName+"@"+tc.constraint) + svc := newService(t, pkgclient.Adapt(upfake.NewClient(reg)), filter) - svc := newService(t, pkgclient.Adapt(upfake.NewClient(buildTestRegistry(registryVersions))), filter) require.NoError(t, svc.PullModules(context.Background())) - dl := svc.modulesDownloadList.list[testModule] - require.NotNil(t, dl) - - got := extractTaggedRefs(dl) - assert.ElementsMatch(t, taggedModuleRefs(testModule, tc.expected), got) - assert.NotContains(t, got, taggedRef(testModule, tc.mustNotContain), - "constraint %q must exclude %s", tc.constraint, tc.mustNotContain) + got := pulledModuleVersionRefs(t, svc, testModuleName) + assert.ElementsMatch(t, taggedModuleRefs(testModuleName, tc.wantTags), got) + assert.NotContains(t, got, taggedModuleRef(testModuleName, tc.rejectTag), + "constraint %q must reject %s", tc.constraint, tc.rejectTag) }) } } -// Constraints on different --include-module flags must not leak across modules. -// Mixes semver and exact constraints to also exercise both filter branches in one pull. -func TestPullSingleModule_MultipleModulesIndependentConstraints(t *testing.T) { +// Each --include-module flag carries its own constraint - the matcher must +// scope to the named module and not leak across modules. Mixes a semver and +// an exact constraint to exercise both filter branches in one shot. +func TestPullModules_PerModuleConstraintIsolation(t *testing.T) { const ( consoleName = "console" commanderName = "commander" ) reg := upfake.NewRegistry(testHost) - addModule(reg, consoleName, "v1.40.1", []string{"v1.40.0", "v1.40.1", "v1.41.0", "v1.45.2"}) + addModule(reg, consoleName, "v1.40.1", []string{"v1.40.0", "v1.40.1", "v1.41.0", channelVersion}) addModule(reg, commanderName, "v0.5.1", []string{"v0.5.0", "v0.5.1", "v0.6.0"}) - filter, err := NewFilter( - []string{ - consoleName + "@~1.40.0", // tilde: only v1.40.x + channel snapshot - commanderName + "@=v0.6.0", // exact: only v0.6.0, no channels - }, - FilterTypeWhitelist, + filter := mustNewFilter(t, FilterTypeWhitelist, + consoleName+"@~1.40.0", // tilde matches v1.40.x only + commanderName+"@=v0.6.0", // exact tag ) - require.NoError(t, err) - svc := newService(t, pkgclient.Adapt(upfake.NewClient(reg)), filter) + require.NoError(t, svc.PullModules(context.Background())) - consoleDL := svc.modulesDownloadList.list[consoleName] - require.NotNil(t, consoleDL) assert.ElementsMatch(t, taggedModuleRefs(consoleName, []string{"v1.40.0", "v1.40.1"}), - extractTaggedRefs(consoleDL), - "tilde constraint on console must match only v1.40.x (channel v1.40.1 dedups)") - - commanderDL := svc.modulesDownloadList.list[commanderName] - require.NotNil(t, commanderDL) + pulledModuleVersionRefs(t, svc, consoleName), + "console: tilde must match only v1.40.x") assert.ElementsMatch(t, taggedModuleRefs(commanderName, []string{"v0.6.0"}), - extractTaggedRefs(commanderDL), - "exact constraint on commander must match only v0.6.0 and skip channels") + pulledModuleVersionRefs(t, svc, commanderName), + "commander: exact must match only v0.6.0 - no leak from console's tilde") } -// Exact-tag branch never reads Module.Releases, so per-module ListTags is skipped. -func TestPullSingleModule_ExactTagConstraintSkipsPerModuleListTags(t *testing.T) { - filter, err := NewFilter([]string{testModule + "@=v1.40.0"}, FilterTypeWhitelist) - require.NoError(t, err) +// ============================================================================= +// Tests: per-module ListTags policy +// ============================================================================= - counter := runPullWithCounter(t, filter) +// Per-module ListTags is only needed for non-exact constraints. The baseline +// cost of a default pull or an exact-tag pull must not regress by adding an +// unconditional per-module call. +func TestPullModules_PerModuleListTagsCallCount(t *testing.T) { + // PullModules lists tags at the registry root twice: + // - validateModulesAccess (reachability check) + // - pullModules (module-name enumeration) + const baselineRootCalls int64 = 2 - assert.Equal(t, baselineListTagsCalls, counter.calls.Load(), - "exact-tag constraint must skip per-module ListTags") -} + cases := []struct { + name string + filterType FilterType + filterExprs []string + wantExtra int64 // extra ListTags calls on top of the baseline + }{ + { + name: "exact constraint skips per-module ListTags", + filterType: FilterTypeWhitelist, + filterExprs: []string{testModuleName + "@=v1.40.0"}, + wantExtra: 0, + }, + { + name: "blacklist filter (no constraint) skips per-module ListTags", + filterType: FilterTypeBlacklist, + filterExprs: nil, + wantExtra: 0, + }, + { + name: "semver constraint triggers one per-module ListTags", + filterType: FilterTypeWhitelist, + filterExprs: []string{testModuleName + "@1.40.0"}, + wantExtra: 1, + }, + } -// Blacklist mode short-circuits VersionsToMirror, so per-module ListTags is skipped. -func TestPullSingleModule_BlacklistModeSkipsPerModuleListTags(t *testing.T) { - filter, err := NewFilter(nil, FilterTypeBlacklist) - require.NoError(t, err) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reg := singleModuleRegistry(testModuleName, channelVersion, defaultRegistryVersions) + counter := newListTagsCounter(upfake.NewClient(reg)) + filter := mustNewFilter(t, tc.filterType, tc.filterExprs...) + svc := newService(t, pkgclient.Adapt(counter), filter) - counter := runPullWithCounter(t, filter) + require.NoError(t, svc.PullModules(context.Background())) - assert.Equal(t, baselineListTagsCalls, counter.calls.Load(), - "blacklist mode must skip per-module ListTags for non-excluded modules") + assert.Equal(t, baselineRootCalls+tc.wantExtra, counter.calls.Load()) + }) + } } -// Counter-check: semver constraint MUST trigger per-module ListTags, or the original bug returns. -func TestPullSingleModule_SemverConstraintTriggersPerModuleListTags(t *testing.T) { - filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) - require.NoError(t, err) +// Per-module ListTags reuses validateModulesAccess's error policy: +// - ErrImageNotFound: warn-and-skip (the module repo simply isn't there). +// - any other error: fail-fast (we cannot verify the constraint and refuse +// to silently produce a partial bundle). +func TestPullModules_PerModuleListTagsErrorHandling(t *testing.T) { + transientErr := errors.New("simulated registry 503") - counter := runPullWithCounter(t, filter) + cases := []struct { + name string + injected error + wantErr error // nil = pull must succeed + wantTags []string // checked only on success - the channel snapshot is the sole contributor when per-module ListTags is skipped + }{ + { + name: "ErrImageNotFound is warned and skipped", + injected: dkpreg.ErrImageNotFound, + wantTags: []string{channelVersion}, + }, + { + name: "transient error fails fast", + injected: transientErr, + wantErr: transientErr, + }, + } - assert.Equal(t, baselineListTagsCalls+perModuleListTagsCall, counter.calls.Load(), - "semver constraint must trigger per-module ListTags exactly once") + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + reg := singleModuleRegistry(testModuleName, channelVersion, defaultRegistryVersions) + client := newListTagsErrAtModule(upfake.NewClient(reg), tc.injected) + // Semver constraint to actually trigger the per-module ListTags. + filter := mustNewFilter(t, FilterTypeWhitelist, testModuleName+"@1.40.0") + svc := newService(t, pkgclient.Adapt(client), filter) + + err := svc.PullModules(context.Background()) + + if tc.wantErr == nil { + require.NoError(t, err) + assert.ElementsMatch(t, + taggedModuleRefs(testModuleName, tc.wantTags), + pulledModuleVersionRefs(t, svc, testModuleName)) + return + } + + require.Error(t, err) + assert.ErrorIs(t, err, tc.wantErr) + assert.Contains(t, err.Error(), "list tags for module "+testModuleName) + }) + } } -// Missing module repo is warned and skipped (same policy as validateModulesAccess), -// not propagated as a fatal error. -func TestPullSingleModule_PerModuleListTagsImageNotFoundIsSkipped(t *testing.T) { - failingClient := newFailingListTagsClient( - upfake.NewClient(buildTestRegistry(defaultModuleVersions)), - []string{"modules", testModule}, - dkpreg.ErrImageNotFound, - ) - - filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) - require.NoError(t, err) +// ============================================================================= +// Service & filter builders +// ============================================================================= - svc := newService(t, pkgclient.Adapt(failingClient), filter) - require.NoError(t, svc.PullModules(context.Background()), - "pull must not fail when per-module ListTags reports the module repo is missing") - - dl := svc.modulesDownloadList.list[testModule] - require.NotNil(t, dl) +// newService wires a Service against the given fake client, with logs muted. +func newService(t *testing.T, client dkpreg.Client, filter *Filter) *Service { + t.Helper() - // With per-module tag list skipped, only the channel snapshot contributes. - assert.ElementsMatch(t, - taggedModuleRefs(testModule, []string{channelVersion}), - extractTaggedRefs(dl), - "bundle should contain only the channel snapshot") -} + logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) + userLogger := log.NewSLogger(slog.LevelWarn) + regSvc := registryservice.NewService(client, pkg.NoEdition, logger) -// Anything other than ErrImageNotFound is fail-fast: we cannot verify the -// constraint and refuse to produce a silently-wrong partial bundle. -func TestPullSingleModule_PerModuleListTagsTransientErrorFailsFast(t *testing.T) { - transientErr := errors.New("simulated registry 503") - failingClient := newFailingListTagsClient( - upfake.NewClient(buildTestRegistry(defaultModuleVersions)), - []string{"modules", testModule}, - transientErr, + return NewService( + regSvc, + t.TempDir(), + &Options{ + BundleDir: t.TempDir(), + Filter: filter, + SkipVexImages: true, + }, + logger, + userLogger, ) +} - filter, err := NewFilter([]string{testModule + "@1.40.0"}, FilterTypeWhitelist) +func mustNewFilter(t *testing.T, ftype FilterType, exprs ...string) *Filter { + t.Helper() + f, err := NewFilter(exprs, ftype) require.NoError(t, err) - - svc := newService(t, pkgclient.Adapt(failingClient), filter) - pullErr := svc.PullModules(context.Background()) - - require.Error(t, pullErr, "non-NotFound errors from per-module ListTags must propagate") - assert.ErrorIs(t, pullErr, transientErr, "wrapped error must preserve the underlying cause") - assert.Contains(t, pullErr.Error(), "list tags for module "+testModule) + return f } -// addModule mirrors the production "modules/" + "modules//release/" -// layout. All 5 release channels point at channelVer. -func addModule(reg *upfake.Registry, name, channelVer string, versions []string) { - reg.MustAddImage("modules", name, makeVersionImage(channelVer)) +// ============================================================================= +// Registry fixture builders +// ============================================================================= +// addModule populates a fake registry with one module's worth of refs: +// +// modules: - modules-list entry, points at channelVer +// modules/: - one image per version +// modules//release: - 5 release channels, all pointing at channelVer +func addModule(reg *upfake.Registry, name, channelVer string, versions []string) { + reg.MustAddImage("modules", name, versionImage(channelVer)) for _, v := range versions { - reg.MustAddImage("modules/"+name, v, makeVersionImage(v)) + reg.MustAddImage("modules/"+name, v, versionImage(v)) } - for _, ch := range []string{"alpha", "beta", "early-access", "stable", "rock-solid"} { - reg.MustAddImage("modules/"+name+"/release", ch, makeVersionImage(channelVer)) + reg.MustAddImage("modules/"+name+"/release", ch, versionImage(channelVer)) } } -// buildTestRegistry builds a single-module fixture. Multi-module tests should -// call addModule directly on a fresh upfake.NewRegistry. -func buildTestRegistry(moduleVersions []string) *upfake.Registry { +// singleModuleRegistry builds a fake registry containing exactly one module. +func singleModuleRegistry(name, channelVer string, versions []string) *upfake.Registry { reg := upfake.NewRegistry(testHost) - addModule(reg, testModule, channelVersion, moduleVersions) + addModule(reg, name, channelVer, versions) return reg } -// makeVersionImage builds a v1.Image carrying only version.json. Missing +// versionImage builds a v1.Image carrying only version.json. Missing // images_digests.json / extra_images.json is tolerated downstream. -func makeVersionImage(version string) v1.Image { +func versionImage(version string) v1.Image { return upfake.NewImageBuilder(). WithFile("version.json", `{"version":"`+version+`"}`). WithLabel("org.opencontainers.image.version", version). MustBuild() } -// taggedRef builds the registry URL production code uses for a single version-tagged image. -func taggedRef(moduleName, version string) string { +// ============================================================================= +// Assertion helpers +// ============================================================================= + +// taggedModuleRef is the registry URL the production code uses for a single +// version-tagged module image. +func taggedModuleRef(moduleName, version string) string { return testHost + "/modules/" + moduleName + ":" + version } -// taggedModuleRefs is taggedRef applied to a slice; convenient for ElementsMatch assertions. func taggedModuleRefs(moduleName string, versions []string) []string { refs := make([]string, 0, len(versions)) for _, v := range versions { - refs = append(refs, taggedRef(moduleName, v)) + refs = append(refs, taggedModuleRef(moduleName, v)) } return refs } -// extractTaggedRefs returns tagged refs from the download list, dropping -// @sha256: refs added by extra-image resolution (irrelevant to constraint tests). -func extractTaggedRefs(dl *ImageDownloadList) []string { +// pulledModuleVersionRefs returns the version-tagged refs the service recorded +// for the given module, dropping @sha256: refs (these are added by extra-image +// resolution and are not relevant to constraint tests). +func pulledModuleVersionRefs(t *testing.T, svc *Service, moduleName string) []string { + t.Helper() + dl := svc.modulesDownloadList.Module(moduleName) + require.NotNil(t, dl, "no download list recorded for module %s", moduleName) + refs := make([]string, 0, len(dl.Module)) for ref := range dl.Module { if strings.Contains(ref, "@sha256:") { @@ -299,8 +352,12 @@ func extractTaggedRefs(dl *ImageDownloadList) []string { return refs } -// listTagsCounter counts ListTags calls. The counter is shared across -// sub-clients spawned by WithSegment, so all scoped calls increment one counter. +// ============================================================================= +// Test doubles +// ============================================================================= + +// listTagsCounter counts every ListTags call on this client and on any +// sub-client spawned via WithSegment (the counter is shared across the chain). type listTagsCounter struct { dkpreg.Client calls *atomic.Int64 @@ -311,10 +368,7 @@ func newListTagsCounter(c dkpreg.Client) *listTagsCounter { } func (c *listTagsCounter) WithSegment(segments ...string) dkpreg.Client { - return &listTagsCounter{ - Client: c.Client.WithSegment(segments...), - calls: c.calls, - } + return &listTagsCounter{Client: c.Client.WithSegment(segments...), calls: c.calls} } func (c *listTagsCounter) ListTags(ctx context.Context, opts ...dkpreg.ListTagsOption) ([]string, error) { @@ -322,77 +376,34 @@ func (c *listTagsCounter) ListTags(ctx context.Context, opts ...dkpreg.ListTagsO return c.Client.ListTags(ctx, opts...) } -// failingListTagsClient replaces ListTags with failErr only when the accumulated -// WithSegment chain matches failOn exactly. Other paths pass through unchanged. -type failingListTagsClient struct { +// listTagsErrAtModule returns the configured error from ListTags only at the +// per-module path (modules/, two segments below root). Root-level +// ListTags - validateModulesAccess and module-name enumeration - pass through. +type listTagsErrAtModule struct { dkpreg.Client - segments []string - failOn []string - failErr error + depth int + err error } -func newFailingListTagsClient(c dkpreg.Client, failOn []string, failErr error) *failingListTagsClient { - return &failingListTagsClient{Client: c, failOn: failOn, failErr: failErr} -} +// perModuleDepth is the WithSegment depth at which a client points at +// modules/: root -> "modules" -> "". +const perModuleDepth = 2 -func (c *failingListTagsClient) WithSegment(segments ...string) dkpreg.Client { - next := make([]string, 0, len(c.segments)+len(segments)) - next = append(next, c.segments...) - next = append(next, segments...) - return &failingListTagsClient{ - Client: c.Client.WithSegment(segments...), - segments: next, - failOn: c.failOn, - failErr: c.failErr, - } +func newListTagsErrAtModule(c dkpreg.Client, err error) *listTagsErrAtModule { + return &listTagsErrAtModule{Client: c, depth: 0, err: err} } -func (c *failingListTagsClient) ListTags(ctx context.Context, opts ...dkpreg.ListTagsOption) ([]string, error) { - if segmentsEqual(c.segments, c.failOn) { - return nil, c.failErr +func (c *listTagsErrAtModule) WithSegment(segments ...string) dkpreg.Client { + return &listTagsErrAtModule{ + Client: c.Client.WithSegment(segments...), + depth: c.depth + len(segments), + err: c.err, } - return c.Client.ListTags(ctx, opts...) } -func segmentsEqual(a, b []string) bool { - if len(a) != len(b) { - return false +func (c *listTagsErrAtModule) ListTags(ctx context.Context, opts ...dkpreg.ListTagsOption) ([]string, error) { + if c.depth == perModuleDepth { + return nil, c.err } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} - -// newService wires a Service against the given client and filter, muting logs. -func newService(t *testing.T, stubClient dkpreg.Client, filter *Filter) *Service { - t.Helper() - - logger := dkplog.NewLogger(dkplog.WithLevel(slog.LevelWarn)) - userLogger := log.NewSLogger(slog.LevelWarn) - regSvc := registryservice.NewService(stubClient, pkg.NoEdition, logger) - - return NewService( - regSvc, - t.TempDir(), - &Options{ - BundleDir: t.TempDir(), - Filter: filter, - SkipVexImages: true, - }, - logger, - userLogger, - ) -} - -// runPullWithCounter runs PullModules against the default fixture, returning the call counter. -func runPullWithCounter(t *testing.T, filter *Filter) *listTagsCounter { - t.Helper() - - counter := newListTagsCounter(upfake.NewClient(buildTestRegistry(defaultModuleVersions))) - svc := newService(t, pkgclient.Adapt(counter), filter) - require.NoError(t, svc.PullModules(context.Background())) - return counter + return c.Client.ListTags(ctx, opts...) } From 762e624973e5035cf29ead2c1522371b9b2dd6d2 Mon Sep 17 00:00:00 2001 From: Roman Berezkin Date: Wed, 6 May 2026 11:44:15 +0300 Subject: [PATCH 15/15] Rename pull_versions_test.go -> pull_modules_test.go It's more specific and appropriate name for the version range check tests. Signed-off-by: Roman Berezkin --- .../modules/{pull_versions_test.go => pull_modules_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internal/mirror/modules/{pull_versions_test.go => pull_modules_test.go} (100%) diff --git a/internal/mirror/modules/pull_versions_test.go b/internal/mirror/modules/pull_modules_test.go similarity index 100% rename from internal/mirror/modules/pull_versions_test.go rename to internal/mirror/modules/pull_modules_test.go