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

Wed Aug 26 06:20:11 UTC 2015

On 26.8.2015 08:16, Alexander Bokovoy wrote:
> On Wed, 26 Aug 2015, Jan Cholasta wrote:
>> On 25.8.2015 21:25, Martin Kosek wrote:
>>> On 08/25/2015 09:22 PM, Alexander Bokovoy wrote:
>>>> On Tue, 25 Aug 2015, Martin Kosek wrote:
>>>>> On 08/25/2015 08:59 PM, Alexander Bokovoy wrote:
>>>>>> On Tue, 25 Aug 2015, Martin Kosek wrote:
>>>>>>> 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.
>>>>>> I've looked at the rpm source code and here is the list of all
>>>>>> supported
>>>>>> requires/dependencies types:
>>>>>> https://github.com/rpm-software-management/rpm/blob/140744377b019e0de81d76d0931c32228d2ed57e/build/rpmfc.c#L1056
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Requires(posttrans) is there so we could use this one too but it was
>>>>>> added only in 4.12-alpha which means it is missing in RHEL/CentOS 7,
>>>>>> for
>>>>>> example, as they are only up to 4.11.
>>>>>>> Maybe the new selinux-policy is required for certmonger itself or
>>>>>>> some other
>>>>>>> event during upgrade?
>>>>>> No, I don't think so. However, we cannot set Requires(posttrans),
>>>>>> thus
>>>>>> we should be using closest target before it, i.e. Requires(post).
>>>>>
>>>>> Thank you, but I think I still did not get an answer for my question.
>>>>>
>>>>> IIUC, the rough rpm process with regards to freeipa-server package
>>>>> upgrade,
>>>>> it should be in this order:
>>>>>
>>>>> _
>>>>> |
>>>>> v
>>>>> RPM installs some dependencies of freeipa-server
>>>>> |
>>>>> V
>>>>> RPM installs "Requires(pre)" of freeipa-server
>>>>> freeipa-server pre scriptlet runs
>>>>> |
>>>>> v
>>>>> RPM installs freeipa-server
>>>>> |
>>>>> v
>>>>> RPM installs "Requires(post)" of freeipa-server
>>>>> freeipa-server post scriptlet runs
>>>>> |
>>>>> v
>>>>> RPM installs some dependencies of freeipa-server
>>>>> |
>>>>> v
>>>>> RPM executes posttrans scriptlets, including "ipa-server-upgrade".
>>>> The flow above is not correct. Each scriptlet of the package is
>>>> executed
>>>> when package is installed. In particular, there is no period of waiting
>>>> until end of whole transaction to start executing %posttrans
>>>> scriptlet of
>>>> a specific package. RPM only guarantees you that %posttrans
>>>> scriptlet is
>>>> executed as the last thing of this package intall, after all
>>>> %post/%postun scriptlets were executed for this package and all
>>>> triggers
>>>> for affected packages were executed.
>>>
>>> Ah, so this was the key misunderstanding I was having. I thought that
>>> all posttrans scripts for all packages are run in the end.
>>>
>>>>
>>>> This does not guarantee that selinux-policy processing would be before
>>>> freeipa-server processing unless we explicitly ask for the ordering via
>>>> Requires(<stage>) tag. The order of processing packages is affected by
>>>> Requires(<stage>) tag, but processing of each single package is still
>>>> sequential.
>>>>
>>>>> My question is, if all the magic happens in the last step, how does
>>>>> adding
>>>>> (pre) or (post) Requires help, given we already have the "normal"
>>>>> Requires?
>>>> Because with Requires nothing guarantees selinux-policy is
>>>> installed before freeipa-server in the same transaction, we have to use
>>>> Requires(<stage>) that would ensure we have the selinux-policy
>>>> installation/upgrade completed by the time that stage is reached in
>>>> freeipa-server installation/upgrade process. In reality
>>>> Requires(pre) is
>>>> often enough too but not recommended as it also could cause loop
>>>> breaking issues.
>>>>
>>>
>>> Thanks for the explanation. If this is the case, the patch should be OK
>>> as is indeed and Requires on post should be enough.
>>
>> OK. Is this an ACK?
> At least from my side it is ACK.

Thanks for the review.

Pushed to:
master: aebb72e1fb144939285380a6a9261c4d4177195e
ipa-4-2: 94adf097ec22b8b71ba339a9619c891f4d515ecd

-- 
Jan Cholasta