Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support RPMs installing in /opt and /usr/local #4728

Merged
merged 4 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile-rpm-ostree.am
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ librpmostreeinternals_la_CXXFLAGS = $(AM_CXXFLAGS) $(sanitizer_flags) $(rpmostre
librpmostreeinternals_la_LIBADD = $(rpmostree_common_libs)

privdatadir=$(pkglibdir)
privdata_DATA = src/app/rpm-ostree-0-integration.conf src/app/rpm-ostree-0-integration-opt-usrlocal.conf
privdata_DATA = src/app/rpm-ostree-0-integration.conf src/app/rpm-ostree-0-integration-opt-usrlocal.conf src/app/rpm-ostree-0-integration-opt-usrlocal-compat.conf

# Propagate automake verbose mode
cargo_build = $(cargo) build $(if $(subst 0,,$(V)),--verbose,)
Expand Down
6 changes: 6 additions & 0 deletions docs/treefile.md
Original file line number Diff line number Diff line change
Expand Up @@ -479,3 +479,9 @@ version of `rpm-ostree`.
names to use when substituting variables in yum repo files. The `releasever`
variable name is invalid. Use the `releasever` key instead. The `basearch`
name is invalid; it is filled in automatically.
* `opt-usrlocal-overlays`: boolean, optional: Defaults to `false`. By
default, `/opt` and `/usr/local` are symlinks to subdirectories in `/
var`. This prevents the ability to compose with packages that install in
those directories. If enabled, RPMs with `/opt` and `/usr/local` content
are allowed; client-side, both paths are writable overlay directories on.
Requires libostree v2023.9+.
10 changes: 10 additions & 0 deletions rpmostree-cxxrs.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -1774,6 +1774,7 @@ struct Treefile final : public ::rust::Opaque
::rpmostreecxx::RepoMetadataTarget get_repo_metadata_target () const noexcept;
bool rpmdb_backend_is_target () const noexcept;
bool should_normalize_rpmdb () const noexcept;
bool get_opt_usrlocal_overlays () const noexcept;
::rust::Vec< ::rust::String> get_files_remove_regex (::rust::Str package) const noexcept;
::rust::String get_checksum (::rpmostreecxx::OstreeRepo const &repo) const;
::rust::String get_ostree_ref () const noexcept;
Expand Down Expand Up @@ -2643,6 +2644,9 @@ extern "C"
bool rpmostreecxx$cxxbridge1$Treefile$should_normalize_rpmdb (
::rpmostreecxx::Treefile const &self) noexcept;

bool rpmostreecxx$cxxbridge1$Treefile$get_opt_usrlocal_overlays (
::rpmostreecxx::Treefile const &self) noexcept;

void rpmostreecxx$cxxbridge1$Treefile$get_files_remove_regex (
::rpmostreecxx::Treefile const &self, ::rust::Str package,
::rust::Vec< ::rust::String> *return$) noexcept;
Expand Down Expand Up @@ -5257,6 +5261,12 @@ Treefile::should_normalize_rpmdb () const noexcept
return rpmostreecxx$cxxbridge1$Treefile$should_normalize_rpmdb (*this);
}

bool
Treefile::get_opt_usrlocal_overlays () const noexcept
{
return rpmostreecxx$cxxbridge1$Treefile$get_opt_usrlocal_overlays (*this);
}

::rust::Vec< ::rust::String>
Treefile::get_files_remove_regex (::rust::Str package) const noexcept
{
Expand Down
1 change: 1 addition & 0 deletions rpmostree-cxxrs.h
Original file line number Diff line number Diff line change
Expand Up @@ -1556,6 +1556,7 @@ struct Treefile final : public ::rust::Opaque
::rpmostreecxx::RepoMetadataTarget get_repo_metadata_target () const noexcept;
bool rpmdb_backend_is_target () const noexcept;
bool should_normalize_rpmdb () const noexcept;
bool get_opt_usrlocal_overlays () const noexcept;
::rust::Vec< ::rust::String> get_files_remove_regex (::rust::Str package) const noexcept;
::rust::String get_checksum (::rpmostreecxx::OstreeRepo const &repo) const;
::rust::String get_ostree_ref () const noexcept;
Expand Down
82 changes: 76 additions & 6 deletions rust/src/composepost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,20 +145,32 @@ fn compose_init_rootfs_transient(rootfs_dfd: &cap_std::fs::Dir) -> Result<()> {
/// This is hardcoded; in the future we may make more things configurable,
/// but the goal is for all state to be in `/etc` and `/var`.
#[context("Initializing rootfs")]
fn compose_init_rootfs_strict(rootfs_dfd: &cap_std::fs::Dir, tmp_is_dir: bool) -> Result<()> {
fn compose_init_rootfs_strict(
rootfs_dfd: &cap_std::fs::Dir,
tmp_is_dir: bool,
opt_state_overlay: bool,
) -> Result<()> {
println!("Initializing rootfs");

compose_init_rootfs_base(rootfs_dfd, tmp_is_dir)?;

const OPT_SYMLINK_LEGACY: &str = "var/opt";
const OPT_SYMLINK_STATEOVERLAY: &str = "usr/lib/opt";
let opt_symlink = if opt_state_overlay {
OPT_SYMLINK_STATEOVERLAY
} else {
OPT_SYMLINK_LEGACY
};

// This is used in the case where we don't have a transient rootfs; redirect
// these toplevel directories underneath /var.
const OSTREE_STRICT_MODE_SYMLINKS: &[(&str, &str)] = &[
("var/opt", "opt"),
let ostree_strict_mode_symlinks: &[(&str, &str)] = &[
(opt_symlink, "opt"),
("var/srv", "srv"),
("var/mnt", "mnt"),
("run/media", "media"),
];
OSTREE_STRICT_MODE_SYMLINKS
ostree_strict_mode_symlinks
.par_iter()
.try_for_each(|&(dest, src)| {
rootfs_dfd
Expand Down Expand Up @@ -212,7 +224,15 @@ pub fn compose_prepare_rootfs(
return Ok(());
}

compose_init_rootfs_strict(target_rootfs_dfd, tmp_is_dir)?;
compose_init_rootfs_strict(
target_rootfs_dfd,
tmp_is_dir,
treefile
.parsed
.base
.opt_usrlocal_overlays
.unwrap_or_default(),
)?;

println!("Moving /usr to target");
src_rootfs_dfd.rename("usr", target_rootfs_dfd, "usr")?;
Expand Down Expand Up @@ -606,6 +626,32 @@ fn compose_postprocess_rpmdb(rootfs_dfd: &Dir) -> Result<()> {
Ok(())
}

/// Enables [email protected] for /usr/lib/opt and /usr/local. These
/// symlinks are also used later in the compose process (and client-side composes)
/// as a way to check that state overlays are turned on.
fn compose_postprocess_state_overlays(rootfs_dfd: &Dir) -> Result<()> {
let mut db = cap_std::fs::DirBuilder::new();
db.recursive(true);
db.mode(0o755);
let localfs_requires = Path::new("usr/lib/systemd/system/local-fs.target.requires");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one could probably live as a const given it's referenced twice.

rootfs_dfd.ensure_dir_with(localfs_requires, &db)?;

const UNITS: &[&str] = &[
"[email protected]",
"[email protected]",
];

UNITS.par_iter().try_for_each(|&unit| {
let target = Path::new("..").join(unit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, doesn't cap-std barf on this? I did this PR a while ago which we use in other places; though maybe it doesn't actually check for ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, seems to work here! :)

let linkpath = localfs_requires.join(unit);
rootfs_dfd
.symlink(target, linkpath)
.with_context(|| format!("Enabling {unit}"))
})?;

Ok(())
}

/// Rust portion of rpmostree_treefile_postprocessing()
pub fn compose_postprocess(
rootfs_dfd: i32,
Expand All @@ -627,6 +673,15 @@ pub fn compose_postprocess(
compose_postprocess_default_target(rootfs, t)?;
}

if treefile
.parsed
.base
.opt_usrlocal_overlays
.unwrap_or_default()
{
compose_postprocess_state_overlays(rootfs)?;
}

treefile.write_compose_json(rootfs)?;

let etc_guard = crate::core::prepare_tempetc_guard(rootfs_dfd.as_raw_fd())?;
Expand Down Expand Up @@ -955,6 +1010,17 @@ fn convert_path_to_tmpfiles_d_recurse(
Ok(())
}

fn state_overlay_enabled(rootfs_dfd: &cap_std::fs::Dir, state_overlay: &str) -> Result<bool> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function in theory could live in ostree's rust code or ostree-ext, not that it matters much in practice. (Please don't move it just for this, but just a thought)

let linkname = format!(
"usr/lib/systemd/system/local-fs.target.requires/ostree-state-overlay@{state_overlay}.service"
);
match rootfs_dfd.symlink_metadata_optional(&linkname)? {
Some(meta) if meta.is_symlink() => Ok(true),
Some(_) => Err(anyhow!("{linkname} is not a symlink")),
None => Ok(false),
}
}

/// Walk over the root filesystem and perform some core conversions
/// from RPM conventions to OSTree conventions.
///
Expand All @@ -969,7 +1035,11 @@ pub fn rootfs_prepare_links(rootfs_dfd: i32, skip_usrlocal: bool) -> CxxResult<(
db.recursive(true);

if !skip_usrlocal {
if !crate::ostree_prepareroot::transient_root_enabled(rootfs)? {
if state_overlay_enabled(rootfs, "usr-local")? {
// because of the filesystem lua issue (see
// compose_init_rootfs_base()) we need to create this manually
rootfs.ensure_dir_with("usr/local", &db)?;
} else if !crate::ostree_prepareroot::transient_root_enabled(rootfs)? {
// Unconditionally drop /usr/local and replace it with a symlink.
rootfs
.remove_all_optional("usr/local")
Expand Down
4 changes: 2 additions & 2 deletions rust/src/importer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ fn path_is_ostree_compliant(path: &str) -> bool {
return true;
}

if path.starts_with("/usr/") && !path.starts_with("/usr/local") {
if path.starts_with("/usr/") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this change needs to be gated on us detecting that a state overlay is enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be strict, yes. I didn't bother with this because /usr/local RPMs are known broken right now anyway and this PR is working to improve the situation. With this hunk but without state overlays enabled, it'll still be broken, but in a different way (though true, in a more subtle way than the current explicit error message users get -- content in /usr/local will just be deleted in postprocessing when replaced by the /var/usrlocal symlink).

Can pipe through the needed flag to retain current behaviour if wanted.

return true;
}

Expand Down Expand Up @@ -491,7 +491,7 @@ mod tests {
assert_eq!(path_is_ostree_compliant(entry), false, "{}", entry);
}

let denied_cases = &["/var", "/etc", "/var/run", "/usr/local", "", "./", "usr/"];
let denied_cases = &["/var", "/etc", "/var/run", "", "./", "usr/"];
for entry in denied_cases {
assert_eq!(path_is_ostree_compliant(entry), false, "{}", entry);
}
Expand Down
1 change: 1 addition & 0 deletions rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ pub mod ffi {
fn get_repo_metadata_target(&self) -> RepoMetadataTarget;
fn rpmdb_backend_is_target(&self) -> bool;
fn should_normalize_rpmdb(&self) -> bool;
fn get_opt_usrlocal_overlays(&self) -> bool;
fn get_files_remove_regex(&self, package: &str) -> Vec<String>;
fn get_checksum(&self, repo: &OstreeRepo) -> Result<String>;
fn get_ostree_ref(&self) -> String;
Expand Down
7 changes: 7 additions & 0 deletions rust/src/treefile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ fn treefile_merge(dest: &mut TreeComposeConfig, src: &mut TreeComposeConfig) {
documentation,
boot_location,
tmp_is_dir,
opt_usrlocal_overlays,
default_target,
machineid_compat,
releasever,
Expand Down Expand Up @@ -1411,6 +1412,10 @@ impl Treefile {
self.parsed.base.rpmdb_normalize.unwrap_or(false)
}

pub(crate) fn get_opt_usrlocal_overlays(&self) -> bool {
self.parsed.base.opt_usrlocal_overlays.unwrap_or_default()
}

pub(crate) fn get_files_remove_regex(&self, package: &str) -> Vec<String> {
let mut files_to_remove: Vec<String> = Vec::new();
if let Some(ref packages) = self.parsed.base.remove_from_packages {
Expand Down Expand Up @@ -2531,6 +2536,8 @@ pub(crate) struct BaseComposeConfigFields {
pub(crate) boot_location: Option<BootLocation>,
#[serde(skip_serializing_if = "Option::is_none")]
pub(crate) tmp_is_dir: Option<bool>,
#[serde(skip_serializing_if = "Option::is_none")]
pub(crate) opt_usrlocal_overlays: Option<bool>,

// systemd
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down
5 changes: 5 additions & 0 deletions src/app/rpm-ostree-0-integration-opt-usrlocal-compat.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Traditionally, /usr/local has been a link to /var/usrlocal and /opt to /var/opt.
# A new model now is to allow OSTree commit content in those directories. For
# backwards compatibility, we keep the /var paths but flip the symlinks around.
L /var/usrlocal - - - - ../usr/local
L /var/opt - - - - ../usr/lib/opt
3 changes: 3 additions & 0 deletions src/app/rpm-ostree-0-integration-opt-usrlocal.conf
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
# Traditionally, /usr/local has been a link to /var/usrlocal and /opt to /var/opt.
# A new model now is to allow OSTree commit content in those directories. But
# this dropin implements the old model.
d /var/opt 0755 root root -
d /var/usrlocal 0755 root root -
49 changes: 49 additions & 0 deletions src/libpriv/rpmostree-core.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -4062,6 +4062,55 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G
if (!build_rootfs_usrlinks (self, error))
return FALSE;

/* This is purely for making it easier for people to test out the
* state-overlay stuff until it's stabilized and part of base composes. */
if (g_getenv ("RPMOSTREE_EXPERIMENTAL_FORCE_OPT_USRLOCAL_OVERLAY"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to document how to do this in a derived container build? The ergonomics around providing an environment variable in this context seems...difficult.

{
rpmostree_output_message (
"Enabling experimental state overlay support for /opt and /usr/local");

struct stat stbuf;

if (!glnx_fstatat_allow_noent (tmprootfs_dfd, "opt", &stbuf, AT_SYMLINK_NOFOLLOW, error))
return FALSE;
if (errno == ENOENT || (errno == 0 && S_ISLNK (stbuf.st_mode)))
{
if (errno == 0 && !glnx_unlinkat (tmprootfs_dfd, "opt", 0, error))
return FALSE;
if (symlinkat ("usr/lib/opt", tmprootfs_dfd, "opt") < 0)
return glnx_throw_errno_prefix (error, "symlinkat(/opt)");
}

if (!glnx_fstatat_allow_noent (tmprootfs_dfd, "usr/local", &stbuf, AT_SYMLINK_NOFOLLOW,
error))
return FALSE;
if (errno == ENOENT || (errno == 0 && S_ISLNK (stbuf.st_mode)))
{
if (errno == 0 && !glnx_unlinkat (tmprootfs_dfd, "usr/local", 0, error))
return FALSE;
if (mkdirat (tmprootfs_dfd, "usr/local", 0755) < 0)
return glnx_throw_errno_prefix (error, "mkdirat(/usr/local)");
}

if (!glnx_shutil_mkdir_p_at (tmprootfs_dfd, "usr/lib/systemd/system/local-fs.target.wants",
0755, cancellable, error))
return FALSE;

if (symlinkat ("[email protected]", tmprootfs_dfd,
"usr/lib/systemd/system/local-fs.target.wants/"
"[email protected]")
< 0
&& errno != EEXIST)
return glnx_throw_errno_prefix (error, "enabling ostree-state-overlay for /usr/lib/opt");

if (symlinkat (
"[email protected]", tmprootfs_dfd,
"usr/lib/systemd/system/local-fs.target.wants/[email protected]")
< 0
&& errno != EEXIST)
return glnx_throw_errno_prefix (error, "enabling ostree-state-overlay for /usr/local");
}

/* We need up to date labels; the set of things needing relabeling
* will have been calculated in sort_packages()
*/
Expand Down
24 changes: 18 additions & 6 deletions src/libpriv/rpmostree-postprocess.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -461,12 +461,24 @@ postprocess_final (int rootfs_dfd, rpmostreecxx::Treefile &treefile, gboolean un
cancellable, error))
return FALSE;

if (!glnx_file_copy_at (pkglibdir_dfd, "rpm-ostree-0-integration-opt-usrlocal.conf", NULL,
rootfs_dfd,
"usr/lib/tmpfiles.d/rpm-ostree-0-integration-opt-usrlocal.conf",
GLNX_FILE_COPY_NOXATTRS, /* Don't take selinux label */
cancellable, error))
return FALSE;
if (treefile.get_opt_usrlocal_overlays ())
{
if (!glnx_file_copy_at (
pkglibdir_dfd, "rpm-ostree-0-integration-opt-usrlocal-compat.conf", NULL, rootfs_dfd,
"usr/lib/tmpfiles.d/rpm-ostree-0-integration-opt-usrlocal-compat.conf",
GLNX_FILE_COPY_NOXATTRS, /* Don't take selinux label */
cancellable, error))
return FALSE;
}
else
{
if (!glnx_file_copy_at (pkglibdir_dfd, "rpm-ostree-0-integration-opt-usrlocal.conf", NULL,
rootfs_dfd,
"usr/lib/tmpfiles.d/rpm-ostree-0-integration-opt-usrlocal.conf",
GLNX_FILE_COPY_NOXATTRS, /* Don't take selinux label */
cancellable, error))
return FALSE;
}

/* Handle kernel/initramfs if we're not doing a container */
if (!container)
Expand Down
45 changes: 45 additions & 0 deletions tests/compose/test-state-overlays.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/bin/bash
set -xeuo pipefail

dn=$(cd "$(dirname "$0")" && pwd)
# shellcheck source=libcomposetest.sh
. "${dn}/libcomposetest.sh"

# Add a local rpm-md repo so we can mutate local test packages
treefile_append "repos" '["test-repo"]'

# An RPM that installs in /opt
build_rpm test-opt \
install "mkdir -p %{buildroot}/opt/megacorp/bin
install %{name} %{buildroot}/opt/megacorp/bin" \
files "/opt/megacorp"

# An RPM that installs in /usr/local
build_rpm test-usr-local \
install "mkdir -p %{buildroot}/usr/local/bin
install %{name} %{buildroot}/usr/local/bin" \
files "/usr/local/bin/%{name}"

echo gpgcheck=0 >> yumrepo.repo
ln "$PWD/yumrepo.repo" config/yumrepo.repo

# the top-level manifest doesn't have any packages, so just set it
treefile_append "packages" '["test-opt", "test-usr-local"]'

# enable state overlays
treefile_set "opt-usrlocal-overlays" 'True'

runcompose

# shellcheck disable=SC2154
ostree --repo="${repo}" ls -R "${treeref}" /usr/lib/opt > opt.txt
Fixed Show fixed Hide fixed
Fixed Show fixed Hide fixed
assert_file_has_content opt.txt "/usr/lib/opt/megacorp/bin/test-opt"

ostree --repo="${repo}" ls -R "${treeref}" /usr/local > usr-local.txt
assert_file_has_content usr-local.txt "/usr/local/bin/test-usr-local"

ostree --repo="${repo}" ls -R "${treeref}" /usr/lib/systemd/system/local-fs.target.requires > local-fs.txt
assert_file_has_content local-fs.txt "[email protected]"
assert_file_has_content local-fs.txt "[email protected]"

echo "ok /opt and /usr/local RPMs"
Loading
Loading