733536 - set up postfix in PuppetAgain

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task, P4)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

Systems should be able to send mail using a simple smarthost arrangement, probably with postfix.

This is not a B2G priority.

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: patch

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

I think postfix::atboot was accidentally included in the patch..

Also, the module name implies postfix specifically, but when we install this on windows systems, it won't be postfix - maybe "smarthost" or just "mail"?

Which brings me to a question I should probably know the answer to, since I filed the bug: do we need this on slaves, or just on servers?

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 630332 [details] [diff] [review]
patch

Review of attachment 630332 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not as hung up over the module name as dustin's comment (I feel if we need/want a generic module name we would be served well by a simple include of postfix in there for linux, then to magically switch linux/windows to use postfix in one case, etc.

Though I do agree with the "we likely don't want/need this everywhere" and is why I set r- here, among my other nits :-)

::: modules/postfix/manifests/atboot.pp
@@ +1,1 @@
> +class postfix::atboot {

Nothing in this patch uses this class, intentional? And actually since postfix::daemon is instantiated directly from init, this would conflict with things as written.

::: modules/postfix/manifests/config.pp
@@ +1,3 @@
> +class postfix::config {
> +
> +#include postfix::params

Were you intending to use some params this way? either way use it or remove the comment.

I suspect using the config magic (the csv's) would be a better way to do so though.

::: modules/postfix/templates/main.cf.erb
@@ +6,5 @@
> +mydomain = <%= domain %>
> +myorigin = $mydomain
> +mydestination = $myhostname, localhost.$mydomain, localhost, $mydomain
> +unknown_local_recipient_reject_code = 550
> +relay_domains = smtp.mozilla.org

Even though most of our stuff will use this, I'd suggest/request we just make relay_domains a default value in our .csv from the config module. Though I'm unsure if that suggests we'd need/want other configurables that way.. (I'm also no postfix config-file expert, so I would also ask that you get an rs+ from someone who is better than me for this postfix config itself)

Comment 4

[Kim Moir [:kmoir] ET]

Attachment: patch

Changed name to smarthost, now only installed on servers, mail relay now loaded from default-config.csv file, superfluous comments and files removed. 

Callek, you suggested getting a postfix expert to review, who would you recommend?

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 631385 [details] [diff] [review]
patch

Review of attachment 631385 [details] [diff] [review]:
-----------------------------------------------------------------

As I said before I'm not great on postfix stuff, so would be best if we had someone else skim this who is familiar, but imo not a blocker [I read descriptions of the configuration args we used at http://www.postfix.org/postconf.5.html and comments there is strictly based on what I read there, so if you disagree with a comment to that file, *please* don't take my word as authoratative]

I have a few "bikeshed" nits as well, feel free to do as you like with them, they don't affect my acceptance of the patch.

Marking r+ as I feel all notes are trivial, if you feel unsure, re-request

::: modules/smarthost/manifests/config.pp
@@ +1,3 @@
> +class smarthost::config {
> +
> +include ::config

nit: indent

bikeshed nit: if we name this something like smarthost::setup we can avoid the need to |include ::config| with the global namespace, which might confuse people at a glance.

@@ +5,5 @@
> +  File {
> +    owner => "postfix",
> +    group => "postfix",
> +    mode => 0644,
> +  }

I was just looking for the doc I wanted on puppetlabs but I can't find it. Short answer is doing this |File { ... }| for defaults like this can bite us hard. Please use explicit values where you need this.

::: modules/smarthost/templates/main.cf.erb
@@ +1,5 @@
> +soft_bounce = no
> +command_directory = /usr/sbin
> +daemon_directory = /usr/libexec/postfix
> +mail_owner = postfix
> +myhostname = <%= hostname %>

Should we use fqdn here instead?

@@ +5,5 @@
> +myhostname = <%= hostname %>
> +mydomain = <%= domain %>
> +myorigin = $mydomain
> +mydestination = $myhostname, localhost.$mydomain, localhost, $mydomain
> +unknown_local_recipient_reject_code = 550

just a general note, per docs: "The default setting is 550 (reject mail) but it is safer to initially use 450 (try again later) so you have time to find out if your local_recipient_maps settings are OK. "

::: modules/toplevel/manifests/server.pp
@@ +3,5 @@
>  
>  class toplevel::server inherits toplevel::base {
>      include puppet::periodic
>      include ntp::daemon
> +    include smarthost

bikeshed nit: I'm not a fan of the `smarthost` module name, but I don't have a better suggestion. So I'd go with this unless you come up with something, we can always change it later.

Comment 6

[Kim Moir [:kmoir] ET]

Attachment: patch

Renamed config to setup to avoid confusion, fixed indenting, I think the domain is fine instead of fqdn.  I'm not a fan of the name smarthost either yet can't think of a better name :-)

Not sure what you mean by "Short answer is doing this |File { ... }| for defaults like this can bite us hard. Please use explicit values where you need this."

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: patch

changed the setting of the permissions on /etc/postfix/main.cf to be safer :-) Thanks Callek for the suggestion.

Comment 8

[Justin Wood (:Callek)]

Comment on attachment 632339 [details] [diff] [review]
patch

>+class smarthost::setup {
>+
>+    include config
>+
>+   file { "/etc/postfix/main.cf":

nit: inconsistent indent width. I'm not picky which one you choose.

Comment 9

[Kim Moir [:kmoir] ET]

Closing.

Comment 10

[Kim Moir [:kmoir] ET]

I should have mentioned that there isn't any Darwin configuration for postfix because at this time we're just installing it on the servers.