[Freeipa-devel] [PATCH 477] spec file: Add Requires(pre) on selinux-policy

[Martin Kosek]

On 08/25/2015 05:37 PM, Alexander Bokovoy wrote:
> On Tue, 25 Aug 2015, Martin Kosek wrote:
>> On 08/25/2015 04:37 PM, Jan Cholasta wrote:
>>> On 25.8.2015 14:50, Alexander Bokovoy wrote:
>>>> On Tue, 25 Aug 2015, Jan Cholasta wrote:
>>>>> On 25.8.2015 14:23, Alexander Bokovoy wrote:
>>>>>> On Tue, 25 Aug 2015, Jan Cholasta wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> the attached patch fixes
>>>>>>> <https://fedorahosted.org/freeipa/ticket/5256>.
>>>>>>>
>>>>>>> Honza
>>>>>>>
>>>>>>> --
>>>>>>> Jan Cholasta
>>>>>>
>>>>>>> From 216be8de30747f80f490d4e91a7cca4af3e767d6 Mon Sep 17 00:00:00 2001
>>>>>>> From: Jan Cholasta <jcholast at redhat.com>
>>>>>>> Date: Tue, 25 Aug 2015 14:14:25 +0200
>>>>>>> Subject: [PATCH] spec file: Add Requires(pre) on selinux-policy
>>>>>>>
>>>>>>> This prevents ipa-server-upgrade failures on SELinux AVCs because of
>>>>>>> old
>>>>>>> selinux-policy version.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/5256
>>>>>>> ---
>>>>>>> freeipa.spec.in | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>>>>>>> index cba91fe..fd73cda 100644
>>>>>>> --- a/freeipa.spec.in
>>>>>>> +++ b/freeipa.spec.in
>>>>>>> @@ -139,6 +139,7 @@ Requires: systemd-units >= 38
>>>>>>> Requires(pre): shadow-utils
>>>>>>> Requires(pre): systemd-units
>>>>>>> Requires(post): systemd-units
>>>>>>> +Requires(pre): selinux-policy >= %{selinux_policy_version}
>>>>>>> Requires: selinux-policy >= %{selinux_policy_version}
>>>>>>> Requires(post): selinux-policy-base
>>>>>>> Requires: slapi-nis >= 0.54.2-1
>>>>>> If we have it in Requires(pre), we don't need it in Requires, as
>>>>>> Requires(pre) is a superset of guarantees that Requires gives you.
>>>>>
>>>>> Martin (CCed) told me Requires(pre) does not imply Requires.
>>>> See http://rpm.org/api/4.4.2.2/tsort.html (available since 2007):
>>>> ----------------
>>>> Since the only way out of a dependency loop is to snip the loop
>>>> somewhere, rpm uses hints from Requires: dependencies to distinguish
>>>> co-requisite (these are not needed to install, only to use, a package)
>>>> from pre-requisite (these are guaranteed to be installed before the
>>>> package that includes the dependency) relations.
>>>> ----------------
>>>>
>>>>>>
>>>>>> Requires(pre) ensures that selinux-policy of specific version is
>>>>>> installed before pre scripts of freeipa-server would run, be it in the
>>>>>> same transaction or in a previous one.
>>>>>>
>>>>>
>>>>> Hmm, ipa-server-upgrade is run in posttrans. Should the Requires(pre)
>>>>> be changed to Required(posttrans)?
>>>> I don't think there is posttrans target. Perhaps, we can just make sure
>>>> Requires(post) is enough.
>>>
>>> OK, let's try that. Updated patch attached.
>>>
>>
>> Will this really make a difference? I thought the problem is caused by
>> selinux-policy being installed after freeipa-server package upgrade. We already
>> have Requires on selinux-policy, so I am not sure what is actually changed by
>> this patch.
> The change is that with Requires(pre) or Requires(post) we are
> guaranteed that selinux-policy is installed and available before our pre
> or post scriptlets are run. With Requires only we are not guaranteed to
> be installed after selinux-policy, only that it would be available as
> part of the same transaction we are installed in.
>
> We don't really need to have Requires(pre) because we don't rely on
> selinux-policy being available in pre scriptlet. Forcing Requires(pre)
> doesn't help anyone else (rpm/yum/dnf need to solve dependency loops and
> we are only complicating with Requires(pre) if we don't actually need
> it). Thus, choosing Require(post) is more correct from distribution
> point of view.

Sure, but given that FreeIPA upgrade is run in the posttrans phase:

%posttrans server
# This must be run in posttrans so that updates from previous
# execution that may no longer be shipped are not applied.
/usr/sbin/ipa-server-upgrade --quiet >/dev/null || :

I am now not sure how Requires(pre) or Requires(post) help here, in all cases, 
the right selinux-policy should be there before all the posttrans scripts are 
being run.

Maybe the new selinux-policy is required for certmonger itself or some other 
event during upgrade?