Opened 4 years ago

Closed 3 years ago

#3789 closed change (fixed)

Explicitly set $group parameter in resource definitions

Reported by: matze Assignee: ferris
Priority: P3 Milestone:
Module: Infrastructure Keywords:
Cc: Blocked By:
Blocking: #3576 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29363759/
https://codereview.adblockplus.org/29363762/

Description

Whenever the $group parameter for Puppet file and exec resource definitions is not set explicitly (analogous to $owner/$user), the default applied corresponds to the current run-time environment, which leads to inconsistent values: root in production, vagrant in development, i.e.:

==> hg1: notice: /Stage[main]/Adblockplus/File[/etc/ssh/ssh_host_rsa_key]/content: content changed '{md5}16ee67b865ad81b1ada528be31edb577' to '{md5}19ff52c2a0abd86b70feca6a587f3277'
==> hg1: notice: /Stage[main]/Adblockplus/File[/etc/ssh/ssh_host_rsa_key]/group: group changed 'root' to 'vagrant'
==> hg1: notice: /Stage[main]/Adblockplus/File[/etc/ssh/ssh_host_rsa_key.pub]/content: content changed '{md5}39cc0037dec4b4691ff3f318037bf6ff' to '{md5}7fb0b47fd38884895
c4a85d2b629677a'
==> hg1: notice: /Stage[main]/Adblockplus/File[/etc/ssh/ssh_host_rsa_key.pub]/group: group changed 'root' to 'vagrant'

Thus we need to define appropriate global defaults, and ensure them being applied to any resource not explicitly defined with the respective parameter being set.

Change History (9)

comment:1 Changed 3 years ago by ferris

There are some considerations here: https://projects.puppetlabs.com/issues/5240 - especially regarding files that already exist and whose group would be changed by puppet (let's say someone has set a group manually in the past).

We can set group in all the resource definitions, as the title here suggests, or we can set the global default (will also affect already existing files) in sites.pp. Quite a few File defaults exist at the module level already.

A quick search says there are about 160 file resources declared, with quite a few of them explicitly having group declared already, and more of them inherit it through their module defaults.

I guess the safest thing would be to go through those files on the servers that do not have a declared group and set it explicitly. One way to find them is to check on the vagrant boxes locally which files have group vagrant:

vagrant@issues1:~$ sudo find / \( -path /proc -o -path /home/vagrant -o -path /tmp -o -path /etc/puppet \) -prune -o -group vagrant -print
/etc/nginx/issues.adblockplus.org_sslcert.pem
/etc/nginx/issues.adblockplus.org_sslcert.key
/etc/nginx/dhparam.pem
/etc/ssh/ssh_host_rsa_key.pub
/etc/ssh/ssh_host_rsa_key
/etc/logrotate.d/nginx
/home/trac/environment-issues/htdocs/theme.css
/home/trac/environment-issues/conf/permissions.csv
/home/trac/trac.ini
/home/trac/htdocs-orders/htdocs/common/logo.png
/home/trac/environment-orders/htdocs/theme.css
/home/trac/environment-orders/conf/permissions.csv
/home/trac/robots.txt
/home/trac/htdocs-issues/htdocs/common/logo.png
/usr/local/lib/python2.7/dist-packages/trac_performance_fix.py
/usr/share/nginx/html/50x.html

Sound like a plan?

comment:2 Changed 3 years ago by ferris

  • Owner set to ferris

comment:3 Changed 3 years ago by matze

I see your point, but I don't think that we need that much effort. Nobody has changed groups in production in manual fashion, and even if so we would see that something has changed during provisioning and could decide per-case.

comment:4 Changed 3 years ago by ferris

OK. Should I just prep a patch where we set it at the top level (sites.pp)?

Should we then also remove the module-level group=>root defaults, or should we leave them how they are?

comment:5 Changed 3 years ago by matze

Yeah the top-level stuff should be enough, no need to remove module-level group => root defaults.

comment:6 Changed 3 years ago by ferris

  • Review URL(s) modified (diff)

comment:7 Changed 3 years ago by ferris

  • Review URL(s) modified (diff)

comment:9 Changed 3 years ago by ferris

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.