CLOUDP-349087: Block simultaneous TLS disabling and scaling for ReplicaSets (#549)

# Summary

We have code in the replica set controller dedicated to handling a
scenario where the user disable TLS, and ascale their cluster at the
same time. The test for this behaviour was broken. Because there were
two functions with the name `test_tls_is_disabled_and_scaled_up`, so
pytest would run only one of them.

When I fixed the test, it failed:
https://spruce.mongodb.com/task/mongodb_kubernetes_e2e_mdb_kind_ubi_cloudqa_e2e_disable_tls_scale_up_patch_dfb9424e9b34ddd048a725a9988114ca4032f9bf_68de79b2fccf070007a2c51e_25_10_02_13_10_12/tests?execution=4&sorts=STATUS%3AASC

Which means the code to handle it was incorrect. My opinion is that it
is better to block this change, rather than introducing complexity to
handle it. It is not a common use case.

A changelog entry was added. Question: should it be explicitly mentioned
in our public documentation ?

Old related PR (2021):
10gen/ops-manager-kubernetes#1444
And ticket: https://jira.mongodb.org/browse/CLOUDP-80768

## Proof of Work

Unit and e2e test for blocking the operation should pass.
No regression due to the change in `updateOmDeploymentRs` should be
observed.

## Checklist

- [X] Have you linked a jira ticket and/or is the ticket in the title?
- [X] Have you checked whether your jira ticket required DOCSP changes?
- [X] Have you added changelog file?
    - use `skip-changelog` label if not needed
- refer to [Changelog files and Release
Notes](https://github.com/mongodb/mongodb-kubernetes/blob/master/CONTRIBUTING.md#changelog-files-and-release-notes)
section in CONTRIBUTING.md for more details

Author: Julien-Ben

.evergreen-tasks.yml

@@ -502,7 +502,7 @@ tasks:
     commands:
       - func: "e2e_test"
 
-  - name: e2e_disable_tls_scale_up
+  - name: e2e_disable_tls_and_scale
     tags: [ "patch-run" ]
     commands:
       - func: "e2e_test"

.evergreen.yml

@@ -679,7 +679,7 @@ task_groups:
       - e2e_tls_rs_external_access
       - e2e_replica_set_tls_require
       - e2e_replica_set_tls_certs_secret_prefix
-      - e2e_disable_tls_scale_up
+      - e2e_disable_tls_and_scale
       - e2e_replica_set_tls_require_and_disable
       # e2e_x509_task_group
       - e2e_configure_tls_and_x509_simultaneously_st

api/v1/mdb/mongodb_validation.go

@@ -369,6 +369,25 @@ func noTopologyMigration(newObj, oldObj MongoDbSpec) v1.ValidationResult {
 	return v1.ValidationSuccess()
 }
 
+func noSimultaneousTLSDisablingAndScaling(newObj, oldObj MongoDbSpec) v1.ValidationResult {
+	if newObj.ResourceType != ReplicaSet {
+		return v1.ValidationSuccess()
+	}
+
+	// Check if TLS is being disabled (was enabled, now disabled)
+	tlsWasEnabled := oldObj.IsSecurityTLSConfigEnabled()
+	tlsWillBeDisabled := !newObj.IsSecurityTLSConfigEnabled()
+
+	// Check if members count is changing
+	membersChanging := oldObj.Members != newObj.Members
+
+	if tlsWasEnabled && tlsWillBeDisabled && membersChanging {
+		return v1.ValidationError("Cannot disable TLS and change member count simultaneously. Please apply these changes separately.")
+	}
+
+	return v1.ValidationSuccess()
+}
+
 // specWithExactlyOneSchema checks that exactly one among "Project/OpsManagerConfig/CloudManagerConfig"
 // is configured, doing the "oneOf" validation in the webhook.
 func specWithExactlyOneSchema(d DbCommonSpec) v1.ValidationResult {
@@ -437,6 +456,7 @@ func (m *MongoDB) RunValidations(old *MongoDB) []v1.ValidationResult {
 	updateValidators := []func(newObj MongoDbSpec, oldObj MongoDbSpec) v1.ValidationResult{
 		resourceTypeImmutable,
 		noTopologyMigration,
+		noSimultaneousTLSDisablingAndScaling,
 	}
 
 	var validationResults []v1.ValidationResult

api/v1/mdb/mongodb_validation_test.go

@@ -80,6 +80,88 @@ func TestMongoDB_ResourceTypeImmutable(t *testing.T) {
 	assert.Errorf(t, err, "'resourceType' cannot be changed once created")
 }
 
+func TestMongoDB_NoSimultaneousTLSDisablingAndScaling(t *testing.T) {
+	tests := []struct {
+		name                 string
+		oldTLSEnabled        bool
+		oldMembers           int
+		newTLSEnabled        bool
+		newMembers           int
+		expectError          bool
+		expectedErrorMessage string
+	}{
+		{
+			name:                 "Simultaneous TLS disabling and scaling is blocked",
+			oldTLSEnabled:        true,
+			oldMembers:           3,
+			newTLSEnabled:        false,
+			newMembers:           5,
+			expectError:          true,
+			expectedErrorMessage: "Cannot disable TLS and change member count simultaneously. Please apply these changes separately.",
+		},
+		{
+			name:          "TLS disabling without scaling is allowed",
+			oldTLSEnabled: true,
+			oldMembers:    3,
+			newTLSEnabled: false,
+			newMembers:    3,
+			expectError:   false,
+		},
+		{
+			name:          "Scaling without TLS changes is allowed",
+			oldTLSEnabled: true,
+			oldMembers:    3,
+			newTLSEnabled: true,
+			newMembers:    5,
+			expectError:   false,
+		},
+		{
+			name:          "TLS enabling with scaling is allowed",
+			oldTLSEnabled: false,
+			oldMembers:    3,
+			newTLSEnabled: true,
+			newMembers:    5,
+			expectError:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Build old ReplicaSet
+			oldBuilder := NewReplicaSetBuilder()
+			if tt.oldTLSEnabled {
+				oldBuilder = oldBuilder.SetSecurityTLSEnabled()
+			}
+			oldRs := oldBuilder.Build()
+			oldRs.Spec.CloudManagerConfig = &PrivateCloudConfig{
+				ConfigMapRef: ConfigMapRef{Name: "cloud-manager"},
+			}
+			oldRs.Spec.Members = tt.oldMembers
+
+			// Build new ReplicaSet
+			newBuilder := NewReplicaSetBuilder()
+			if tt.newTLSEnabled {
+				newBuilder = newBuilder.SetSecurityTLSEnabled()
+			}
+			newRs := newBuilder.Build()
+			newRs.Spec.CloudManagerConfig = &PrivateCloudConfig{
+				ConfigMapRef: ConfigMapRef{Name: "cloud-manager"},
+			}
+			newRs.Spec.Members = tt.newMembers
+
+			// Validate
+			_, err := newRs.ValidateUpdate(oldRs)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Equal(t, tt.expectedErrorMessage, err.Error())
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestSpecProjectOnlyOneValue(t *testing.T) {
 	rs := NewReplicaSetBuilder().Build()
 	rs.Spec.CloudManagerConfig = &PrivateCloudConfig{

changelog/20251024_fix_block_simultaneous_tls_disabling_and_scaling.md

@@ -0,0 +1,6 @@
+---
+kind: fix
+date: 2025-10-24
+---
+
+* **ReplicaSet**: Blocked disabling TLS and changing member count simultaneously. These operations must now be applied separately to prevent configuration inconsistencies.

controllers/om/deployment.go

@@ -100,24 +100,6 @@ func NewDeployment() Deployment {
 	return ans
 }
 
-// TLSConfigurationWillBeDisabled checks that if applying this security configuration the Deployment
-// TLS configuration will go from Enabled -> Disabled.
-func (d Deployment) TLSConfigurationWillBeDisabled(security *mdbv1.Security) bool {
-	tlsIsCurrentlyEnabled := false
-
-	// To detect that TLS is enabled, it is sufficient to check for the
-	// d["tls"]["CAFilePath"] attribute to have a value different from nil.
-	if tlsMap, ok := d["tls"]; ok {
-		if caFilePath, ok := tlsMap.(map[string]interface{})["CAFilePath"]; ok {
-			if caFilePath != nil {
-				tlsIsCurrentlyEnabled = true
-			}
-		}
-	}
-
-	return tlsIsCurrentlyEnabled && !security.IsTLSEnabled()
-}
-
 // ConfigureTLS configures the deployment's TLS settings from the security
 // specification provided by the user in the mongodb resource.
 func (d Deployment) ConfigureTLS(security *mdbv1.Security, caFilePath string) {

controllers/om/deployment_test.go

@@ -152,20 +152,6 @@ func TestConfigureSSL_Deployment(t *testing.T) {
 	assert.Equal(t, d["tls"], map[string]any{"clientCertificateMode": string(automationconfig.ClientCertificateModeOptional)})
 }
 
-func TestTLSConfigurationWillBeDisabled(t *testing.T) {
-	d := Deployment{}
-	d.ConfigureTLS(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: false}}, util.CAFilePathInContainer)
-
-	assert.False(t, d.TLSConfigurationWillBeDisabled(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: false}}))
-	assert.False(t, d.TLSConfigurationWillBeDisabled(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: true}}))
-
-	d = Deployment{}
-	d.ConfigureTLS(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: true}}, util.CAFilePathInContainer)
-
-	assert.False(t, d.TLSConfigurationWillBeDisabled(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: true}}))
-	assert.True(t, d.TLSConfigurationWillBeDisabled(&mdbv1.Security{TLSConfig: &mdbv1.TLSConfig{Enabled: false}}))
-}
-
 // TestMergeDeployment_BigReplicaset ensures that adding a big replica set (> 7 members) works correctly and no more than
 // 7 voting members are added
 func TestMergeDeployment_BigReplicaset(t *testing.T) {

controllers/operator/mongodbreplicaset_controller.go

@@ -578,32 +578,8 @@ func (r *ReconcileMongoDbReplicaSet) updateOmDeploymentRs(ctx context.Context, c
 	}
 
 	caFilePath := fmt.Sprintf("%s/ca-pem", util.TLSCaMountPath)
-	// If current operation is to Disable TLS, then we should the current members of the Replica Set,
-	// this is, do not scale them up or down util TLS disabling has completed.
-	shouldLockMembers, err := updateOmDeploymentDisableTLSConfiguration(conn, r.imageUrls[mcoConstruct.MongodbImageEnv], r.forceEnterprise, membersNumberBefore, rs, log, caFilePath, tlsCertPath)
-	if err != nil && !isRecovering {
-		return workflow.Failed(err)
-	}
-
-	var updatedMembers int
-	// This lock member logic will be removed soon, we should rather block possibility to disable tls + scale
-	// Tracked in CLOUDP-349087
-	if shouldLockMembers {
-		// We should not add or remove members during this run, we'll wait for
-		// TLS to be completely disabled first.
-		// However, on first reconciliation (membersNumberBefore=0), we need to use replicasTarget
-		// because the OM deployment is initialized with TLS enabled by default.
-		log.Debugf("locking members for this reconciliation because TLS was disabled")
-		if membersNumberBefore == 0 {
-			updatedMembers = replicasTarget
-		} else {
-			updatedMembers = membersNumberBefore
-		}
-	} else {
-		updatedMembers = replicasTarget
-	}
 
-	replicaSet := replicaset.BuildFromMongoDBWithReplicas(r.imageUrls[mcoConstruct.MongodbImageEnv], r.forceEnterprise, rs, updatedMembers, rs.CalculateFeatureCompatibilityVersion(), tlsCertPath)
+	replicaSet := replicaset.BuildFromMongoDBWithReplicas(r.imageUrls[mcoConstruct.MongodbImageEnv], r.forceEnterprise, rs, replicasTarget, rs.CalculateFeatureCompatibilityVersion(), tlsCertPath)
 	processNames := replicaSet.GetProcessNames()
 
 	status, additionalReconciliationRequired := r.updateOmAuthentication(ctx, conn, processNames, rs, deploymentOptionsRS.agentCertPath, caFilePath, internalClusterCertPath, isRecovering, log)
@@ -668,40 +644,6 @@ func (r *ReconcileMongoDbReplicaSet) updateOmDeploymentRs(ctx context.Context, c
 	return workflow.OK()
 }
 
-// updateOmDeploymentDisableTLSConfiguration checks if TLS configuration needs
-// to be disabled. In which case it will disable it and inform to the calling
-// function.
-func updateOmDeploymentDisableTLSConfiguration(conn om.Connection, mongoDBImage string, forceEnterprise bool, membersNumberBefore int, rs *mdbv1.MongoDB, log *zap.SugaredLogger, caFilePath, tlsCertPath string) (bool, error) {
-	tlsConfigWasDisabled := false
-
-	err := conn.ReadUpdateDeployment(
-		func(d om.Deployment) error {
-			if !d.TLSConfigurationWillBeDisabled(rs.Spec.GetSecurity()) {
-				return nil
-			}
-
-			tlsConfigWasDisabled = true
-			d.ConfigureTLS(rs.Spec.GetSecurity(), caFilePath)
-
-			// configure as many agents/Pods as we currently have, no more (in case
-			// there's a scale up change at the same time).
-			replicaSet := replicaset.BuildFromMongoDBWithReplicas(mongoDBImage, forceEnterprise, rs, membersNumberBefore, rs.CalculateFeatureCompatibilityVersion(), tlsCertPath)
-
-			lastConfig, err := rs.GetLastAdditionalMongodConfigByType(mdbv1.ReplicaSetConfig)
-			if err != nil {
-				return err
-			}
-
-			d.MergeReplicaSet(replicaSet, rs.Spec.AdditionalMongodConfig.ToMap(), lastConfig.ToMap(), log)
-
-			return nil
-		},
-		log,
-	)
-
-	return tlsConfigWasDisabled, err
-}
-
 func (r *ReconcileMongoDbReplicaSet) OnDelete(ctx context.Context, obj runtime.Object, log *zap.SugaredLogger) error {
 	rs := obj.(*mdbv1.MongoDB)
 

controllers/operator/mongodbreplicaset_controller_test.go

@@ -68,7 +68,7 @@ func TestCreateReplicaSet(t *testing.T) {
 
 	connection := omConnectionFactory.GetConnection()
 	connection.(*om.MockedOmConnection).CheckDeployment(t, deployment.CreateFromReplicaSet("fake-mongoDBImage", false, rs), "auth", "ssl")
-	connection.(*om.MockedOmConnection).CheckNumberOfUpdateRequests(t, 2)
+	connection.(*om.MockedOmConnection).CheckNumberOfUpdateRequests(t, 1)
 }
 
 func TestReplicaSetRace(t *testing.T) {
@@ -385,39 +385,6 @@ func TestCreateReplicaSet_TLS(t *testing.T) {
 	assert.Equal(t, "OPTIONAL", sslConfig["clientCertificateMode"])
 }
 
-// TestUpdateDeploymentTLSConfiguration a combination of tests checking that:
-//
-// TLS Disabled -> TLS Disabled: should not lock members
-// TLS Disabled -> TLS Enabled: should not lock members
-// TLS Enabled -> TLS Enabled: should not lock members
-// TLS Enabled -> TLS Disabled: *should lock members*
-func TestUpdateDeploymentTLSConfiguration(t *testing.T) {
-	rsWithTLS := mdbv1.NewReplicaSetBuilder().SetSecurityTLSEnabled().Build()
-	rsNoTLS := mdbv1.NewReplicaSetBuilder().Build()
-	deploymentWithTLS := deployment.CreateFromReplicaSet("fake-mongoDBImage", false, rsWithTLS)
-	deploymentNoTLS := deployment.CreateFromReplicaSet("fake-mongoDBImage", false, rsNoTLS)
-
-	// TLS Disabled -> TLS Disabled
-	shouldLockMembers, err := updateOmDeploymentDisableTLSConfiguration(om.NewMockedOmConnection(deploymentNoTLS), "fake-mongoDBImage", false, 3, rsNoTLS, zap.S(), util.CAFilePathInContainer, "")
-	assert.NoError(t, err)
-	assert.False(t, shouldLockMembers)
-
-	// TLS Disabled -> TLS Enabled
-	shouldLockMembers, err = updateOmDeploymentDisableTLSConfiguration(om.NewMockedOmConnection(deploymentNoTLS), "fake-mongoDBImage", false, 3, rsWithTLS, zap.S(), util.CAFilePathInContainer, "")
-	assert.NoError(t, err)
-	assert.False(t, shouldLockMembers)
-
-	// TLS Enabled -> TLS Enabled
-	shouldLockMembers, err = updateOmDeploymentDisableTLSConfiguration(om.NewMockedOmConnection(deploymentWithTLS), "fake-mongoDBImage", false, 3, rsWithTLS, zap.S(), util.CAFilePathInContainer, "")
-	assert.NoError(t, err)
-	assert.False(t, shouldLockMembers)
-
-	// TLS Enabled -> TLS Disabled
-	shouldLockMembers, err = updateOmDeploymentDisableTLSConfiguration(om.NewMockedOmConnection(deploymentWithTLS), "fake-mongoDBImage", false, 3, rsNoTLS, zap.S(), util.CAFilePathInContainer, "")
-	assert.NoError(t, err)
-	assert.True(t, shouldLockMembers)
-}
-
 // TestCreateDeleteReplicaSet checks that no state is left in OpsManager on removal of the replicaset
 func TestCreateDeleteReplicaSet(t *testing.T) {
 	ctx := context.Background()