Tails

#1 Updated by intrigeri 2013-12-29 03:20:40

• Category set to Infrastructure

#2 Updated by intrigeri 2015-05-28 15:43:07

• blocks deleted (Feature #6296: Configure reprepro to pull from foreign APT repositories)

#3 Updated by intrigeri 2015-05-28 15:43:21

• Parent task changed from Feature #5926 to Feature #9489

#4 Updated by intrigeri 2015-05-28 15:43:43

• Assignee set to intrigeri
• Target version changed from Sustainability_M1 to Tails_2.2

#5 Updated by intrigeri 2015-05-28 15:44:10

• blocks #8668 added

#6 Updated by intrigeri 2015-05-28 15:44:54

• Description updated

#7 Updated by intrigeri 2015-05-28 15:51:07

• blocks deleted (Feature #6297: Save list of packages used at ISO build time)

#8 Updated by intrigeri 2015-05-28 15:53:41

• blocks deleted (Feature #6303: Adapt our infrastructure to be able to handle tons of packages)

#9 Updated by intrigeri 2015-08-26 05:59:48

• Deliverable for set to 269

#10 Updated by intrigeri 2015-12-13 12:28:43

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 10

This is done in https://git-tails.immerda.ch/puppet-tails/tree/manifests/reprepro/snapshots, and a PoC deployment lives on misc.lizard, but it can’t be fully deployed yet as we lack disk space (Feature #6303) and garbage collection mechanisms (see the todo list on the blueprint).

#11 Updated by intrigeri 2016-02-05 20:49:40

• Target version changed from Tails_2.2 to Tails_2.3

#12 Updated by intrigeri 2016-04-08 14:17:52

• blocked by deleted (Feature #6310: Publish source Debian packages used by Tails ISO images)

#13 Updated by intrigeri 2016-04-25 13:01:08

• Target version changed from Tails_2.3 to Tails_2.4

#14 Updated by intrigeri 2016-05-10 02:56:50

• Assignee changed from intrigeri to bertagaz
• % Done changed from 10 to 50
• QA Check set to Ready for QA

bertagaz, do you want to review the Puppet code and Perl scripts, or?

#15 Updated by bertagaz 2016-05-22 14:18:05

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

For the perl code, I’m maybe not the best person, given that the two that wrote them are very much knowledgeable than me. I guess the back and forth you did on some of the scripts is already quite enough.

I did review the Puppet code though. Nice code, learned some stuffs while reading. :)

Here’s what I found:

• tails::reprepro::custom
• source parameters of the file types are arrays. That’s not really how we declare them usually.
• According to upstream, vcsrepo supports mirror in later version. Should be noted somewhere maybe that we should switch to it once updated (comment maybe).
• "tails-update-reprepro-config" cron require parameter could be simplified. Too many ‘[’ ‘]’.

• tails::reprepro::custom::nginx
• No validation, guess because it’s not supposed to be declared but rather only used internally, but then private() is missing.
• Lint: exec name could be single quoted
• exec command is maybe missing single quotes around the arguments containing variables.

• tails::reprepro::snapshots::base
• requires a private() too maybe?

• tails::reprepro::snapshots::secret_keys
• "tails-reprepro-import-private-keys_${name}" exec command param has private key path hardcoded. Shouldn’t it be ${homedir}/private-keys.asc rather? If not, then single quotes are enough. Same for the cron declaration right below.

• tails::reprepro::snapshots::tagged
• $prepare_tagged_snapshot_import_pkg_deps packages are not quoted.
• Missing ensure param for nginx::vhostsd?

• tails::reprepro::snapshots::time_based
• Pkg dependencies quoting is not consistent.
• Why not add $tails_compact_reprepro_db_pkg_deps to the union just above?

• tails::reprepro::snapshots::time_based::cronjob
• In "tails-update-time-based-apt-snapshots-${repository}-${id}" cron command param, date format uses escapes (e.g \%Y). This results in the puppetmaster complaining about "Unrecognised escape sequence '\%' in file /etc/puppet/environments/production/modules/tails/manifests/reprepro/snapshots/time_based/cronjob.pp at line 30" (same for line 43) when compiling the apt.lizard catalog. This warning can be seen in the puppet-git.lizard journal when running the agent on apt.lizard. It results in the serial being written as "$(date '+\%Y\%m\%d')03" in the cronjob (see apt.lizard:/var/spool/cron/crontabs/reprepro-time-based-snapshots). That’s maybe not a problem as it’s just a serial, but worth fixing maybe. Specially as tails-update-time-based-apt-snapshots documents a different format (no escape char).
• Puppetmaster is also complaining when compiling apt.li catalog about "(Scope(Class[Tails::Reprepro::All])) Could not look up qualified variable 'tails::reprepro::snapshots::params::architectures'; class tails::reprepro::snapshots::params has not been evaluated". Not sure if it means troubles though, first time we have classes that use the ::params trick.

• tails::reprepro::snapshots::time_based::repository
• two ensure param :)
• unquoted mode param for File["${basedir}/key.asc"]

I’ve had a look at the apt-snapshots scripts too, here’s what I found (only minor and non-blocker though):

• tails-bump-apt-snapshot-valid-until
• echo and exit 1 calls could be factorize

• tails-compact-reprepro-db
• function print_stats is not used anywhere I can see

• tails-update-time-based-apt-snapshots
• function error unused too

#16 Updated by intrigeri 2016-05-23 11:33:35

• Assignee changed from intrigeri to bertagaz
• % Done changed from 50 to 70
• QA Check changed from Dev Needed to Info Needed

Thanks for this thorough review!

> For the perl code, I’m maybe not the best person, given that the two that wrote them are very much knowledgeable than me. I guess the back and forth you did on some of the scripts is already quite enough.

Sounds reasonable.

> * tails::reprepro::custom
> * source parameters of the file types are arrays. That’s not really how we declare them usually.

Right. Apparently this has propagated via copy’n’paste in this class since 2012, and I did not introduce this during this work (the file was merely renamed), but still, while we’re on it I’ve fixed this in commit b4ec82b.

> * According to upstream, vcsrepo supports mirror in later version. Should be noted somewhere maybe that we should switch to it once updated (comment maybe).

Good catch! Note that the mirror support is not part of an upstream release yet. Updated the comment accordingly in commit 3edead9.

> * "tails-update-reprepro-config" cron require parameter could be simplified. Too many ‘[’ ‘]’.

Yet another thing we’ve had for years, but still: fixed in commit 125d7c3. It’s good to polish our old stuff whenever an occasion presents itself, and that’s the case here :)

> * tails::reprepro::custom::nginx
> * No validation, guess because it’s not supposed to be declared but rather only used internally, but then private() is missing.

It’s been this way since 2012, but still: added private() in commit cd3ff50.

> * Lint: exec name could be single quoted

… since 2012, but: absolutely! Done this and some more linting in commit c654637.

> * exec command is maybe missing single quotes around the arguments containing variables.

… since 2012. That’s commit a6be452.

> * tails::reprepro::snapshots::base
> * requires a private() too maybe?

Sure! Commit 44e6e22.

> * tails::reprepro::snapshots::secret_keys
> * "tails-reprepro-import-private-keys_${name}" exec command param has private key path hardcoded. Shouldn’t it be ${homedir}/private-keys.asc rather? If not, then single quotes are enough. Same for the cron declaration right below.

Indeed! Fixed in commit 5fb983a, thanks!

(In passing: now, TBH this is a bit futile since tails_secrets_apt hard-codes the default value for $homedir, and thus doesn’t honor the custom value one might pass to tails::reprepro::snapshots::time_based. I’ve found this acceptable for the time being, since tails_secrets_apt is… secret, so there are basically no chances that someone combines it with tails::reprepro::snapshots::time_based called with a custom $homedir.)

> * tails::reprepro::snapshots::tagged
> * $prepare_tagged_snapshot_import_pkg_deps packages are not quoted.

Why do you think this is a problem? (That’s a genuine question :) My copy of puppet-lint is perfectly happy with that. Any reference documenting why it’s Bad™ practice?

> * Missing ensure param for nginx::vhostsd?

Good catch, thank you! Commit 15f62ba.

> * tails::reprepro::snapshots::time_based
> * Pkg dependencies quoting is not consistent.

Indeed. If you don’t mind, I’ll make them consistent once we’re done with the above discussion about whether we should quote them or not.

> * Why not add $tails_compact_reprepro_db_pkg_deps to the union just above?

I’ve tried it initially but then I realized it could not work (see stdlib doc: “`union`: This function returns a union of two arrays”), so 4689f4d reverted it.

Now, I realize this is confusing, so I’ve dropped the union() entirely in 2c94549.

> * tails::reprepro::snapshots::time_based::cronjob
> * In "tails-update-time-based-apt-snapshots-${repository}-${id}" cron command param, date format uses escapes (e.g \%Y). This results in the puppetmaster complaining about "Unrecognised escape sequence '\%' in file /etc/puppet/environments/production/modules/tails/manifests/reprepro/snapshots/time_based/cronjob.pp at line 30" (same for line 43) when compiling the apt.lizard catalog. This warning can be seen in the puppet-git.lizard journal when running the agent on apt.lizard. It results in the serial being written as "$(date '+\%Y\%m\%d')03" in the cronjob (see apt.lizard:/var/spool/cron/crontabs/reprepro-time-based-snapshots). That’s maybe not a problem as it’s just a serial, but worth fixing maybe.

In 785088b I merely made it work, but indeed it’s worth fixing => commit 4c535c1.

> Specially as tails-update-time-based-apt-snapshots documents a different format (no escape char).

That usage works perfectly fine for me, and I think it’s OK in the context of tails-update-time-based-apt-snapshots (no cron daemon passing the whole thing through a shell in its own way). Let’s special case only cases that are indeed special :)

> * Puppetmaster is also complaining when compiling apt.li catalog about "(Scope(Class[Tails::Reprepro::All])) Could not look up qualified variable 'tails::reprepro::snapshots::params::architectures'; class tails::reprepro::snapshots::params has not been evaluated". Not sure if it means troubles though, first time we have classes that use the ::params trick.

Great that you noticed that! In the current state of things it isn’t a problem in practice, but still, I’ve fixed that in commit c6e5bbe.

> * tails::reprepro::snapshots::time_based::repository
> * two ensure param :)

Commit 3a87241.

> * unquoted mode param for File["${basedir}/key.asc"]

Commit 2205b0f.

> I’ve had a look at the apt-snapshots scripts too, here’s what I found (only minor and non-blocker though):

> * tails-bump-apt-snapshot-valid-until
> * echo and exit 1 calls could be factorize

Commit 34f2a13.

> * tails-bump-apt-snapshot-valid-until
> * function print_stats is not used anywhere I can see

(That’s rather in tails-compact-reprepro-db.)

Commit ce026ea.

> * tails-update-time-based-apt-snapshots
> * function error unused too

Commit 40ade4e.

#17 Updated by bertagaz 2016-05-23 12:42:18

• Assignee changed from bertagaz to intrigeri
• % Done changed from 70 to 50
• QA Check changed from Info Needed to Dev Needed

Replying only where I have something to say. The rest is fine with me. :)

intrigeri wrote:
> Yet another thing we’ve had for years, but still: fixed in commit 125d7c3. It’s good to polish our old stuff whenever an occasion presents itself, and that’s the case here :)

Yeah, I noticed later that you basically just moved this classes, but my review was already done, so I just posted it.

> (In passing: now, TBH this is a bit futile since tails_secrets_apt hard-codes the default value for $homedir, and thus doesn’t honor the custom value one might pass to tails::reprepro::snapshots::time_based. I’ve found this acceptable for the time being, since tails_secrets_apt is… secret, so there are basically no chances that someone combines it with tails::reprepro::snapshots::time_based called with a custom $homedir.)

True, sounds a reasonable assumption.

> > * tails::reprepro::snapshots::tagged
> > * $prepare_tagged_snapshot_import_pkg_deps packages are not quoted.
>
> Why do you think this is a problem? (That’s a genuine question :) My copy of puppet-lint is perfectly happy with that. Any reference documenting why it’s Bad™ practice?
>
> > * tails::reprepro::snapshots::time_based
> > * Pkg dependencies quoting is not consistent.
>
> Indeed. If you don’t mind, I’ll make them consistent once we’re done with the above discussion about whether we should quote them or not.

Not that I think (or know) it’s bad practice, nor that my puppet-lint is complaining either, I’m was only raising this because so far we were quoting them, and now we had two ways to write that in the same file. I agree we should choose one and stick to it for coding style consistency. I don’t have preferences here, so I let you choose. If you choose not to use quotes, then I guess you don’t have to change every manifests we have, we can still implement that slowly while modifying other manifests.

> Now, I realize this is confusing, so I’ve dropped the union() entirely in 2c94549.

Ah, didn’t notice it was working for only two arrays. Good to know.

> That usage works perfectly fine for me, and I think it’s OK in the context of tails-update-time-based-apt-snapshots (no cron daemon passing the whole thing through a shell in its own way). Let’s special case only cases that are indeed special :)

Make sense.

Once done with the quoting thing, feel free to mark this ticket as done.

#18 Updated by bertagaz 2016-05-23 12:42:48

• % Done changed from 50 to 80

#19 Updated by intrigeri 2016-05-23 13:05:34

• Status changed from In Progress to Resolved
• Assignee deleted (intrigeri)
• % Done changed from 80 to 100
• QA Check changed from Dev Needed to Pass

Commit 09a2c29, and we’re done here :)