Issue 29551585: #2317 - provide role fileserver for eyeofiles.com (Closed)
|
Created:
Sept. 21, 2017, 8:46 a.m. by f.nicolaisen Modified:
Sept. 29, 2017, 10:31 a.m. Base URL:
https://hg1/infrastructure Visibility:
Public. |
Description#2317 - provide role fileserver for eyeofiles.com
Patch Set 1 #
Total comments: 18
Patch Set 2 : move into adblockplus::web namespace and other improvements #Patch Set 3 : replace hardcoded repository with definition #
Total comments: 16
Patch Set 4 : make repository into subdomains #Patch Set 5 : use single nginx conf based on $host #
Total comments: 14
Patch Set 6 : more flexible resource definitions #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... File modules/fileserver/manifests/init.pp (right): https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:2: class fileserver( This should be an entity within namespace adblockplus::web, e.g. adblockplus::web::fileserver. My further feedback will assume it is (you'll see why). https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:9: class {'nginx': Please use `include` when there are no explicit parameters required. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:19: file {'/var/files': Probably obsolete, see below. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:23: file {'/var/www': You can drop this one in favor of including the adblockplus::web class. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:27: # Directory with new group www-v8 where uploaders can be made members of: What for? That should be documented. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:31: }-> Please avoid using this form of relationship notation, it's easily overseen and hard to grep for. You can always use Explicit[foo] -> Bar[baz] notation after the resource definition (recommended), or just the plain old parameters where applicable (outside the ensure_resource stuff). https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:33: file {'/var/files/v8': What for? This should be something with a more meaningful basename, and it should be a sub-directory of $adblockplus::directory (requires inclusion of class adblockplus and explicit realization). https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:40: file {'/var/www/v8': What for? That should be documented. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:54: log => 'access_log_update' Why *_update?
https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... File modules/fileserver/manifests/init.pp (right): https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:2: class fileserver( On 2017/09/21 11:43:20, mathias wrote: > This should be an entity within namespace adblockplus::web, e.g. > adblockplus::web::fileserver. My further feedback will assume it is (you'll see > why). Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:9: class {'nginx': On 2017/09/21 11:43:20, mathias wrote: > Please use `include` when there are no explicit parameters required. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:19: file {'/var/files': On 2017/09/21 11:43:20, mathias wrote: > Probably obsolete, see below. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:23: file {'/var/www': On 2017/09/21 11:43:20, mathias wrote: > You can drop this one in favor of including the adblockplus::web class. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:27: # Directory with new group www-v8 where uploaders can be made members of: On 2017/09/21 11:43:20, mathias wrote: > What for? That should be documented. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:31: }-> On 2017/09/21 11:43:20, mathias wrote: > Please avoid using this form of relationship notation, it's easily overseen and > hard to grep for. You can always use Explicit[foo] -> Bar[baz] notation after > the resource definition (recommended), or just the plain old parameters where > applicable (outside the ensure_resource stuff). Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:33: file {'/var/files/v8': On 2017/09/21 11:43:20, mathias wrote: > What for? This should be something with a more meaningful basename, and it > should be a sub-directory of $adblockplus::directory (requires inclusion of > class adblockplus and explicit realization). Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:40: file {'/var/www/v8': On 2017/09/21 11:43:19, mathias wrote: > What for? That should be documented. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551586/modules/fileserver/... modules/fileserver/manifests/init.pp:54: log => 'access_log_update' On 2017/09/21 11:43:20, mathias wrote: > Why *_update? Acknowledged.
PS2 addresses all the comments. I wasn't sure how to work with the virtual resources, so that is mostly guesswork. It is still hard-coded to only one directory for 'v8', or "repository" as I called it now. It should be possible to implement repositories as instances of a potential adblockplus::web::fileserver:repository class, but I'm not quite sure how to make that work yet.
On 2017/09/21 20:43:40, f.nicolaisen wrote: > PS2 addresses all the comments. I wasn't sure how to work with the virtual > resources, so that is mostly guesswork. > > It is still hard-coded to only one directory for 'v8', or "repository" as I > called it now. It should be possible to implement repositories as instances of a > potential adblockplus::web::fileserver:repository class, but I'm not quite sure > how to make that work yet. OK, I figured out how to replace the hard-coded stuff with a adblockplus::web::fileserver:repository in PS3.
https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:4: location / Actually the idea was to use sub-domains (e.g. v8.eyeofiles.com), where each sub-domain would map to an element within a root directory (i.e. ${adblockplus::web::directory}/v8.eyeofiles.com -> ${adblockplus::directory}/fileserver/v8). https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver.pp (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:8: # A string for the domain serving traffic. Why would the domain want to have some string? And how does a domain serve traffic anyway? Isn't this more the name of the domain associated with the service, given as a string? https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:18: # [*is_default*] The fileserver is a standalone HTTPd setup intended for use with a wildcard-cert, hence I'd say $nginx::hostconfig::is_default should always be true (hard-coded inside this class here, not a parameter from the outside). https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:21: # [*repository*] The documentation of the $repository parameter does not fit anymore. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:29: $certificate, Both $certificate and $private_key should default to undef, which allows to not use a self-signed certificate in development but plain HTTP, which is what we do in new setups since a few years. The idea is to not introduce complexity in development, so one does not have fun clicking away pointless browser messages, does not need hacking exceptions into adjacent modules and scripts (i.e. --no-check-certificate), does not need to maintain development CAs, and does not learn how to ignore security warnings. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:59: if !empty($repositories) { This conditional is not necessary: Iterating through an empty $repositories hash (the default) won't do anything. You of course need to remove the pointless assignment of undef/null in the associated role.yaml file. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:60: ensure_resources('repository', $repositories, { Please use the fully qualified name here, e.g. adblockplus::web::repository. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:15: define adblockplus::web::fileserver::repository ( Why ::repository? This is rather misleading. Or do you plan to actually migrate this to a repository-based approach (exclusively) some day? IMHO this should be ::directory (as it was in your suggestions in the ticket) or ::subdomain or something.
https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:4: location / On 2017/09/22 07:00:14, mathias wrote: > Actually the idea was to use sub-domains (e.g. http://v8.eyeofiles.com), where each > sub-domain would map to an element within a root directory (i.e. > ${adblockplus::web::directory}/v8.eyeofiles.com -> > ${adblockplus::directory}/fileserver/v8). Acknowledged. Assuming this is a must-have. If this is rather a nice-to-have that we can postpone for the sake of expediency, let me know. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver.pp (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:8: # A string for the domain serving traffic. On 2017/09/22 07:00:14, mathias wrote: > Why would the domain want to have some string? And how does a domain serve > traffic anyway? Isn't this more the name of the domain associated with the > service, given as a string? Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:18: # [*is_default*] On 2017/09/22 07:00:14, mathias wrote: > The fileserver is a standalone HTTPd setup intended for use with a > wildcard-cert, hence I'd say $nginx::hostconfig::is_default should always be > true (hard-coded inside this class here, not a parameter from the outside). Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:21: # [*repository*] On 2017/09/22 07:00:14, mathias wrote: > The documentation of the $repository parameter does not fit anymore. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:29: $certificate, On 2017/09/22 07:00:14, mathias wrote: > Both $certificate and $private_key should default to undef, which allows to not > use a self-signed certificate in development but plain HTTP, which is what we do > in new setups since a few years. The idea is to not introduce complexity in > development, so one does not have fun clicking away pointless browser messages, > does not need hacking exceptions into adjacent modules and scripts (i.e. > --no-check-certificate), does not need to maintain development CAs, and does not > learn how to ignore security warnings. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:59: if !empty($repositories) { On 2017/09/22 07:00:14, mathias wrote: > This conditional is not necessary: Iterating through an empty $repositories hash > (the default) won't do anything. You of course need to remove the pointless > assignment of undef/null in the associated role.yaml file. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:60: ensure_resources('repository', $repositories, { On 2017/09/22 07:00:14, mathias wrote: > Please use the fully qualified name here, e.g. adblockplus::web::repository. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29551713/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:15: define adblockplus::web::fileserver::repository ( On 2017/09/22 07:00:15, mathias wrote: > Why ::repository? This is rather misleading. Or do you plan to actually migrate > this to a repository-based approach (exclusively) some day? IMHO this should be > ::directory (as it was in your suggestions in the ticket) or ::subdomain or > something. Since I don't know where this is going to end up, I went for the more abstract term. I can change it to directory, calling a spade a spade.
Didn't you want switch to the directory terminology? https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:1: # Generated by puppet module adblockplus::web::fileserver Use `# Puppet: <%= $title %>` to not hard-code the caller here. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:42: "$adblockplus::directory/fileserver/repositories" I don't think the additional `repositories` directory level is necessary. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:56: log => "access_log_fileserver_repository", Why not just `access_log_fileserver`? https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:28: if $ensure !~ /^(absent|purged)$/ { Can't you avoid the conditional and base parameter $ensure of the contained resources on values computed by the ensure_*_state() functions? https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:45: # TODO Figure out how to use $adblockplus::web::directory insetad of hardcoded path ?
> Didn't you want switch to the directory terminology? I'm not sure. I mused a bit more about this in the repository.pp documentation, writing "In its current form, a repository is simply a directory exposed on a web server. This may evolve to make use of more advanced repositories in the future (proxy to repository manager, or 3rd-party service, etc)." So, I still like the term repository, but if you are not convinced, I can switch to directory. Let me know. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:1: # Generated by puppet module adblockplus::web::fileserver On 2017/09/25 17:06:03, mathias wrote: > Use `# Puppet: <%= $title %>` to not hard-code the caller here. Should I switch this static file to be a template for only that reason? https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:42: "$adblockplus::directory/fileserver/repositories" On 2017/09/25 17:06:03, mathias wrote: > I don't think the additional `repositories` directory level is necessary. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver.pp:56: log => "access_log_fileserver_repository", On 2017/09/25 17:06:04, mathias wrote: > Why not just `access_log_fileserver`? Acknowledged. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:28: if $ensure !~ /^(absent|purged)$/ { On 2017/09/25 17:06:04, mathias wrote: > Can't you avoid the conditional and base parameter $ensure of the contained > resources on values computed by the ensure_*_state() functions? I can try! https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:45: # TODO Figure out how to use $adblockplus::web::directory insetad of hardcoded path On 2017/09/25 17:06:04, mathias wrote: > ? I discussed this with Paco, and he said it would not be trivial to fix. So I decided to just remove the TODO and stick with the /var/www/ path as it is.
https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:1: # Generated by puppet module adblockplus::web::fileserver On 2017/09/25 20:21:18, f.nicolaisen wrote: > On 2017/09/25 17:06:03, mathias wrote: > > Use `# Puppet: <%= $title %>` to not hard-code the caller here. > > Should I switch this static file to be a template for only that reason? You can also remove that stuff, or just use another term than "generated" (i.e. "managed") and leave the class name out of there. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:45: # TODO Figure out how to use $adblockplus::web::directory insetad of hardcoded path On 2017/09/25 20:21:18, f.nicolaisen wrote: > On 2017/09/25 17:06:04, mathias wrote: > > ? > > I discussed this with Paco, and he said it would not be trivial to fix. So I > decided to just remove the TODO and stick with the /var/www/ path as it is. The '/var/www' path is not configurable for that directory, see class adblockplus::web. Hence you can hard-code those bits.
PS6 addresses these plus the acks in my previous mail. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/files/nginx/fileserver.conf (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/files/nginx/fileserver.conf:1: # Generated by puppet module adblockplus::web::fileserver On 2017/09/25 21:08:50, mathias wrote: > On 2017/09/25 20:21:18, f.nicolaisen wrote: > > On 2017/09/25 17:06:03, mathias wrote: > > > Use `# Puppet: <%= $title %>` to not hard-code the caller here. > > > > Should I switch this static file to be a template for only that reason? > > You can also remove that stuff, or just use another term than "generated" (i.e. > "managed") and leave the class name out of there. Acknowledged. https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... File modules/adblockplus/manifests/web/fileserver/repository.pp (right): https://codereview.adblockplus.org/29551585/diff/29555710/modules/adblockplus... modules/adblockplus/manifests/web/fileserver/repository.pp:45: # TODO Figure out how to use $adblockplus::web::directory insetad of hardcoded path On 2017/09/25 21:08:50, mathias wrote: > On 2017/09/25 20:21:18, f.nicolaisen wrote: > > On 2017/09/25 17:06:04, mathias wrote: > > > ? > > > > I discussed this with Paco, and he said it would not be trivial to fix. So I > > decided to just remove the TODO and stick with the /var/www/ path as it is. > > The '/var/www' path is not configurable for that directory, see class > adblockplus::web. Hence you can hard-code those bits. Acknowledged.
LGTM. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
