diff --git a/cmd/osbuild-composer/composer.go b/cmd/osbuild-composer/composer.go index a3e560343..f4f6066ff 100644 --- a/cmd/osbuild-composer/composer.go +++ b/cmd/osbuild-composer/composer.go @@ -17,6 +17,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/distroregistry" "github.com/osbuild/osbuild-composer/internal/jobqueue/fsjobqueue" "github.com/osbuild/osbuild-composer/internal/kojiapi" + "github.com/osbuild/osbuild-composer/internal/reporegistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/store" "github.com/osbuild/osbuild-composer/internal/weldr" @@ -85,15 +86,21 @@ func (c *Composer) InitWeldr(repoPaths []string, weldrListener net.Listener) err return fmt.Errorf("Host distro does not support host architecture: %v", err) } - repos, err := rpmmd.LoadRepositories(repoPaths, hostDistro.Name()) + rr, err := reporegistry.New(repoPaths) if err != nil { - return fmt.Errorf("Error loading repositories for %s: %v", hostDistro.Name(), err) + return fmt.Errorf("error loading repository definitions: %v", err) + } + + // Check if repositories for the host distro and arch were loaded + _, err = rr.ReposByArch(arch, false) + if err != nil { + return fmt.Errorf("loaded repository definitions don't contain any for the host distro/arch: %v", err) } store := store.New(&c.stateDir, arch, c.logger) compatOutputDir := path.Join(c.stateDir, "outputs") - c.weldr = weldr.New(c.rpm, arch, hostDistro, repos[archName], c.logger, store, c.workers, compatOutputDir) + c.weldr = weldr.New(c.rpm, arch, hostDistro, rr, c.logger, store, c.workers, compatOutputDir) c.weldrListener = weldrListener diff --git a/internal/client/unit_test.go b/internal/client/unit_test.go index 28885f411..9fa0db91e 100644 --- a/internal/client/unit_test.go +++ b/internal/client/unit_test.go @@ -16,6 +16,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/distro/test_distro" rpmmd_mock "github.com/osbuild/osbuild-composer/internal/mocks/rpmmd" + "github.com/osbuild/osbuild-composer/internal/reporegistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/weldr" ) @@ -49,9 +50,17 @@ func executeTests(m *testing.M) int { if err != nil { panic(err) } - repos := []rpmmd.RepoConfig{{Name: "test-system-repo", BaseURL: "http://example.com/test/os/test_arch"}} + + rr := reporegistry.NewFromDistrosRepoConfigs(rpmmd.DistrosRepoConfigs{ + test_distro.TestDistroName: { + test_distro.TestArchName: { + {Name: "test-system-repo", BaseURL: "http://example.com/test/os/test_arch"}, + }, + }, + }) + logger := log.New(os.Stdout, "", 0) - api := weldr.New(rpm, arch, distro, repos, logger, fixture.Store, fixture.Workers, "") + api := weldr.New(rpm, arch, distro, rr, logger, fixture.Store, fixture.Workers, "") server := http.Server{Handler: api} defer server.Close() diff --git a/internal/reporegistry/reporegistry.go b/internal/reporegistry/reporegistry.go index 013eeb1ed..8cb494035 100644 --- a/internal/reporegistry/reporegistry.go +++ b/internal/reporegistry/reporegistry.go @@ -26,6 +26,10 @@ func New(repoConfigPaths []string) (*RepoRegistry, error) { return &RepoRegistry{repositories}, nil } +func NewFromDistrosRepoConfigs(distrosRepoConfigs rpmmd.DistrosRepoConfigs) *RepoRegistry { + return &RepoRegistry{distrosRepoConfigs} +} + // ReposByImageType returns a slice of rpmmd.RepoConfig instances, which should be used for building the specific // image type. All repositories for the associated distribution and architecture, without any ImageTypeTags set, // are always part of the returned slice. In addition, if there are repositories tagged with the specific image @@ -48,13 +52,9 @@ func (r *RepoRegistry) ReposByImageType(imageType distro.ImageType) ([]rpmmd.Rep func (r *RepoRegistry) reposByImageTypeName(distro, arch, imageType string) ([]rpmmd.RepoConfig, error) { repositories := []rpmmd.RepoConfig{} - distroRepos, found := r.repos[distro] - if !found { - return nil, fmt.Errorf("there are no repositories for distribution '%s'", distro) - } - archRepos, found := distroRepos[arch] - if !found { - return nil, fmt.Errorf("there are no repositories for distribution '%s' and architecture '%s'", distro, arch) + archRepos, err := r.reposByArchName(distro, arch, true) + if err != nil { + return nil, err } for _, repo := range archRepos { @@ -75,3 +75,44 @@ func (r *RepoRegistry) reposByImageTypeName(distro, arch, imageType string) ([]r return repositories, nil } + +// ReposByArch returns a slice of rpmmd.RepoConfig instances, which should be used for building image types for the +// specific architecture. This includes by default all repositories without any image type tags specified. +// Depending on the `includeTagged` argument value, repositories with image type tags set will be added to the returned +// slice or not. +func (r *RepoRegistry) ReposByArch(arch distro.Arch, includeTagged bool) ([]rpmmd.RepoConfig, error) { + if arch.Distro() == nil || reflect.ValueOf(arch.Distro()).IsNil() { + return nil, fmt.Errorf("there is no distribution associated with the provided architecture") + } + return r.reposByArchName(arch.Distro().Name(), arch.Name(), includeTagged) +} + +// reposByArchName returns a slice of rpmmd.RepoConfig instances, which should be used for building image types for the +// specific architecture and distribution. This includes by default all repositories without any image type tags specified. +// Depending on the `includeTagged` argument value, repositories with image type tags set will be added to the returned +// slice or not. +// +// The method does not verify if the given architecture name is actually part of the specific distribution definition. +func (r *RepoRegistry) reposByArchName(distro, arch string, includeTagged bool) ([]rpmmd.RepoConfig, error) { + repositories := []rpmmd.RepoConfig{} + + distroRepos, found := r.repos[distro] + if !found { + return nil, fmt.Errorf("there are no repositories for distribution '%s'", distro) + } + archRepos, found := distroRepos[arch] + if !found { + return nil, fmt.Errorf("there are no repositories for distribution '%s' and architecture '%s'", distro, arch) + } + + for _, repo := range archRepos { + // skip repos with image type tags if specified to do so + if !includeTagged && len(repo.ImageTypeTags) != 0 { + continue + } + + repositories = append(repositories, repo) + } + + return repositories, nil +} diff --git a/internal/reporegistry/reporegistry_test.go b/internal/reporegistry/reporegistry_test.go index 31b9ee50d..a5b5e7010 100644 --- a/internal/reporegistry/reporegistry_test.go +++ b/internal/reporegistry/reporegistry_test.go @@ -225,3 +225,180 @@ func TestInvalidReposByImageTypeName(t *testing.T) { }) } } + +func TestReposByArch(t *testing.T) { + rr := getTestingRepoRegistry() + testDistro := test_distro.New() + + ta, _ := testDistro.GetArch(test_distro.TestArchName) + ta2, _ := testDistro.GetArch(test_distro.TestArch2Name) + + type args struct { + arch distro.Arch + taggedRepos bool + } + tests := []struct { + name string + args args + want []string + }{ + { + name: "Test Arch 1, without tagged repos", + args: args{ + arch: ta, + taggedRepos: false, + }, + want: []string{"baseos", "appstream"}, + }, + { + name: "Test Arch 1, with tagged repos", + args: args{ + arch: ta, + taggedRepos: true, + }, + want: []string{"baseos", "appstream"}, + }, + { + name: "Test Arch 2, without tagged repos", + args: args{ + arch: ta2, + taggedRepos: false, + }, + want: []string{"baseos", "appstream"}, + }, + { + name: "Test Arch 2, with tagged repos", + args: args{ + arch: ta2, + taggedRepos: true, + }, + want: []string{"baseos", "appstream", "google-compute-engine", "google-cloud-sdk"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := rr.ReposByArch(tt.args.arch, tt.args.taggedRepos) + assert.Nil(t, err) + + var gotNames []string + for _, r := range got { + gotNames = append(gotNames, r.Name) + } + + if !reflect.DeepEqual(gotNames, tt.want) { + t.Errorf("ReposByArch() =\n got: %#v\n want: %#v", gotNames, tt.want) + } + + got, err = rr.reposByArchName(tt.args.arch.Distro().Name(), tt.args.arch.Name(), tt.args.taggedRepos) + assert.Nil(t, err) + gotNames = []string{} + for _, r := range got { + gotNames = append(gotNames, r.Name) + } + + if !reflect.DeepEqual(gotNames, tt.want) { + t.Errorf("reposByArchName() =\n got: %#v\n want: %#v", gotNames, tt.want) + } + }) + } +} + +// TestInvalidReposByArch tests return values from ReposByArch +// for invalid arch value +func TestInvalidReposByArch(t *testing.T) { + rr := getTestingRepoRegistry() + + ta := test_distro.TestArch{} + + repos, err := rr.ReposByArch(&ta, false) + assert.Nil(t, repos) + assert.NotNil(t, err) + + repos, err = rr.ReposByArch(&ta, true) + assert.Nil(t, repos) + assert.NotNil(t, err) +} + +// TestInvalidReposByArchName tests return values from reposByArchName +// for invalid distro name and arch +func TestInvalidReposByArchName(t *testing.T) { + rr := getTestingRepoRegistry() + + type args struct { + distro string + arch string + taggedRepos bool + } + tests := []struct { + name string + args args + want func(repos []rpmmd.RepoConfig, err error) bool + }{ + { + name: "invalid distro, valid arch, without tagged repos", + args: args{ + distro: test_distro.TestDistroName + "-invalid", + arch: test_distro.TestArch2Name, + taggedRepos: false, + }, + want: func(repos []rpmmd.RepoConfig, err error) bool { + // the list of repos should be nil and an error should be returned + if repos != nil || err == nil { + return false + } + return true + }, + }, + { + name: "invalid distro, valid arch, with tagged repos", + args: args{ + distro: test_distro.TestDistroName + "-invalid", + arch: test_distro.TestArch2Name, + taggedRepos: true, + }, + want: func(repos []rpmmd.RepoConfig, err error) bool { + // the list of repos should be nil and an error should be returned + if repos != nil || err == nil { + return false + } + return true + }, + }, + { + name: "invalid arch, valid distro, without tagged repos", + args: args{ + distro: test_distro.TestDistroName, + arch: test_distro.TestArch2Name + "-invalid", + taggedRepos: false, + }, + want: func(repos []rpmmd.RepoConfig, err error) bool { + // the list of repos should be nil and an error should be returned + if repos != nil || err == nil { + return false + } + return true + }, + }, + { + name: "invalid arch, valid distro, with tagged repos", + args: args{ + distro: test_distro.TestDistroName, + arch: test_distro.TestArch2Name + "-invalid", + taggedRepos: true, + }, + want: func(repos []rpmmd.RepoConfig, err error) bool { + // the list of repos should be nil and an error should be returned + if repos != nil || err == nil { + return false + } + return true + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := rr.reposByArchName(tt.args.distro, tt.args.arch, tt.args.taggedRepos) + assert.True(t, tt.want(got, err)) + }) + } +} diff --git a/internal/weldr/api.go b/internal/weldr/api.go index ab3cd7c91..169c9854a 100644 --- a/internal/weldr/api.go +++ b/internal/weldr/api.go @@ -33,6 +33,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/jobqueue" osbuild "github.com/osbuild/osbuild-composer/internal/osbuild1" + "github.com/osbuild/osbuild-composer/internal/reporegistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/store" "github.com/osbuild/osbuild-composer/internal/target" @@ -43,10 +44,10 @@ type API struct { store *store.Store workers *worker.Server - rpmmd rpmmd.RPMMD - arch distro.Arch - distro distro.Distro - repos []rpmmd.RepoConfig + rpmmd rpmmd.RPMMD + arch distro.Arch + distro distro.Distro + repoRegistry *reporegistry.RepoRegistry logger *log.Logger router *httprouter.Router @@ -82,8 +83,11 @@ func (cs ComposeState) ToString() string { // systemRepoIDs returns a list of the system repos // NOTE: The system repos have no concept of id vs. name so the id is returned func (api *API) systemRepoNames() (names []string) { - for _, repo := range api.repos { - names = append(names, repo.Name) + repos, err := api.repoRegistry.ReposByArch(api.arch, false) + if err == nil { + for _, repo := range repos { + names = append(names, repo.Name) + } } return names } @@ -91,14 +95,14 @@ func (api *API) systemRepoNames() (names []string) { var ValidBlueprintName = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) var ValidOSTreeRef = regexp.MustCompile(`^(?:[\w\d][-._\w\d]*\/)*[\w\d][-._\w\d]*$`) -func New(rpmmd rpmmd.RPMMD, arch distro.Arch, distro distro.Distro, repos []rpmmd.RepoConfig, logger *log.Logger, store *store.Store, workers *worker.Server, compatOutputDir string) *API { +func New(rpmmd rpmmd.RPMMD, arch distro.Arch, distro distro.Distro, repoRegistry *reporegistry.RepoRegistry, logger *log.Logger, store *store.Store, workers *worker.Server, compatOutputDir string) *API { api := &API{ store: store, workers: workers, rpmmd: rpmmd, arch: arch, distro: distro, - repos: repos, + repoRegistry: repoRegistry, logger: logger, compatOutputDir: compatOutputDir, } @@ -465,17 +469,26 @@ func (api *API) getSourceConfigs(params httprouter.Params) (map[string]store.Sou sources := map[string]store.SourceConfig{} errors := []responseError{} + repos, err := api.repoRegistry.ReposByArch(api.arch, false) + if err != nil { + error := responseError{ + ID: "InternalError", + Msg: fmt.Sprintf("error while getting system repos: %v", err), + } + errors = append(errors, error) + } + // if names is "*" we want all sources if names == "*" { sources = api.store.GetAllSourcesByID() - for _, repo := range api.repos { + for _, repo := range repos { sources[repo.Name] = store.NewSourceConfig(repo, true) } } else { for _, name := range strings.Split(names, ",") { // check if the source is one of the base repos found := false - for _, repo := range api.repos { + for _, repo := range repos { if name == repo.Name { sources[repo.Name] = store.NewSourceConfig(repo, true) found = true @@ -1008,8 +1021,18 @@ func (api *API) modulesInfoHandler(writer http.ResponseWriter, request *http.Req packageInfos := foundPackages.ToPackageInfos() if modulesRequested { + repos, err := api.repoRegistry.ReposByArch(api.arch, false) + if err != nil { + errors := responseError{ + ID: "InternalError", + Msg: fmt.Sprintf("error while getting system repos: %v", err), + } + statusResponseError(writer, http.StatusBadRequest, errors) + return + } + for i := range packageInfos { - err := packageInfos[i].FillDependencies(api.rpmmd, api.repos, api.distro.ModulePlatformID(), api.arch.Name()) + err := packageInfos[i].FillDependencies(api.rpmmd, repos, api.distro.ModulePlatformID(), api.arch.Name()) if err != nil { errors := responseError{ ID: errorId, @@ -1053,7 +1076,17 @@ func (api *API) projectsDepsolveHandler(writer http.ResponseWriter, request *htt projects = projects[1:] names := strings.Split(projects, ",") - packages, _, err := api.rpmmd.Depsolve(rpmmd.PackageSet{Include: names}, api.repos, api.distro.ModulePlatformID(), api.arch.Name()) + repos, err := api.repoRegistry.ReposByArch(api.arch, false) + if err != nil { + errors := responseError{ + ID: "InternalError", + Msg: fmt.Sprintf("error while getting system repos: %v", err), + } + statusResponseError(writer, http.StatusBadRequest, errors) + return + } + + packages, _, err := api.rpmmd.Depsolve(rpmmd.PackageSet{Include: names}, repos, api.distro.ModulePlatformID(), api.arch.Name()) if err != nil { errors := responseError{ @@ -1843,8 +1876,14 @@ func ostreeResolveRef(location, ref string) (string, error) { func (api *API) depsolveBlueprintForImageType(bp *blueprint.Blueprint, imageType distro.ImageType) (map[string][]rpmmd.PackageSpec, error) { packageSets := imageType.PackageSets(*bp) packageSpecSets := make(map[string][]rpmmd.PackageSpec) + + imageTypeRepos, err := api.allRepositoriesByImageType(imageType) + if err != nil { + return nil, err + } + for name, packageSet := range packageSets { - packageSpecs, _, err := api.rpmmd.Depsolve(packageSet, api.allRepositories(), api.distro.ModulePlatformID(), api.arch.Name()) + packageSpecs, _, err := api.rpmmd.Depsolve(packageSet, imageTypeRepos, api.distro.ModulePlatformID(), api.arch.Name()) if err != nil { return nil, err } @@ -2003,6 +2042,17 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request } seed := bigSeed.Int64() + imageRepos, err := api.allRepositoriesByImageType(imageType) + // this shoudl not happen if the api.depsolveBlueprintForImageType() call above worked + if err != nil { + errors := responseError{ + ID: "InternalError", + Msg: err.Error(), + } + statusResponseError(writer, http.StatusInternalServerError, errors) + return + } + manifest, err := imageType.Manifest(bp.Customizations, distro.ImageOptions{ Size: size, @@ -2012,7 +2062,7 @@ func (api *API) composeHandler(writer http.ResponseWriter, request *http.Request URL: cr.OSTree.URL, }, }, - api.allRepositories(), + imageRepos, packageSets, seed) if err != nil { @@ -2755,21 +2805,55 @@ func (api *API) composeFailedHandler(writer http.ResponseWriter, request *http.R } func (api *API) fetchPackageList() (rpmmd.PackageList, error) { - packages, _, err := api.rpmmd.FetchMetadata(api.allRepositories(), api.distro.ModulePlatformID(), api.arch.Name()) + repos, err := api.allRepositories() + if err != nil { + return nil, err + } + + packages, _, err := api.rpmmd.FetchMetadata(repos, api.distro.ModulePlatformID(), api.arch.Name()) return packages, err } -// Returns all configured repositories (base + sources) as rpmmd.RepoConfig -func (api *API) allRepositories() []rpmmd.RepoConfig { - repos := append([]rpmmd.RepoConfig{}, api.repos...) +// Returns all configured repositories (base, depending on the image type + sources) as rpmmd.RepoConfig +// The difference from allRepositories() is that this method may return additional repositories, +// which are needed to build the specific image type. The allRepositories() can't do this, because +// it is used in paces where image types are not considered. +func (api *API) allRepositoriesByImageType(imageType distro.ImageType) ([]rpmmd.RepoConfig, error) { + imageTypeRepos, err := api.repoRegistry.ReposByImageType(imageType) + if err != nil { + return nil, err + } + + repos := append([]rpmmd.RepoConfig{}, imageTypeRepos...) for id, source := range api.store.GetAllSourcesByID() { repos = append(repos, source.RepoConfig(id)) } - return repos + + return repos, nil +} + +// Returns all configured repositories (base + sources) as rpmmd.RepoConfig +func (api *API) allRepositories() ([]rpmmd.RepoConfig, error) { + archRepos, err := api.repoRegistry.ReposByArch(api.arch, false) + if err != nil { + return nil, err + } + + repos := append([]rpmmd.RepoConfig{}, archRepos...) + for id, source := range api.store.GetAllSourcesByID() { + repos = append(repos, source.RepoConfig(id)) + } + + return repos, nil } func (api *API) depsolveBlueprint(bp *blueprint.Blueprint) ([]rpmmd.PackageSpec, error) { - packages, _, err := api.rpmmd.Depsolve(rpmmd.PackageSet{Include: bp.GetPackages()}, api.allRepositories(), api.distro.ModulePlatformID(), api.arch.Name()) + repos, err := api.allRepositories() + if err != nil { + return nil, err + } + + packages, _, err := api.rpmmd.Depsolve(rpmmd.PackageSet{Include: bp.GetPackages()}, repos, api.distro.ModulePlatformID(), api.arch.Name()) if err != nil { return nil, err } diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go index 18d33365f..82c7a6b47 100644 --- a/internal/weldr/api_test.go +++ b/internal/weldr/api_test.go @@ -20,6 +20,7 @@ import ( "github.com/osbuild/osbuild-composer/internal/distro" "github.com/osbuild/osbuild-composer/internal/distro/test_distro" rpmmd_mock "github.com/osbuild/osbuild-composer/internal/mocks/rpmmd" + "github.com/osbuild/osbuild-composer/internal/reporegistry" "github.com/osbuild/osbuild-composer/internal/rpmmd" "github.com/osbuild/osbuild-composer/internal/store" "github.com/osbuild/osbuild-composer/internal/target" @@ -34,14 +35,22 @@ import ( func createWeldrAPI(tempdir string, fixtureGenerator rpmmd_mock.FixtureGenerator) (*API, *store.Store) { fixture := fixtureGenerator(tempdir) rpm := rpmmd_mock.NewRPMMDMock(fixture) - repos := []rpmmd.RepoConfig{{Name: "test-id", BaseURL: "http://example.com/test/os/x86_64", CheckGPG: true}} + + rr := reporegistry.NewFromDistrosRepoConfigs(rpmmd.DistrosRepoConfigs{ + test_distro.TestDistroName: { + test_distro.TestArchName: { + {Name: "test-id", BaseURL: "http://example.com/test/os/x86_64", CheckGPG: true}, + }, + }, + }) + d := test_distro.New() arch, err := d.GetArch(test_distro.TestArchName) if err != nil { panic(err) } - return New(rpm, arch, d, repos, nil, fixture.Store, fixture.Workers, ""), fixture.Store + return New(rpm, arch, d, rr, nil, fixture.Store, fixture.Workers, ""), fixture.Store } func TestBasic(t *testing.T) {