Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[Paul Gevers]

Hi,

On 08-03-2019 09:38, Amos Jeffries wrote:
> On 8/03/19 6:58 am, Paul Gevers wrote:
>> On 07-03-2019 17:51, Luigi Gangitano wrote:
>>> 4.6-2 has just been uploaded with a fix for 923213. I really appreciate
>>> the opportunity to let it in Buster.
>>
>> Please file an unblock bug. I have one question about the proposed
>> solution: you preinst doesn't honor local changes if the package is
>> uninstalled (but not purged) and installed again AFAICT.
> 
> How so? the preinst script only installs the disable symlink and skips
> doing so if there is any existing profile. Everything else is done by
> the debhelper script.

Because it does $(ln -sf) during install, even if the admin removed that
link manually. If the package wasn't purged, the package must honor
those changes by the admin in /etc/.

> This particular chunk of the change is taken verbatim from what Ubuntu
> have been using for years without an issue coming up.

That may be true, it doesn't mean it is policy correct.

On top of that, the autopkgtest fails, so you didn't fix it correctly.

I suggest to revert the changes in the latest upload, disable the
failing test, and I'll unblock the package (so the only delta with
testing should be the disabled test).

Paul



signature.asc
Description: OpenPGP digital signature

Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[Amos Jeffries]

On 8/03/19 6:58 am, Paul Gevers wrote:
> Luigi,
> 
> On 07-03-2019 17:51, Luigi Gangitano wrote:
>> 4.6-2 has just been uploaded with a fix for 923213. I really appreciate
>> the opportunity to let it in Buster.
> 
> Please file an unblock bug. I have one question about the proposed
> solution: you preinst doesn't honor local changes if the package is
> uninstalled (but not purged) and installed again AFAICT.


How so? the preinst script only installs the disable symlink and skips
doing so if there is any existing profile. Everything else is done by
the debhelper script.

This particular chunk of the change is taken verbatim from what Ubuntu
have been using for years without an issue coming up.

Amos



signature.asc
Description: OpenPGP digital signature

Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[Paul Gevers]

Luigi,

On 07-03-2019 17:51, Luigi Gangitano wrote:
> 4.6-2 has just been uploaded with a fix for 923213. I really appreciate
> the opportunity to let it in Buster.

Please file an unblock bug. I have one question about the proposed
solution: you preinst doesn't honor local changes if the package is
uninstalled (but not purged) and installed again AFAICT. I stopped
reviewing there and like others to be able to chime in as well.

Personally I rather have just the test disabled or ignored. I feel
slightly uncomfortable with the proposed changes.

Paul



signature.asc
Description: OpenPGP digital signature

Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[Luigi Gangitano]

Hi Paul,

> On Mon, 25 Feb 2019 07:31:42 +0100 intrig...@debian.org 
>  wrote:
>> If I got it right, on Debian we don't ship
>> /etc/apparmor.d/usr.sbin.squid, so none of the tests in the
>> test_zz_apparmor function are meaningful in this context.
>> So I think this entire test case should be skipped on Debian.
> 
> I have decided to accept this regression to migrate to buster for
> apparmor to migrate to buster. Targeted fixes to fix the squid
> autopkgtest can be reason for an unblock.

4.6-2 has just been uploaded with a fix for 923213. I really appreciate the 
opportunity to let it in Buster.

Regards,

L

--
Luigi Gangitano -- mailto:lu...@debian.org>> -- 
mailto:gangit...@lugroma3.org>>
GPG: 1024D/924C0C26: 12F8 9C03 89D3 DB4A 9972  C24A F19B A618 924C 0C26
GPG: 4096R/2BA97CED: 8D48 5A35 FF1E 6EB7 90E5  0F6D 0284 F20C 2BA9 7CED

Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[Paul Gevers]

Hi all,

On Mon, 25 Feb 2019 07:31:42 +0100 intrig...@debian.org wrote:
> If I got it right, on Debian we don't ship
> /etc/apparmor.d/usr.sbin.squid, so none of the tests in the
> test_zz_apparmor function are meaningful in this context.
> So I think this entire test case should be skipped on Debian.

I have decided to accept this regression to migrate to buster for
apparmor to migrate to buster. Targeted fixes to fix the squid
autopkgtest can be reason for an unblock.

Paul



signature.asc
Description: OpenPGP digital signature

Bug#923213: squid: Please disable the "Test apparmor" autopkgtest

[intrigeri]

Source: squid
Severity: normal
Version: 4.6-1

Hi,

it seems that test_zz_apparmor only passed on Debian previously
because it implicitly relied on a bug in apparmor_parser,
which used to return 0 if called on a non-existent profile.

This bug was fixed in apparmor 2.13.2-8, which broke squid's
autopkgtests:
https://ci.debian.net/data/autopkgtest/testing/amd64/s/squid/1998185/log.gz

The corresponding code is:

# Verify it is syntactically correct
ret, report = cmd(['apparmor_parser', '-p', self.aa_abs_profile])
expected = 0
result = 'Got exit code %d, expected %d\n' % (ret, expected)
self.assertEquals(ret, expected, result + report)

… and the parser now returns exit code 2, because
/etc/apparmor.d/usr.sbin.squid does not exist. Previously the test
would happily pretend that there's a syntactically correct AppArmor
profile, while there's no such profile at all.

If I got it right, on Debian we don't ship
/etc/apparmor.d/usr.sbin.squid, so none of the tests in the
test_zz_apparmor function are meaningful in this context.
So I think this entire test case should be skipped on Debian.

Cheers,
-- 
intrigeri