#760 closed change (fixed)
Make our ssh configuration used automatically for all servers
Reported by: | trev | Assignee: | matze |
---|---|---|---|
Priority: | P2 | Milestone: | |
Module: | Infrastructure | Keywords: | |
Cc: | fhd, matze | Blocked By: | #765, #766 |
Blocking: | Platform: | Unknown | |
Ready: | yes | Confidential: | no |
Tester: | Verified working: | no | |
Review URL(s): |
http://codereview.adblockplus.org/5766400081657856 |
Description
Background
It seems that not all servers are currently using our sshd_config. The ssh module isn't being used consistently, currently it is used by filtermaster and statsclient modules (with some custom configuration) and in the monitoring server configuration. Other servers still use the default configuration which accepts passwords for example.
What to change
Make sure the ssh module is included by the base module, so that it is used by all servers consistently. We won't be able to pass in class parameters then, since the base module cannot know whether custom configuration is necessary. We can add custom configuration directives using the concat module however, we've already implemented it this way for the sitescripts.ini file.
Change History (12)
comment:1 Changed 5 years ago by matze
- Owner set to matze
comment:2 Changed 5 years ago by matze
comment:3 Changed 5 years ago by trev
The issue is really that the ssh module can only be included once. If we include it from the base module then we won't be able to invoke it later with different parameters. The sitescripts module solves this issue by using the concat class - one fragment is defined when the sitescripts module is included, other modules can define additional fragments as needed. We can use the same approach for the ssh module, meaning that filtermaster or statsclient will define concat fragments for their server-specific SSH directives.
comment:4 Changed 5 years ago by matze
OK, got it, thank you.
comment:5 Changed 5 years ago by matze
- Blocked By 765 added
comment:6 Changed 5 years ago by matze
- Blocked By 766 added; 765 removed
comment:7 Changed 5 years ago by matze
- Blocked By 772 added; 766 removed
comment:8 Changed 5 years ago by trev
- Blocked By 765, 766 added
comment:9 Changed 5 years ago by matze
- Review URL(s) modified (diff)
- Status changed from new to reviewing
One question: Is the "base" relation configured via manifests/*server.pp enough?
mhennig@kali:~/AdBlockPlus/infrastructure$ grep base manifests/* manifests/codereviewserver.pp: include base manifests/downloadserver.pp: include base, statsclient manifests/filtermasterserver.pp: include base, filtermaster manifests/filterserver.pp: include base, statsclient manifests/intraforumserver.pp: include base, statsclient manifests/issuesserver.pp: include base manifests/monitoringserver.pp: include base, ssh, puppetmaster manifests/statsserver.pp: include base manifests/updateserver.pp: include base, statsclient manifests/webserver.pp: include base, statsclient
Especially when using concat::fragment for sshd_config (which is set up in modules/ssh and then included in modules/base), wouldn't it be more clean if we include base or include ssh explicitly?
(See also the review link.)
comment:10 Changed 5 years ago by matze
I've just pushed the changes, after the review was successful. How do we proceed? Who's going to deploy the modifications? And do I need to re-assign the ticket or just close it?
comment:11 Changed 5 years ago by trev
- Blocked By 772 removed
- Resolution set to fixed
- Status changed from reviewing to closed
Fixed: https://hg.adblockplus.org/infrastructure/rev/8d7c8df29eba. Please close the review as well.
Let's have a brief screen sharing session tomorrow so that you can see how deployment works. Normally we deploy immediately after the changes landed.
comment:12 Changed 5 years ago by matze
- Review URL(s) modified (diff)
Please explain the issue you've mentioned in the "What to change" section above.
As far as I can see, custom configuration is already implemented in the ssh module, using templating (modules/ssh/templates/sshd_config.erb, last line) and an empty default parameter (modules/ssh/manifests/init.pp), and applied as parameter in e.g. modules/statsclient/manifests/init.pp.
Although I understand that this is more a hack than a clean solution (like the concat module would have been), I do not see an upcoming issue - except maybe in case of multiple roles and inheritance. So, before I convert this to use concat, I just want to make sure we're on the same page --