Bug #11181

Better abstraction of ::local::node

Added by bertagaz 2016-02-29 19:44:01 . Updated 2019-03-14 13:34:40 .

Status:
In Progress
Priority:
Normal
Assignee:
bertagaz
Category:
Infrastructure
Target version:
Start date:
2016-02-29
Due date:
% Done:

30%

Feature Branch:
puppet-lizard-manifests:bugfix/11181-abstract-local-node,puppet-tails:bugfix/11181-abstract-local-node
Type of work:
Sysadmin
Blueprint:

Starter:
0
Affected tool:
Deliverable for:

Description

When installing ecours.t.b.o (Feature #8647), we had to move out some parts of our ::local::node declaration that were too tied to lizard’s setup:

Part of it has been poorly handled into a ::tails::base:grub_setup class, but it’s taking care of too many things at once: apparmor and the serial cmdline, which can be different from host to host.

Another part is the postfix canonical_sender setup which is tied to services we host on lizard.

We turned them into parameters to ::local::node one can deactivate, but it would be better if we abstracted that a bit more this class.


Subtasks


Related issues

Related to Tails - Feature #8647: Install an OS on the machine that will host the production monitoring setup Resolved 2015-12-15
Related to Tails - Bug #16167: move classes.pp from manifests to its own module Resolved 2018-11-28

History

#1 Updated by intrigeri 2016-03-01 17:40:40

  • Assignee set to bertagaz
  • Deliverable for set to SponsorS_Internal

I’ve done some initial cleanup, and moved more stuff to our tails::base class in order to help clarify what local::node is about. I’d like to keep this ticket about dealing with the regressions introduced when working on Feature #8647, so I’ll retitle it accordingly. We can create more precise tickets if we see other, more specific action we want to track, but that are not part of this task.

> Part of it has been poorly handled into a ::tails::base:grub_setup class, but it’s taking care of too many things at once: apparmor and the serial cmdline, which can be different from host to host.

The main regression I see here is that we have a tails::grub class that does quite some stuff, and is not configurable at all, so basically either what it does is OK for a given host and we can include that class, or we have to do things manually, and lose the benefits of having shared resources we can rely upon and rely to (e.g. Exec['update-grub']). I think this should be fixed by making configurable what needs to vary depending on the system:

  • AppArmor: I don’t see why this should depend on the system; so far we’ve been enabling it everywhere; why exactly do we need exceptions now?
  • GRUB_CMDLINE_LINUX: indeed, at least the part that’s about the serial console can vary, and more generally it’s good to have a parameter that we can use to add kernel parameters as needed;
    • short-term, we could give the class a $console_kernel_parameters parameter (defaulting to what we generally want with libvirt, as done already in the class), and while we’re at it a $additional_kernel_parameters parameter (empty by default), and then in Augeas[GRUB_CMDLINE_LINUX] we can concat these 2 variables to the AppArmor kernel parameters; I think it’s the easiest good-enough way to clean up the mess, and that’s all I’m expecting from this ticket in the GRUB area; and then, the $setup_grub parameter to local::node can be replaced by more meaningful ones, that will be passed through to tails::grub.
    • long-term, we could use https://github.com/hercules-team/augeasproviders_grub/ once it is fixed, or use grub2 2.00-10 (Jessie and onwards)’s new support for /etc/default/grub.d/*.cfg, that are read after /etc/default/grub => so we could drop snippets in there with content such as GRUB_CMDLINE_LINUX="$GRUB_CMDLINE_LINUX apparmor=1 security=apparmor", etc., so we can deal with all situations just with tiny config files. But let’s focus on the short-term for now.

> Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.

I did some initial research on this one, and dumped my conclusions as a comment in classes.pp. tl;dr: this would not be specific to lizard at all, if we were doing things right. Anyway, I realize that revamping our MTA configuration is not what this ticket is about, so I guess we’ll prefer a short-term workaround here again, so please just refactor things a bit so that their name makes sense:

  • Given that “local::” now means “common to all nodes managed by this set of manifests”, apparently, in this context: rename local::postfix::canonical_senders to something that makes it clear it is about stuff hosted on lizard, such as lizard::postfix::canonical_senders.
  • Drop the $set_canonical_senders parameter: it’s useless, instead we should guess dynamically if lizard::postfix::canonical_senders must be included (if $::hostname == lizard or $::fqdn =~ /\.lizard$/).

#2 Updated by intrigeri 2016-03-12 13:47:13

  • related to Feature #8647: Install an OS on the machine that will host the production monitoring setup added

#3 Updated by bertagaz 2016-04-15 08:21:02

  • Target version set to Tails_2.5

#4 Updated by bertagaz 2016-07-19 04:17:25

  • Target version changed from Tails_2.5 to Tails_2.6

#5 Updated by anonym 2016-09-20 16:54:04

  • Target version changed from Tails_2.6 to Tails_2.7

#6 Updated by bertagaz 2016-09-22 05:47:58

  • Target version changed from Tails_2.7 to Tails_2.9.1

#7 Updated by anonym 2016-12-14 20:11:23

  • Target version changed from Tails_2.9.1 to Tails 2.10

#8 Updated by anonym 2017-01-24 20:48:50

  • Target version changed from Tails 2.10 to Tails_2.11

#9 Updated by bertagaz 2017-03-08 10:38:04

  • Target version changed from Tails_2.11 to Tails_2.12

#10 Updated by bertagaz 2017-03-08 11:09:20

  • Target version changed from Tails_2.12 to Tails_3.0

#11 Updated by bertagaz 2017-05-15 13:33:19

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

#12 Updated by bertagaz 2017-07-05 15:27:46

  • Status changed from Confirmed to In Progress
  • Assignee changed from bertagaz to intrigeri
  • % Done changed from 0 to 30
  • QA Check set to Ready for QA
  • Feature Branch set to puppet-tails:bugfix/11181-abstract-local-node

intrigeri wrote:
> * AppArmor: I don’t see why this should depend on the system; so far we’ve been enabling it everywhere; why exactly do we need exceptions now?

True.

> * GRUB_CMDLINE_LINUX: indeed, at least the part that’s about the serial console can vary, and more generally it’s good to have a parameter that we can use to add kernel parameters as needed;

I’ve pushed a branch implementing the short-term solution. I’ve also pushed a branch of the same name in the main lizard manifest Git repo, with 2 commits on top implementing the needed changes there. Please have a look if it is ready to be deployed.

> > Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.
> * Given that “local::” now means “common to all nodes managed by this set of manifests”, apparently, in this context: rename local::postfix::canonical_senders to something that makes it clear it is about stuff hosted on lizard, such as lizard::postfix::canonical_senders.
> * Drop the $set_canonical_senders parameter: it’s useless, instead we should guess dynamically if lizard::postfix::canonical_senders must be included (if $::hostname == lizard or $::fqdn =~ /\.lizard$/).

Pushed 502dd23 in the main lizard manifest Git repo, which does just that.

#13 Updated by intrigeri 2017-07-06 14:58:29

I’ll try to look at this, but I want to prioritize more pressing issues this month (my own Foundations Team work, and reviewing your work on the ISO build regressions brought by the new build setup). So I suspect I’ll eventually have to postpone this to 3.2. It’s been waiting for 16 months so it’s not exactly urgent right now :)

#14 Updated by intrigeri 2017-07-30 13:28:58

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

#15 Updated by intrigeri 2017-08-13 15:37:46

  • Feature Branch changed from puppet-tails:bugfix/11181-abstract-local-node to puppet-lizard-manifests:bugfix/11181-abstract-local-node,puppet-tails:bugfix/11181-abstract-local-node

(Apparently there are changes to review there as well.)

#16 Updated by intrigeri 2017-08-13 15:40:08

  • Description updated

#17 Updated by intrigeri 2017-08-13 16:01:53

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

>> * GRUB_CMDLINE_LINUX: indeed, at least the part that’s about the serial console can vary, and more generally it’s good to have a parameter that we can use to add kernel parameters as needed;

> I’ve pushed a branch implementing the short-term solution. I’ve also pushed a branch of the same name in the main lizard manifest Git repo, with 2 commits on top implementing the needed changes there. Please have a look if it is ready to be deployed.

Regarding commit 90b10dd6819438518c8fd2fcbfd6cf5505f2415c in puppet-lizard-manifests:

  • Please use Hiera to pass parameters (and the you can revert back to include ::local::node): the usual rationale for separating code from config applies here.
  • For the buse.riseup.net node, you passed console=tty0 via additional_kernel_parameters. Any reason not to pass it through console_kernel_parameters?

Regarding commit 06644a7fe31cbc8a7796a248381ea970927ea892 in puppet-lizard-manifests: I don’t understand what these new params are for. They don’t seem to be used anywhere. I don’t understand how this whole thing can work: I don’t see where we’re passing to tails::grub the parameters we want. Did I miss something? Perhaps just revert this change and pass them with Hiera?

I doubt $kernel_parameters = "${kernel_parameters} elevator=noop" can work: IIRC one cannot reassign a value to an already set variable in Puppet, since order does not matter and thus state can’t change.

The default value of $terminal and $console_kernel_parameters feels wrong: it means we’ll have no usable console on any new bare metal host we set up, unless it has something useful on ttyS0. I suggest using “serial console” for the terminal (so both work) and always passing console=tty0 in addition to whatever serial console we want (e.g. that’s what we currently do on buse).

>> > Another part is the postfix canonical_sender setup which is tight to services we host on Lizard.
[…]
> Pushed 502dd23 in the main lizard manifest Git repo, which does just that.

Looks good!

#18 Updated by bertagaz 2017-09-07 13:03:39

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

#19 Updated by bertagaz 2017-10-23 09:54:03

  • Target version changed from Tails_3.3 to Tails_3.5

Realistically reschedule for 3.4.

#20 Updated by bertagaz 2017-11-15 14:39:50

  • Target version changed from Tails_3.5 to Tails_3.6

#21 Updated by bertagaz 2018-03-14 11:32:10

  • Target version changed from Tails_3.6 to Tails_3.7

#23 Updated by bertagaz 2018-05-10 11:09:16

  • Target version changed from Tails_3.7 to Tails_3.8

#24 Updated by intrigeri 2018-06-26 16:27:53

  • Target version changed from Tails_3.8 to Tails_3.9

#25 Updated by Anonymous 2018-08-17 16:26:42

No progress since ~1 year but there is still some work needed. There might also be some simplifications and refactoring to do. @intrigeri, do you want to take this over? bertagaz would that work for you?

#26 Updated by intrigeri 2018-08-17 17:50:28

> intrigeri, do you want to take this over?

I’d rather mentor groente so this can be used as an opportunity to make our sysadmin team stronger.

#27 Updated by intrigeri 2018-09-05 16:26:53

  • Target version changed from Tails_3.9 to Tails_3.10.1

#28 Updated by intrigeri 2018-10-24 17:03:37

  • Target version changed from Tails_3.10.1 to Tails_3.11

#29 Updated by intrigeri 2018-11-15 10:32:45

  • blocks Bug #15071: Make our server backup process more usable added

#30 Updated by intrigeri 2018-11-15 10:35:58

For Bug #15071, due to the fact our backup machine will be managed with masterless Puppet, we need at least some of the code from classes.pp to be moved to proper modules that work with Puppet autoloading. I think there’s quite some overlap with this ticket, at least because the same pieces of code may be affected, which will result in painful merge conflicts if the work on this ticket is not completed first.

#31 Updated by intrigeri 2018-11-29 10:49:26

  • blocked by deleted (Bug #15071: Make our server backup process more usable)

#32 Updated by intrigeri 2018-11-29 10:50:22

intrigeri wrote:
> For Bug #15071, due to the fact our backup machine will be managed with masterless Puppet, we need at least some of the code from classes.pp to be moved to proper modules that work with Puppet autoloading. I think there’s quite some overlap with this ticket, at least because the same pieces of code may be affected, which will result in painful merge conflicts if the work on this ticket is not completed first.

Update: work is in progress on that other front (Bug #16167) so whoever completes the work here will have to deal with the merge conflicts.

#33 Updated by intrigeri 2018-11-29 10:50:38

  • related to Bug #16167: move classes.pp from manifests to its own module added

#34 Updated by CyrilBrulebois 2018-12-16 13:53:40

  • Target version changed from Tails_3.11 to Tails_3.12

#35 Updated by anonym 2019-01-30 11:59:14

  • Target version changed from Tails_3.12 to Tails_3.13

#36 Updated by intrigeri 2019-03-14 13:34:40

  • Target version deleted (Tails_3.13)
  • Deliverable for deleted (SponsorS_Internal)

Let’s do this as a team, as part of “Fix the design of our Puppet codebase & document design guidelines for the future” next year.