mail-stack-delivery charm

[Ante Karamatić]

Please review mail-stack-delivery charm:

https://code.launchpad.net/~ivoks/charms/precise/mail-stack-delivery/trunk

[James Page (james-page)]

Hi Ante

I took a run through your charm - heres some feedback:

1) Error handling in hooks

Recommend that hooks be executed with -e to ensure that any errors during hook execution and propagated back to the end user through failed hooks; handle any non-zero exit codes as appropriate and let the rest bubble up.

2) 'provides' hooks

The charm provides three interfaces; however no hooks are present in the charm to handle these.

Not sure about the typing of the interfaces; they look the wrong way around - for example the type of the postfix interface is smtp rather then the type of the smtp interface being postfix.

That way when someone writes an exim charm it can fulfill the smtp interface in the same way.

3) exposing the service

Recommend that you open ports for potential exposure using 'open-ports' - otherwise in a secured environment the service won't ever be accessible remotely. They won't be exposed unless an admin does 'juju expose' so its opt in from an end user perspective.

4) empty defaults in config.yaml

default: "" is not really required - config-get returns nothing if nothing is provided.

5) relayhost config

I can see how this is a useful config when routing to an externally managed relayhost; but I guess this could also be a juju managed instance as well so maybe having a 'requires' relationship of type smtp could be an option as well?

Default configuration of local only gets a +1000 for good security.

This charm would be nicely complemented with a sub-ordinate charm that could be deployed alongside other services to provide satellite routing to a central mail relay/delivery location. You could use the information provided over such a relation to limit incoming mail acceptance to related service units only.

HTH

James

[Ante Karamatić (ivoks)]

Hi James

I've fixed 1), 2), 3) and 4). I didn't address 5 since I don't think it belongs to this charm. Ideally, instance running this charm will be end-point for all email. Relayhost would be used only for delivering email to 'out of cloud' services. Reason why I say that is cause this charm deploys imap and pop3 services, suggesting that is it end point.

[James Page (james-page)]

Hi Ante

More feedback post updates

1) dovecot-relation-joined

#!/bin/bash
set -eu

myip=`unit-get private-address`

relation-set imap-host=$my_ip pop-host=$myip

Couple of comments here; the remote service will automatically be able to determine the IP address of the mail-stack-delivery service using:

relation-get private-address

This is implicit - so providing it over the relation explicitly is not required; However I would provide the ports for these services as relation data - for example:

relation-set imap-port=143....

2) postfix-relation-joined

#!/bin/bash
set -eu

current_mynetworks=`postconf mynetworks | cut -d' ' -f3-`
my_ip=`unit-get private-address`
client_ip=`relation-get private-address`
mtatype=`config-get mtatype`

postconf mynetworks="$current_mynetworks $client_ip"

case $mtatype in
  3|4)
        juju-log -l INFO "MTA is listening only on localhost interface!"
        ;;
esac

relation-set smtp-host=$my_ip

Same comment re ip/ports as in 1) plus this needs splitting:

client_ip=`relation-get private-address`

This might not work in -joined; but should always work in -changed; I'd recommend you always write code that relies on relation data (implicit or otherwise) in the -changed hook.

3) postfix-relation-broken/departed

These hooks have subtly different behavior; departed fires when a unit leaves a relation (and you can still do 'relation-get private-address' to find out which one I think - I need to check that one) wheres -broken fires when the entire relationship is removed - and you won't be able todo relation-get's in this type of hook.

Other than those 3 looking pretty good.

Cheers

James

[Clint Byrum (clint-fewbar)]

Excerpts from James Page's message of 2012-12-03 14:01:29 UTC:
> Hi Ante
>
> More feedback post updates
>
> 1) dovecot-relation-joined
>
> #!/bin/bash
> set -eu
>
> myip=`unit-get private-address`
>
> relation-set imap-host=$my_ip pop-host=$myip
>
> Couple of comments here; the remote service will automatically be able
> to determine the IP address of the mail-stack-delivery service using:
>
> relation-get private-address
>
> This is implicit - so providing it over the relation explicitly is not
> required; However I would provide the ports for these services as
> relation data - for example:
>
> relation-set imap-port=143....
>

Note that one is not always intending to say "connect to me". Sometimes one
wants to say "connect to *that host*". So private-address, while convenient,
is not always required.

[Ante Karamatić (ivoks)]

I've resolved these 3 issues, but I'm having problems with 'relation-get private-address' within -departed.

I need to be able to remove the IP of departing unit from postfix configuration (otherwise, it will be an open relay). But it looks like 'relation-get private-address' returns nothing (just running relation-get returns nothing within -departed).

Am I doing something wrong or is this a bug in juju?

[Clint Byrum (clint-fewbar)]

Its entirely possible that the local provider is removing the entries before the departed hook returns, leading to empty entries. Can you try on EC2?

[Ante Karamatić (ivoks)]

Same behavior can be seen in EC2:

2013-01-15 17:02:40,622 Machine:3: juju.agents.machine DEBUG: Units changed old:set(['mail-client/2']) new:set([])
2013-01-15 17:02:40,623 Machine:3: juju.agents.machine DEBUG: Stopping service unit: mail-client/2 ...
2013-01-15 17:02:40,626 Machine:3: unit.deploy INFO: Stopping service unit mail-client/2...
2013-01-15 17:02:40,736 unit:mail-client/2: unit.lifecycle DEBUG: pre-stop acquire, running:True
2013-01-15 17:02:40,736 unit:mail-client/2: unit.lifecycle DEBUG: stopping relation lifecycles
2013-01-15 17:02:40,737 unit:mail-client/2: unit.relation.watch DEBUG: relation watcher stop
2013-01-15 17:02:40,737 unit:mail-client/2: hook.scheduler DEBUG: stopping
2013-01-15 17:02:40,737 unit:mail-client/2: hook.scheduler INFO: stopped
2013-01-15 17:02:40,738 unit:mail-client/2: unit.relation.lifecycle DEBUG: stopped relation:postfix lifecycle
2013-01-15 17:02:40,738 unit:mail-client/2: unit.lifecycle DEBUG: stopped unit lifecycle
2013-01-15 17:02:40,738 unit:mail-client/2: hook.executor DEBUG: stopped
2013-01-15 17:02:40,952 Machine:3: unit.deploy INFO: Stopped service unit mail-client/2
2013-01-15 17:02:51,836 unit:mail-stack-delivery/0: unit.relation.watch DEBUG: relation membership change
2013-01-15 17:02:51,836 unit:mail-stack-delivery/0: unit.relation.watch DEBUG: relation membership change
2013-01-15 17:02:51,840 unit:mail-stack-delivery/0: hook.scheduler DEBUG: members changed: old=['mail-client/0', 'mail-client/2'], new=['mail-client/0']
2013-01-15 17:02:51,842 unit:mail-stack-delivery/0: hook.scheduler DEBUG: executing hook for mail-client/2:departed
2013-01-15 17:02:51,842 unit:mail-stack-delivery/0: unit.relation.lifecycle DEBUG: Executing hook postfix-relation-departed
2013-01-15 17:02:51,842 unit:mail-stack-delivery/0: hook.executor DEBUG: Running hook: /var/lib/juju/units/mail-stack-delivery-0/charm/hooks/postfix-relation-departed
2013-01-15 17:02:52,470 unit:mail-stack-delivery/0: hook.output ERROR: Traceback (most recent call last):
Failure: juju.hooks.protocol.NoSuchUnit: The relation 'postfix' has no unit state for 'mail-client/2'
2013-01-15 16:02:52,463 ERROR: Traceback (most recent call last):
Failure: juju.hooks.protocol.NoSuchUnit: The relation 'postfix' has no unit state for 'mail-client/2'

2013-01-15 16:02:52,463 ERROR: [Failure instance: Traceback (failure with no frames): <class 'juju.hooks.protocol.NoSuchUnit'>: The relation 'postfix' has no unit state for 'mail-client/2'
]

2013-01-15 17:02:52,583 unit:mail-stack-delivery/0: hook.output DEBUG: hook postfix-relation-departed exited, exit code 0.
2013-01-15 17:02:52,610 unit:mail-stack-delivery/0: hook.executor DEBUG: Hook complete: /var/lib/juju/units/mail-stack-delivery-0/charm/hooks/postfix-relation-departed

IMHO, it's a bug in workflow if a unit is removed before the relation is broken.

[Jorge Castro (jorge)]

Ante, are you still working on this? It's up for review in the queue but if you haven't worked on it then perhaps we should resolve this bug?