Commit Graph

16 Commits

Author SHA1 Message Date
Colin Walters ed15723cd1 lib/commit: Fix hardlink checkout commit with bare-user + mod xattrs
This is more subtle fallout from:
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536257

Before, if we found a devino cache hit, we'd use it unconditionally.

Recall that `bare-user` repositories are very special in that they're the only
mode where the on disk state ("physical state") is not the "real" state. The
latter is stored in the `user.ostreemeta` xattr. (`bare-user` repos are also
highly special in that symlinks are regular files physically, but that's not
immediately relevant here).

Since we now have `bare-user-only` for the "pure unprivileged container" case,
`bare-user` should just be used for "OS builds" which have nonzero uids (and
possibly SELinux labels etc.)

In an experimental tool I'm writing "skopeo2ostree" which imports OCI images
into refs, then squashes them together into a single final commit, we lost the
the `81` group ID for `/usr/libexec/dbus-1/dbus-daemon-launch-helper`.

This happened because the commit code was loading the "physical" disk state,
where the uid/gid are zero because that's the uid I happened to be using. We
didn't just directly do the link speedup because I was using `--selinux-policy`
which caused the xattrs to change, which caused us to re-commit objects from the
physical state.

The unit test I added actually doesn't quite trigger this, but I left
it because "why not".  Really testing this requires the installed test
which uses SELinux policy from `/`.

The behavior without this fix looks like:

```
-00755 0 0     12 { [(b'user.ostreemeta', [byte 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x51, 0x00, 0x00, 0x81, 0xed]), (b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/dbus-daemon-helper
```

which was obviously totally broken - we shouldn't be picking up the
`user.ostreemeta` xattr and actually committing it of course.

Closes: #1297
Approved by: jlebon
2017-10-23 17:02:28 +00:00
Jonathan Lebon 4262a4b016 tests/installed: also run test-basic-c
Since we now have a subtest there that needs full xattr support.

Closes: #1170
Approved by: cgwalters
2017-09-30 00:05:07 +00:00
Colin Walters 5963d5a2a9 tests,ci: Move "test-basic" (bare mode) to installed test
Our CI uses default Docker, which has SELinux labeling but is rather
evil in returning `EOPNOTSUPP` to any attempts to set `security.selinux`,
even if to the same value.

The previous fire 🔥 for this was: https://github.com/ostreedev/ostree/pull/759

The `bare` repo mode really only makes sense as uid 0, so our installed
test framework is a good match for this.  However, the unit tests *do*
work in a privileged container even as non-root, and *also* should
work on SELinux-disabled systems.  So let's teach the test framework
how to skip in those situations.

I tested this both in a priv container (my default builder) and an unpriv
container (like our CI).

At the same time, start executing the `test-basic.sh` from an installed test,
so we get better coverage than before.

This is just the start - all of the sysroot tests really need the
same treatment.

Closes: #1217
Approved by: jlebon
2017-09-27 13:13:14 +00:00
Colin Walters 8a7a359709 lib/commit: Add a copy fastpath for imports
This fixes up the last of the embarassing bits I saw from
the stack trace in:
https://github.com/ostreedev/ostree/issues/1184

We had a hardlink fast path, but that doesn't apply across
devices, which occurs in two notable cases:

 - Installer ISO with local repo
 - Tools like pungi that copy the repo to a local snapshot

Obviously there are a lot of subtleties here around things like the
bare-user-only conversions as well as exactly what data we copy. I think to get
better test coverage we may want to add `pull-local --no-hardlink` or so.

Closes: #1197
Approved by: jlebon
2017-09-26 16:50:41 +00:00
Jonathan Lebon 12114ce382 commit: filter out selinux label before commit
The new --selinux-policy added in [0] exposed a subtle issue in the way
we handle labeling during commit. The CI system in rpm-ostree hit this
when trying to make use of it[1].

Basically, because of the way we use a GVariant to represent xattrs, if
a file to be committed already has an SELinux label, the xattr object
ends up with *two* label entries. This of course throws off fsck later
on, since the checksum will have gone over both entries, even though the
on-disk file will only have a single label (in which the second entry
wins).

I confirmed that the `fsck` added in the installed test fails without
the rest of this patch.

[0] https://github.com/ostreedev/ostree/pull/1114
[1] https://github.com/projectatomic/rpm-ostree/pull/953

Closes: #1121
Approved by: cgwalters
2017-08-31 12:07:46 +00:00
Colin Walters 5e30bbd1b9 bin/commit: Add --selinux-policy option
This was really straightforward to implement, and is useful
for dev/test scenarios mainly like we have in rpm-ostree at least.

Closes: https://github.com/ostreedev/ostree/issues/1113

Closes: #1114
Approved by: jlebon
2017-08-25 16:39:53 +00:00
Colin Walters e0346c1494 Add a notion of "physical" sysroot, use for remote writing
(Note this PR was reverted in <https://github.com/ostreedev/ostree/pull/902>;
 this version should be better)

Using `${sysroot}` to mean the physical storage root: We don't want to write to
`${sysroot}/etc/ostree/remotes.d`, since nothing will read it, and really
`${sysroot}` should just have `/ostree` (ideally). Today the Anaconda rpmostree
code ends up writing there. Fix this by adding a notion of "physical" sysroot.
We determine whether the path is physical by checking for `/sysroot`, which
exists in deployment roots (and there shouldn't be a `${sysroot}/sysroot`).

In order to unit test this, I added a `--sysroot` argument to `remote add`.
However, doing this better would require reworking the command line parsing for
the `remote` argument to support specifying `--repo` or `--sysroot`, and I
didn't quite want to do that yet in this patch.

This second iteration of this patch fixes the bug we hit the first time;
embarassingly enough I broke `ostree remote list` finding system remotes.
The fix is to have `ostree_repo_open()` figure out whether it's the same
as `/ostree/repo` for now.

Down the line...we might consider having the `ostree remote` command line itself
instatiate an `OstreeSysroot` by default, but this maximizes compatibility; we
just have to pay a small cost that `ostree` usage outside of that case like
`ostree static-delta` in a releng Jenkins job or whatever will do this `stat()`
too.

Closes: https://github.com/ostreedev/ostree/issues/892

Closes: #1008
Approved by: mbarnes
2017-07-18 18:58:06 +00:00
Colin Walters 9d941dcebb checkout: Don't set dir mtime to 0 when doing a force copy checkout
When we [switched to using checkout + force_copy](e8efd1c8dc),
a side effect that went unnoticed at the time is that we started
setting directory mtimes to zero.

See the below bug where we long ago set the file times to zero, which got fixed,
so let's not regress things by setting the directory times to zero either. (Even
though AFAICS GNU tar doesn't complain about those)

This semantic is somewhat "overloaded" onto `force_copy`, but it avoids adding
yet another boolean; we don't have that many reserved boolean slots left. I
can't really think of many good use cases for `force_copy` *other* than the
`/etc` merge anyways.

https://bugzilla.redhat.com/show_bug.cgi?id=1229160

Closes: https://github.com/ostreedev/ostree/issues/995

Closes: #997
Approved by: jlebon
2017-07-07 15:01:51 +00:00
Colin Walters 23b93a3eb6 lib/repo: Immediately error creating bare-user repo on tmpfs
And in general, if for some reason we can't write `user.` xattrs, provide an
error immediately rather than doing it during a later pull. This way the failure
cause is a lot more obvious.

Related: https://github.com/ostreedev/ostree/issues/991

Closes: #993
Approved by: jlebon
2017-07-06 14:31:37 +00:00
Colin Walters 1f5ce1a9f7 lib/repo: Add min-free-space-percent option, default 3%
For ostree-as-host, we're the superuser, so we'll blow past
any reserved free space by default.  While deltas have size
metadata, if one happens to do a loose fetch, we can fill
up the disk.

Another case is flatpak: the system helper has similar concerns
here as ostree-as-host, and for `flatpak --user`, we also
want to be nice and avoid filling up the user's quota.

Closes: https://github.com/ostreedev/ostree/issues/962

Closes: #987
Approved by: jlebon
2017-07-04 16:15:11 +00:00
Colin Walters fb2c3c1db3 tests: Fix previous commit for selinux testing
I only checked the test passed, I didn't read the output closely, and
made it succeed without testing anything.

Fix the absolute/relative `/etc` references.

Closes: #937
Approved by: jlebon
2017-06-16 15:46:07 +00:00
Colin Walters 371b4a5e7e checkout: Fix SELinux policy labeling when recursing
The code here tried to truncate the string to the previous length,
but that doesn't work when recursing, since further calls change the
length.

What actually ended up happening was the string would get corrupted
after the first level of recursion.

Closes: #936
Approved by: jlebon
2017-06-16 14:54:29 +00:00
Colin Walters af7fed94ed ci: Extend FAH rootfs for installed tests
These at the moment aren't in a container, and may need space. In the future
overlay2 will help here, we can more easily extend the rootfs.

Closes: #840
Approved by: jlebon
2017-05-09 15:08:26 +00:00
Colin Walters 48d2637e98 tests: Migrate test-pull-many.sh to installed on FAH
`test-pull-many.sh` is was just too slow to be a unit test.  Generating
a bunch of files via shell is slow, the delta generation is slow, etc.
Every developer doesn't need to run it every time.

Somewhat address this by converting it into our installed test framework, which
moves it out of the developer fast paths.  Another advantage to this is
that we can simply reuse the FAH tree content rather than synthesizing
new bits each time.

Closes: #840
Approved by: jlebon
2017-05-09 15:08:26 +00:00
Colin Walters 2800d176bc tests: For installed, s/test-/itest-/ to avoid in-tree name clashes
I want to migrate `test-pull-many.sh` → `itest-pull.sh`, hence not
conflicting with the unit test `test-pull.sh.

Closes: #840
Approved by: jlebon
2017-05-09 15:08:26 +00:00
Colin Walters 654b0c4877 tests/installed: New installed, privileged tests using Fedora AH
Our container-driven tests can't e.g. test SELinux sanely, and
have to support being run as root *and* non-root too.

Use redhat-ci to provision a VM and run tests directly there. These are
installed tests too.

Closes: https://github.com/ostreedev/ostree/issues/806

Closes: #807
Approved by: jlebon
2017-04-25 15:15:06 +00:00