Issue 6029451183783936: Add Filtermaster (Closed)
|
Created:
Nov. 8, 2013, 8:35 a.m. by christian Modified:
Nov. 15, 2013, 8:05 a.m. Visibility:
Public. |
DescriptionAdd Filtermaster
Patch Set 1 #
Total comments: 37
Patch Set 2 : Make style changes to the Filtermaster #Patch Set 3 : Use a define for the repo download #
Total comments: 13
Patch Set 4 : Added all Changes, needs testing #Patch Set 5 : Finishing Filtermaster and Testing #
Total comments: 18
Patch Set 6 : Add the Changes #
Total comments: 4
Patch Set 7 : Finish for rollout #
Total comments: 2
Patch Set 8 : Fix it #Patch Set 9 : Add ssh #
Total comments: 11
Patch Set 10 : Fixes for ssh #Patch Set 11 : Remove duplicate file #Patch Set 12 : Remove file #
Total comments: 17
Patch Set 13 : All Done #MessagesTotal messages: 25
Added the Filtermaster to the puppet
Looks good overall, mostly style comments. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/mani... File manifests/nodes.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/mani... manifests/nodes.pp:8: import 'filtermaster.pp' We typically add "server" to all these files. So "filtermasterserver.pp", I suppose. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/files/sitescripts (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/files/sitescripts:14: #cvsroot=:pserver:trev@mozdev.org:/cvs I guess this CVS stuff is not needed anymore? Let's get rid of it then. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/files/sitescripts.ini (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/files/sitescripts.ini:1: [stats] This file is unused, isn't it? Let's remove it then. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:2: #changing cron default I find this rather obvious, same goes for the other comments below like "add user". Don't think they add anything here. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:3: Cron{ Space before "{" http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:14: } Indentation is off here. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:17: user { 'rsync': No space between "{" and "'". http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:18: ensure => present, We never align operators on columns, so just a single space before "=>" please. Likewise below. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:25: ensure => directory, Indentation is off here, should be two spaces. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:28: mode => 0600; No semicolon here. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:32: ensure => file, Indentation is off here as well. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:34: File['/home/rsync/.ssh'], Puppet normally requires parent directories automatically, are you sure this is necessary? http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:42: file {'/etc/sitescripts': I think you should pass this to the sitescripts class, instead of sitescripts.ini which I don't think we need. Or am I missing something? http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:43: ensure => file, Indentation's off again. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:51: exec { "fetch_easylist": No space before ", " should actually be '. Likewise below. More importantly, seeing how the below exec blocks are nearly identical, I think we should use a class for this to reduce duplication. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:125: cron { updateSubscription: No space after "{", and updateSubscription should be 'update_subscription' for consistency. Likewise below ("updateMalware"). http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:134: One empty line is enough. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:147: # cron { forwardErrors: Please remove this code, it's commented out anyway.
Make the Changes http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/mani... File manifests/nodes.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/mani... manifests/nodes.pp:8: import 'filtermaster.pp' On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > We typically add "server" to all these files. So "filtermasterserver.pp", I > suppose. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/files/sitescripts (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/files/sitescripts:14: #cvsroot=:pserver:trev@mozdev.org:/cvs On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > I guess this CVS stuff is not needed anymore? Let's get rid of it then. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/files/sitescripts.ini (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/files/sitescripts.ini:1: [stats] On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > This file is unused, isn't it? Let's remove it then. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:2: #changing cron default On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > I find this rather obvious, same goes for the other comments below like "add > user". Don't think they add anything here. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:3: Cron{ On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Space before "{" Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:14: } On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Indentation is off here. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:17: user { 'rsync': On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > No space between "{" and "'". Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:18: ensure => present, On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > We never align operators on columns, so just a single space before "=>" please. > Likewise below. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:25: ensure => directory, On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Indentation is off here, should be two spaces. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:28: mode => 0600; On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > No semicolon here. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:32: ensure => file, On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Indentation is off here as well. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:34: File['/home/rsync/.ssh'], On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Puppet normally requires parent directories automatically, are you sure this is > necessary? I think it's needed to set the permissions of the .ssh dir right. SSH don't work if the permissions are not correct. So yes this is necessary in my opinion. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:42: file {'/etc/sitescripts': On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > I think you should pass this to the sitescripts class, instead of > sitescripts.ini which I don't think we need. Or am I missing something? The scripts are not taking the sitescripts.ini file. They need a sitescripts file. Don't know why. Also the content of the file is only need for the filtermaster http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:43: ensure => file, On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Indentation's off again. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:125: cron { updateSubscription: On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > No space after "{", and updateSubscription should be 'update_subscription' for > consistency. Likewise below ("updateMalware"). Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:134: On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > One empty line is enough. Done. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:147: # cron { forwardErrors: On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > Please remove this code, it's commented out anyway. Done.
The authorized_keys should be inside the private-stub directory - we should have one for testing and a private one for production. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... File modules/filtermaster/files/sitescripts (right): http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/files/sitescripts:13: outdir=%(root)s/files/subscriptions A few more entries have been added recently: easylistdutch_repository=%(root)s/easylistdutch facebookfilters_repository=%(root)s/facebookfilters antiadblockfilters_repository=%(root)s/antiadblockfilters http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:3: environment => ['MAILTO=ROOT', 'PYTHONPATH=/opt/sitescripts'], Add a TODO comment so that you don't forget to change the MAILTO variable before committing? http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:12: ForceCommand rsync --server --sender -vltprz --delete-excluded --exclude CVS . /home/rsync/subscriptions/' /home/rsync/subscriptions isn't what you want to sync, /home/rsync/subscriptions/files/subscriptions is (as per your sitescripts.ini). --exclude CVS is unnecessary, you don't have CVS syncing any more. Note: there is a change under review (http://codereview.adblockplus.org/28037010/), once it is committed there will be an additional data/ at the end of the path. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:47: #donwload the repos Typo: donwload => download http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:57: } What about a cron job to update these repositories? http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:65: name => "easylistgermany" This looks redundant, why not drop the $name parameter and only use $title? Then it would simply be: repo_download {'easylistgermany': }
http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:34: File['/home/rsync/.ssh'], On 2013/11/08 11:36:56, cvervoorts wrote: > On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > > Puppet normally requires parent directories automatically, are you sure this > is > > necessary? > > I think it's needed to set the permissions of the .ssh dir right. SSH don't work > if the permissions are not correct. So yes this is necessary in my opinion. I'd be really surprised if it was necessary. Can you try a fresh provision without this? It should work. If it doesn't, we should figure out why and add a comment here. http://codereview.adblockplus.org/6029451183783936/diff/5629499534213120/modu... modules/filtermaster/manifests/init.pp:42: file {'/etc/sitescripts': On 2013/11/08 11:36:56, cvervoorts wrote: > On 2013/11/08 08:55:58, Felix H. Dahlke wrote: > > I think you should pass this to the sitescripts class, instead of > > sitescripts.ini which I don't think we need. Or am I missing something? > > The scripts are not taking the sitescripts.ini file. They need a sitescripts > file. Don't know why. Also the content of the file is only need for the > filtermaster You use sitescriptsini_source at the bottom of the file - it points to a now nonexistent file (sitescripts.ini). How I see it, you should be able to get rid of this file resource here and just point to puppet:///modules/filtermaster/sitescripts down there. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:47: #donwload the repos On 2013/11/08 15:32:06, Wladimir Palant wrote: > Typo: donwload => download I'd actually remove this, "repo_download" pretty much says it all. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:65: name => "easylistgermany" On 2013/11/08 15:32:06, Wladimir Palant wrote: > This looks redundant, why not drop the $name parameter and only use $title? Then > it would simply be: > > repo_download {'easylistgermany': } Yes, then you could actually do this: repo_download {['easylistgermany', 'easylist_italy', '...']: } http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:92: cron {update_subscription: Still needs to go in '' for consistency, same below.
Ok, have add the changes. I will test it tommorrow. But send in what I have so far. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:3: environment => ['MAILTO=ROOT', 'PYTHONPATH=/opt/sitescripts'], On 2013/11/08 15:32:06, Wladimir Palant wrote: > Add a TODO comment so that you don't forget to change the MAILTO variable before > committing? Done. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:12: ForceCommand rsync --server --sender -vltprz --delete-excluded --exclude CVS . /home/rsync/subscriptions/' On 2013/11/08 15:32:06, Wladimir Palant wrote: > /home/rsync/subscriptions isn't what you want to sync, > /home/rsync/subscriptions/files/subscriptions is (as per your sitescripts.ini). > > --exclude CVS is unnecessary, you don't have CVS syncing any more. > > Note: there is a change under review > (http://codereview.adblockplus.org/28037010/), once it is committed there will > be an additional data/ at the end of the path. Done. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:47: #donwload the repos On 2013/11/08 15:32:06, Wladimir Palant wrote: > Typo: donwload => download Done. http://codereview.adblockplus.org/6029451183783936/diff/5676830073815040/modu... modules/filtermaster/manifests/init.pp:57: } On 2013/11/08 15:32:06, Wladimir Palant wrote: > What about a cron job to update these repositories? Don't "updateSubscriptionDownloads" update all repos? Is there a need for an additional cron job?
I finished the filtermaster and added a script which updates the repos. It's a bash script until this feature is added to the sitescripts repo.
http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... File modules/filtermaster/files/update_repos.sh (right): http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/files/update_repos.sh:9: cd - These four commands can be replaced by one: hg pull -R $i -q --update Note that the -q parameter is required, otherwise cron will spam us with the Mercurial output. Note that this will also run for the directory containing generated files - best to move it out of /home/rsync/subscription/ IMHO. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/files/update_repos.sh:10: done I thought by "batch script" you actually meant Python :) Well, I guess this is fine since we have no intention of keeping this script. However, please don't use bash in future - maintaining it is horrible. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:13: ForceCommand rsync --server --sender -vltprz --delete-excluded . /home/rsync/subscription/files/subscriptions/data' I think the slash at the end of the path isn't optional here. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:49: ], These requires aren't necessary - both parent directory and owner are autorequires (see http://docs.puppetlabs.com/references/latest/type.html#file-description). Same applies to the other files you are creating of course. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:53: file {'/home/rsync/subscription/files/subscriptions': Why so complicated? Why not go with something like /home/rsync/generated and change the configuration in sitescripts.ini? http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:97: ]: Please indent using spaces, not smart tabs. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:123: command => "/home/rsync/update_repos.sh 1>/dev/null", Please drop stream redirection here, the script simply shouldn't generate any output. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:127: File['/home/rsync/update_repos.sh'] Wrong indentation here, the tw lines above should be indented by two additional spaces (and similarly in a bunch of other places). http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:129: minute => '*/5' It doesn't make sense to update repositories at the same time when subscriptions are being generated. It also doesn't make sense to update repositories twice as frequently as subscriptions are being generated. I think you want minute => '8-58/10'
I think the filtermaster is ready (of course, i have to change the mail address and the ssh file) http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... File modules/filtermaster/files/update_repos.sh (right): http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/files/update_repos.sh:9: cd - On 2013/11/12 13:04:03, Wladimir Palant wrote: > These four commands can be replaced by one: > > hg pull -R $i -q --update > > Note that the -q parameter is required, otherwise cron will spam us with the > Mercurial output. > > Note that this will also run for the directory containing generated files - best > to move it out of /home/rsync/subscription/ IMHO. Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/files/update_repos.sh:10: done On 2013/11/12 13:04:03, Wladimir Palant wrote: > I thought by "batch script" you actually meant Python :) > > Well, I guess this is fine since we have no intention of keeping this script. > However, please don't use bash in future - maintaining it is horrible. I think i sad "bash script" but sometimes hard understand if I say something in English ;) Ok, just thought it will be much easier to build in bash than in python. But next time I make a python one. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:13: ForceCommand rsync --server --sender -vltprz --delete-excluded . /home/rsync/subscription/files/subscriptions/data' On 2013/11/12 13:04:03, Wladimir Palant wrote: > I think the slash at the end of the path isn't optional here. Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:49: ], On 2013/11/12 13:04:03, Wladimir Palant wrote: > These requires aren't necessary - both parent directory and owner are > autorequires (see > http://docs.puppetlabs.com/references/latest/type.html#file-description). Same > applies to the other files you are creating of course. Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:53: file {'/home/rsync/subscription/files/subscriptions': On 2013/11/12 13:04:03, Wladimir Palant wrote: > Why so complicated? Why not go with something like /home/rsync/generated and > change the configuration in sitescripts.ini? Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:97: ]: On 2013/11/12 13:04:03, Wladimir Palant wrote: > Please indent using spaces, not smart tabs. Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:123: command => "/home/rsync/update_repos.sh 1>/dev/null", On 2013/11/12 13:04:03, Wladimir Palant wrote: > Please drop stream redirection here, the script simply shouldn't generate any > output. Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:127: File['/home/rsync/update_repos.sh'] On 2013/11/12 13:04:03, Wladimir Palant wrote: > Wrong indentation here, the tw lines above should be indented by two additional > spaces (and similarly in a bunch of other places). Done. http://codereview.adblockplus.org/6029451183783936/diff/5689792285114368/modu... modules/filtermaster/manifests/init.pp:129: minute => '*/5' On 2013/11/12 13:04:03, Wladimir Palant wrote: > It doesn't make sense to update repositories at the same time when subscriptions > are being generated. It also doesn't make sense to update repositories twice as > frequently as subscriptions are being generated. I think you want minute => > '8-58/10' Done.
I think that the pointless requires are the only issue left - if Felix agrees we can start testing this on a real server. http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... modules/filtermaster/manifests/init.pp:46: require => User['rsync'], Requiring the directory owner is still unnecessary - here and for the other files created... http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... modules/filtermaster/manifests/init.pp:55: ], Both these requires are unnecessary (autorequired).
Ready :) http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... modules/filtermaster/manifests/init.pp:46: require => User['rsync'], On 2013/11/13 11:59:34, Wladimir Palant wrote: > Requiring the directory owner is still unnecessary - here and for the other > files created... Done. http://codereview.adblockplus.org/6029451183783936/diff/5697982787747840/modu... modules/filtermaster/manifests/init.pp:55: ], On 2013/11/13 11:59:34, Wladimir Palant wrote: > Both these requires are unnecessary (autorequired). Done.
LGTM with the remaining nit fixed. http://codereview.adblockplus.org/6029451183783936/diff/5665117697998848/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5665117697998848/modu... modules/filtermaster/manifests/init.pp:48: require => File['/home/rsync/.ssh'], As I said, *both* requires are unnecessary - the parent directory is automatically "required".
Sorry, overlooked it. http://codereview.adblockplus.org/6029451183783936/diff/5665117697998848/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5665117697998848/modu... modules/filtermaster/manifests/init.pp:48: require => File['/home/rsync/.ssh'], On 2013/11/13 14:03:51, Wladimir Palant wrote: > As I said, *both* requires are unnecessary - the parent directory is > automatically "required". Done.
LGTM
On 2013/11/13 15:05:51, Felix H. Dahlke wrote: > LGTM I take that back actually, the authorized_key files is not in private-stub yet.
http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster-authorized_keys (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster-authorized_keys:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDX4XK+lIIFKbqahR9AWxFtMoAbsZkwZXXffzgNvTbgWE0+4JujBQvoP4Nz2gbg18M/pN0pPgm2Lda1WwSs79qKOC9iRVb5LtEQHYLaif78E/rGEuqlclnkzybqx9+0qGdTwVoN4NylaKhLnvWWOlWRlk8FMKwhc68cnW7vIJYx4Wy27moHG5Nfiv9r3+bi70brfM9paQ/nWZCBFNphKAXyRx1rhutheU4MdGNqyPln2QC8x4sP5AgKnX4txSuNbYMCrMiBh3JrJrLIe+V+J056ciseyUmBix53LZKvOTHVRHrYA6wcJDyF6grhtk6jvx1TAqoCzf4smaivppIke/qr root@easylist-downloads.adblockplus.org No need to duplicate that file, just use rsync@easylist-downloads.adblockplus.org.pub instead (feel free to replace root@ by rsync@ while at it). We can use concat module should we ever need to authorize multiple keys here. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster.adblockplus.org (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster.adblockplus.org:27: -----END RSA PRIVATE KEY----- I think that file should be called filtermaster.adblockplus.org_ssh.key for consistency - otherwise it isn't clear what service this belongs to. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster.adblockplus.org.pub (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster.adblockplus.org.pub:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzh5xuxjobpDqF6QhpCiD9SDg8MBZ4ki4g9Da2FXIyoUv9bsv9Fdy/cXoaQ8vJY/+iM7Ibu31FqjsF68kS48NKj19AjHK8D7EHWhxGJGICF3zgIwjzkj3bB3heQA2SwMjoVauPdaygQXd7qihunR9A5ViCED7RtCL+HyECXPd2isAluaaQnh4Yt3ueF9Bd6bfu9aLwo7eRgGaswvUd99B3ZtjgoSXh8e3OmVqAGdnQRPPsxrs1Q+wA8geitSJMA+MUBdVcihXAlCp6AblgiWcy9vIGA7ZNCIvqFwROx0cz96lun5v0vWw2xAUZD0ScqUW0HMq3Gcj92uJZfXTnqect christian@thinkpad I think that file should be called filtermaster.adblockplus.org_ssh.pub for consistency - otherwise it isn't clear what service this belongs to. Also, please replace christian@thinkpad by root@filtermaster.adblockplus.org. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/rsync@filterserver_known_hosts (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/rsync@filterserver_known_hosts:2: filtermaster1.adblockplus.org,10.8.0.127 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzh5xuxjobpDqF6QhpCiD9SDg8MBZ4ki4g9Da2FXIyoUv9bsv9Fdy/cXoaQ8vJY/+iM7Ibu31FqjsF68kS48NKj19AjHK8D7EHWhxGJGICF3zgIwjzkj3bB3heQA2SwMjoVauPdaygQXd7qihunR9A5ViCED7RtCL+HyECXPd2isAluaaQnh4Yt3ueF9Bd6bfu9aLwo7eRgGaswvUd99B3ZtjgoSXh8e3OmVqAGdnQRPPsxrs1Q+wA8geitSJMA+MUBdVcihXAlCp6AblgiWcy9vIGA7ZNCIvqFwROx0cz96lun5v0vWw2xAUZD0ScqUW0HMq3Gcj92uJZfXTnqect No point duplicating the information here. The first line is unnecessary anyway, you only need the second line. You can use concat module to generate that file (http://forge.puppetlabs.com/puppetlabs/concat). Something along these lines: concat{'/home/rsync/.ssh/known_hosts': owner => rsync, mode => 0444, } concat::fragment{'filtermaster_hostname': target => '/home/rsync/.ssh/known_hosts', content => 'filtermaster.adblockplus.org ', order => 1, } concat::fragment{'filtermaster_hostkey': target => '/home/rsync/.ssh/known_hosts', source => 'puppet:///modules/private/filtermaster.adblockplus.org_ssh.pub', order => 2, } Note that the server should be referred to as filtermaster, not filtermaster1. Normally the DNS entry for filtermaster will point to filtermaster1 but we might change it if we decide to replace that server.
Make the changes to the ssh handling http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster-authorized_keys (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster-authorized_keys:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDX4XK+lIIFKbqahR9AWxFtMoAbsZkwZXXffzgNvTbgWE0+4JujBQvoP4Nz2gbg18M/pN0pPgm2Lda1WwSs79qKOC9iRVb5LtEQHYLaif78E/rGEuqlclnkzybqx9+0qGdTwVoN4NylaKhLnvWWOlWRlk8FMKwhc68cnW7vIJYx4Wy27moHG5Nfiv9r3+bi70brfM9paQ/nWZCBFNphKAXyRx1rhutheU4MdGNqyPln2QC8x4sP5AgKnX4txSuNbYMCrMiBh3JrJrLIe+V+J056ciseyUmBix53LZKvOTHVRHrYA6wcJDyF6grhtk6jvx1TAqoCzf4smaivppIke/qr root@easylist-downloads.adblockplus.org On 2013/11/14 13:02:27, Wladimir Palant wrote: > No need to duplicate that file, just use > mailto:rsync@easylist-downloads.adblockplus.org.pub instead (feel free to replace root@ > by rsync@ while at it). We can use concat module should we ever need to > authorize multiple keys here. Done. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster.adblockplus.org (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster.adblockplus.org:27: -----END RSA PRIVATE KEY----- On 2013/11/14 13:02:27, Wladimir Palant wrote: > I think that file should be called filtermaster.adblockplus.org_ssh.key for > consistency - otherwise it isn't clear what service this belongs to. Done. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster.adblockplus.org.pub (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster.adblockplus.org.pub:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzh5xuxjobpDqF6QhpCiD9SDg8MBZ4ki4g9Da2FXIyoUv9bsv9Fdy/cXoaQ8vJY/+iM7Ibu31FqjsF68kS48NKj19AjHK8D7EHWhxGJGICF3zgIwjzkj3bB3heQA2SwMjoVauPdaygQXd7qihunR9A5ViCED7RtCL+HyECXPd2isAluaaQnh4Yt3ueF9Bd6bfu9aLwo7eRgGaswvUd99B3ZtjgoSXh8e3OmVqAGdnQRPPsxrs1Q+wA8geitSJMA+MUBdVcihXAlCp6AblgiWcy9vIGA7ZNCIvqFwROx0cz96lun5v0vWw2xAUZD0ScqUW0HMq3Gcj92uJZfXTnqect christian@thinkpad On 2013/11/14 13:02:27, Wladimir Palant wrote: > I think that file should be called filtermaster.adblockplus.org_ssh.pub for > consistency - otherwise it isn't clear what service this belongs to. > > Also, please replace christian@thinkpad by mailto:root@filtermaster.adblockplus.org. Done. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/rsync@filterserver_known_hosts (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/rsync@filterserver_known_hosts:2: filtermaster1.adblockplus.org,10.8.0.127 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzh5xuxjobpDqF6QhpCiD9SDg8MBZ4ki4g9Da2FXIyoUv9bsv9Fdy/cXoaQ8vJY/+iM7Ibu31FqjsF68kS48NKj19AjHK8D7EHWhxGJGICF3zgIwjzkj3bB3heQA2SwMjoVauPdaygQXd7qihunR9A5ViCED7RtCL+HyECXPd2isAluaaQnh4Yt3ueF9Bd6bfu9aLwo7eRgGaswvUd99B3ZtjgoSXh8e3OmVqAGdnQRPPsxrs1Q+wA8geitSJMA+MUBdVcihXAlCp6AblgiWcy9vIGA7ZNCIvqFwROx0cz96lun5v0vWw2xAUZD0ScqUW0HMq3Gcj92uJZfXTnqect On 2013/11/14 13:02:27, Wladimir Palant wrote: > No point duplicating the information here. The first line is unnecessary anyway, > you only need the second line. You can use concat module to generate that file > (http://forge.puppetlabs.com/puppetlabs/concat). Something along these lines: > > concat{'/home/rsync/.ssh/known_hosts': > owner => rsync, > mode => 0444, > } > > concat::fragment{'filtermaster_hostname': > target => '/home/rsync/.ssh/known_hosts', > content => 'filtermaster.adblockplus.org ', > order => 1, > } > > concat::fragment{'filtermaster_hostkey': > target => '/home/rsync/.ssh/known_hosts', > source => 'puppet:///modules/private/filtermaster.adblockplus.org_ssh.pub', > order => 2, > } > > Note that the server should be referred to as filtermaster, not filtermaster1. > Normally the DNS entry for filtermaster will point to filtermaster1 but we might > change it if we decide to replace that server. Done.
http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster-authorized_keys (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster-authorized_keys:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDX4XK+lIIFKbqahR9AWxFtMoAbsZkwZXXffzgNvTbgWE0+4JujBQvoP4Nz2gbg18M/pN0pPgm2Lda1WwSs79qKOC9iRVb5LtEQHYLaif78E/rGEuqlclnkzybqx9+0qGdTwVoN4NylaKhLnvWWOlWRlk8FMKwhc68cnW7vIJYx4Wy27moHG5Nfiv9r3+bi70brfM9paQ/nWZCBFNphKAXyRx1rhutheU4MdGNqyPln2QC8x4sP5AgKnX4txSuNbYMCrMiBh3JrJrLIe+V+J056ciseyUmBix53LZKvOTHVRHrYA6wcJDyF6grhtk6jvx1TAqoCzf4smaivppIke/qr root@easylist-downloads.adblockplus.org On 2013/11/14 13:31:26, cvervoorts wrote: > Done. Doesn't look like it. http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/rsync@filterserver_known_hosts (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/rsync@filterserver_known_hosts:2: filtermaster1.adblockplus.org,10.8.0.127 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDzh5xuxjobpDqF6QhpCiD9SDg8MBZ4ki4g9Da2FXIyoUv9bsv9Fdy/cXoaQ8vJY/+iM7Ibu31FqjsF68kS48NKj19AjHK8D7EHWhxGJGICF3zgIwjzkj3bB3heQA2SwMjoVauPdaygQXd7qihunR9A5ViCED7RtCL+HyECXPd2isAluaaQnh4Yt3ueF9Bd6bfu9aLwo7eRgGaswvUd99B3ZtjgoSXh8e3OmVqAGdnQRPPsxrs1Q+wA8geitSJMA+MUBdVcihXAlCp6AblgiWcy9vIGA7ZNCIvqFwROx0cz96lun5v0vWw2xAUZD0ScqUW0HMq3Gcj92uJZfXTnqect Looks like you forgot to remove this file however.
Fixed http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... File modules/private-stub/files/filtermaster-authorized_keys (right): http://codereview.adblockplus.org/6029451183783936/diff/5784178486411264/modu... modules/private-stub/files/filtermaster-authorized_keys:1: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDX4XK+lIIFKbqahR9AWxFtMoAbsZkwZXXffzgNvTbgWE0+4JujBQvoP4Nz2gbg18M/pN0pPgm2Lda1WwSs79qKOC9iRVb5LtEQHYLaif78E/rGEuqlclnkzybqx9+0qGdTwVoN4NylaKhLnvWWOlWRlk8FMKwhc68cnW7vIJYx4Wy27moHG5Nfiv9r3+bi70brfM9paQ/nWZCBFNphKAXyRx1rhutheU4MdGNqyPln2QC8x4sP5AgKnX4txSuNbYMCrMiBh3JrJrLIe+V+J056ciseyUmBix53LZKvOTHVRHrYA6wcJDyF6grhtk6jvx1TAqoCzf4smaivppIke/qr root@easylist-downloads.adblockplus.org On 2013/11/14 14:10:23, Wladimir Palant wrote: > On 2013/11/14 13:31:26, cvervoorts wrote: > > Done. > > Doesn't look like it. Overlooked it, sorry.
modules/private-stub/files/rsync@filterserver_known_hosts still seems to be part of the patch even though it isn't used. LGTM with that fixed.
On 2013/11/14 14:42:54, Wladimir Palant wrote: > modules/private-stub/files/rsync@filterserver_known_hosts still seems to be part > of the patch even though it isn't used. LGTM with that fixed. Fixed
LGTM
http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:30: file {'/home/rsync/.ssh': Can you move this down so it's right before authorized_keys? http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:53: file {'/etc/ssh/ssh_host_rsa_key': This should require the ssh package to be installed, the same goes for the resource below. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:54: notify => Service["ssh"], Only once space before =>. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:71: define repo_download( ) { No space between ( and ). http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:72: exec { "fetch_${title}": No space before ", and ' instead of ". http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:111: User['rsync'] Just |require => User['rsync']| please. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:122: User['rsync'], Can you indent this and the File below one level? http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... File modules/filterserver/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filterserver/manifests/init.pp:87: concat{'/home/rsync/.ssh/known_hosts': Space before {, likewise below. Using a template here would really be easier, but this is fine by me.
Should be everything http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:30: file {'/home/rsync/.ssh': On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > Can you move this down so it's right before authorized_keys? Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:53: file {'/etc/ssh/ssh_host_rsa_key': On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > This should require the ssh package to be installed, the same goes for the > resource below. Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:54: notify => Service["ssh"], On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > Only once space before =>. Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:71: define repo_download( ) { On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > No space between ( and ). Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:72: exec { "fetch_${title}": On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > No space before ", and ' instead of ". remove space, will broke if i replace the " because of the variable replacement will not work. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:111: User['rsync'] On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > Just |require => User['rsync']| please. Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:122: User['rsync'], On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > Can you indent this and the File below one level? Done. http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... File modules/filterserver/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filterserver/manifests/init.pp:87: concat{'/home/rsync/.ssh/known_hosts': On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > Space before {, likewise below. > > Using a template here would really be easier, but this is fine by me. Done.
Nice, LGTM! http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/6029451183783936/diff/5763418761986048/modu... modules/filtermaster/manifests/init.pp:72: exec { "fetch_${title}": On 2013/11/14 15:26:43, cvervoorts wrote: > On 2013/11/14 15:07:31, Felix H. Dahlke wrote: > > No space before ", and ' instead of ". > > remove space, will broke if i replace the " because of the variable replacement > will not work. Oops, right. 
|
