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

@@ -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));
     }
 

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;
 

src/libpriv/rpmostree-core.c

@@ -1846,48 +1846,60 @@ setup_rojig_state (RpmOstreeContext *self,
   return TRUE;
 }
 
-static gboolean
-check_locked_pkgs (RpmOstreeContext *self,
+/* 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;
+  g_assert (self->vlockmap);
+  DnfContext *dnfctx = self->dnfctx;
+  DnfSack *sack = dnf_context_get_sack (dnfctx);
 
-  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);
-
-  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);
 
+      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 (pkg, &repodata_chksum, error))
-        return FALSE;
+              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)
+        {
+          /* 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);
+          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);
-          g_autoptr(GPtrArray) pkglist = hy_query_run (query);
-          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);
-                }
-            }
-          dnf_sack_add_excludes (sack, pset);
+              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 */

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,

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 \

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 <<EOF
+{
+  "packages": {
+    "unlocked-pkg": {
+      "evra": "2.0-1.x86_64"
+    }
+  }
+}
+EOF
+if runcompose \
+    --ex-lockfile-strict \
+    --ex-lockfile="$PWD/versions.lock" \
+    --ex-lockfile="$PWD/override.lock" \
+    --dry-run "${treefile}" &>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 <<EOF
+{
+  "packages": {
+    "unmatched-pkg": {
+      "evra": "1.0-1.x86_64"
+    }
+  }
+}
+EOF
+if runcompose \
+    --ex-lockfile-strict \
+    --ex-lockfile="$PWD/versions.lock" \
+    --ex-lockfile="$PWD/override.lock" \
+    --dry-run "${treefile}" &>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"