On Tue, Apr 01, 2025 at 07:15:46AM -0700, Andrea Bolognani via Devel wrote: > On Tue, Apr 01, 2025 at 04:00:42PM +0200, Alessandro wrote: > > On Tue, 1 Apr 2025 at 14:22, Andrea Bolognani <abolo...@redhat.com> wrote: > > > On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote: > > > > We attempted multiple ways to clean up dynamic files; however, we must > > > > preserve user overrides, which requires keeping the file > > > > /etc/apparmor.d/libvirt/libvirt-uuid > > > > > > > > This commit proposes to move user overrides into > > > > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present, > > > > unconditionally. When we stop the domain, we remove libvirt.uuid and > > > > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present. > > > > > > The way you describe things, it sounds like the AppArmor driver > > > already expects local overrides to exist. Is that documented > > > anywhere? If so, an update is probably needed. And either way, this > > > file you're introducing and its purpose will have to be documented. > > > > Thank you for your remark, Andrea. > > AFAICT, it's documented here > > https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage > > and in docs/drvqemu.rst. If my proposal is accepted, I'll update those > > pages accordingly with a separate patch, clearly stating that the > > behaviour has changed and the user overrides must be saved into the > > /etc/apparmor.d/libvirt/libvirt-uuid.local file. > > I don't know if I can modify the Gitlab wiki's sending a patch though :) > > Probably not, but that's something that we don't have control over > anyway so you'll need to figure it out together with the maintainers > of the AppArmor project. > > For libvirt, the documentation should be updated at the same time as > the code it refers to is changed, meaning you should do so as part of > your patch rather than as a follow-up. > > The fact that the profile itself now gets deleted when the domain is > undefined is a pretty significant behavioral change that IMO needs to > be mentioned in the release notes too. > > More importantly, I'm not convinced that you can just start deleting > that file unconditionally. Since, as you note, it's explicitly > documented that the user might add local customizations to the > profile, deleting it might result in the loss of those local changes. > > So I think you need something more like:> > expected = generate_default_profile(); > actual = read_existing_profile(); > if (STREQ(actual, expected)) { > delete_profile(); > } > > In other words, only delete the existing profile if you can prove > that it contains no valuable information.
Not sure its that's simple as we've changed what's in the profile over time, especially if the host in question upgraded from apparmor 2.x to 3.x, so such a comparison is likely to get false results a decent amount of the time. It might need to be more of a config file setting to allow people turn off, and/or a build time default if a distro really wants to preserve old behaviour a bit longer With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
