Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5766400081657856: #760 - Make our ssh configuration used automatically for all servers (Closed)

Created:
July 15, 2014, 11:03 a.m. by mathias
Modified:
July 16, 2014, 12:29 p.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

See https://issues.adblockplus.org/ticket/760

Patch Set 1 #

Total comments: 10

Patch Set 2 : #760 - Make our ssh configuration used automatically for all servers #

Patch Set 3 : #760 - Make our ssh configuration used automatically for all servers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -20 lines) Patch
M modules/base/manifests/init.pp View 1 chunk +1 line, -1 line 0 comments Download
M modules/filtermaster/manifests/init.pp View 1 2 1 chunk +19 lines, -10 lines 0 comments Download
M modules/ssh/files/sshd_config View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M modules/ssh/manifests/init.pp View 1 2 1 chunk +10 lines, -5 lines 0 comments Download
M modules/statsclient/manifests/init.pp View 1 2 1 chunk +7 lines, -3 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp#newcode8 modules/filtermaster/manifests/init.pp:8: order => '02', This doesn't need to be sequential, ...
July 15, 2014, 11:56 a.m. (2014-07-15 11:56:47 UTC) #1
mathias
http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp File modules/filtermaster/manifests/init.pp (right): http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modules/filtermaster/manifests/init.pp#newcode17 modules/filtermaster/manifests/init.pp:17: content => ' On 2014/07/15 11:56:47, Wladimir Palant wrote: ...
July 15, 2014, 12:03 p.m. (2014-07-15 12:03:12 UTC) #2
mathias
See https://issues.adblockplus.org/ticket/760
July 15, 2014, 12:59 p.m. (2014-07-15 12:59:35 UTC) #3
mathias
July 15, 2014, 1:21 p.m. (2014-07-15 13:21:54 UTC) #4
Wladimir Palant
July 15, 2014, 2:18 p.m. (2014-07-15 14:18:35 UTC) #5
LGTM

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
File modules/filtermaster/manifests/init.pp (right):

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
modules/filtermaster/manifests/init.pp:17: content => '
On 2014/07/15 12:03:12, matze wrote:
> Either the non-block settings (see your comment above) or block settings
should
> become sequential. I dislike to do that for both types. Which do you prefer?
(I
> prefer the former option.)

I don't really understand your comment. My understanding of sshd_config is that
ordering matters - Match blocks always have to be placed at the end of the file.
It might be that omitting the order parameter will also place the fragment at
the end but the documentation doesn't really say what will happen in that
scenario so I would consider the behavior as undefined. Hence my suggestion to
always use '99' for blocks.

Using '50' instead of '02' was meant simply in case we need to add more
fragments to the generic part. We can have as convention that anything below
'50' is generic whereas '50' and above marks custom configuration.

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
File modules/ssh/templates/sshd_config.erb (right):

http://codereview.adblockplus.org/5766400081657856/diff/5629499534213120/modu...
modules/ssh/templates/sshd_config.erb:90: UsePAM yes
On 2014/07/15 12:03:12, matze wrote:
> Well, I could imagine performing some future global setup by passing class
> parameters through to the template

Well, I cannot - that fragment is being created via base module, we don't have
any server-specific parameters at this point so I don't see any reason why we
would use templates. Either way, our general approach is not to keep things
around just because we might need it at some point.

Powered by Google App Engine
This is Rietveld