Merge pull request rook#14248 from BlaineEXE/fix-mon-ipv6-networking

mon: fix bind addr when IPv6 and msgr2 are enabled

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) {