Tails

#1 Updated by intrigeri 2016-07-16 08:13:56

• Description updated
• Starter changed from Yes to No

#2 Updated by intrigeri 2016-08-18 09:09:15

• Description updated
• Starter changed from No to Yes

#3 Updated by intrigeri 2017-03-26 12:19:06

• Description updated

#4 Updated by groente 2017-04-29 16:46:59

• File check_upgradeable.sh added
• Status changed from Confirmed to In Progress
• Assignee set to groente

Here’s a monitoring plugin that checks for upgradeable packages

#5 Updated by groente 2017-04-29 16:53:54

• Assignee changed from groente to intrigeri
• % Done changed from 0 to 10
• QA Check set to Ready for QA

#6 Updated by intrigeri 2017-05-01 08:36:22

• Assignee changed from intrigeri to groente
• % Done changed from 10 to 20
• QA Check changed from Ready for QA to Dev Needed

Thanks! Here’s an initial review.

First, let’s make sure we’re on the same page wrt. the goal of this ticket:

• What’s the output of the script in the scenario I wrote in the ticket description?
• On my system, the script tells me that some package are upgradeable, while my APT pinning explicitly prefers the currently installed version (even though a newer one is indeed available). So it seems that apt-show-versions ignores APT pinning. This can cause false positives: raising monitoring warnings for upgrades we explicitly block. So I see two options:
  • give admins a way to list some packages that this plugin should ignore: cheap, might be good enough; I suggest you try this option first, then we deploy this in production and see how it behaves; if we then realize it’s not good enough, we can switch to a more elaborate approach;
  • use a better suited tool than apt-show-versions (e.g. APT knows what packages are “kept back” during upgrades, and this surely can be queried programmatically in a nice way): harder, might be overkill; let’s keep it for later.

Now, about the actual implementation:

• About PROGPATH=`echo $0 | sed -e 's,[\\/][^\\/][^\\/]*$,,'`: why not use dirname(1) instead? I see that some other Nagios plugins coming from Debian packages use that sed command-line, possibly for increased portability, but this one only makes sense on Debian, where dirname is guaranteed to be available, and we’re going to maintain it ourselves, so I’d rather prioritize maintainable code over portability :)
• Please use set -eu.
• Please quote all variables (except of course when there’s a good reason not to :)
• Where does TRUSTED_PATH come from?
• I misunderstood “CRITICAL if dependencies are missing” the first time I read it, and only got it right after reading the rest of the script ⇒ please clarify that this is about the dependencies of the script, not about dependencies of potentially upgradeable packages.
• apt-show-versions |grep upgradeable feels very brittle, e.g. there could be a package whose name matches the “upgradeable” regexp some day. I was going to ask for a more accurate regexp, and then, after having a look at the manpage: why not use apt-show-versions --upgradeable instead?
• |sed -e 's/:.*//' drops the architecture info, which makes sense in most cases, but loses useful info on systems with more than one architecture enabled (APT/dpkg now support this just fine); I suggest using apt-show-versions’s --brief option, whose output seems OK, and that doesn’t require regexp magics that not everyone is at ease with on our team.
• In upgradeables=$(for i in `apt-show-versions |grep upgradeable |sed -e 's/:.*//'`;do echo -n "$i ";done), I dislike the for/echo loop, and would rather see something like upgradeables=$(apt-show-versions [… options …]) | tr “\n” " “, that makes less implicit assumptions and is a bit more declarative. * So all in all, we could maybe simply write upgradeables=$(apt-show-versions —upgradeable —brief | tr ”\n" " ". This would make less implicit assumptions, feels more robust and easier to maintain. I'd like to hear your opinion though: what do you think? * Please use -z@ in [ "$upgradeables" = "" ]: you can safely assume such basic shell scripting knowledge from whoever will maintain this plugin :)

Cheers!

#7 Updated by groente 2017-05-01 11:28:26

• File check_upgradeable.sh added
• % Done changed from 20 to 30
• QA Check changed from Dev Needed to Ready for QA

Here’s an updated version.
The output of the script after replaying the ticket description is now:

The following packages are upgradeable: letsencrypt:all/jessie-backports python-cryptography:amd64/jessie-backports python-openssl:all/jessie-backports

with exit status 1.

If I understood you correctly, the problem with explicit preferences is now mitigated by filtering out packages that are labeled manually upgradeable.

This filtering doesn’t work with the —brief flag, though, so even though the for loop is now gone, there’s still some sed and grep needed there.
TRUSTED_PATH was a macro in the monitoring-plugins build environment that i overlooked, it’s now replaced by a very minimal (but sufficient) path.

#8 Updated by groente 2017-05-01 11:28:51

• Assignee changed from groente to intrigeri

#9 Updated by intrigeri 2017-05-03 08:19:18

• Target version set to Tails_3.0~rc1

#10 Updated by intrigeri 2017-05-19 20:17:51

• Target version changed from Tails_3.0~rc1 to Tails_3.0

Sorry!

#11 Updated by intrigeri 2017-05-28 10:14:34

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

In a Jessie chroot:

echo "deb http://snapshot.debian.org/archive/debian/20160415T115342Z/ jessie-backports main" \
> /etc/apt/sources.list.d/jessie-backports.list
apt -o Acquire::Check-Valid-Until=false update
cat > /etc/apt/preferences.d/jessie-backports <<EOF
Package: *
Pin: release o=Debian Backports,a=jessie-backports
Pin-Priority: -1
EOF
cat > /etc/apt/preferences.d/letsencrypt <<EOF
Package: letsencrypt python-letsencrypt python-acme python-configargparse python-cryptography python-dialog python-rfc3339 python-openssl python-idna python-ipaddress python-pyasn1 python-cffi-backend
Pin: release o=Debian Backports,a=jessie-backports
Pin-Priority: 500
EOF
apt install letsencrypt
echo "deb http://ftp.us.debian.org/debian/ jessie-backports main" \
> /etc/apt/sources.list.d/jessie-backports.list
apt update
apt install apt-show-versions

At this point I see:

# apt list --upgradable
Listing... Done
letsencrypt/jessie-backports 0.10.2-1~bpo8+1 all [upgradable from: 0.4.1-1~bpo8+1]
python-acme/jessie-backports 0.10.2-1~bpo8+1 all [upgradable from: 0.4.1-1~bpo8+1]
python-configargparse/jessie-backports 0.11.0-1~bpo8+1 all [upgradable from: 0.10.0-2~bpo8+1]
python-cryptography/jessie-backports 1.3.4-1~bpo8+2 i386 [upgradable from: 1.1.1-1~bpo8+1]
python-openssl/jessie-backports 16.0.0-1~bpo8+1 all [upgradable from: 0.15.1-2~bpo8+1]

Let’s manually pin python-configargparse to the old version, in order to test the situation I’ve described in my previous review:

cat > /etc/apt/preferences.d/python-configargparse <<EOF
Package: python-configargparse
Pin: version 0.10.*
Pin-Priority: 1000
EOF
cat > /etc/apt/preferences.d/letsencrypt <<EOF
Package: letsencrypt python-letsencrypt python-acme python-cryptography python-dialog python-rfc3339 python-openssl python-idna python-ipaddress python-pyasn1 python-cffi-backend
Pin: release o=Debian Backports,a=jessie-backports
Pin-Priority: 500
EOF

That has the expected effect:

# apt list --upgradable
Listing... Done
letsencrypt/jessie-backports 0.10.2-1~bpo8+1 all [upgradable from: 0.4.1-1~bpo8+1]
python-acme/jessie-backports 0.10.2-1~bpo8+1 all [upgradable from: 0.4.1-1~bpo8+1]
python-cryptography/jessie-backports 1.3.4-1~bpo8+2 i386 [upgradable from: 1.1.1-1~bpo8+1]
python-openssl/jessie-backports 16.0.0-1~bpo8+1 all [upgradable from: 0.15.1-2~bpo8+1]
# apt upgrade
Reading package lists... Done
Building dependency tree       
Reading state information... Done
Calculating upgrade... Done
0 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
# apt-show-versions --upgradeable
letsencrypt:all/jessie-backports 0.4.1-1~bpo8+1 upgradeable to 0.10.2-1~bpo8+1
python-acme:all/jessie-backports 0.4.1-1~bpo8+1 upgradeable to 0.10.2-1~bpo8+1
python-configargparse:all/jessie-backports *manually* upgradeable from 0.10.0-2~bpo8+1 to 0.11.0-1~bpo8+1
python-cryptography:i386/jessie-backports 1.1.1-1~bpo8+1 upgradeable to 1.3.4-1~bpo8+2
python-openssl:all/jessie-backports 0.15.1-2~bpo8+1 upgradeable to 16.0.0-1~bpo8+1
# apt-show-versions --upgradeable --brief
letsencrypt:all/jessie-backports
python-acme:all/jessie-backports
python-configargparse:all/jessie-backports
python-cryptography:i386/jessie-backports
python-openssl:all/jessie-backports

Then let’s test the script:

# apt install monitoring-plugins-common
# install -o root -g root -m 0755 /tmp/check_upgradeable.sh /usr/lib/nagios/plugins/
# /usr/lib/nagios/plugins/check_upgradeable.sh -c     
The following packages are upgradeable: letsencrypt:all/jessie-backports python-acme:all/jessie-backports python-cryptography:i386/jessie-backports python-openssl:all/jessie-backports
# echo $?
1

So yeah, it does what’s written on the box \o/ :)

> If I understood you correctly, the problem with explicit preferences is now mitigated by filtering out packages that are labeled manually upgradeable.

This makes sense to me, and indeed python-configargparse is not listed, which is exactly what we want (I think). But perhaps there’s a simpler/nicer way to do it:

> This filtering doesn’t work with the —brief flag, though, so even though the for loop is now gone, there’s still some sed and grep needed there.

Interestingly, as we can see above, apt list --upgradable seems to do the correct filtering. Using it instead of apt-show-versions would avoid the need for regexp magics. Now, apt “does not have a stable CLI interface yet” so perhaps using it is not such a good idea. How would you make the decision between these two options?

Meta: I’m asking for a reasoning, pros/cons, likely failure modes of either option and corresponding possible mitigation measures, cost/benefit of implementing these mitigation measures, and finally your personal opinion taking all this into account. I care a lot more about the thought process than about the conclusion here.

In any case, some more style nitpicking about the current implementation, not all relevant if we move to apt list --upgradable:

• Please use long option names (e.g. apt-show-versions --upgradeable) in general. Very well-known and widespread abbreviated args (e.g. sed -e, grep -v) are OK though :)
• Please use extended regexps everywhere: not everyone on our team is at ease with regexps, so I want to ease the learning process by focusing on one single RE language. Ideally we would standardise on PCRE (used in most programming languages) but sadly grep’s PCRE support “is highly experimental”, and ERE are close enough to PCRE so that’s good enough for our usual grep/sed needs.
• The final * in grep -v '*manually*' doesn’t do what I think you think it does (both in BRE and ERE), and regardless it doesn’t do what it should, as demonstrated if you replace it with grep -v '*manuall*'.
• The first * in this regexp does what it should almost by chance, merely because it’s the first char in the regexp: it’ll silently stop matching exactly what it should as soon as someone less knowledgeable at REs adds something else in front, which feels fragile ⇒ I’d rather see it escaped: \* or [*] (I prefer the latter, see Damian Conway’s “Perl Best Practices” for details).

Woohoo, we’re almost there, I expect I can Puppetize and deploy this after the next review round :)))

Feel free to adjust the Target version according to your availability.

Cheers!

#12 Updated by groente 2017-05-29 10:49:14

• File check_upgradeable.sh added
• Assignee changed from groente to intrigeri
• % Done changed from 30 to 40
• QA Check changed from Dev Needed to Ready for QA

Hey, thanks for the feedback and nice catch on the manually, I completely forgot to escape there.

I’m a bit uncomfortable relying on tools that do not have a fixed interface yet, so I’d rather stick with apt-show-versions and perhaps simplify using apt at some point in the future when apt does have a fixed cli interface.

Simplifying the code is nice, but the grep and sed in question are hardly ‘magic’ - in fact they illustrate the intent a lot better than apt if you ask me - and it’ll be easier to comprehend than some future broken script that relied on a deprecated interface.

That said, here’s another version, this time with escaping and explicitly using ERE.

#13 Updated by intrigeri 2017-05-31 09:31:39

• Target version changed from Tails_3.0 to Tails_3.1
• % Done changed from 40 to 50
• QA Check deleted (Ready for QA)

Thanks, I’m happy with the latest version of the script! :) Next step: turn it into an icinga2 check (in puppet-tails.git) and deploy. I can’t promise I’ll do that during the 3.0 cycle, this will probably have to wait until late June.

#14 Updated by intrigeri 2017-06-22 15:35:20

• Priority changed from Normal to Elevated

The earlier this is done, the better wrt. the process we’re in with groente.

#15 Updated by intrigeri 2017-06-25 08:59:46

• % Done changed from 50 to 60

Puppet-ized (following https://tails.boum.org/contribute/working_together/roles/sysadmins/adding_icinga2_checks/ → https://git-tails.immerda.ch/puppet-tails/commit/?id=a6e2990f936a366e9dfe1b754932193e0044b39d and https://git-tails.immerda.ch/puppet-tails/commit/?id=a7221d31bfb9dc4759dd56c22dc78cea51cfe97a), enabled on one of our VMs. Let’s see how it goes.

#16 Updated by intrigeri 2017-06-25 14:01:38

• % Done changed from 60 to 70

Works fine, will deploy on all systems :)

#17 Updated by intrigeri 2017-06-25 14:12:50

• Assignee changed from intrigeri to bertagaz
• % Done changed from 70 to 80
• QA Check set to Ready for QA

Done, and added these checks to the “APT” ServiceGroup.

#18 Updated by bertagaz 2017-06-25 16:01:54

• Status changed from In Progress to Resolved
• Assignee deleted (bertagaz)
• % Done changed from 80 to 100
• QA Check changed from Ready for QA to Pass

intrigeri wrote:
> Done, and added these checks to the “APT” ServiceGroup.

Looks good, simple and effective, so closing this ticket, congrats to both of you!

#19 Updated by intrigeri 2017-06-29 09:53:02

• blocks Feature #13232: Core work 2017Q2: Sysadmin (Maintain our already existing services) added

#20 Updated by intrigeri 2017-06-30 06:20:50

• Target version changed from Tails_3.1 to Tails_3.0.1