diff --git a/cmd/gen-manifests/main.go b/cmd/gen-manifests/main.go index 8d1f3dfcf3..89081d5745 100644 --- a/cmd/gen-manifests/main.go +++ b/cmd/gen-manifests/main.go @@ -35,7 +35,6 @@ import ( "github.com/osbuild/images/pkg/distrofactory" "github.com/osbuild/images/pkg/experimentalflags" "github.com/osbuild/images/pkg/manifest" - "github.com/osbuild/images/pkg/manifestgen" "github.com/osbuild/images/pkg/manifestgen/manifestmock" "github.com/osbuild/images/pkg/ostree" "github.com/osbuild/images/pkg/rhsm/facts" @@ -301,7 +300,7 @@ func makeManifestJob( var depsolvedSets map[string]depsolvednf.DepsolveResult if content["packages"] { solver := depsolvednf.NewSolver(distribution.ModulePlatformID(), distribution.Releasever(), archName, distribution.Name(), cacheDir) - depsolvedSets, err = manifestgen.DefaultDepsolve(solver, cacheDir, os.Stderr, common.Must(manifest.GetPackageSetChains()), distribution, archName) + depsolvedSets, err = solver.DepsolveAll(common.Must(manifest.GetPackageSetChains())) if err != nil { err = fmt.Errorf("[%s] depsolve failed: %s", filename, err.Error()) return @@ -318,7 +317,7 @@ func makeManifestJob( var containerSpecs map[string][]container.Spec if content["containers"] { - containerSpecs, err = manifestgen.DefaultContainerResolver(manifest.GetContainerSourceSpecs(), archName) + containerSpecs, err = container.NewResolver(archName).ResolveAll(manifest.GetContainerSourceSpecs()) if err != nil { return fmt.Errorf("[%s] container resolution failed: %s", filename, err.Error()) } @@ -328,7 +327,7 @@ func makeManifestJob( var commitSpecs map[string][]ostree.CommitSpec if content["commits"] { - commitSpecs, err = manifestgen.DefaultCommitResolver(manifest.GetOSTreeSourceSpecs()) + commitSpecs, err = ostree.ResolveAll(manifest.GetOSTreeSourceSpecs()) if err != nil { return fmt.Errorf("[%s] ostree commit resolution failed: %s", filename, err.Error()) } diff --git a/pkg/container/blocking_resolver_test.go b/pkg/container/blocking_resolver_test.go index fe907f05fe..56b63cb748 100644 --- a/pkg/container/blocking_resolver_test.go +++ b/pkg/container/blocking_resolver_test.go @@ -63,6 +63,51 @@ func TestBlockingResolver(t *testing.T) { assert.ElementsMatch(t, have, want) } +func TestBlockingResolverResolveAll(t *testing.T) { + assert := assert.New(t) + registry := testregistry.New() + defer registry.Close() + repo := registry.AddRepo("library/osbuild") + ref := registry.GetRef("library/osbuild") + + sources := make(map[string][]container.SourceSpec, 2) + expected := make(map[string][]container.Spec, 2) + for _, name := range []string{"pipeline-one", "pipeline-two"} { + for idx := 0; idx < 10; idx++ { + checksum := repo.AddImage( + []testregistry.Blob{testregistry.NewDataBlobFromBase64(testregistry.RootLayer)}, + []string{"amd64", "ppc64le"}, + fmt.Sprintf("image %s %d", name, idx), + time.Time{}) + + tag := fmt.Sprintf("%s%d", name, idx) + repo.AddTag(checksum, tag) + + plSources := sources[name] + refTag := fmt.Sprintf("%s:%s", ref, tag) + sources[name] = append(plSources, container.SourceSpec{ + Source: refTag, + Name: "", + Digest: common.ToPtr(""), + TLSVerify: common.ToPtr(false), + Local: false, + }) + + expSpec, err := registry.Resolve(refTag, arch.ARCH_X86_64) + assert.NoError(err) + expSpecs := expected[name] + expected[name] = append(expSpecs, expSpec) + } + } + + resolver := container.NewBlockingResolver("amd64") + + have, err := resolver.ResolveAll(sources) + assert.NoError(err) + assert.NotNil(have) + assert.Equal(have, expected) +} + func TestBlockingResolverFail(t *testing.T) { resolver := container.NewBlockingResolver("amd64") diff --git a/pkg/container/resolver.go b/pkg/container/resolver.go index 04d8d5973e..a08080c7e6 100644 --- a/pkg/container/resolver.go +++ b/pkg/container/resolver.go @@ -5,6 +5,7 @@ import ( "fmt" "sort" "strings" + "time" ) type resolveResult struct { @@ -15,14 +16,15 @@ type resolveResult struct { type Resolver interface { Add(spec SourceSpec) Finish() ([]Spec, error) + + Resolve(spec SourceSpec) (Spec, error) + ResolveAll(sources map[string][]SourceSpec) (map[string][]Spec, error) } type asyncResolver struct { jobs int queue chan resolveResult - ctx context.Context - Arch string AuthFilePath string @@ -42,7 +44,6 @@ func NewResolver(arch string) *asyncResolver { // NOTE: this should return the Resolver interface, but osbuild-composer // sets the AuthFilePath and for now we don't want to break the API. return &asyncResolver{ - ctx: context.Background(), queue: make(chan resolveResult, 2), Arch: arch, @@ -66,7 +67,9 @@ func (r *asyncResolver) Add(spec SourceSpec) { } go func() { - spec, err := client.Resolve(r.ctx, spec.Name, spec.Local) + ctx, cancelTimeout := context.WithTimeout(context.Background(), 60*time.Second) + defer cancelTimeout() + spec, err := client.Resolve(ctx, spec.Name, spec.Local) if err != nil { err = fmt.Errorf("'%s': %w", spec.Source, err) } @@ -75,7 +78,6 @@ func (r *asyncResolver) Add(spec SourceSpec) { } func (r *asyncResolver) Finish() ([]Spec, error) { - specs := make([]Spec, 0, r.jobs) errs := make([]string, 0, r.jobs) for r.jobs > 0 { @@ -100,6 +102,37 @@ func (r *asyncResolver) Finish() ([]Spec, error) { return specs, nil } +func (r *asyncResolver) Resolve(spec SourceSpec) (Spec, error) { + r.Add(spec) + resSlice, err := r.Finish() + if err != nil { + return Spec{}, err + } + + if len(resSlice) != 1 { + return Spec{}, fmt.Errorf("unexpected results in container resolver: expected 1 but received %d", len(resSlice)) + } + + return resSlice[0], nil +} + +func (r *asyncResolver) ResolveAll(sources map[string][]SourceSpec) (map[string][]Spec, error) { + containers := make(map[string][]Spec, len(sources)) + for name, srcList := range sources { + containerSpecs := make([]Spec, len(srcList)) + for idx, src := range srcList { + res, err := r.Resolve(src) + if err != nil { + return nil, fmt.Errorf("failed to resolve container: %w", err) + } + containerSpecs[idx] = res + } + containers[name] = containerSpecs + } + + return containers, nil +} + type blockingResolver struct { Arch string AuthFilePath string @@ -120,19 +153,7 @@ func NewBlockingResolver(arch string) Resolver { } func (r *blockingResolver) Add(src SourceSpec) { - client, err := r.newClient(src.Source) - if err != nil { - r.results = append(r.results, resolveResult{err: err}) - return - } - - client.SetTLSVerify(src.TLSVerify) - client.SetArchitectureChoice(r.Arch) - if r.AuthFilePath != "" { - client.SetAuthFilePath(r.AuthFilePath) - } - - spec, err := client.Resolve(context.TODO(), src.Name, src.Local) + spec, err := r.Resolve(src) if err != nil { err = fmt.Errorf("'%s': %w", src.Source, err) } @@ -160,3 +181,39 @@ func (r *blockingResolver) Finish() ([]Spec, error) { return specs, nil } + +func (r *blockingResolver) Resolve(source SourceSpec) (Spec, error) { + client, err := r.newClient(source.Source) + if err != nil { + return Spec{}, err + } + + client.SetTLSVerify(source.TLSVerify) + client.SetArchitectureChoice(r.Arch) + if r.AuthFilePath != "" { + client.SetAuthFilePath(r.AuthFilePath) + } + + ctx, cancelTimeout := context.WithTimeout(context.Background(), 60*time.Second) + defer cancelTimeout() + return client.Resolve(ctx, source.Name, source.Local) +} + +// ResolveAll calls [Resolve] with each container source spec in the map and +// returns a map of results with the corresponding keys as the input argument. +func (r *blockingResolver) ResolveAll(sources map[string][]SourceSpec) (map[string][]Spec, error) { + containers := make(map[string][]Spec, len(sources)) + for name, srcList := range sources { + containerSpecs := make([]Spec, len(srcList)) + for idx, src := range srcList { + res, err := r.Resolve(src) + if err != nil { + return nil, fmt.Errorf("failed to resolve container: %w", err) + } + containerSpecs[idx] = res + } + containers[name] = containerSpecs + } + + return containers, nil +} diff --git a/pkg/depsolvednf/depsolvednf.go b/pkg/depsolvednf/depsolvednf.go index 4d2c712de9..a64c5638f6 100644 --- a/pkg/depsolvednf/depsolvednf.go +++ b/pkg/depsolvednf/depsolvednf.go @@ -155,6 +155,8 @@ type Solver struct { subscriptions *rhsm.Subscriptions subscriptionsErr error + sbomType sbom.StandardType + // Stderr is the stderr output from osbuild-depsolve-dnf, if unset os.Stderr // will be used. // @@ -252,6 +254,11 @@ func collectRepos(pkgSets []rpmmd.PackageSet) []rpmmd.RepoConfig { return repos } +// Set the SBOM type to generate with the depsolve. +func (s *Solver) SetSBOMType(sbomType sbom.StandardType) { + s.sbomType = sbomType +} + // Depsolve the list of required package sets with explicit excludes using // their associated repositories. Each package set is depsolved as a separate // transactions in a chain. It returns a list of all packages (with solved @@ -317,6 +324,20 @@ func (s *Solver) Depsolve(pkgSets []rpmmd.PackageSet, sbomType sbom.StandardType }, nil } +// DepsolveAll calls [Solver.Depsolve] with each package set slice in the map and +// returns a map of results with the corresponding keys as the input argument. +func (s *Solver) DepsolveAll(pkgSetsMap map[string][]rpmmd.PackageSet) (map[string]DepsolveResult, error) { + results := make(map[string]DepsolveResult, len(pkgSetsMap)) + for name, pkgSet := range pkgSetsMap { + res, err := s.Depsolve(pkgSet, s.sbomType) + if err != nil { + return nil, fmt.Errorf("error depsolving package sets for %q: %w", name, err) + } + results[name] = *res + } + return results, nil +} + // FetchMetadata returns the list of all the available packages in repos and // their info. func (s *Solver) FetchMetadata(repos []rpmmd.RepoConfig) (rpmmd.PackageList, error) { diff --git a/pkg/depsolvednf/depsolvednf_test.go b/pkg/depsolvednf/depsolvednf_test.go index cec4996aeb..3f2e2a33ee 100644 --- a/pkg/depsolvednf/depsolvednf_test.go +++ b/pkg/depsolvednf/depsolvednf_test.go @@ -159,6 +159,214 @@ func TestSolverDepsolve(t *testing.T) { } } +func TestSolverDepsolveAll(t *testing.T) { + if !*forceDNF { + // dnf tests aren't forced: skip them if the dnf sniff check fails + if findDepsolveDnf() == "" { + t.Skip("Test needs an installed osbuild-depsolve-dnf") + } + } + + s := rpmrepo.NewTestServer() + defer s.Close() + + type testCase struct { + packageSets map[string][]rpmmd.PackageSet + sbomType sbom.StandardType + // slice of message fragments which are all expected to be in the error + expErrs []string + } + + tmpdir := t.TempDir() + + testCases := map[string]testCase{ + "flat": { + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"kernel", "vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + }, + "chain": { + // chain depsolve of the same packages in order should produce the same result (at least in this case) + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"kernel"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + }, + "multi-chain": { + // two chain depsolves for different pipelines + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"kernel", "tmux"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + "second": { + { + Include: []string{"kernel"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + }, + "multi-chain-error-first": { + // two chain depsolves for different pipelines, first one fails + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"does-not-exist"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + "second": { + { + Include: []string{"kernel", "tmux"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + expErrs: []string{"error depsolving package sets for \"first\"", "missing packages: does-not-exist"}, + }, + "multi-chain-error-second": { + // two chain depsolves for different pipelines, second one fails + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"kernel", "tmux"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + "second": { + { + Include: []string{"does-not-exist"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + expErrs: []string{"error depsolving package sets for \"second\"", "missing packages: does-not-exist"}, + }, + "multi-chain-with-sbom": { + // two chain depsolves for different pipelines with sbom + packageSets: map[string][]rpmmd.PackageSet{ + "first": { + { + Include: []string{"kernel", "tmux"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + "second": { + { + Include: []string{"kernel"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + { + Include: []string{"vim-minimal", "tmux", "zsh"}, + Repositories: []rpmmd.RepoConfig{s.RepoConfig}, + InstallWeakDeps: true, + }, + }, + }, + sbomType: sbom.StandardTypeSpdx, + }, + } + + for tcName := range testCases { + t.Run(tcName, func(t *testing.T) { + assert := assert.New(t) + tc := testCases[tcName] + + solver := NewSolver("platform:el9", "9", "x86_64", "rhel9.0", tmpdir) + solver.SetSBOMType(tc.sbomType) + res, err := solver.DepsolveAll(tc.packageSets) + if len(tc.expErrs) != 0 { + assert.Error(err) + for _, expErr := range tc.expErrs { + assert.Contains(err.Error(), expErr) + } + return + } else { + assert.Nil(err) + require.NotNil(t, res) + } + + assert.Equal(len(res), len(tc.packageSets)) + + for pipelineName := range tc.packageSets { + pipelineResult := res[pipelineName] + assert.NotNil(pipelineResult) + assert.Equal(len(pipelineResult.Repos), 1) + assert.Equal(expectedDepsolveResult(pipelineResult.Repos[0]), pipelineResult.Packages) + + if tc.sbomType != sbom.StandardTypeNone { + require.NotNil(t, pipelineResult.SBOM) + assert.Equal(sbom.StandardTypeSpdx, pipelineResult.SBOM.DocType) + } else { + assert.Nil(pipelineResult.SBOM) + } + } + }) + } +} + func TestSolverFetchMetadata(t *testing.T) { requireDNF(t) repoServer := rpmrepo.NewTestServer() @@ -647,7 +855,7 @@ func TestSolverRunWithSolverNoError(t *testing.T) { fakeSolver := `#!/bin/sh -e cat - > "$0".stdin echo '{"solver": "zypper"}' ->&2 echo "output-on-stderr" +>&2 echo "output-on-stderr" ` fakeSolverPath := filepath.Join(tmpdir, "fake-solver") err := os.WriteFile(fakeSolverPath, []byte(fakeSolver), 0755) //nolint:gosec diff --git a/pkg/manifestgen/manifestgen.go b/pkg/manifestgen/manifestgen.go index ffb2d73a17..c1c64fdf5b 100644 --- a/pkg/manifestgen/manifestgen.go +++ b/pkg/manifestgen/manifestgen.go @@ -123,10 +123,12 @@ func New(reporegistry *reporegistry.RepoRegistry, opts *Options) (*Generator, er mg.depsolve = DefaultDepsolve } if mg.containerResolver == nil { - mg.containerResolver = DefaultContainerResolver + mg.containerResolver = func(containerSources map[string][]container.SourceSpec, archName string) (map[string][]container.Spec, error) { + return container.NewBlockingResolver(archName).ResolveAll(containerSources) + } } if mg.commitResolver == nil { - mg.commitResolver = DefaultCommitResolver + mg.commitResolver = ostree.ResolveAll } if mg.cacheDir == "" { xdgCacheHomeDir, err := xdgCacheHome() @@ -280,66 +282,13 @@ func DefaultDepsolve(solver *depsolvednf.Solver, cacheDir string, depsolveWarnin solver.Stderr = depsolveWarningsOutput } - depsolvedSets := make(map[string]depsolvednf.DepsolveResult) - for name, pkgSet := range packageSets { - // Always generate Spdx SBOMs for now, this makes the - // default depsolve slightly slower but it means we - // need no extra argument here to select the SBOM - // type. Once we have more types than Spdx of course - // we need to add a option to select the type. - res, err := solver.Depsolve(pkgSet, sbom.StandardTypeSpdx) - if err != nil { - return nil, fmt.Errorf("error depsolving: %w", err) - } - depsolvedSets[name] = *res - } - return depsolvedSets, nil -} - -func resolveContainers(containers []container.SourceSpec, archName string) ([]container.Spec, error) { - resolver := container.NewBlockingResolver(archName) - - for _, c := range containers { - resolver.Add(c) - } - - return resolver.Finish() -} - -// DefaultContainersResolve provides a default implementation for -// container resolving. -// It should rarely be necessary to use it directly and will be used -// by default by manifestgen (unless overriden) -func DefaultContainerResolver(containerSources map[string][]container.SourceSpec, archName string) (map[string][]container.Spec, error) { - containerSpecs := make(map[string][]container.Spec, len(containerSources)) - for plName, sourceSpecs := range containerSources { - specs, err := resolveContainers(sourceSpecs, archName) - if err != nil { - return nil, fmt.Errorf("error container resolving: %w", err) - } - containerSpecs[plName] = specs - } - return containerSpecs, nil -} - -// DefaultCommitResolver provides a default implementation for -// ostree commit resolving. -// It should rarely be necessary to use it directly and will be used -// by default by manifestgen (unless overriden) -func DefaultCommitResolver(commitSources map[string][]ostree.SourceSpec) (map[string][]ostree.CommitSpec, error) { - commits := make(map[string][]ostree.CommitSpec, len(commitSources)) - for name, commitSources := range commitSources { - commitSpecs := make([]ostree.CommitSpec, len(commitSources)) - for idx, commitSource := range commitSources { - var err error - commitSpecs[idx], err = ostree.Resolve(commitSource) - if err != nil { - return nil, fmt.Errorf("error ostree commit resolving: %w", err) - } - } - commits[name] = commitSpecs - } - return commits, nil + // Always generate Spdx SBOMs for now, this makes the + // default depsolve slightly slower but it means we + // need no extra argument here to select the SBOM + // type. Once we have more types than Spdx of course + // we need to add a option to select the type. + solver.SetSBOMType(sbom.StandardTypeSpdx) + return solver.DepsolveAll(packageSets) } type ( diff --git a/pkg/ostree/ostree.go b/pkg/ostree/ostree.go index cbc6118b12..1b68ea7d62 100644 --- a/pkg/ostree/ostree.go +++ b/pkg/ostree/ostree.go @@ -308,3 +308,21 @@ func Resolve(source SourceSpec) (CommitSpec, error) { } return commit, nil } + +// ResolveAll calls [Resolve] with each commit slice in the map and returns a +// map of results with the corresponding keys as the input argument. +func ResolveAll(commitSources map[string][]SourceSpec) (map[string][]CommitSpec, error) { + commits := make(map[string][]CommitSpec, len(commitSources)) + for name, commitSources := range commitSources { + commitSpecs := make([]CommitSpec, len(commitSources)) + for idx, commitSource := range commitSources { + var err error + commitSpecs[idx], err = Resolve(commitSource) + if err != nil { + return nil, fmt.Errorf("error resolving ostree commit for %q: %w", name, err) + } + } + commits[name] = commitSpecs + } + return commits, nil +}