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

treefile: Add option to label /usr/etc as /etc #4640

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

cgwalters
Copy link
Member

Depends on ostreedev/ostree#3063

This is intended for use with ostreedev/ostree#2868 but is conceptually orthogonal to it; we probably want to try switching to this by default actually.

@openshift-ci
Copy link

openshift-ci bot commented Oct 2, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cgwalters
Copy link
Member Author

Running the coreos kola tests against this, ext.config.extensions.package fails with
Oct 02 15:22:01 qemu0 kola-runext-package[1876]: error: Changed directories are not supported yet

because we need to make client side layering also auto-inherit this behavior if present in the base image.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Not strictly required, but would be good to add a substitution rule for it too so that SELinux userspace tooling is aware of this too. (Could probably just add it as part of postprocess_subs_dist().)

docs/treefile.md Outdated
@@ -138,6 +138,10 @@ It supports the following parameters:
to the UNIX epoch time to be used as the build timestamp. Currently only
fully supports the `bdb` backend. Somewhat experimental.

* `label-usretc-as-etc`: boolean, optional: If enabled, this sets the SELinux label for
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we even need a new knob for this. Should be safe to always do this, right? Since we basically own /usr/etc anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm being conservative here by making it opt in (both here and on the ostree side).

Dunno. I guess we could try to actually do it by default ostree side, and add an option to disable it in case we regress things somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a nontrivial change though, and so far we've been really conservative with options that should be on by default but aren't (sysroot.readonly is a good example) to ensure that updating ostree doesn't break anything.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm less sure about making it default in libostree. I think it makes sense to make it opt in there. For rpm-ostree, it seems safer to do but cool of course to be conservative there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think making it opt-in in ostree. but always enable in rpm-ostree (possibly with an opt-out) is the right approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's painful every time we add an opt-in for sure because we end up having to update so many different manifest/treefiles (coreos vs edge vs atomic desktop etc etc.)

However I still just feel we need to be conservative because this can easily have non-obvious effects.

Even on-by-default with a knob to turn it off gets tricky because then the build system requires a new rpm-ostree (old ones will barf on the unexpected field).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not only that, we need a new version of osbuild, and any other tools that call out to rpm-ostree to compose a commit.

@alexlarsson
Copy link
Collaborator

So, all the base stuff in ostree has landed, maybe time to un-draft this?

@cgwalters
Copy link
Member Author

Well, we have the annoying thing that we have to wait through the new ostree to go all the way through the fedora 7 days bodhi update right now.

I just did coreos/fedora-coreos-config#2684
which should cut that down to one day at least...

@cgwalters cgwalters marked this pull request as ready for review October 24, 2023 14:43
@cgwalters cgwalters force-pushed the label-usretc-as-etc branch 3 times, most recently from 5dfdf32 to a70e776 Compare October 24, 2023 16:59
@cgwalters
Copy link
Member Author

Ah right, the ostree update hasn't made it out to c9s prod. Let's split out that hack to #4672

Man, it's times like this that really makes one feel actively punished for splitting things out into multiple components...

(Or of course, using Nix where we aren't playing "rpmfind" but are just referencing source code that gets built if not present in the build cache...)

Prep for new release.

osid="$(. /etc/os-release && echo $ID)"
if [ "${osid}" == centos ]; then
dnf -y update https://kojihub.stream.centos.org/kojifiles/packages/ostree/2023.7/2.el9/$(arch)/ostree-{,libs-,devel-}2023.7-2.el9.$(arch).rpm

Check warning

Code scanning / shellcheck

Quote this to prevent word splitting. Warning

Quote this to prevent word splitting.

osid="$(. /etc/os-release && echo $ID)"
if [ "${osid}" == centos ]; then
dnf -y update https://kojihub.stream.centos.org/kojifiles/packages/ostree/2023.7/2.el9/$(arch)/ostree-{,libs-,devel-}2023.7-2.el9.$(arch).rpm

Check warning

Code scanning / shellcheck

Quote this to prevent word splitting. Warning

Quote this to prevent word splitting.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM overall. Thoughts on #4640 (review) ?

tests/compose/test-misc-tweaks.sh Show resolved Hide resolved
Depends on ostreedev/ostree#3063

This is intended for use with ostreedev/ostree#2868
but is conceptually orthogonal to it; we probably want to try
switching to this by default actually.
@cgwalters
Copy link
Member Author

Not strictly required, but would be good to add a substitution rule for it too so that SELinux userspace tooling is aware of this too. (Could probably just add it as part of postprocess_subs_dist().)

Hmmm. I think what's a bit weird about that is logically nothing else should put things in /usr/etc. It's effectively an ostree implementation detail. It's hence not totally clear to me what we'd gain by doing that then.

That said I had completely forgotten that postprocess_subs_dist existed. I feel like it'd be better for it not to exist at all? Hmm, why did we not land the /home -> /var/home equivalency in upstream policy?

@jlebon
Copy link
Member

jlebon commented Oct 25, 2023

Not strictly required, but would be good to add a substitution rule for it too so that SELinux userspace tooling is aware of this too. (Could probably just add it as part of postprocess_subs_dist().)

Hmmm. I think what's a bit weird about that is logically nothing else should put things in /usr/etc. It's effectively an ostree implementation detail. It's hence not totally clear to me what we'd gain by doing that then.

One concrete example is doing usroverlay and running restorecon on /usr/etc would mess up all the labels again.

That said I had completely forgotten that postprocess_subs_dist existed. I feel like it'd be better for it not to exist at all? Hmm, why did we not land the /home -> /var/home equivalency in upstream policy?

The history there is messy, but I think we do still need it. IIRC this is related to the /etc/default/useradd hack we do. That lives here so it makes sense to have this live alongside it. Lots more context in https://src.fedoraproject.org/rpms/selinux-policy/pull-request/14 and #1754.

This avoids the unnecessary `b` for bytestrings and literal `\n`
newlines.
This ensures file labeling equivalency outside of ostree too.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM!

@cgwalters cgwalters merged commit 8c83311 into coreos:main Oct 25, 2023
16 of 17 checks passed
alexlarsson added a commit to alexlarsson/osbuild that referenced this pull request Oct 27, 2023
This is a feature that was added in rpm-ostree 2023.10 and is needed
for the new transient /etc feature to work. What it does is change the
labeling of /usr/etc to match those of /etc, so that /usr/etc can be used
directly as a bind-mount or an overlay mount when mounted on /etc.

See coreos/rpm-ostree#4640 for details.
cgwalters pushed a commit to osbuild/osbuild that referenced this pull request Oct 28, 2023
This is a feature that was added in rpm-ostree 2023.10 and is needed
for the new transient /etc feature to work. What it does is change the
labeling of /usr/etc to match those of /etc, so that /usr/etc can be used
directly as a bind-mount or an overlay mount when mounted on /etc.

See coreos/rpm-ostree#4640 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants