mon: fix bind addr when IPv6 and msgr2 are enabled

When IPv6-only network mode is enabled and when mons are configured to
listen only on msgrV2 port, Rook has to add the port to the mon
--public-bind-addr in order to force mons to stop listening on the
msgrV1 port. This can cause problems for IPv6 clusters where adding
':<port>' to the end of an IP addr looks like a valid IPv6 address.

In IPv6 mode, Rook needs to set this to '[<ip>]:<port>' with the
brackets to be a non-ambiguous IPv6 address.

In IPv4 mode, Rook needs to keep its existing behavior. IPv4 mode should
also continue to be the defualt assumption if the network config isn't
specified.

If 'dualStack: true' is specified, Rook is informed that the environment
is dual stack, but Rook doesn't know which IP family to expect from the
pod IP. In this scenario, it is safest for Rook not to apply the port,
since assuming either IPv4 or IPv6 format could be incorrect. In this
scenario, mons may still listen on msgrV1 ports, but this is preferable
to an environment where mons perpetually crash.

Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>
(cherry picked from commit 573a47e736)

pkg/operator/ceph/cluster/mon/spec.go

@@ -327,8 +327,22 @@ func (c *Cluster) makeMonDaemonContainer(monConfig *monConfig) corev1.Container
 	bindaddr := controller.ContainerEnvVarReference(podIPEnvVar)
 	if monConfig.Port == DefaultMsgr2Port {
 		container.Args = append(container.Args, config.NewFlag("ms_bind_msgr1", "false"))
-		// To avoid binding to the msgr1 port, ceph expects to find the msgr2 port on the bind address
-		bindaddr = fmt.Sprintf("%s:%d", bindaddr, DefaultMsgr2Port)
+
+		// mons don't use --ms-bind-msgr1 to control whether they bind to v1 port or not.
+		// in order to force use of only v2 port, Rook must include the port in the bind addr
+		if c.spec.Network.DualStack {
+			// in a dual stack environment, Rook can't know whether IPv4 or IPv6 will be used.
+			// in order to be safe, don't add the port to the bind addr. this will mean that mons
+			// might listen on both msgr1 and msgr2 ports, but it is more critical to make sure mons
+			// don't crash than to forcefully disable msgr1
+		} else if c.spec.Network.IPFamily == cephv1.IPv6 {
+			// IPv6 addrs have to be surrounded in square brackets when a port is given
+			bindaddr = fmt.Sprintf("[%s]:%d", bindaddr, DefaultMsgr2Port)
+		} else if c.spec.Network.IPFamily == cephv1.IPv4 || c.spec.Network.IPFamily == "" {
+			// IPv4 addrs must have the port added without any special syntax
+			// if the IP family is unset, IPv4 is a safe assumption
+			bindaddr = fmt.Sprintf("%s:%d", bindaddr, DefaultMsgr2Port)
+		}
 	} else {
 		// Add messenger 1 port
 		container.Ports = append(container.Ports, corev1.ContainerPort{

pkg/operator/ceph/cluster/mon/spec_test.go

@@ -18,6 +18,7 @@ package mon
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
@@ -105,29 +106,72 @@ func testPodSpec(t *testing.T, monID string, pvc bool) {
 
 	t.Run(("msgr2 not required"), func(t *testing.T) {
 		container := c.makeMonDaemonContainer(monConfig)
-		checkMsgr2Required(t, container, false)
+		checkMsgr2Required(t, container, false, false, false)
 	})
 
 	t.Run(("require msgr2"), func(t *testing.T) {
 		monConfig.Port = DefaultMsgr2Port
 		container := c.makeMonDaemonContainer(monConfig)
-		checkMsgr2Required(t, container, true)
+		checkMsgr2Required(t, container, true, true, false)
+	})
+
+	t.Run(("require msgr2 -- dual stack"), func(t *testing.T) {
+		monConfig.Port = DefaultMsgr2Port
+		c.spec.Network = cephv1.NetworkSpec{
+			DualStack: true,
+		}
+		monConfig.Port = DefaultMsgr2Port
+		container := c.makeMonDaemonContainer(monConfig)
+		checkMsgr2Required(t, container, true, false, false)
+	})
+
+	t.Run(("require msgr2 -- IPv4"), func(t *testing.T) {
+		monConfig.Port = DefaultMsgr2Port
+		c.spec.Network = cephv1.NetworkSpec{
+			DualStack: false,
+			IPFamily:  cephv1.IPv4,
+		}
+		monConfig.Port = DefaultMsgr2Port
+		container := c.makeMonDaemonContainer(monConfig)
+		checkMsgr2Required(t, container, true, true, false)
+	})
+
+	t.Run(("require msgr2 -- IPv6"), func(t *testing.T) {
+		monConfig.Port = DefaultMsgr2Port
+		c.spec.Network = cephv1.NetworkSpec{
+			DualStack: false,
+			IPFamily:  cephv1.IPv6,
+		}
+		monConfig.Port = DefaultMsgr2Port
+		container := c.makeMonDaemonContainer(monConfig)
+		checkMsgr2Required(t, container, true, true, true)
 	})
 }
 
-func checkMsgr2Required(t *testing.T, container v1.Container, expectedRequireMsgr2 bool) {
+func checkMsgr2Required(t *testing.T, container v1.Container, expectedRequireMsgr2, expectedPort, expectedBrackets bool) {
 	foundDisabledMsgr1 := false
 	foundMsgr2Port := false
+	foundBrackets := false
+
 	for _, arg := range container.Args {
 		if arg == "--ms-bind-msgr1=false" {
 			foundDisabledMsgr1 = true
 		}
-		if arg == "--public-bind-addr=$(ROOK_POD_IP):3300" {
-			foundMsgr2Port = true
+		if strings.HasPrefix(arg, "--public-bind-addr=") {
+			// flag should always refer to the env var, no matter what
+			assert.Contains(t, arg, "$(ROOK_POD_IP)")
+			if strings.HasSuffix(arg, ":3300") {
+				foundMsgr2Port = true
+			}
+			// brackets are expected for IPv6 addrs
+			if strings.Contains(arg, "[$(ROOK_POD_IP)]") {
+				foundBrackets = true
+			}
 		}
 	}
 	assert.Equal(t, expectedRequireMsgr2, foundDisabledMsgr1)
-	assert.Equal(t, expectedRequireMsgr2, foundMsgr2Port)
+	assert.Equal(t, expectedPort, foundMsgr2Port)
+	assert.Equal(t, expectedBrackets, foundBrackets)
 }
 
 func TestDeploymentPVCSpec(t *testing.T) {