From 05d19abc693badfb4ed7ad029f46f21c00768d3b Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 14 Apr 2020 16:44:07 -0400 Subject: [PATCH] compose: Add --ex-lockfile-strict Today, lockfiles only restrict the NEVRA of specifc package names from which libsolv can pick. But nothing stops libsolv from picking entirely different packages which still satisfy the manifest requests. This was mostly a theoretical issue in Fedora CoreOS, but became reality with the addition of Fedora 32 packages in the pool. libsolv would happily try to pick e.g. `libcurl-minimal` from f32 instead of sticking with the f31 `libcurl` from the lockfiles: https://github.com/coreos/fedora-coreos-streams/issues/75#issuecomment-610734584 (But more generally, see https://github.com/coreos/fedora-coreos-tracker/issues/454). Let's add a `--ex-lockfile-strict` mode, which in CI and production pipeline build contexts will require that (1) *only* locked packages are considered by libsolv, and (2) *all* locked packages were marked for install. One important thing to note here is that we don't short-circuit libsolv and manually `hy_goal_install` lockfile packages. We want to make sure the treefile is still canonical. Strict mode simply ensures that the result agrees with the lockfile. That said, even in developer contexts, we don't want the `libcurl-minimal` issue that happened to be triggered. But we still want to allow flexibility in adding and removing packages to make hacking easier. I have some follow-up patches which will enable this. --- src/app/rpmostree-compose-builtin-tree.c | 4 +- src/libpriv/rpmostree-core-private.h | 1 + src/libpriv/rpmostree-core.c | 174 ++++++++++++++--------- src/libpriv/rpmostree-core.h | 5 +- tests/compose/libcomposetest.sh | 4 + tests/compose/test-lockfile.sh | 89 +++++++++++- 6 files changed, 206 insertions(+), 71 deletions(-) diff --git a/src/app/rpmostree-compose-builtin-tree.c b/src/app/rpmostree-compose-builtin-tree.c index 458b2a50cc..8652d82b71 100644 --- a/src/app/rpmostree-compose-builtin-tree.c +++ b/src/app/rpmostree-compose-builtin-tree.c @@ -79,6 +79,7 @@ static char *opt_write_composejson_to; static gboolean opt_no_parent; static char *opt_write_lockfile_to; static char **opt_lockfiles; +static gboolean opt_lockfile_strict; static char *opt_parent; /* shared by both install & commit */ @@ -104,6 +105,7 @@ static GOptionEntry install_option_entries[] = { { "workdir-tmpfs", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &opt_workdir_tmpfs, "Use tmpfs for working state", NULL }, { "ex-write-lockfile-to", 0, 0, G_OPTION_ARG_STRING, &opt_write_lockfile_to, "Write lockfile to FILE", "FILE" }, { "ex-lockfile", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_lockfiles, "Read lockfile from FILE", "FILE" }, + { "ex-lockfile-strict", 0, 0, G_OPTION_ARG_NONE, &opt_lockfile_strict, "With --ex-lockfile, require full match", NULL }, { NULL } }; @@ -702,7 +704,7 @@ rpm_ostree_compose_context_new (const char *treefile_pathstr, g_autoptr(GHashTable) map = ror_lockfile_read (opt_lockfiles, error); if (!map) return FALSE; - rpmostree_context_set_vlockmap (self->corectx, map); + rpmostree_context_set_vlockmap (self->corectx, map, opt_lockfile_strict); g_print ("Loaded lockfiles:\n %s\n", g_strjoinv ("\n ", opt_lockfiles)); } diff --git a/src/libpriv/rpmostree-core-private.h b/src/libpriv/rpmostree-core-private.h index c0ce31bca2..03cd7f19c7 100644 --- a/src/libpriv/rpmostree-core-private.h +++ b/src/libpriv/rpmostree-core-private.h @@ -73,6 +73,7 @@ struct _RpmOstreeContext { GHashTable *pkgs_to_replace; /* new gv_nevra --> old gv_nevra */ GHashTable *vlockmap; /* nevra --> repochecksum */ + gboolean vlockmap_strict; GLnxTmpDir tmpdir; diff --git a/src/libpriv/rpmostree-core.c b/src/libpriv/rpmostree-core.c index 1614bd0f1b..a44e01bcba 100644 --- a/src/libpriv/rpmostree-core.c +++ b/src/libpriv/rpmostree-core.c @@ -1846,48 +1846,60 @@ setup_rojig_state (RpmOstreeContext *self, return TRUE; } -static gboolean -check_locked_pkgs (RpmOstreeContext *self, - GError **error) +/* Return all the packages that match lockfile constraints. Multiple packages may be + * returned per NEVRA so that libsolv can respect e.g. repo costs. */ +static GPtrArray* +find_locked_packages (RpmOstreeContext *self, + GError **error) { - if (!self->vlockmap) - return TRUE; - - HyGoal goal = dnf_context_get_goal (self->dnfctx); - - /* sanity check it's all pure installs */ - { g_autoptr(GPtrArray) packages = dnf_goal_get_packages (goal, - DNF_PACKAGE_INFO_REMOVE, - DNF_PACKAGE_INFO_OBSOLETE, - DNF_PACKAGE_INFO_REINSTALL, - DNF_PACKAGE_INFO_UPDATE, - DNF_PACKAGE_INFO_DOWNGRADE, - -1); - if (packages->len > 0) - return throw_package_list (error, "Packages not marked for pure install", packages); - } - - g_autoptr(GPtrArray) packages = - dnf_goal_get_packages (goal, DNF_PACKAGE_INFO_INSTALL, -1); + g_assert (self->vlockmap); + DnfContext *dnfctx = self->dnfctx; + DnfSack *sack = dnf_context_get_sack (dnfctx); - for (guint i = 0; i < packages->len; i++) + g_autoptr(GPtrArray) pkgs = + g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref); + GLNX_HASH_TABLE_FOREACH_KV (self->vlockmap, const char*, nevra, const char*, chksum) { - DnfPackage *pkg = packages->pdata[i]; - const char *nevra = dnf_package_get_nevra (pkg); - const char *chksum = g_hash_table_lookup (self->vlockmap, nevra); - if (!chksum || !*chksum) - continue; + g_assert (chksum); + hy_autoquery HyQuery query = hy_query_create (sack); + hy_query_filter (query, HY_PKG_NEVRA_STRICT, HY_EQ, nevra); + g_autoptr(GPtrArray) matches = hy_query_run (query); - g_autofree char *repodata_chksum = NULL; - if (!rpmostree_get_repodata_chksum_repr (pkg, &repodata_chksum, error)) - return FALSE; + gboolean at_least_one = FALSE; + guint n_checksum_mismatches = 0; + for (guint i = 0; i < matches->len; i++) + { + DnfPackage *match = matches->pdata[i]; + if (!*chksum) + { + /* we could optimize this path outside the loop in the future using the + * g_ptr_array_extend_and_steal API, though that's still too new for e.g. el8 */ + g_ptr_array_add (pkgs, g_object_ref (match)); + at_least_one = TRUE; + } + else + { + g_autofree char *repodata_chksum = NULL; + if (!rpmostree_get_repodata_chksum_repr (match, &repodata_chksum, error)) + return NULL; - if (chksum && !g_str_equal (chksum, repodata_chksum)) - return glnx_throw (error, "Locked package %s checksum mismatch: expected %s, got %s", - nevra, chksum, repodata_chksum); + if (!g_str_equal (chksum, repodata_chksum)) + n_checksum_mismatches++; + else + { + g_ptr_array_add (pkgs, g_object_ref (match)); + at_least_one = TRUE; + } + } + } + if (!at_least_one) + return glnx_null_throw (error, "Couldn't find locked package '%s'%s%s " + "(pkgs matching NEVRA: %d; mismatched checksums: %d)", + nevra, *chksum ? " with checksum " : "", + *chksum ? chksum : "", matches->len, n_checksum_mismatches); } - return TRUE; + return g_steal_pointer (&pkgs); } /* Check for/download new rpm-md, then depsolve */ @@ -1930,6 +1942,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, journal_rpmmd_info (self); } DnfSack *sack = dnf_context_get_sack (dnfctx); + HyGoal goal = dnf_context_get_goal (dnfctx); /* Don't try to keep multiple kernels per root; that's a traditional thing, * ostree binds kernel + userspace. @@ -1945,6 +1958,15 @@ rpmostree_context_prepare (RpmOstreeContext *self, g_assert_cmpint (g_strv_length (removed_base_pkgnames), ==, 0); } + if (self->vlockmap) + { + /* we only support pure installs for now (compose case) */ + g_assert (!self->rojig_pure); + g_assert_cmpint (g_strv_length (cached_pkgnames), ==, 0); + g_assert_cmpint (g_strv_length (cached_replace_pkgs), ==, 0); + g_assert_cmpint (g_strv_length (removed_base_pkgnames), ==, 0); + } + /* track cached pkgs already added to the sack so far */ g_autoptr(GHashTable) local_pkgs_to_install = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); @@ -1999,40 +2021,60 @@ rpmostree_context_prepare (RpmOstreeContext *self, * uninstall. We don't want to mix those two steps, otherwise we might confuse libdnf, * see: https://github.com/rpm-software-management/libdnf/issues/700 */ - /* Before adding any pkgs to the goal; filter out from the sack all pkgs which don't match - * our lockfile. (But of course, leave other pkgs untouched.) */ if (self->vlockmap != NULL) { - g_assert_cmpuint (g_strv_length (cached_replace_pkgs), ==, 0); - g_assert_cmpuint (g_strv_length (removed_base_pkgnames), ==, 0); + /* first, find our locked pkgs in the rpmmd */ + g_autoptr(GPtrArray) locked_pkgs = find_locked_packages (self, error); + if (!locked_pkgs) + return FALSE; - GLNX_HASH_TABLE_FOREACH_KV (self->vlockmap, const char*, nevra, const char*, chksum) + /* build a packageset from it */ + DnfPackageSet *locked_pset = dnf_packageset_new (sack); + for (guint i = 0; i < locked_pkgs->len; i++) + dnf_packageset_add (locked_pset, locked_pkgs->pdata[i]); + + if (self->vlockmap_strict) { - g_autofree char *name = NULL; - if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error)) - return FALSE; - hy_autoquery HyQuery query = hy_query_create (sack); - hy_query_filter (query, HY_PKG_NAME, HY_EQ, name); - g_autoptr(GPtrArray) pkglist = hy_query_run (query); + /* In strict mode, we basically *only* want locked packages to be considered, so + * exclude everything else. Note we still don't directly do `hy_goal_install` + * here; we want the treefile to still be canonical, but we just make sure that + * the end result matches what we expect. */ DnfPackageSet *pset = dnf_packageset_new (sack); - for (guint i = 0; i < pkglist->len; i++) - { - DnfPackage *pkg = pkglist->pdata[i]; - const char *pkg_nevra = dnf_package_get_nevra (pkg); - if (!g_str_equal (pkg_nevra, nevra)) - dnf_packageset_add (pset, pkg); - else if (chksum && *chksum) - { - g_autofree char *pkg_chksum = NULL; - if (!rpmostree_get_repodata_chksum_repr (pkg, &pkg_chksum, error)) - return FALSE; - if (!g_str_equal (chksum, pkg_chksum)) - dnf_packageset_add (pset, pkg); - } - } + Map *map = dnf_packageset_get_map (pset); + map_setall (map); + map_subtract (map, dnf_packageset_get_map (locked_pset)); dnf_sack_add_excludes (sack, pset); dnf_packageset_free (pset); } + else + { + /* In relaxed mode, we allow packages to be added or removed without having to + * edit lockfiles. However, we still want to make sure that if a package does get + * installed which is in the lockfile, it can only pick that NEVRA. To do this, we + * filter out from the sack all pkgs which don't match. */ + + /* map of all packages with names found in the lockfile */ + DnfPackageSet *named_pkgs = dnf_packageset_new (sack); + Map *named_pkgs_map = dnf_packageset_get_map (named_pkgs); + GLNX_HASH_TABLE_FOREACH (self->vlockmap, const char*, nevra) + { + g_autofree char *name = NULL; + if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error)) + return FALSE; + hy_autoquery HyQuery query = hy_query_create (sack); + hy_query_filter (query, HY_PKG_NAME, HY_EQ, name); + DnfPackageSet *pset = hy_query_run_set (query); + map_or (named_pkgs_map, dnf_packageset_get_map (pset)); + dnf_packageset_free (pset); + } + + /* remove our locked packages from the exclusion set */ + map_subtract (named_pkgs_map, dnf_packageset_get_map (locked_pset)); + dnf_sack_add_excludes (sack, named_pkgs); + dnf_packageset_free (named_pkgs); + } + + dnf_packageset_free (locked_pset); } /* Process excludes */ @@ -2059,7 +2101,6 @@ rpmostree_context_prepare (RpmOstreeContext *self, } /* Then, handle local packages to install */ - HyGoal goal = dnf_context_get_goal (dnfctx); GLNX_HASH_TABLE_FOREACH_V (local_pkgs_to_install, DnfPackage*, pkg) hy_goal_install (goal, pkg); @@ -2108,8 +2149,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, /* XXX: consider a --allow-uninstall switch? */ if (!dnf_goal_depsolve (goal, actions, error) || - !check_goal_solution (self, removed_pkgnames, replaced_nevras, error) || - !check_locked_pkgs (self, error)) + !check_goal_solution (self, removed_pkgnames, replaced_nevras, error)) return FALSE; g_clear_pointer (&self->pkgs, (GDestroyNotify)g_ptr_array_unref); self->pkgs = dnf_goal_get_packages (dnf_context_get_goal (dnfctx), @@ -2177,11 +2217,15 @@ rpmostree_context_get_packages_to_import (RpmOstreeContext *self) return g_ptr_array_ref (self->pkgs_to_import); } +/* Note this must be called *before* rpmostree_context_setup(). */ void -rpmostree_context_set_vlockmap (RpmOstreeContext *self, GHashTable *map) +rpmostree_context_set_vlockmap (RpmOstreeContext *self, + GHashTable *map, + gboolean strict) { g_clear_pointer (&self->vlockmap, (GDestroyNotify)g_hash_table_unref); self->vlockmap = g_hash_table_ref (map); + self->vlockmap_strict = strict; } /* XXX: push this into libdnf */ diff --git a/src/libpriv/rpmostree-core.h b/src/libpriv/rpmostree-core.h index 80ba69ce5a..089953a86e 100644 --- a/src/libpriv/rpmostree-core.h +++ b/src/libpriv/rpmostree-core.h @@ -173,7 +173,10 @@ gboolean rpmostree_context_set_packages (RpmOstreeContext *self, GPtrArray *rpmostree_context_get_packages_to_import (RpmOstreeContext *self); -void rpmostree_context_set_vlockmap (RpmOstreeContext *self, GHashTable *map); +void +rpmostree_context_set_vlockmap (RpmOstreeContext *self, + GHashTable *map, + gboolean strict); gboolean rpmostree_context_download (RpmOstreeContext *self, GCancellable *cancellable, diff --git a/tests/compose/libcomposetest.sh b/tests/compose/libcomposetest.sh index 555570f631..ed36946be1 100644 --- a/tests/compose/libcomposetest.sh +++ b/tests/compose/libcomposetest.sh @@ -56,6 +56,10 @@ else: tf['$1'] += $2" } +treefile_remove() { + treefile_pyedit "tf['$1'].remove($2)" +} + # for tests that need direct control on rpm-ostree export compose_base_argv="\ --unified-core \ diff --git a/tests/compose/test-lockfile.sh b/tests/compose/test-lockfile.sh index dba59a899e..11aa45f366 100755 --- a/tests/compose/test-lockfile.sh +++ b/tests/compose/test-lockfile.sh @@ -58,15 +58,96 @@ EOF runcompose \ --ex-lockfile="$PWD/versions.lock" \ --ex-lockfile="$PWD/override.lock" \ - --ex-write-lockfile-to="$PWD/versions.lock" \ + --ex-write-lockfile-to="$PWD/versions.lock.new" \ --dry-run "${treefile}" |& tee out.txt -echo "ok compose with lockfile" - assert_file_has_content out.txt 'test-pkg-1.0-1.x86_64' assert_file_has_content out.txt 'test-pkg-common-1.0-1.x86_64' assert_file_has_content out.txt 'another-test-pkg-2.0-1.x86_64' -assert_jq versions.lock \ +assert_jq versions.lock.new \ '.packages["test-pkg"].evra = "1.0-1.x86_64"' \ '.packages["test-pkg-common"].evra = "1.0-1.x86_64"' \ '.packages["another-test-pkg"].evra = "2.0-1.x86_64"' echo "ok override" + +# sanity-check that we can remove packages in relaxed mode +treefile_remove "packages" '"another-test-pkg"' +runcompose \ + --ex-lockfile="$PWD/versions.lock" \ + --ex-lockfile="$PWD/override.lock" \ + --ex-write-lockfile-to="$PWD/versions.lock.new" \ + --dry-run "${treefile}" |& tee out.txt +assert_file_has_content out.txt 'test-pkg-1.0-1.x86_64' +assert_file_has_content out.txt 'test-pkg-common-1.0-1.x86_64' +assert_not_file_has_content out.txt 'another-test-pkg' +echo "ok relaxed mode can remove pkg" + +# test strict mode + +# sanity-check that refeeding the output lockfile as input satisfies strict mode +mv versions.lock{.new,} +runcompose \ + --ex-lockfile-strict \ + --ex-lockfile="$PWD/versions.lock" \ + --ex-write-lockfile-to="$PWD/versions.lock.new" \ + --dry-run "${treefile}" |& tee out.txt +assert_streq \ + "$(jq .packages versions.lock | sha256sum)" \ + "$(jq .packages versions.lock.new | sha256sum)" +echo "ok strict mode sanity check" + +# check that trying to install a pkg that's not in the lockfiles fails +build_rpm unlocked-pkg +treefile_append "packages" '["unlocked-pkg"]' +if runcompose \ + --ex-lockfile-strict \ + --ex-lockfile="$PWD/versions.lock" \ + --dry-run "${treefile}" &>err.txt; then + fatal "compose unexpectedly succeeded" +fi +assert_file_has_content err.txt 'Packages not found: unlocked-pkg' +echo "ok strict mode no unlocked pkgs" + +# check that a locked pkg with unlocked deps causes an error +build_rpm unlocked-pkg version 2.0 requires unlocked-pkg-dep +build_rpm unlocked-pkg-dep version 2.0 +# notice we add unlocked-pkg, but not unlocked-pkg-dep +cat > override.lock <err.txt; then + fatal "compose unexpectedly succeeded" +fi +assert_file_has_content err.txt 'Could not depsolve transaction' +assert_file_has_content err.txt 'unlocked-pkg-dep-2.0-1.x86_64 is filtered out by exclude filtering' +treefile_remove "packages" '"unlocked-pkg"' +echo "ok strict mode no unlocked pkg deps" + +# check that a locked pkg which isn't actually in the repos causes an error +cat > override.lock <err.txt; then + fatal "compose unexpectedly succeeded" +fi +assert_file_has_content err.txt "Couldn't find locked package 'unmatched-pkg-1.0-1.x86_64'" +echo "ok strict mode locked pkg missing from rpmmd"