courier: Tidy up the Procmail courier

This patch tidies up the Procmail courier:
 - Move the configuration options to the courier instance, instead of using
   global variables.
 - Implement more useful string replacement options.
 - Use exec.CommandContext for running the command with a timeout.

As a consequence of the first item, the queue now takes the couriers via its
constructor.

chasquid.go

@@ -74,9 +74,6 @@ func main() {
 		go http.ListenAndServe(conf.MonitoringAddress, nil)
 	}
 
-	courier.MailDeliveryAgentBin = conf.MailDeliveryAgentBin
-	courier.MailDeliveryAgentArgs = conf.MailDeliveryAgentArgs
-
 	s := NewServer()
 	s.Hostname = conf.Hostname
 	s.MaxDataSize = conf.MaxDataSizeMb * 1024 * 1024
@@ -108,7 +105,13 @@ func main() {
 	// as a remote domain (for loops, alias resolutions, etc.).
 	s.AddDomain("localhost")
 
-	s.InitQueue(conf.DataDir+"/queue", aliasesR)
+	localC := &courier.Procmail{
+		Binary:  conf.MailDeliveryAgentBin,
+		Args:    conf.MailDeliveryAgentArgs,
+		Timeout: 30 * time.Second,
+	}
+	remoteC := &courier.SMTP{}
+	s.InitQueue(conf.DataDir+"/queue", aliasesR, localC, remoteC)
 
 	// Load the addresses and listeners.
 	systemdLs, err := systemd.Listeners()
@@ -265,8 +268,10 @@ func (s *Server) AddUserDB(domain string, db *userdb.DB) {
 	s.userDBs[domain] = db
 }
 
-func (s *Server) InitQueue(path string, aliasesR *aliases.Resolver) {
-	q := queue.New(path, s.localDomains, aliasesR)
+func (s *Server) InitQueue(path string, aliasesR *aliases.Resolver,
+	localC, remoteC courier.Courier) {
+
+	q := queue.New(path, s.localDomains, aliasesR, localC, remoteC)
 	err := q.Load()
 	if err != nil {
 		glog.Fatalf("Error loading queue: %v", err)

chasquid_test.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"blitiri.com.ar/go/chasquid/internal/aliases"
+	"blitiri.com.ar/go/chasquid/internal/courier"
 	"blitiri.com.ar/go/chasquid/internal/userdb"
 
 	"github.com/golang/glog"
@@ -431,7 +432,9 @@ func realMain(m *testing.M) int {
 		s.AddAddr(submissionAddr, ModeSubmission)
 
 		ars := aliases.NewResolver()
-		s.InitQueue(tmpDir+"/queue", ars)
+		localC := &courier.Procmail{}
+		remoteC := &courier.SMTP{}
+		s.InitQueue(tmpDir+"/queue", ars, localC, remoteC)
 
 		udb := userdb.New("/dev/null")
 		udb.AddUser("testuser", "testpasswd")

internal/config/config.go

@@ -55,7 +55,7 @@ func Load(path string) (*Config, error) {
 	}
 	if len(c.MailDeliveryAgentArgs) == 0 {
 		c.MailDeliveryAgentArgs = append(c.MailDeliveryAgentArgs,
-			"-d", "%user%")
+			"-f", "%from%", "-d", "%to_user%")
 	}
 
 	if c.DataDir == "" {

internal/courier/procmail.go

@@ -2,6 +2,7 @@ package courier
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"os/exec"
 	"strings"
@@ -12,44 +13,46 @@ import (
 	"blitiri.com.ar/go/chasquid/internal/trace"
 )
 
-var (
-	// Location of the procmail binary, and arguments to use.
-	// The string "%user%" will be replaced with the local user.
-	// TODO: Make these a part of the courier instance itself? Why do they
-	// have to be global?
-	MailDeliveryAgentBin  = "procmail"
-	MailDeliveryAgentArgs = []string{"-d", "%user%"}
-
-	// Give procmail 1m to deliver mail.
-	procmailTimeout = 1 * time.Minute
-)
-
 var (
 	errTimeout = fmt.Errorf("Operation timed out")
 )
 
 // Procmail delivers local mail via procmail.
 type Procmail struct {
+	Binary  string        // Path to the binary.
+	Args    []string      // Arguments to pass.
+	Timeout time.Duration // Timeout for each invocation.
 }
 
 func (p *Procmail) Deliver(from string, to string, data []byte) error {
 	tr := trace.New("Procmail", "Deliver")
 	defer tr.Finish()
 
-	// Get the user, and sanitize to be extra paranoid.
-	user := sanitizeForProcmail(envelope.UserOf(to))
-	domain := sanitizeForProcmail(envelope.DomainOf(to))
-	tr.LazyPrintf("%s  ->  %s (%s @ %s)", from, user, to, domain)
+	// Sanitize, just in case.
+	from = sanitizeForProcmail(from)
+	to = sanitizeForProcmail(to)
+
+	tr.LazyPrintf("%s -> %s", from, to)
 
 	// Prepare the command, replacing the necessary arguments.
 	replacer := strings.NewReplacer(
-		"%user%", user,
-		"%domain%", domain)
+		"%from%", from,
+		"%from_user%", envelope.UserOf(from),
+		"%from_domain%", envelope.DomainOf(from),
+
+		"%to%", to,
+		"%to_user%", envelope.UserOf(to),
+		"%to_domain%", envelope.DomainOf(to),
+	)
+
 	args := []string{}
-	for _, a := range MailDeliveryAgentArgs {
+	for _, a := range p.Args {
 		args = append(args, replacer.Replace(a))
 	}
-	cmd := exec.Command(MailDeliveryAgentBin, args...)
+
+	ctx, _ := context.WithDeadline(context.Background(),
+		time.Now().Add(p.Timeout))
+	cmd := exec.CommandContext(ctx, p.Binary, args...)
 
 	cmdStdin, err := cmd.StdinPipe()
 	if err != nil {
@@ -72,13 +75,9 @@ func (p *Procmail) Deliver(from string, to string, data []byte) error {
 
 	cmdStdin.Close()
 
-	timer := time.AfterFunc(procmailTimeout, func() {
-		cmd.Process.Kill()
-	})
 	err = cmd.Wait()
-	timedOut := !timer.Stop()
 
-	if timedOut {
+	if ctx.Err() == context.DeadlineExceeded {
 		return tr.Error(errTimeout)
 	}
 	if err != nil {

internal/courier/procmail_test.go

@@ -15,10 +15,11 @@ func TestProcmail(t *testing.T) {
 	}
 	defer os.RemoveAll(dir)
 
-	MailDeliveryAgentBin = "tee"
-	MailDeliveryAgentArgs = []string{dir + "/%user%"}
-
-	p := Procmail{}
+	p := Procmail{
+		Binary:  "tee",
+		Args:    []string{dir + "/%to_user%"},
+		Timeout: 1 * time.Minute,
+	}
 
 	err = p.Deliver("from@x", "to@local", []byte("data"))
 	if err != nil {
@@ -32,39 +33,27 @@ func TestProcmail(t *testing.T) {
 }
 
 func TestProcmailTimeout(t *testing.T) {
-	MailDeliveryAgentBin = "/bin/sleep"
-	MailDeliveryAgentArgs = []string{"1"}
-	procmailTimeout = 100 * time.Millisecond
-
-	p := Procmail{}
+	p := Procmail{"/bin/sleep", []string{"1"}, 100 * time.Millisecond}
 
 	err := p.Deliver("from", "to@local", []byte("data"))
 	if err != errTimeout {
 		t.Errorf("Unexpected error: %v", err)
 	}
-
-	procmailTimeout = 1 * time.Second
 }
 
 func TestProcmailBadCommandLine(t *testing.T) {
-	p := Procmail{}
-
 	// Non-existent binary.
-	MailDeliveryAgentBin = "thisdoesnotexist"
+	p := Procmail{"thisdoesnotexist", nil, 1 * time.Minute}
 	err := p.Deliver("from", "to", []byte("data"))
 	if err == nil {
-		t.Errorf("Unexpected success: %q %v",
-			MailDeliveryAgentBin, MailDeliveryAgentArgs)
+		t.Errorf("unexpected success for non-existent binary")
 	}
 
 	// Incorrect arguments.
-	MailDeliveryAgentBin = "cat"
-	MailDeliveryAgentArgs = []string{"--fail_unknown_option"}
-
+	p = Procmail{"cat", []string{"--fail_unknown_option"}, 1 * time.Minute}
 	err = p.Deliver("from", "to", []byte("data"))
 	if err == nil {
-		t.Errorf("Unexpected success: %q %v",
-			MailDeliveryAgentBin, MailDeliveryAgentArgs)
+		t.Errorf("unexpected success for incorrect arguments")
 	}
 }
 

internal/queue/queue.go

@@ -98,13 +98,15 @@ type Queue struct {
 }
 
 // Load the queue and launch the sending loops on startup.
-func New(path string, localDomains *set.String, aliases *aliases.Resolver) *Queue {
+func New(path string, localDomains *set.String, aliases *aliases.Resolver,
+	localC, remoteC courier.Courier) *Queue {
+
 	os.MkdirAll(path, 0700)
 
 	return &Queue{
 		q:            map[string]*Item{},
-		localC:       &courier.Procmail{},
-		remoteC:      &courier.SMTP{},
+		localC:       localC,
+		remoteC:      remoteC,
 		localDomains: localDomains,
 		path:         path,
 		aliases:      aliases,

internal/queue/queue_test.go

@@ -60,9 +60,8 @@ func newTestCourier() *TestCourier {
 func TestBasic(t *testing.T) {
 	localC := newTestCourier()
 	remoteC := newTestCourier()
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver())
-	q.localC = localC
-	q.remoteC = remoteC
+	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+		localC, remoteC)
 
 	localC.wg.Add(2)
 	remoteC.wg.Add(1)
@@ -101,7 +100,8 @@ func TestBasic(t *testing.T) {
 }
 
 func TestFullQueue(t *testing.T) {
-	q := New("/tmp/queue_test", set.NewString(), aliases.NewResolver())
+	q := New("/tmp/queue_test", set.NewString(), aliases.NewResolver(),
+		dumbCourier, dumbCourier)
 
 	// Force-insert maxQueueSize items in the queue.
 	oneID := ""
@@ -145,10 +145,11 @@ func (c DumbCourier) Deliver(from string, to string, data []byte) error {
 	return nil
 }
 
+var dumbCourier DumbCourier
+
 func TestAliases(t *testing.T) {
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver())
-	q.localC = DumbCourier{}
-	q.remoteC = DumbCourier{}
+	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+		dumbCourier, dumbCourier)
 
 	q.aliases.AddDomain("loco")
 	q.aliases.AddAliasForTesting("ab@loco", "pq@loco", aliases.EMAIL)
@@ -184,7 +185,8 @@ func TestAliases(t *testing.T) {
 }
 
 func TestPipes(t *testing.T) {
-	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver())
+	q := New("/tmp/queue_test", set.NewString("loco"), aliases.NewResolver(),
+		dumbCourier, dumbCourier)
 	item := &Item{
 		Message: Message{
 			ID:   <-newID,

test/t-01-simple_local/config/chasquid.conf

@@ -3,6 +3,6 @@ submission_address: ":1587"
 monitoring_address: ":1099"
 
 mail_delivery_agent_bin: "test-mda"
-mail_delivery_agent_args: "%user%@%domain%"
+mail_delivery_agent_args: "%to%"
 
 data_dir: "../.data"

test/t-02-exim/config/chasquid.conf

@@ -3,6 +3,6 @@ submission_address: ":1587"
 monitoring_address: ":1099"
 
 mail_delivery_agent_bin: "test-mda"
-mail_delivery_agent_args: "%user%@%domain%"
+mail_delivery_agent_args: "%to%"
 
 data_dir: "../.data"

test/t-03-queue_persistency/config/chasquid.conf

@@ -3,6 +3,6 @@ submission_address: ":1587"
 monitoring_address: ":1099"
 
 mail_delivery_agent_bin: "test-mda"
-mail_delivery_agent_args: "%user%@%domain%"
+mail_delivery_agent_args: "%to%"
 
 data_dir: "../.data"

test/t-04-aliases/config/chasquid.conf

@@ -3,7 +3,7 @@ submission_address: ":1587"
 monitoring_address: ":1099"
 
 mail_delivery_agent_bin: "test-mda"
-mail_delivery_agent_args: "%user%@%domain%"
+mail_delivery_agent_args: "%to%"
 
 data_dir: "../.data"