Opened 7 weeks ago

Closed 6 weeks ago

#33277 closed task (fixed)

adopt puppetlabs apt module

Reported by: anarcat Owned by: anarcat
Priority: Low Milestone:
Component: Internal Services/Tor Sysadmin Team Version:
Severity: Major Keywords: tpa-roadmap-february
Cc: Actual Points: 0.5
Parent ID: Points: 1
Reviewer: Sponsor:

Description

we currently have two codebases to manage APT repositories and keyrings: some stuff spread around torproject_org::apt and base::aptrepo and the puppetlabs-apt module. the latter was imported in our codebase when the postgresql module was imported, as a dependency. it normally does not conflict with our stuff because it's not enabled.

but as part of #31957 we *have* to enable it because it's a dependency of the unattended-upgrades module. since we have to bite that bullet anyways, might as well make the best of it and start using the module proper and remove our tpo-specific code associated with it.

so far the only places i spotted use of that code is in torproject_org::apt and the proliant module, which is only used on listera, which probably deserves to be shutdown itself (#33276), so this is not as complex a transition as I thought it would be.

the first phase of the transition is to enable the apt module everywhere, with minimal changes. that is a requirement for the deployment of the unattended-upgrades module. then we convert the existing code to *use* the apt module to create sources.list files and so on.

Child Tickets

Change History (10)

comment:1 Changed 7 weeks ago by anarcat

Resolution: fixed
Status: assignedclosed

we are now using the upstream apt module.

i've also took this opportunity to move the gpg trust anchor out of trusted.gpg.d and into /usr/share/keyrings as per https://wiki.debian.org/DebianRepository/UseThirdParty for the servers that support it (stretch and up).

there are two downsides with the switch:

  1. we cannot define multiple mirrors at once
  2. we cannot define multiple suites at once

The latter is not a big problem: just create another entry alongside the other, it's very similar to how things currently work except you have two files instead of one, and you need to name them differently. Because suite names are short, they can easily be used in the filename as well.

But the former is a bigger problem: we can't really name the sources.list file after the mirror, because we don't have a good short name for those. We would need to implement the same kind of logic that was in the previous template, by looping over the provided mirrors. But that would require an upstream change and I'm not sure we can convince upstream to provide support for multiple mirrors.

It seems the tradeoff isn't worth it anyways: either the POP mirror is reliable, or it isn't. If it's not then we ditch it. If it is, then we don't need the fallback.

So I favor consolidating our work with upstream and losing that functionality over complicating code and forking even deeper than we already have.

We have *one* patch to the upstream module right now, documented here:

https://github.com/puppetlabs/puppetlabs-apt/pull/904
https://tickets.puppetlabs.com/browse/MODULES-10543

It's a fairly trivial patch and I believe it has good chances to be accepted. But if it is refused, we can just accept that we have an empty sources.list instead of no file at all, that seems like a compromise we could live with, in a pinch.

That was quite a ride, but we're now "apt-safe", as long as we don't start asking it for "keys", because of the various problems with that module.

We might want to implement a wrapper around apt::source so it has a better "key" semantic than the current one to workaround that problem, but I'll cross that bridge when I get there. I'll wait for that issue to get more traction before I venture down that larger refactoring:

https://tickets.puppetlabs.com/browse/MODULES-9695

comment:2 Changed 7 weeks ago by anarcat

Actual Points: 0.5

comment:3 Changed 7 weeks ago by anarcat

for what it's worth, this is the actual final diff on a host like pauli:

diff --git a/apt/apt.conf.d/15update-stamp b/apt/apt.conf.d/15update-stamp
new file mode 100644
index 00000000..d818d2dd
--- /dev/null
+++ b/apt/apt.conf.d/15update-stamp
@@ -0,0 +1,2 @@
+// This file is managed by Puppet. DO NOT EDIT.
+APT::Update::Post-Invoke-Success {"touch /var/lib/apt/periodic/update-success-stamp 2>/dev/null || true";};
diff --git a/apt/apt.conf.d/60https-db-torproject-org-x509 b/apt/apt.conf.d/60https-db-torproject-org-x509
new file mode 100644
index 00000000..3c292c21
--- /dev/null
+++ b/apt/apt.conf.d/60https-db-torproject-org-x509
@@ -0,0 +1,2 @@
+// This file is managed by Puppet. DO NOT EDIT.
+Acquire::https::db.torproject.org::CaInfo "/usr/share/ca-certificates/mozilla/DST_Root_CA_X3.crt";
\ No newline at end of file
diff --git a/apt/apt.conf.d/60no-recommends b/apt/apt.conf.d/60no-recommends
new file mode 100644
index 00000000..a96b8742
--- /dev/null
+++ b/apt/apt.conf.d/60no-recommends
@@ -0,0 +1,2 @@
+// This file is managed by Puppet. DO NOT EDIT.
+APT::Install-Recommends 0;
\ No newline at end of file
diff --git a/apt/apt.conf.d/60pdiffs b/apt/apt.conf.d/60pdiffs
new file mode 100644
index 00000000..58fecbd6
--- /dev/null
+++ b/apt/apt.conf.d/60pdiffs
@@ -0,0 +1,2 @@
+// This file is managed by Puppet. DO NOT EDIT.
+Acquire::PDiffs "false";
\ No newline at end of file
diff --git a/apt/apt.conf.d/local-pdiffs b/apt/apt.conf.d/local-pdiffs
deleted file mode 100644
index 5fa0fc77..00000000
--- a/apt/apt.conf.d/local-pdiffs
+++ /dev/null
@@ -1,5 +0,0 @@
-//
-// THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-//
-
-Acquire::PDiffs "false";
diff --git a/apt/apt.conf.d/local-recommends b/apt/apt.conf.d/local-recommends
deleted file mode 100644
index ff0e86d0..00000000
--- a/apt/apt.conf.d/local-recommends
+++ /dev/null
@@ -1,5 +0,0 @@
-//
-// THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-//
-
-APT::Install-Recommends 0;
diff --git a/apt/apt.conf.d/puppet-https-db b/apt/apt.conf.d/puppet-https-db
deleted file mode 100644
index 28a359fb..00000000
--- a/apt/apt.conf.d/puppet-https-db
+++ /dev/null
@@ -1 +0,0 @@
-Acquire::https::db.torproject.org::CaInfo "/usr/share/ca-certificates/mozilla/DST_Root_CA_X3.crt";
diff --git a/apt/sources.list.d/backports.debian.org.list b/apt/sources.list.d/backports.debian.org.list
index 33cf3464..c8960e35 100644
--- a/apt/sources.list.d/backports.debian.org.list
+++ b/apt/sources.list.d/backports.debian.org.list
@@ -1,6 +1,3 @@
-##
-### THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-###
-
-deb	https://mirror.hetzner.de/debian/packages/	buster-backports	main contrib non-free
-deb	https://deb.debian.org/debian/	buster-backports	main contrib non-free
+# This file is managed by Puppet. DO NOT EDIT.
+# backports.debian.org
+deb https://mirror.hetzner.de/debian/packages/ buster-backports main contrib non-free
diff --git a/apt/sources.list.d/debian.list b/apt/sources.list.d/debian.list
index 5621f25d..648b45a9 100644
--- a/apt/sources.list.d/debian.list
+++ b/apt/sources.list.d/debian.list
@@ -1,6 +1,3 @@
-##
-### THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-###
-
-deb	https://mirror.hetzner.de/debian/packages/	buster	main contrib non-free
-deb	https://deb.debian.org/debian/	buster	main contrib non-free
+# This file is managed by Puppet. DO NOT EDIT.
+# debian
+deb https://mirror.hetzner.de/debian/packages/ buster main contrib non-free
diff --git a/apt/sources.list.d/security.list b/apt/sources.list.d/security.list
index 1ebe9f98..b2292251 100644
--- a/apt/sources.list.d/security.list
+++ b/apt/sources.list.d/security.list
@@ -1,5 +1,3 @@
-##
-### THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-###
-
-deb	http://security.debian.org/	buster/updates	main contrib non-free
+# This file is managed by Puppet. DO NOT EDIT.
+# security
+deb http://security.debian.org/ buster/updates main contrib non-free
diff --git a/apt/sources.list.d/torproject.org.list b/apt/sources.list.d/torproject.org.list
index d56a66eb..25c1f026 100644
--- a/apt/sources.list.d/torproject.org.list
+++ b/apt/sources.list.d/torproject.org.list
@@ -1,6 +1,3 @@
-##
-### THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-###
-
-deb	https://db.torproject.org/torproject-admin	tpo-all	main
-deb	https://db.torproject.org/torproject-admin	buster	main
+# This file is managed by Puppet. DO NOT EDIT.
+# torproject.org
+deb [signed-by=/usr/share/keyrings/torproject-archive-keyring.gpg] https://db.torproject.org/torproject-admin tpo-all main
diff --git a/apt/sources.list.d/updates.list b/apt/sources.list.d/updates.list
index 802f6b00..6e93638c 100644
--- a/apt/sources.list.d/updates.list
+++ b/apt/sources.list.d/updates.list
@@ -1,6 +1,3 @@
-##
-### THIS FILE IS UNDER PUPPET CONTROL. DON'T EDIT IT HERE.
-###
-
-deb	https://mirror.hetzner.de/debian/packages/	buster-updates	main contrib non-free
-deb	https://deb.debian.org/debian/	buster-updates	main contrib non-free
+# This file is managed by Puppet. DO NOT EDIT.
+# updates
+deb https://mirror.hetzner.de/debian/packages/ buster-updates main contrib non-free
diff --git a/apt/trusted.gpg.d/torproject.org.gpg b/apt/trusted.gpg.d/torproject.org.gpg
deleted file mode 100644
index e0296a6d..00000000
Binary files a/apt/trusted.gpg.d/torproject.org.gpg and /dev/null differ

comment:4 Changed 7 weeks ago by anarcat

Resolution: fixed
Status: closedreopened

might want to purge preferences.d and sources.list.d, which we don't do right now.

i also found some more repositories manually configured (e.g. grafana) and pins (e.g. dip and prometheus) which i converted. i found them by grepping for /etc/apt.

comment:5 Changed 7 weeks ago by anarcat

The following changes happened when purging the configs:

bungei

Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/xx-debian-stretch.list]: Filebucketed /etc/apt/sources.list.d/xx-debian-stretch.list to puppet with sum f73de9694541f5cc5cc34d2b773c5578

stretch sources:

deb	https://mirror.hetzner.de/debian/packages/	stretch	main contrib non-free
deb	https://deb.debian.org/debian/	stretch	main contrib non-free
deb	http://security.debian.org/	stretch/updates	main contrib non-free
deb	https://mirror.hetzner.de/debian/packages/	stretch-updates	main contrib non-free
deb	https://deb.debian.org/debian/	stretch-updates	main contrib non-free

to investigate.

forrestii

Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/local-stretch.list]: Filebucketed /etc/apt/sources.list.d/local-stretch.list to puppet with sum 604cda983ad33ab65b1f0b3e024422a0

stretch sources:

deb    https://deb.debian.org/debian/  stretch main contrib non-free
deb    http://security.debian.org/     stretch/updates main contrib non-free

nutans

Info: Computing checksum on file /etc/apt/sources.list.d/backports.debian.org.list.orig
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/backports.debian.org.list.orig]: Filebucketed /etc/apt/sources.list.d/backports.debian.org.list.orig to puppet with sum 2b70c2603d578164a2f6523ccb2b2df5
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/backports.debian.org.list.orig]/ensure: removed
Info: Computing checksum on file /etc/apt/sources.list.d/debian.list.orig
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/debian.list.orig]: Filebucketed /etc/apt/sources.list.d/debian.list.orig to puppet with sum 50c6f30738b503725e11c14a8e161635
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/debian.list.orig]/ensure: removed
Info: Computing checksum on file /etc/apt/sources.list.d/security.list.orig
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/security.list.orig]: Filebucketed /etc/apt/sources.list.d/security.list.orig to puppet with sum 93e0b824be73b94d623c01e929d82c85
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/security.list.orig]/ensure: removed
Info: Computing checksum on file /etc/apt/sources.list.d/torproject.org.list.orig
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/torproject.org.list.orig]: Filebucketed /etc/apt/sources.list.d/torproject.org.list.orig to puppet with sum 082d56c99c9f6a5aba5a5629bb82a1f9
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/torproject.org.list.orig]/ensure: removed
Info: Computing checksum on file /etc/apt/sources.list.d/updates.list.orig
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/updates.list.orig]: Filebucketed /etc/apt/sources.list.d/updates.list.orig to puppet with sum b90e986882b54d5e07fdf700a70ef957
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/updates.list.orig]/ensure: removed

backup files?

rouyi

Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/jenkins.list]: Filebucketed /etc/apt/sources.list.d/jenkins.list to puppet with sum 10db996dde7e27a40aac8f8290c38dae

jenkins sources! to restore

hetzner-nbg1-01

Info: Computing checksum on file /etc/apt/sources.list.d/hetzner-mirror.list
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/hetzner-mirror.list]: Filebucketed /etc/apt/sources.list.d/hetzner-mirror.list to puppet with sum 125a6cac2cc4845daf6723f3702d3f47
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/hetzner-mirror.list]/ensure: removed
Info: Computing checksum on file /etc/apt/sources.list.d/hetzner-security-updates.list
Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/hetzner-security-updates.list]: Filebucketed /etc/apt/sources.list.d/hetzner-security-updates.list to puppet with sum c1a42b02ff219721ba7b20da0cf1ef02
Notice: /Stage[main]/Apt/File[/etc/apt/sources.list.d/hetzner-security-updates.list]/ensure: removed

tbd?

cache01

Info: /Stage[main]/Apt/File[/etc/apt/sources.list.d/bullseye.list]: Filebucketed /etc/apt/sources.list.d/bullseye.list to puppet with sum 439e60396fe58f14570f8f60ecbb19bf

bullseye?

comment:6 Changed 7 weeks ago by anarcat

  • bungei: clear. no leftovers from stretch (according to apt-show-versions | grep -v /buster)
  • forrestii: not good. mongodb leftover from stretch
  • nutans: clear. apt-show-versions | grep -v /buster looks clean
  • rouyi: todo
  • hetzner-nbg1-01: clear. just copies of stretch sources
  • cache01: all clear

so, need to deploy sources.list files on forrestii and rouyi.

comment:7 Changed 7 weeks ago by anarcat

filed a ticket about forrestii in #33288 since mongodb is dead.

added the jenkins thing to rouyi in puppet.

now we need to push that somewhat stupid fix upstream:

9317a8e6 hotfix: only consider lsbdistcodename for apt-transport-https

comment:8 Changed 7 weeks ago by anarcat

Status: reopenedmerge_ready

pushed all our code upstream and merged it in my master branch. we should be good insofar as upstream will hopefully merge our stuff:

https://github.com/puppetlabs/puppetlabs-apt/pull/904
https://github.com/puppetlabs/puppetlabs-apt/pull/905
https://github.com/puppetlabs/puppetlabs-apt/pull/906

Update: one of those was merged, and I'll bring up the other two (along with ideas of other improvements) to the puppet "office hours" this monday.

The other improvements I am thinking of are specifically:

  1. refactor the OpenPGP support: deprecate apt::key in favor of an explicit "keyfile", and enforce /usr/share/keyrings as a standard location
  2. use proper naming for apt::source parameters (url instead of location, suite instead of release, components instead of repos, and a simple hash for the other options, with an implicit "signed-by" if a keyfile is provided)

This would require dropping jessie support from the module, but that seems like an acceptable switch at this point, especially since that would affect third-party repositories.

Last edited 7 weeks ago by anarcat (previous) (diff)

comment:9 Changed 7 weeks ago by anarcat

first two PRs merged, woot!

third PR seems blocked on a weird code coverage issue but i don't see why that happens...

the puppet refactoring work was agreed on, and I filed https://tickets.puppetlabs.com/browse/MODULES-10555 to cover the naming convention change.

comment:10 Changed 6 weeks ago by anarcat

Resolution: fixed
Status: merge_readyclosed

all our patches were merged. future improvements are welcome, but are probably better followed up in the upstream issue tracker.

ie. we'll fix those when we have time, they are not blockers.

Note: See TracTickets for help on using tickets.