Project

General

Profile

Bug #12617

We don't detect when our Tor Browser AppArmor profile patch is fuzzy and leaves a .orig file

Added by anonym over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Build system
Target version:
Start date:
05/30/2017
Due date:
% Done:

100%

Feature Branch:
bugfix/12617-fuzzy-patch-detection-improvements
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

The patch currently (on devel) leaves a /etc/apparmor.d/torbrowser.orig file. It has some surprising consequences, the first being that it leads to the following for apparmor.service:

May 30 17:31:54 amnesia apparmor[14777]: AppArmor parser error for /etc/apparmor.d/torbrowser.orig in /etc/apparmor.d/torbrowser.orig at line 97: Could not open 'local/torbrowser.Browser.firefox'

So the patch must be refreshed. However...

This in turn breaks all automated tests using Tor, since the Tor is ready step runs systemctl is-system-running. The placement of that check in that step is my second issue: it doesn't belong. It should be moved into a dedicated step, run in a scenario about the health of the system. A failure would then in itself immediately signal something quite descriptive about the actual failure, instead of falsely signaling that something about little-t tor is utterly broken, blocking a huge part of the test suite.

Lastly, my third problem: given how we order our build hooks, i.e.:

config/chroot_local-hooks/01-check-for-dot-orig-files
config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile

we don't detect this source of fuzzy patches (but it's the only relevant one at the moment). IIRC we had a reason for this at some point, but I cannot recall or rediscover it. Any ideas? Otherwise I propose that we move this hook to one of the very last ones, like 99-zzz_check-for-dot-orig-files. To avoid frequent FTBFS we should add it to DOT_ORIG_WHITELIST however.

(BTW: shouldn't we rm -f ${DOT_ORIG_WHITELIST}? Why would we want to ship crap? Assuming, of course, that these files don't matter, which I might be wrong about.)


Related issues

Duplicated by Tails - Bug #11233: Build doesn't abort when Tor Browser AppArmor patches does not apply cleanly Duplicate 03/14/2016

Associated revisions

Revision 661a2ac1 (diff)
Added by anonym over 2 years ago

Also check for fuzzy patches' .orig files at the end of our build hooks.

The current position is useful to detect fuzzy patches from
config/chroot_local-patches but won't detect any fuzzy patches applied
among later hooks. So let's re-run the check at the end of our build
hooks.

Refs: #12617

Revision 3cad513c (diff)
Added by anonym over 2 years ago

Don't FTBFS when the torbrowser AppArmor profile patch is fuzzy.

That patch becomes fuzzy too often.

Refs: #12617

Revision 60ef4e4d (diff)
Added by anonym over 2 years ago

Remove .orig files for patches we allow to be fuzzy.

We still shouldn't ship crap on our ISO.

Will-fix: #12617

Revision fa01f0c7 (diff)
Added by anonym over 2 years ago

Revert "Don't FTBFS when the torbrowser AppArmor profile patch is fuzzy."

This reverts commit 3cad513c7c9d16daa9734009b5ee4ca1b7dc042b.

Refs: #12617

Revision ab358afa (diff)
Added by anonym over 2 years ago

Refresh torbrowser-AppArmor-profile.patch against torbrowser-launcher 0.2.7-2.

Will-fix: #12617

Revision 92aaf651 (diff)
Added by intrigeri over 2 years ago

Revert "Refresh torbrowser-AppArmor-profile.patch against torbrowser-launcher 0.2.7-2."

This reverts commit ab358afa8e34dd8c8eb7e83f0e9f5be121962056.

refs: #12617
Our testing branch still uses the AppArmor profile that comes with
torbrowser-launcher 0.2.6-3.1, so the commit I'm reverting breaks
the build of this branch.

Revision cf5a2f66
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/bugfix/12617-fuzzy-patch-detection-improvements' into testing (Fix-committed: #12617)

History

#1 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri
  • QA Check set to Info Needed

I think this should be fixed until 3.0 and I volunteer to do so as soon as we have an agreement about the the problems and the proposed solutions in the description.

#2 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Info Needed to Dev Needed

This in turn breaks all automated tests using Tor, since the Tor is ready step runs systemctl is-system-running. The placement of that check in that step is my second issue: it doesn't belong. It should be moved into a dedicated step, run in a scenario about the health of the system. A failure would then in itself immediately signal something quite descriptive about the actual failure, instead of falsely signaling that something about little-t tor is utterly broken, blocking a huge part of the test suite.

Well, IMO it's good that we run systemctl is-system-running as often as we can, before testing other stuff, to identify issues as early as possible. IIRC we can't run it for offline scenarios since this check will fail if Tor hasn't started or bootstrapped. But I agree the current way this is implemented gives suboptimal feedback. Perhaps renaming the "Tor is ready" step to describe more accurately what we test in it would help. If we don't easily agree on a solution here, let's maybe move this to a dedicated ticket: it's definitely not a 3.0 blocker IMO so I'd like to focus on the fuzzy patch issue.

Lastly, my third problem: given how we order our build hooks, i.e.:

> config/chroot_local-hooks/01-check-for-dot-orig-files
> config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile
> 

we don't detect this source of fuzzy patches (but it's the only relevant one at the
moment). IIRC we had a reason for this at some point, but I cannot recall or
rediscover it.

We do it this way to fail earlier, and report something clear about why the build fails, when one of our chroot_local-patches is fuzzy: it's plausible that a hook can fail due to a fuzzy patch, and then we would be left wondering what's going on if we did this check later.

Otherwise I propose that we move this hook to one of the very last ones,

I think we should run it both very early and very late. Perhaps a symlink would do the job?

To avoid frequent FTBFS we should add it to DOT_ORIG_WHITELIST however.

"it" == ?

(BTW: shouldn't we rm -f ${DOT_ORIG_WHITELIST}? Why would we want to ship crap? Assuming, of course, that these files don't matter, which I might be wrong about.)

Good idea.

#3 Updated by anonym over 2 years ago

  • Subject changed from When our Tor Browser AppArmor profile patch is fuzzy all Tor-related automated tests break to We don't detect when our Tor Browser AppArmor profile patch is fuzzy and leaves a .orig file
  • Status changed from Confirmed to In Progress
  • Assignee changed from anonym to intrigeri
  • % Done changed from 0 to 50
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch set to bugfix/12617-fuzzy-patch-detection-improvements

intrigeri wrote:

This in turn breaks all automated tests using Tor, since the Tor is ready step runs systemctl is-system-running. The placement of that check in that step is my second issue: it doesn't belong. It should be moved into a dedicated step, run in a scenario about the health of the system. A failure would then in itself immediately signal something quite descriptive about the actual failure, instead of falsely signaling that something about little-t tor is utterly broken, blocking a huge part of the test suite.

Well, IMO it's good that we run systemctl is-system-running as often as we can, before testing other stuff, to identify issues as early as possible. IIRC we can't run it for offline scenarios since this check will fail if Tor hasn't started or bootstrapped. But I agree the current way this is implemented gives suboptimal feedback. Perhaps renaming the "Tor is ready" step to describe more accurately what we test in it would help. If we don't easily agree on a solution here, let's maybe move this to a dedicated ticket: it's definitely not a 3.0 blocker IMO so I'd like to focus on the fuzzy patch issue.

Yeah, I don't agree at all with your current argument so let's not count on a quick agreement. I created #12621, so let's focus on the fuzzy patch issue in this ticket (which I renamed accordingly).

Lastly, my third problem: given how we order our build hooks, i.e.:
[...]
we don't detect this source of fuzzy patches (but it's the only relevant one at the
moment). IIRC we had a reason for this at some point, but I cannot recall or
rediscover it.

We do it this way to fail earlier, and report something clear about why the build fails, when one of our chroot_local-patches is fuzzy: it's plausible that a hook can fail due to a fuzzy patch, and then we would be left wondering what's going on if we did this check later.

True.

Otherwise I propose that we move this hook to one of the very last ones,

I think we should run it both very early and very late. Perhaps a symlink would do the job?

Yes, brilliant!

To avoid frequent FTBFS we should add it to DOT_ORIG_WHITELIST however.

"it" ?

"it" /etc/apparmor.d/torbrowser.orig

The current branch should fix all issues this ticket now is about. Please review'n'merge!

#4 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

I think you forgot to push this branch.

#5 Updated by intrigeri over 2 years ago

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

#6 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Info Needed

I dislike 3cad513c7c9d16daa9734009b5ee4ca1b7dc042b: IMO failing hard and early is the best way to make us (Foundations Team) notice when something is wrong, and the way it's done currently makes it trivial to understand the root cause of the problem (we receive a build notification failure, scroll to the bottom, and then it's clearly written what's wrong). I wonder where the "That patch becomes fuzzy too often" rationale comes from: before the last unfuzzy-ing in May 2017, last times we had to do it were in March and July 2016, so perhaps it's not that big a burden and doesn't warrant building known-bad ISOs just for the sake of avoiding FTBFS?

Other than that, code review passes!

#7 Updated by anonym over 2 years ago

  • QA Check changed from Info Needed to Dev Needed

intrigeri wrote:

I dislike 3cad513c7c9d16daa9734009b5ee4ca1b7dc042b: IMO failing hard and early is the best way to make us (Foundations Team) notice when something is wrong, and the way it's done currently makes it trivial to understand the root cause of the problem (we receive a build notification failure, scroll to the bottom, and then it's clearly written what's wrong). I wonder where the "That patch becomes fuzzy too often" rationale comes from: before the last unfuzzy-ing in May 2017, last times we had to do it were in March and July 2016, so perhaps it's not that big a burden and doesn't warrant building known-bad ISOs just for the sake of avoiding FTBFS?

I'm worrying about when the signal to noise ratio becomes too high, but I now realize this should only affect devel (since it follows the latest snapshots). So I agree, I'll revert that commit, and push a refreshed patch as a fix for this ticket instead.

#8 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri
  • % Done changed from 50 to 60
  • QA Check changed from Dev Needed to Ready for QA

Done!

#9 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

I'll have a look once it builds (again) on Jenkins: I'm sorry to be painful but I'm tired of dealing with #12611 myself on everyone's topic branches.

#10 Updated by intrigeri over 2 years ago

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

#11 Updated by intrigeri over 2 years ago

  • % Done changed from 60 to 70

Code review passes, waiting for a build.

#12 Updated by intrigeri over 2 years ago

The branch doesn't build so I did 92aaf651fed2346bf01ef36767c80962fbd2d20f.

#13 Updated by anonym over 2 years ago

Ah, ok. Well, I don't see any other solution than:

  1. merge the branch as-is (with your revert) into testing
  2. merge testing into devel
  3. revert your revert in devel
  4. when you bump the snapshots in testing, cherry-pick the commit from the previous step

#14 Updated by intrigeri over 2 years ago

Ah, ok. Well, I don't see any other solution than:

That's what I did, indeed.

  1. when you bump the snapshots in testing, cherry-pick the commit from the previous step

Right, unless I first merge devel into testing as we would usually do at freeze/RC time (as written elsewhere, I don't think we can pretend we froze testing 2 weeks ago).

#15 Updated by intrigeri over 2 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 70 to 100

#16 Updated by intrigeri over 2 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#17 Updated by intrigeri over 2 years ago

  • Duplicated by Bug #11233: Build doesn't abort when Tor Browser AppArmor patches does not apply cleanly added

#18 Updated by intrigeri over 2 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF