cleanup branches in tor-puppet.git

marked this issue as related to #40861 (closed)

assigned to @anarcat

i update the status of my branches in the issue summary. (previously here, but moved up)

for historical reference, this was scrape_job_fqdn_targets, maybe only @lelutin understands what's going on there now:

commit 052960b2b87e58ead94fb28c11422c2c18a98dd6
Author: Antoine Beaupré <anarcat@debian.org>
Date:   Wed Nov 6 19:48:21 2024 -0500

    scrape lists.torproject.org instead of lists-01
    
    We are currently scraping lists-01.torproject.org which *works*, but
    it's not *actually* supposed to.
    
    Just allow caller to override the full URL for the scrape
    targets. This allows us to (a) define more than one target and (b)
    have a target that might be a alias (SAN) of the cert.
    
    In this case, $certname is lists-01.torproject.org but the actual
    domain to scrape is lists.torproject.org, a pretty common pattern
    across the infra.
    
    An alternative would be to chop this down further and have a
    "base_url" (which i would call a "path" instead) and "fqdn" that can
    be specified separately, but then we lose the possibility of defining
    multiple targets per cert, which I think might be nice.
    
    We also add a `scrape_job_fqdn` for the less degenerate case of "i
    want to monitor foo.torproject.org, not foo-01.torproject.org" which I
    suspect we have a lot more of.
    
    I am not actually sure I can reuse the variables between define
    parameters like this, so perhaps this doesn't work and we'll need to
    do an undef dance either, to be reviewed.

diff --git a/modules/profile/manifests/mailman.pp b/modules/profile/manifests/mailman.pp
index 6be0ea634..dfed9f460 100644
--- a/modules/profile/manifests/mailman.pp
+++ b/modules/profile/manifests/mailman.pp
@@ -85,12 +85,13 @@ class profile::mailman(
   }
   include apache2
   ssl::service { $facts['networking']['fqdn']:
-    notify              => Exec['service apache2 reload'],
-    onion               => true,
+    notify                   => Exec['service apache2 reload'],
+    onion                    => true,
     # Apache imposes a redirection on the root URL and blackbox exporter is not
     # following redirections. We'd rather know when django is not responding
     # correctly.
-    scrape_job_base_url => '/mailman3/postorius/lists/',
+    http_scrape_job_targets  => ["http://${vhost_name}/mailman3/postorius/lists/"],
+    https_scrape_job_targets => ["https://${vhost_name}/mailman3/postorius/lists/"],
   }
   $vhost_context = {
     vhost_name => $facts['networking']['fqdn'],
diff --git a/modules/ssl/manifests/service.pp b/modules/ssl/manifests/service.pp
index 99951daef..89e53e994 100644
--- a/modules/ssl/manifests/service.pp
+++ b/modules/ssl/manifests/service.pp
@@ -32,12 +32,11 @@
 #   running on the prometheus servers. The the target will automatically get
 #   alerts for if the site is unreachable.
 #
-# @param scrape_job_base_url
-#   Which URL -- always starting with a / -- should blackbox be contacting when
-#   it's testing http/https. Most sites can simply check the root URL, but in
-#   some cases where the root URL is a redirection imposed by the web server to
-#   the service's base URL, we most probably want to verify the service's
-#   response instead of the redirection imposed by the web server.
+# @param scrape_job_fqdn
+#   Which hostname to use when constructing the target URLs to probe
+#   with the blackbox exporter. Defaults to $certname, should be
+#   changed if the certname is based on the host FQDN
+#   (e.g. test-01.tpo) instead of the service name (e.g. test.tpo).
 #
 # @param http_scrape_job
 #   Name of the prometheus scrape job that's used for testing http for this certificate
@@ -45,6 +44,20 @@
 # @param https_scrape_job
 #   Name of the prometheus scrape job that's used for testing https for this certificate.
 #
+# @param http_scrape_job_targets
+#   Which targets to test with the blackbox exporter, defaults to the
+#   root path on the given FQDN (http://$scrape_job_fqdn/). Should be
+#   changed if we want to monitor a different path than the root (/)
+#   for the HTTP job. This is typically unused because most HTTP
+#   endpoints will simply redirect to HTTPS.
+#
+# @param https_scrape_job_targets
+#   Same as http_scrape_job_targets, but for HTTPS. Typically used to
+#   change the root path to some subpath a service redirects to by
+#   default. For example, Mailman redirects you to
+#   /mailman3/postorius/lists by default, so checking / doesn't tell
+#   us if the site is actually up.
+#
 # @param provider
 #   What certificate provider should be used to create this ssl certificate. Can
 #   be one of 'letsencrypt', 'digicert', or 'harica'.
@@ -71,9 +84,11 @@ define ssl::service(
   Boolean $onion = false,
   Boolean $dehydrated = false,
   Boolean $monitoring = true,
-  String $scrape_job_base_url = '/',
+  Stdlib::Fqdn $scrape_job_fqdn = $certname,
   String $http_scrape_job = 'blackbox_http_3xx',
+  Array[Stdlib::HTTPUrl] $http_scrape_job_targets = ["http://${scrape_job_fqdn}/"],
   String $https_scrape_job = 'blackbox_https_2xx_or_3xx',
+  Array[Stdlib::HTTPUrl] $https_scrape_job_targets = ["https://${scrape_job_fqdn}/"],
   Enum['digicert', 'harica', 'letsencrypt'] $provider = 'letsencrypt',
 ) {
   include ssl
@@ -199,7 +214,7 @@ define ssl::service(
   if $ensure != 'absent' and $monitoring {
     @@prometheus::scrape_job { "${http_scrape_job}_${certname}":
       job_name => $http_scrape_job,
-      targets  => [ "http://${certname}${scrape_job_base_url}" ],
+      targets  => $http_scrape_job_targets,
       labels   => {
         'alias' => $certname,
         'team'  => 'TPA',
@@ -208,7 +223,7 @@ define ssl::service(
 
     @@prometheus::scrape_job { "${https_scrape_job}_${certname}":
       job_name => $https_scrape_job,
-      targets  => [ "https://${certname}${scrape_job_base_url}" ],
+      targets  => $https_scrape_job_targets,
       labels   => {
         'alias' => $certname,
         'team'  => 'TPA',

here's ssh-refactor:

commit 654e787e33094d3629c70a31deac4c34d870be85
Author: Antoine Beaupré <anarcat@debian.org>
Date:   Thu Sep 12 17:54:42 2019 -0400

    move ferm firewall rules out of authorized_keys logic
    
    The reason for this is that other consumers of this module might use a
    different firewalling module. For example, shorewall is popular in the
    Puppet community. That would have made it very hard for others to use
    this module.
    
    Furthermore, the way those exported resources are built make it
    difficult to refactor the authorized_keys to support arbitrary
    locations, instead of hardcoding the /etc/ssh/puppetkeys prefix. We
    should instead allow keys to be written to arbitrary locations, as
    other sites will have different paths for SSH keys. Doing so while
    keeping the "user" construct inside the define will be difficult, so
    it's essential the ferm rules move out of the define for that reason
    as well.

diff --git a/modules/ssh/manifests/authorized_key_add.pp b/modules/ssh/manifests/authorized_key_add.pp
index 65b088b05..1e4cb557d 100644
--- a/modules/ssh/manifests/authorized_key_add.pp
+++ b/modules/ssh/manifests/authorized_key_add.pp
@@ -71,9 +71,6 @@ define ssh::authorized_key_add(
     $raw_tags = $collect_tag
   }
   $ssh_tags = $raw_tags.map |$t| { "ssh::authorized_key::fragment::${t}::${target_user}" }
-  $ferm_tags = $raw_tags.map |$t| { "ssh::authorized_key::ferm::${t}::${target_user}" }
-
-  $ferm_from_string = $from.join(' ')
 
   if $key {
     @@concat::fragment { "ssh::authorized_key::${name} from ${::fqdn}":
@@ -89,12 +86,4 @@ define ssh::authorized_key_add(
       loglevel => warning,
     }
   }
-
-  @@ferm::rule { "ssh-${raw_tags[0]}_${target_user}-${name}_from_${::hostname}":
-    tag         => $ferm_tags,
-    description => "allow ssh for ssh to ${target_user}",
-    domain      => '(ip ip6)',
-    chain       => 'ssh',
-    rule        => "saddr (${ferm_from_string}) ACCEPT",
-  }
 }
diff --git a/modules/ssh/manifests/authorized_key_collect.pp b/modules/ssh/manifests/authorized_key_collect.pp
index 7c121e4e7..15a62c1bd 100644
--- a/modules/ssh/manifests/authorized_key_collect.pp
+++ b/modules/ssh/manifests/authorized_key_collect.pp
@@ -9,6 +9,4 @@ define ssh::authorized_key_collect(
     ensure_newline => true,
   }
   Concat::Fragment <<| tag == "ssh::authorized_key::fragment::${collect_tag}::${target_user}" |>>
-
-  Ferm::Rule <<| tag == "ssh::authorized_key::ferm::${collect_tag}::${target_user}" |>>
 }
diff --git a/modules/staticsync/manifests/ssh.pp b/modules/staticsync/manifests/ssh.pp
index d3373f03f..6a72adf78 100644
--- a/modules/staticsync/manifests/ssh.pp
+++ b/modules/staticsync/manifests/ssh.pp
@@ -1,7 +1,7 @@
 # wrapper for ssh setup for statichosts
 class staticsync::ssh  (
-  Variant[Array[String], String] $add_tag,
-  String                         $collect_tag,
+  Array[String] $add_tags,
+  String        $collect_tag,
 ) {
   ssh::keygen { $staticsync::user : }
 
@@ -11,10 +11,21 @@ class staticsync::ssh  (
     command     => "/usr/local/bin/staticsync-ssh-wrap ${::fqdn}",
     options     => ['restrict','pty'],
     from        => $staticsync::public_addresses,
-    collect_tag => $add_tag,
+    collect_tag => $add_tags,
   }
   ssh::authorized_key_collect { "staticsync-${staticsync::user}":
     target_user => $staticsync::user,
     collect_tag => $collect_tag,
   }
+
+  $ferm_tags = $add_tags.map |$t| { "ssh::authorized_key::ferm::${t}::${staticsync::user}" }
+  $ferm_from_string = $staticsync::public_addresses.join(' ')
+  @@ferm::rule { "ssh-${ferm_tags[0]}_${staticsync::user}-staticsync-${staticsync::user}_from_${::hostname}":
+    tag         => $ferm_tags,
+    description => "allow ssh for ssh to ${staticsync::user}",
+    domain      => '(ip ip6)',
+    chain       => 'ssh',
+    rule        => "saddr (${ferm_from_string}) ACCEPT",
+  }
+  Ferm::Rule <<| tag == "ssh::authorized_key::ferm::${collect_tag}::${staticsync::user}" |>>
 }
diff --git a/modules/staticsync/manifests/static_mirror.pp b/modules/staticsync/manifests/static_mirror.pp
index c35e99664..ea83baa55 100644
--- a/modules/staticsync/manifests/static_mirror.pp
+++ b/modules/staticsync/manifests/static_mirror.pp
@@ -15,7 +15,7 @@ class staticsync::static_mirror (
 
   # mirrors talk only to masters
   class { 'staticsync::ssh':
-    add_tag     => 'staticsync-master',
+    add_tag     => ['staticsync-master'],
     collect_tag => 'staticsync-mirror',
   }
 
diff --git a/modules/staticsync/manifests/static_source.pp b/modules/staticsync/manifests/static_source.pp
index 02e993845..09980fc04 100644
--- a/modules/staticsync/manifests/static_source.pp
+++ b/modules/staticsync/manifests/static_source.pp
@@ -15,7 +15,7 @@ class staticsync::static_source (
     # any components which have this system as a source need to also use the same
     # system as their master.
     class { 'staticsync::ssh':
-      add_tag     => 'staticsync-master',
+      add_tag     => ['staticsync-master'],
       collect_tag => 'staticsync-source',
     }
   }

commit 748d23d23bebee70dba4058dea375a68f0355ea3
Author: Antoine Beaupré <anarcat@debian.org>
Date:   Thu Sep 12 17:42:07 2019 -0400

    hostnames may not be unique across puppet, but fqdn should be

diff --git a/modules/ssh/manifests/authorized_key_add.pp b/modules/ssh/manifests/authorized_key_add.pp
index 0b727a193..65b088b05 100644
--- a/modules/ssh/manifests/authorized_key_add.pp
+++ b/modules/ssh/manifests/authorized_key_add.pp
@@ -76,7 +76,7 @@ define ssh::authorized_key_add(
   $ferm_from_string = $from.join(' ')
 
   if $key {
-    @@concat::fragment { "ssh::authorized_key::${name} from ${::hostname}":
+    @@concat::fragment { "ssh::authorized_key::${name} from ${::fqdn}":
       tag     => $ssh_tags,
       target  => "/etc/ssh/puppetkeys/${target_user}",
       content => @("EOF"),

commit 8fe614f81db9293f66f1b95c8dacba2f14a4a91e
Author: Antoine Beaupré <anarcat@debian.org>
Date:   Thu Sep 12 17:41:23 2019 -0400

    remove target_user from fragment name, it should be in $name already

diff --git a/modules/ssh/manifests/authorized_key_add.pp b/modules/ssh/manifests/authorized_key_add.pp
index aae3ed93c..0b727a193 100644
--- a/modules/ssh/manifests/authorized_key_add.pp
+++ b/modules/ssh/manifests/authorized_key_add.pp
@@ -76,7 +76,7 @@ define ssh::authorized_key_add(
   $ferm_from_string = $from.join(' ')
 
   if $key {
-    @@concat::fragment { "ssh::authorized_key::${name} ${target_user} from ${::hostname}":
+    @@concat::fragment { "ssh::authorized_key::${name} from ${::hostname}":
       tag     => $ssh_tags,
       target  => "/etc/ssh/puppetkeys/${target_user}",
       content => @("EOF"),

marked the checklist item @anarcat origin/ssh-refactor as completed

prom-systemd-modules-upgrades: was asked by @lavamind to wait until envs are ready before merging, ready to merge otherwise, i think. i don't remember why i needed this unfortunately, but it's still a good thing, IMHO.

It's hard to make heads or tails of that branch, there's a major revert in the middle it seems, and it's not clear which versions the module are being upgraded to (it should be mentioned in the commit message, imho).

In any case, I think we should instead prepare a branch (and environment!) with only the systemd module upgrade, because this is one thing that's been really hairy in the past.

It's hard to make heads or tails of that branch, there's a major revert in the middle it seems

for the record, the branch is actually a single commit (302c6057a) on top of the merge point on master (75c297dcb).

the revert is actually that master commit, because I first committed that to master and pushed, and it broke everything, so i reverted the commit and spun off the commit on a branch.

and it's not clear which versions the module are being upgraded to (it should be mentioned in the commit message, imho).

fair. i'm happy to rephrase the commit message, but the first hunk of the commit looks like this:

-mod 'puppet/prometheus',
-    :git => 'https://github.com/anarcat/puppet-prometheus.git',
-    :branch => 'process-exporter-service-name-11.0.1-rc0'
+mod 'puppet/prometheus', '12.4.0'
 mod 'dalen/puppetdbquery'
 # to get loginctl
-mod 'camptocamp/systemd', '2.12.0'
+mod 'puppet/systemd', '4.2.0'

so basically it goes from Prom 11.0.1 (with a patch of mine) to 12.4 upstream, and systemd 2.12.0 to 4.2.0,

In any case, I think we should instead prepare a branch (and environment!) with only the systemd module upgrade, because this is one thing that's been really hairy in the past.

well there is a branch here... i'm not sure we can update those two modules separately, as i had to update systemd to update prometheus...

but sure... just be aware that i did put the work in that patch in reviewing what had changed in those modules to make sure i made the minimal change to systemd, so i would much rather we merge this already and work on "update all the things" separately. this is not the latest systemd module!

changed the description

i'm starting to think we just really need to have a copy of the repo here on gitlab so we can do proper reviews. our email workflow is just not working at all, and it's going to be impossible to cleanup those branches otherwise.

i'll file an issue! :)

marked this issue as related to #41971 (closed)

mentioned in issue #41971 (closed)

mentioned in issue #29387

added Doing label and removed Next label

changed milestone to %Puppet CI

changed the description

cleanup branches in tor-puppet.git

Designs

Child items ...

Activity