CLOUDP-119891: Change port of running cluster (#995)

* CLOUDP-119891: Change port of running cluster

This patchset improves reconfiguration of MongoDB's ports in the replica set.
MongoDB Agent does not support changing port of more than one process at a time in a replica set.

Changes:

* Introduced ReplicaSetPortManager to determine processes' ports in Service and Automation Config.
* Fixed handling nested map format (e.g. net.port) when unmarshalling MongodConfiguration
* Improved handling ports both as ints and float64 in MongodConfiguration and AutomationConfig
* Set explicit linux/amd64 platform when building agent-image, which fixes local builds on M1 macs.

Port change process:
* When the port (newPort) is customised, but there aren't any pods yet, then all processes are configured with the newPort in automation config. This works as before.
* When the cluster is running and the port is changed, each process need to be configured one by one:
* ReplicaSetPortManager changes port in automation config only when all pods reached its goal states.
* Until all processes are configured with newPort the service is configured with two ports: oldPort and newPort.
* When all pods are changed one by one, the service is reconfigured to have only newPort.

closes #945

Author: lsierant

.action_templates/jobs/tests.yaml

@@ -54,3 +54,5 @@ tests:
           distro: ubi
         - test-name: replica_set_mount_connection_string
           distro: ubi
+        - test-name: replica_set_mongod_port_change_with_arbiters
+          distro: ubi

.github/workflows/e2e-fork.yml

@@ -136,6 +136,8 @@ jobs:
           distro: ubi
         - test-name: replica_set_mount_connection_string
           distro: ubi
+        - test-name: replica_set_mongod_port_change_with_arbiters
+          distro: ubi
     steps:
     # template: .action_templates/steps/cancel-previous.yaml
     - name: Cancel Previous Runs

.github/workflows/e2e.yml

@@ -142,7 +142,7 @@ jobs:
           distro: ubi
         - test-name: replica_set_mount_connection_string
           distro: ubi
-        - test-name: replica_set_operator_upgrade
+        - test-name: replica_set_mongod_port_change_with_arbiters
           distro: ubi
     steps:
     # template: .action_templates/steps/cancel-previous.yaml

api/v1/mongodbcommunity_types.go

@@ -311,7 +311,20 @@ func (m *MongodConfiguration) UnmarshalJSON(data []byte) error {
 		m.Object = map[string]interface{}{}
 	}
 
-	return json.Unmarshal(data, &m.Object)
+	// Handle keys like net.port to be set as nested maps.
+	// Without this after unmarshalling there is just key "net.port" which is not
+	// a nested map and methods like GetPort() cannot access the value.
+	tmpMap := map[string]interface{}{}
+	err := json.Unmarshal(data, &tmpMap)
+	if err != nil {
+		return err
+	}
+
+	for k, v := range tmpMap {
+		m.SetOption(k, v)
+	}
+
+	return nil
 }
 
 func (m *MongodConfiguration) DeepCopy() *MongodConfiguration {
@@ -339,8 +352,24 @@ func (m MongodConfiguration) GetDBDataDir() string {
 // GetDBPort returns the port that should be used for the mongod process.
 // If port is not specified, the default port of 27017 will be used.
 func (m MongodConfiguration) GetDBPort() int {
-	// When passed as a number "net.port" is unmarshalled into a float64
-	return int(objx.New(m.Object).Get("net.port").Float64(float64(automationconfig.DefaultDBPort)))
+	portValue := objx.New(m.Object).Get("net.port")
+
+	// Underlying map could be manipulated in code, e.g. via SetDBPort (e.g. in unit tests) - then it will be as int,
+	// or it could be deserialized from JSON and then integer in an untyped map will be deserialized as float64.
+	// It's behavior of https://pkg.go.dev/encoding/json#Unmarshal that is converting JSON integers as float64.
+	if portValue.IsInt() {
+		return portValue.Int(automationconfig.DefaultDBPort)
+	} else if portValue.IsFloat64() {
+		return int(portValue.Float64(float64(automationconfig.DefaultDBPort)))
+	}
+
+	return automationconfig.DefaultDBPort
+}
+
+// SetDBPort ensures that port is stored as float64
+func (m MongodConfiguration) SetDBPort(port int) MongodConfiguration {
+	m.SetOption("net.port", float64(port))
+	return m
 }
 
 type MongoDBUser struct {
@@ -747,6 +776,10 @@ func (m MongoDBCommunity) ServiceName() string {
 	return m.Name + "-svc"
 }
 
+func (m MongoDBCommunity) ArbiterNamespacedName() types.NamespacedName {
+	return types.NamespacedName{Namespace: m.Namespace, Name: m.Name + "-arb"}
+}
+
 func (m MongoDBCommunity) AutomationConfigSecretName() string {
 	return m.Name + "-config"
 }

api/v1/mongodbcommunity_types_test.go

@@ -1,6 +1,8 @@
 package v1
 
 import (
+	"encoding/json"
+	"github.com/stretchr/testify/require"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -44,6 +46,25 @@ func TestMongodConfiguration(t *testing.T) {
 	})
 }
 
+func TestMongodConfigurationWithNestedMapsAfterUnmarshalling(t *testing.T) {
+	jsonStr := `
+		{
+			"net.port": 40333,
+			"storage.dbPath": "/other/data/path"
+		}
+	`
+	mc := NewMongodConfiguration()
+	require.NoError(t, json.Unmarshal([]byte(jsonStr), &mc))
+	assert.Equal(t, map[string]interface{}{
+		"net": map[string]interface{}{
+			"port": 40333.,
+		},
+		"storage": map[string]interface{}{
+			"dbPath": "/other/data/path",
+		},
+	}, mc.Object)
+}
+
 func TestGetScramCredentialsSecretName(t *testing.T) {
 	testusers := []struct {
 		in  MongoDBUser

config/crd/bases/mongodbcommunity.mongodb.com_mongodbcommunity.yaml

@@ -5,12 +5,6 @@ kind: CustomResourceDefinition
 metadata:
   annotations:
     controller-gen.kubebuilder.io/version: v0.4.1
-    service.binding/type: 'mongodb'
-    service.binding/provider: 'community'
-    service.binding: 'path={.metadata.name}-{.spec.users[0].db}-{.spec.users[0].name},objectType=Secret'
-    service.binding/connectionString: 'path={.metadata.name}-{.spec.users[0].db}-{.spec.users[0].name},objectType=Secret,sourceKey=connectionString.standardSrv'
-    service.binding/username: 'path={.metadata.name}-{.spec.users[0].db}-{.spec.users[0].name},objectType=Secret,sourceKey=username'
-    service.binding/password: 'path={.metadata.name}-{.spec.users[0].db}-{.spec.users[0].name},objectType=Secret,sourceKey=password'
   creationTimestamp: null
   name: mongodbcommunity.mongodbcommunity.mongodb.com
 spec:

controllers/mongodb_tls_test.go

@@ -313,3 +313,13 @@ func createTLSSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, crt string,
 	s := sBuilder.Build()
 	return c.Create(context.TODO(), &s)
 }
+
+func createUserPasswordSecret(c k8sClient.Client, mdb mdbv1.MongoDBCommunity, userPasswordSecretName string, password string) error {
+	sBuilder := secret.Builder().
+		SetName(userPasswordSecretName).
+		SetNamespace(mdb.Namespace).
+		SetField("password", password)
+
+	s := sBuilder.Build()
+	return c.Create(context.TODO(), &s)
+}

controllers/replica_set_controller.go

@@ -442,17 +442,20 @@ func (r *ReplicaSetReconciler) deployMongoDBReplicaSet(mdb mdbv1.MongoDBCommunit
 // The Service definition is built from the `mdb` resource. If `isArbiter` is set to true, the Service
 // will be created for the arbiters Statefulset.
 func (r *ReplicaSetReconciler) ensureService(mdb mdbv1.MongoDBCommunity) error {
-	name := mdb.ServiceName()
+	processPortManager, err := r.createProcessPortManager(mdb)
+	if err != nil {
+		return err
+	}
 
-	svc := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: mdb.Namespace}}
+	svc := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: mdb.ServiceName(), Namespace: mdb.Namespace}}
 	op, err := controllerutil.CreateOrUpdate(context.TODO(), r.client, svc, func() error {
 		resourceVersion := svc.ResourceVersion // Save resourceVersion for later
-		*svc = buildService(mdb)
+		*svc = r.buildService(mdb, processPortManager)
 		svc.ResourceVersion = resourceVersion
 		return nil
 	})
 	if err != nil {
-		r.log.Infof("Cloud not create or patch the service: %s", err)
+		r.log.Errorf("Could not create or patch the service: %s", err)
 		return nil
 	}
 
@@ -461,12 +464,29 @@ func (r *ReplicaSetReconciler) ensureService(mdb mdbv1.MongoDBCommunity) error {
 	return err
 }
 
+// createProcessPortManager is a helper method for creating new ReplicaSetPortManager.
+// ReplicaSetPortManager needs current automation config and current pod state and the code for getting them
+// was extracted here as it is used in ensureService and buildAutomationConfig.
+func (r *ReplicaSetReconciler) createProcessPortManager(mdb mdbv1.MongoDBCommunity) (*agent.ReplicaSetPortManager, error) {
+	currentAC, err := automationconfig.ReadFromSecret(r.client, types.NamespacedName{Name: mdb.AutomationConfigSecretName(), Namespace: mdb.Namespace})
+	if err != nil {
+		return nil, errors.Errorf("could not read existing automation config: %s", err)
+	}
+
+	currentPodStates, err := agent.GetAllDesiredMembersAndArbitersPodState(mdb.NamespacedName(), r.client, mdb.StatefulSetReplicasThisReconciliation(), mdb.StatefulSetArbitersThisReconciliation(), currentAC.Version, r.log)
+	if err != nil {
+		return nil, fmt.Errorf("cannot get all pods goal state: %w", err)
+	}
+
+	return agent.NewReplicaSetPortManager(r.log, mdb.Spec.AdditionalMongodConfig.GetDBPort(), currentPodStates, currentAC.Processes), nil
+}
+
 func (r *ReplicaSetReconciler) createOrUpdateStatefulSet(mdb mdbv1.MongoDBCommunity, isArbiter bool) error {
 	set := appsv1.StatefulSet{}
 
 	name := mdb.NamespacedName()
 	if isArbiter {
-		name.Name = name.Name + "-arb"
+		name = mdb.ArbiterNamespacedName()
 	}
 
 	err := r.client.Get(context.TODO(), name, &set)
@@ -528,41 +548,37 @@ func buildAutomationConfig(mdb mdbv1.MongoDBCommunity, auth automationconfig.Aut
 		SetOptions(automationconfig.Options{DownloadBase: "/var/lib/mongodb-mms-automation"}).
 		SetAuth(auth).
 		SetDataDir(mdb.GetMongodConfiguration().GetDBDataDir()).
-		SetPort(mdb.GetMongodConfiguration().GetDBPort()).
 		AddModifications(getMongodConfigModification(mdb)).
 		AddModifications(modifications...).
 		Build()
 }
 
 // buildService creates a Service that will be used for the Replica Set StatefulSet
 // that allows all the members of the STS to see each other.
-func buildService(mdb mdbv1.MongoDBCommunity) corev1.Service {
+func (r *ReplicaSetReconciler) buildService(mdb mdbv1.MongoDBCommunity, portManager *agent.ReplicaSetPortManager) corev1.Service {
 	label := make(map[string]string)
 	name := mdb.ServiceName()
 
 	label["app"] = name
 
-	return service.Builder().
+	serviceBuilder := service.Builder().
 		SetName(name).
 		SetNamespace(mdb.Namespace).
 		SetSelector(label).
 		SetLabels(label).
 		SetServiceType(corev1.ServiceTypeClusterIP).
 		SetClusterIP("None").
 		SetPublishNotReadyAddresses(true).
-		SetOwnerReferences(mdb.GetOwnerReferences()).
-		AddPort(mongoDBPort(mdb)).
-		AddPort(prometheusPort(mdb)).
-		Build()
-}
+		SetOwnerReferences(mdb.GetOwnerReferences())
 
-// mongoDBPort returns a `corev1.ServicePort` to be configured in the StatefulSet
-// for this MongoDB resource.
-func mongoDBPort(mdb mdbv1.MongoDBCommunity) *corev1.ServicePort {
-	return &corev1.ServicePort{
-		Port: int32(mdb.GetMongodConfiguration().GetDBPort()),
-		Name: "mongodb",
+	for _, servicePort := range portManager.GetServicePorts() {
+		tmpServicePort := servicePort
+		serviceBuilder.AddPort(&tmpServicePort)
 	}
+
+	serviceBuilder.AddPort(prometheusPort(mdb))
+
+	return serviceBuilder.Build()
 }
 
 // validateSpec checks if the MongoDB resource Spec is valid.
@@ -627,14 +643,21 @@ func (r ReplicaSetReconciler) buildAutomationConfig(mdb mdbv1.MongoDBCommunity)
 		}
 	}
 
+	processPortManager, err := r.createProcessPortManager(mdb)
+	if err != nil {
+		return automationconfig.AutomationConfig{}, err
+	}
+
 	automationConfig, err := buildAutomationConfig(
 		mdb,
 		auth,
 		currentAC,
 		tlsModification,
 		customRolesModification,
 		prometheusModification,
+		processPortManager.GetPortsModification(),
 	)
+
 	if err != nil {
 		return automationconfig.AutomationConfig{}, errors.Errorf("could not create an automation config: %s", err)
 	}
@@ -646,7 +669,7 @@ func (r ReplicaSetReconciler) buildAutomationConfig(mdb mdbv1.MongoDBCommunity)
 	return automationConfig, nil
 }
 
-// overrideToAutomationConfig turns an automation config ovverride from the resource spec into an automation config
+// overrideToAutomationConfig turns an automation config override from the resource spec into an automation config
 // which can be used to merge.
 func overrideToAutomationConfig(override mdbv1.AutomationConfigOverride) automationconfig.AutomationConfig {
 	var processes []automationconfig.Process
@@ -703,7 +726,7 @@ func buildArbitersModificationFunction(mdb mdbv1.MongoDBCommunity) statefulset.M
 	return statefulset.Apply(
 		statefulset.WithReplicas(mdb.StatefulSetArbitersThisReconciliation()),
 		statefulset.WithServiceName(mdb.ServiceName()),
-		statefulset.WithName(mdb.Name+"-arb"),
+		statefulset.WithName(mdb.ArbiterNamespacedName().Name),
 	)
 }