Merge lp:~mpontillo/maas/beaconing-multicast-helper into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Add a helper script to join or leave the MAAS multicast groups.

MAAS will need to execute this on each monitored interface before listening for beacons, so that the NIC knows to forward beacon traffic up the OS (rather than dropping it before even copying it from the hardware buffer).

This approach is necessary in order to receive beacons on interfaces which may not have an IP address configured, and thus will not respond positively to socket-layer multicast join operations.

[Blake Rouse (blake-rouse)]

review: Needs Information

[Andres Rodriguez (andreserl)]

Another question ,

Why are we using helpers I stead of integrating this into the code?

When power scripts were effectively scripts/helpers, we decided that we
were going to move away from that in order to be more robust inside the
MAAS code base. Now with all the discovery stuff we are going again for
adding helper after helper.

Why simply not be more robust and integrate this with the code base and
stop creating helpers!?
On Tue, Jun 6, 2017 at 6:16 AM Blake Rouse <email address hidden>
wrote:

> Review: Needs Information
>
>
>
> Diff comments:
>
> > === added file 'scripts/maas-multicast-helper'
> > --- scripts/maas-multicast-helper 1970-01-01 00:00:00 +0000
> > +++ scripts/maas-multicast-helper 2017-06-06 01:27:48 +0000
> > @@ -0,0 +1,92 @@
> > +#!/bin/sh -euf
> > +# Copyright 2017 Canonical Ltd. This software is licensed under the
> > +# GNU Affero General Public License version 3 (see the file LICENSE).
> > +
> > +# Helper script to join or leave MAAS-reserved multicast groups.
> > +# See the IANA registries for more information:
> > +#
> https://www.iana.org/assignments/multicast-addresses/multicast-addresses.xhtml
> > +#
> https://www.iana.org/assignments/ipv6-multicast-addresses/ipv6-multicast-addresses.xhtml
> > +
> > +IP="/sbin/ip"
> > +
> > +# Note: asking the 'ip' command to join this IPv4 multicast group will
> result
> > +# in the MAC address "e0:00:00:76:00:00" being listened for on the
> interface.
> > +# Be aware that the "ip maddr show dev <ifname>" command will show the
> > +# link-layer listen address, not the IPv4-format address.
> > +IPV4_GROUP="224.0.0.118"
> > +
> > +# Note: The 'ip' command doesn't support IPv6 addresses yet.
> > +# This is the equivalent of the MAAS variable-scope multicast group, and
> > +# corresponds to the link-local scoped group "ff02::15a".
> > +IPV6_GROUP="33:33:00:00:01:5a"
> > +
> > +usage()
> > +{
> > + echo "Usage: $0 <leave | join> [ifname...]" 1>&2
> > + echo " Attempts to join or leave the MAAS multicast groups." 1>&2
> > + echo " If no interfaces are specified, joins or leaves on all "`
> > + `"interfaces." 1>&2
> > +
> > +}
> > +
> > +if [ $# -lt 1 ]; then
> > + usage
> > + exit 1
> > +
> > +fi
> > +
> > +SUCCESSES=0
> > +FAILURES=""
> > +
> > +success()
> > +{
> > + SUCCESSES=$(($SUCCESSES+1))
> > + echo "$*"
> > +}
> > +
> > +join()
> > +{
> > + "$IP" maddr add "$IPV4_GROUP" dev "$1" 2> /dev/null && \
> > + success "$1: joined $IPV4_GROUP" || true
> > + "$IP" maddr add "$IPV6_GROUP" dev "$1" 2> /dev/null && \
> > + success "$1: joined $IPV6_GROUP" || true
> > +}
> > +
> > +leave()
> > +{
> > + "$IP" maddr del "$IPV4_GROUP" dev "$1" 2> /dev/null && \
> > + success "$1: left $IPV4_GROUP" || true
>
> Let say an interface was plugged in after MAAS has started and it doesn't
> have the multicast address. When MAAS goes to leave that group for an
> interface that doesn't have it will this fail? I believe the '|| true' will
> cause it not to fail but placing it after the '&& success' might actually
> allow it to fail.
>
> > + "$IP" maddr del "$IPV6_GROUP" dev "$1" 2> /dev/null && \
> > + success...

[Mike Pontillo (mpontillo)]

Andres, a helper script is required for this because elevated privileges are required in order to add multicast groups. I looked at using the socket layer to join the multicast groups, but that doesn't work for interfaces with no IP address.

[Mike Pontillo (mpontillo)]

Blake, some replies to your questions below.

[Blake Rouse (blake-rouse)]

Thanks for the responses. Makes sense. +1 from me.

Wait on Andres review, but really don't see away around it since we need to escalate privileges.

review: Approve

[Mike Pontillo (mpontillo)]

I just realized that 'ip maddr add' does NOT do what one expects it to do. I wasn't paying enough attention to the resultant multicast MAC that was added to the interface. It turns out the `ip` command was just taking the IP address and directly converting the octets to a MAC.

In Twisted if I do something like `self.transport.joinGroup("224.0.0.118")` inside a DatagramProtocol, it propagates through the socket layer and joins the "01:00:5e:00:00:76" on an interface, if I believe "ip maddr show". however, "ip maddr add 224.0.0.118 dev eth0" would add the multicast MAC "e0:00:00:76:00:00" to eth0. So the script is both broken as-is, and the `ip` command is misleading.

I'll fix this before landing.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

Fixed the above.

review: Abstain

[Lee Trager (ltrager)]

Two small things

1. As part of the MAAS 2.2 dev cycle we switched all of our scripts to using /bin/bash, this should use bash as well.
2. I noticed the script outputs for success but not failure. Shouldn't MAAS log when beaconing isn't setup on all devices? Reading this code we silently ignore failure.

review: Needs Information

[Mike Pontillo (mpontillo)]

Lee, to address your comments:

(1) I think the reason we are using /bin/sh in the scripts/ directory is because these scripts are often called using 'sudo'. Since 'sh' has more limited functionality compared to 'bash', we decided that intentionally. (in order to reduce the attack surface) I would actually have preferred to use bash, since the sh-isms in this script are a bit annoying. ;-)

(2) The script works on a best-effort basis. Not all interfaces will be able to join multicast groups, so forcing a failure isn't really helpful. Lack of success is helpful to know about, which is why a message is printed for each confirmed-successful join or leave.

[Mike Pontillo (mpontillo)]

Also, Lee, good suggestion about the use of 'awk'; thanks! I've made that change.

[Lee Trager (ltrager)]

LGTM!

review: Approve

=== added file 'scripts/maas-multicast-helper'
--- scripts/maas-multicast-helper	1970-01-01 00:00:00 +0000
+++ scripts/maas-multicast-helper	2017-06-06 22:46:41 +0000
@@ -0,0 +1,89 @@
+#!/bin/sh -euf
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# Helper script to join or leave MAAS-reserved multicast groups.
+# See the IANA registries for more information:
+# https://www.iana.org/assignments/multicast-addresses/multicast-addresses.xhtml
+# https://www.iana.org/assignments/ipv6-multicast-addresses/ipv6-multicast-addresses.xhtml
+
+IP="/sbin/ip"
+
+# Note: The 'ip maddr add <mcast-address> dev <ifname>' command only supports
+# link-layer multicast addresses. Specifying an IPv4 address will not work.
+# This is the equivalent of "224.0.0.118".
+IPV4_GROUP="01:00:5e:00:00:76"
+
+# This is the equivalent of the MAAS variable-scope multicast group, and
+# corresponds to the link-local scoped group "ff02::15a".
+IPV6_GROUP="33:33:00:00:01:5a"
+
+usage()
+{
+    echo "Usage: $0 <leave | join> [ifname...]" 1>&2
+    echo "    Attempts to join or leave the MAAS multicast groups." 1>&2
+    echo "    If no interfaces are specified, joins or leaves on all "`
+            `"interfaces." 1>&2
+}
+
+if [ $# -lt 1 ]; then
+    usage
+    exit 1
+
+fi
+
+SUCCESSES=0
+FAILURES=""
+
+success()
+{
+    SUCCESSES=$(($SUCCESSES+1))
+    echo "$*"
+}
+
+join()
+{
+    "$IP" maddr add "$IPV4_GROUP" dev "$1" 2> /dev/null && \
+        success "$1: joined $IPV4_GROUP" || true
+    "$IP" maddr add "$IPV6_GROUP" dev "$1" 2> /dev/null && \
+        success "$1: joined $IPV6_GROUP" || true
+}
+
+leave()
+{
+    "$IP" maddr del "$IPV4_GROUP" dev "$1" 2> /dev/null  && \
+        success "$1: left $IPV4_GROUP" || true
+    "$IP" maddr del "$IPV6_GROUP" dev "$1" 2> /dev/null && \
+        success "$1: left $IPV6_GROUP" || true
+}
+
+cmd="$1"
+shift
+
+if [ "$#" -eq 0 ]; then
+    interfaces="$(ip maddr show | awk '/^[0-9]*:/ { print $2 }')"
+else
+    interfaces="$*"
+fi
+
+
+if [ "$cmd" = "join" ]; then
+    for ifname in $interfaces; do
+        join "$ifname"
+    done
+elif [ "$cmd" = "leave" ]; then
+    for ifname in $interfaces; do
+        leave "$ifname"
+    done
+else
+    usage
+    exit 1
+fi
+
+if [ $SUCCESSES -eq 0 ]; then
+    echo "$0: $cmd failed. (Try re-running with 'sudo'.)" 1>&2
+    if [ "$cmd" = "leave" ]; then
+        echo "(This is normal if no groups were joined to begin with.)" 1>&2
+    fi
+    exit 2
+fi