CLOUDP-351614: Fix monitoring failure after disabling TLS on AppDB

When TLS is disabled on AppDB, the operator now correctly clears stale
TLS params from the monitoring configuration. This prevents the monitoring
agent from trying to use certificate files that no longer exist.

Changes:
- Rename AddMonitoringAndBackup to ConfigureMonitoringAndBackup (better name)
- Clear only TLS-specific fields (useSslForAllConnections,
  sslTrustedServerCertificates, sslClientCertificate) from additionalParams
- Only remove additionalParams map if empty after clearing TLS fields
- Fix applied to both deployment.go (regular MongoDB) and
  appdbreplicaset_controller.go (AppDB)
- Add unit tests for the TLS param clearing functions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Author: nammn

.evergreen.yml

@@ -1439,7 +1439,21 @@ buildvariants:
     tags: [ "pr_patch", "staging", "e2e_test_suite", "static" ]
     run_on:
       - ubuntu2404-large
-    <<: *base_om8_dependency
+    depends_on:
+      - name: build_om_images
+        variant: build_om80_images
+      - name: build_operator_ubi
+        variant: init_test_run
+      - name: build_init_database_image_ubi
+        variant: init_test_run
+      - name: build_test_image
+        variant: init_test_run
+      - name: build_init_appdb_images_ubi
+        variant: init_test_run
+      - name: build_init_om_images_ubi
+        variant: init_test_run
+      - name: publish_helm_chart
+        variant: init_test_run
     tasks:
       - name: e2e_static_ops_manager_kind_only_task_group
       - name: e2e_static_ops_manager_kind_6_0_only_task_group

changelog/20251216_fix_tls_monitoring.md

@@ -0,0 +1,6 @@
+---
+kind: fix
+date: 2025-12-16
+---
+
+* Fixed an issue where monitoring agents would fail after disabling TLS on a MongoDB deployment.

controllers/om/deployment.go

@@ -251,15 +251,16 @@ func (d Deployment) MergeShardedCluster(opts DeploymentShardedClusterMergeOption
 	return shardsScheduledForRemoval, nil
 }
 
-// AddMonitoringAndBackup adds monitoring and backup agents to each process
-// The automation agent will update the agents versions to the latest version automatically
+// ConfigureMonitoringAndBackup configures monitoring and backup agents for each process.
+// This is called on every reconcile to ensure the monitoring/backup config matches the desired state.
+// The automation agent will update the agents versions to the latest version automatically.
 // Note, that these two are deliberately combined as all clients (standalone, rs etc.) need both backup and monitoring
-// together
-func (d Deployment) AddMonitoringAndBackup(log *zap.SugaredLogger, tls bool, caFilepath string) {
+// together.
+func (d Deployment) ConfigureMonitoringAndBackup(log *zap.SugaredLogger, tls bool, caFilepath string) {
 	if len(d.getProcesses()) == 0 {
 		return
 	}
-	d.AddMonitoring(log, tls, caFilepath)
+	d.ConfigureMonitoring(log, tls, caFilepath)
 	d.addBackup(log)
 }
 
@@ -277,8 +278,9 @@ func (d Deployment) GetReplicaSetByName(name string) ReplicaSet {
 	return nil
 }
 
-// AddMonitoring adds monitoring agents for all processes in the deployment
-func (d Deployment) AddMonitoring(log *zap.SugaredLogger, tls bool, caFilePath string) {
+// ConfigureMonitoring configures monitoring agents for all processes in the deployment.
+// This is called on every reconcile to ensure the monitoring config matches the desired state.
+func (d Deployment) ConfigureMonitoring(log *zap.SugaredLogger, tls bool, caFilePath string) {
 	if len(d.getProcesses()) == 0 {
 		return
 	}
@@ -306,23 +308,56 @@ func (d Deployment) AddMonitoring(log *zap.SugaredLogger, tls bool, caFilePath s
 		monitoringVersion["hostname"] = p.HostName()
 
 		if tls {
-			additionalParams := map[string]string{
-				"useSslForAllConnections":      "true",
-				"sslTrustedServerCertificates": caFilePath,
+			pemKeyFile := ""
+			if pem := p.EnsureTLSConfig()["PEMKeyFile"]; pem != nil {
+				pemKeyFile = pem.(string)
 			}
-
-			pemKeyFile := p.EnsureTLSConfig()["PEMKeyFile"]
-			if pemKeyFile != nil {
-				additionalParams["sslClientCertificate"] = pemKeyFile.(string)
-			}
-
-			monitoringVersion["additionalParams"] = additionalParams
+			monitoringVersion["additionalParams"] = buildTLSAdditionalParams(caFilePath, pemKeyFile)
+		} else {
+			// Clear TLS-specific params when TLS is disabled to prevent monitoring from
+			// trying to use certificate files that no longer exist.
+			// We only clear TLS fields, preserving any other additionalParams that may be set.
+			clearTLSParamsFromMonitoringVersion(monitoringVersion)
 		}
 
 	}
 	d.setMonitoringVersions(monitoringVersions)
 }
 
+// buildTLSAdditionalParams creates the additionalParams map for monitoring with TLS enabled.
+// This is the single source of truth for TLS params - clearTLSParamsFromMonitoringVersion
+// uses this function to determine which keys to remove.
+func buildTLSAdditionalParams(caFilePath string, pemKeyFile string) map[string]string {
+	params := map[string]string{
+		"useSslForAllConnections":      "true",
+		"sslTrustedServerCertificates": caFilePath,
+	}
+	if pemKeyFile != "" {
+		params["sslClientCertificate"] = pemKeyFile
+	}
+	return params
+}
+
+// clearTLSParamsFromMonitoringVersion removes TLS-specific fields from the monitoring
+// version's additionalParams. It derives which keys to remove from buildTLSAdditionalParams,
+// ensuring add and remove operations always stay in sync.
+// If additionalParams becomes empty after removing TLS fields, the entire map is removed.
+func clearTLSParamsFromMonitoringVersion(monitoringVersion map[string]interface{}) {
+	additionalParams, ok := monitoringVersion["additionalParams"].(map[string]string)
+	if !ok {
+		return
+	}
+
+	// Use non-empty dummy values to get all possible TLS param keys
+	for key := range buildTLSAdditionalParams("_", "_") {
+		delete(additionalParams, key)
+	}
+
+	if len(additionalParams) == 0 {
+		delete(monitoringVersion, "additionalParams")
+	}
+}
+
 // RemoveMonitoringAndBackup removes both monitoring and backup agent configurations. This must be called when the
 // Mongodb resource is being removed, otherwise UI will show non-existing agents in the "servers" tab
 func (d Deployment) RemoveMonitoringAndBackup(names []string, log *zap.SugaredLogger) {

controllers/om/deployment/testing_utils.go

@@ -37,7 +37,7 @@ func CreateFromReplicaSet(mongoDBImage string, forceEnterprise bool, rs *mdb.Mon
 		lastConfig.ToMap(),
 		zap.S(),
 	)
-	d.AddMonitoringAndBackup(zap.S(), rs.Spec.GetSecurity().IsTLSEnabled(), util.CAFilePathInContainer)
+	d.ConfigureMonitoringAndBackup(zap.S(), rs.Spec.GetSecurity().IsTLSEnabled(), util.CAFilePathInContainer)
 	d.ConfigureTLS(rs.Spec.GetSecurity(), util.CAFilePathInContainer)
 	return d
 }

controllers/om/deployment_test.go

@@ -489,12 +489,12 @@ func TestConfiguringTlsProcessFromOpsManager(t *testing.T) {
 	}
 }
 
-func TestAddMonitoring(t *testing.T) {
+func TestConfigureMonitoring(t *testing.T) {
 	d := NewDeployment()
 
 	rs0 := buildRsByProcesses("my-rs", createReplicaSetProcessesCount(3, "my-rs"))
 	d.MergeReplicaSet(rs0, nil, nil, zap.S())
-	d.AddMonitoring(zap.S(), false, util.CAFilePathInContainer)
+	d.ConfigureMonitoring(zap.S(), false, util.CAFilePathInContainer)
 
 	expectedMonitoringVersions := []interface{}{
 		map[string]interface{}{"hostname": "my-rs-0.some.host", "name": MonitoringAgentDefaultVersion},
@@ -504,16 +504,16 @@ func TestAddMonitoring(t *testing.T) {
 	assert.Equal(t, expectedMonitoringVersions, d.getMonitoringVersions())
 
 	// adding again - nothing changes
-	d.AddMonitoring(zap.S(), false, util.CAFilePathInContainer)
+	d.ConfigureMonitoring(zap.S(), false, util.CAFilePathInContainer)
 	assert.Equal(t, expectedMonitoringVersions, d.getMonitoringVersions())
 }
 
-func TestAddMonitoringTls(t *testing.T) {
+func TestConfigureMonitoringTls(t *testing.T) {
 	d := NewDeployment()
 
 	rs0 := buildRsByProcesses("my-rs", createReplicaSetProcessesCount(3, "my-rs"))
 	d.MergeReplicaSet(rs0, nil, nil, zap.S())
-	d.AddMonitoring(zap.S(), true, util.CAFilePathInContainer)
+	d.ConfigureMonitoring(zap.S(), true, util.CAFilePathInContainer)
 
 	expectedAdditionalParams := map[string]string{
 		"useSslForAllConnections":      "true",
@@ -528,10 +528,39 @@ func TestAddMonitoringTls(t *testing.T) {
 	assert.Equal(t, expectedMonitoringVersions, d.getMonitoringVersions())
 
 	// adding again - nothing changes
-	d.AddMonitoring(zap.S(), false, util.CAFilePathInContainer)
+	d.ConfigureMonitoring(zap.S(), true, util.CAFilePathInContainer)
 	assert.Equal(t, expectedMonitoringVersions, d.getMonitoringVersions())
 }
 
+func TestConfigureMonitoringTLSDisable(t *testing.T) {
+	d := NewDeployment()
+
+	rs0 := buildRsByProcesses("my-rs", createReplicaSetProcessesCount(3, "my-rs"))
+	d.MergeReplicaSet(rs0, nil, nil, zap.S())
+	d.ConfigureMonitoring(zap.S(), true, util.CAFilePathInContainer)
+
+	// verify TLS is present in additionalParams
+	expectedAdditionalParams := map[string]string{
+		"useSslForAllConnections":      "true",
+		"sslTrustedServerCertificates": util.CAFilePathInContainer,
+	}
+	expectedMonitoringVersionsWithTls := []interface{}{
+		map[string]interface{}{"hostname": "my-rs-0.some.host", "name": MonitoringAgentDefaultVersion, "additionalParams": expectedAdditionalParams},
+		map[string]interface{}{"hostname": "my-rs-1.some.host", "name": MonitoringAgentDefaultVersion, "additionalParams": expectedAdditionalParams},
+		map[string]interface{}{"hostname": "my-rs-2.some.host", "name": MonitoringAgentDefaultVersion, "additionalParams": expectedAdditionalParams},
+	}
+	assert.Equal(t, expectedMonitoringVersionsWithTls, d.getMonitoringVersions())
+
+	// disabling TLS should clear additionalParams (CLOUDP-351614)
+	d.ConfigureMonitoring(zap.S(), false, util.CAFilePathInContainer)
+	expectedMonitoringVersionsWithoutTls := []interface{}{
+		map[string]interface{}{"hostname": "my-rs-0.some.host", "name": MonitoringAgentDefaultVersion},
+		map[string]interface{}{"hostname": "my-rs-1.some.host", "name": MonitoringAgentDefaultVersion},
+		map[string]interface{}{"hostname": "my-rs-2.some.host", "name": MonitoringAgentDefaultVersion},
+	}
+	assert.Equal(t, expectedMonitoringVersionsWithoutTls, d.getMonitoringVersions())
+}
+
 func TestAddBackup(t *testing.T) {
 	d := NewDeployment()
 

controllers/operator/appdbreplicaset_controller.go

@@ -1430,9 +1430,18 @@ func addMonitoring(ac *automationconfig.AutomationConfig, log *zap.SugaredLogger
 	monitoringVersions := ac.MonitoringVersions
 	for _, p := range ac.Processes {
 		found := false
-		for _, m := range monitoringVersions {
+		for i, m := range monitoringVersions {
 			if m.Hostname == p.HostName {
 				found = true
+				if !tls {
+					// Clear TLS-specific params when TLS is disabled to prevent monitoring from
+					// trying to use certificate files that no longer exist.
+					// We only clear TLS fields, preserving any other additionalParams that may be set.
+					clearTLSParamsFromAdditionalParams(monitoringVersions[i].AdditionalParams)
+					if len(monitoringVersions[i].AdditionalParams) == 0 {
+						monitoringVersions[i].AdditionalParams = nil
+					}
+				}
 				break
 			}
 		}
@@ -1442,15 +1451,11 @@ func addMonitoring(ac *automationconfig.AutomationConfig, log *zap.SugaredLogger
 				Name:     om.MonitoringAgentDefaultVersion,
 			}
 			if tls {
-				additionalParams := map[string]string{
-					"useSslForAllConnections":      "true",
-					"sslTrustedServerCertificates": appdbCAFilePath,
-				}
-				pemKeyFile := p.Args26.Get("net.tls.certificateKeyFile")
-				if pemKeyFile != nil {
-					additionalParams["sslClientCertificate"] = pemKeyFile.String()
+				pemKeyFile := ""
+				if pem := p.Args26.Get("net.tls.certificateKeyFile"); pem != nil {
+					pemKeyFile = pem.String()
 				}
-				monitoringVersion.AdditionalParams = additionalParams
+				monitoringVersion.AdditionalParams = buildTLSAdditionalParams(appdbCAFilePath, pemKeyFile)
 			}
 			log.Debugw("Added monitoring agent configuration", "host", p.HostName, "tls", tls)
 			monitoringVersions = append(monitoringVersions, monitoringVersion)
@@ -1459,6 +1464,33 @@ func addMonitoring(ac *automationconfig.AutomationConfig, log *zap.SugaredLogger
 	ac.MonitoringVersions = monitoringVersions
 }
 
+// buildTLSAdditionalParams creates the additionalParams map for monitoring with TLS enabled.
+// This is the single source of truth for TLS params - clearTLSParamsFromAdditionalParams
+// uses this function to determine which keys to remove.
+func buildTLSAdditionalParams(caFilePath string, pemKeyFile string) map[string]string {
+	params := map[string]string{
+		"useSslForAllConnections":      "true",
+		"sslTrustedServerCertificates": caFilePath,
+	}
+	if pemKeyFile != "" {
+		params["sslClientCertificate"] = pemKeyFile
+	}
+	return params
+}
+
+// clearTLSParamsFromAdditionalParams removes TLS-specific fields from the monitoring
+// version's additionalParams map. It derives which keys to remove from buildTLSAdditionalParams,
+// ensuring add and remove operations always stay in sync.
+func clearTLSParamsFromAdditionalParams(additionalParams map[string]string) {
+	if additionalParams == nil {
+		return
+	}
+	// Use non-empty dummy values to get all possible TLS param keys
+	for key := range buildTLSAdditionalParams("_", "_") {
+		delete(additionalParams, key)
+	}
+}
+
 // registerAppDBHostsWithProject uses the Hosts API to add each process in the AppDB to the project
 func (r *ReconcileAppDbReplicaSet) registerAppDBHostsWithProject(hostnames []string, conn om.Connection, opsManagerPassword string, log *zap.SugaredLogger) error {
 	getHostsResult, err := conn.GetHosts()

controllers/operator/appdbreplicaset_controller_test.go

@@ -1457,3 +1457,77 @@ func createRunningAppDB(ctx context.Context, t *testing.T, startingMembers int,
 	assert.Equal(t, ok, res)
 	return reconciler
 }
+
+// TestClearTLSParamsFromAdditionalParams tests CLOUDP-351614 fix:
+// When TLS is disabled on AppDB, TLS-specific params should be cleared from
+// the monitoring config's additionalParams to prevent the monitoring agent
+// from trying to use certificate files that no longer exist.
+func TestClearTLSParamsFromAdditionalParams(t *testing.T) {
+	tests := []struct {
+		name           string
+		input          map[string]string
+		expectedOutput map[string]string
+	}{
+		{
+			name:           "nil map",
+			input:          nil,
+			expectedOutput: nil,
+		},
+		{
+			name:           "empty map",
+			input:          map[string]string{},
+			expectedOutput: map[string]string{},
+		},
+		{
+			name: "only TLS params",
+			input: map[string]string{
+				"useSslForAllConnections":      "true",
+				"sslTrustedServerCertificates": "/some/path/ca.pem",
+				"sslClientCertificate":         "/some/path/cert.pem",
+			},
+			expectedOutput: map[string]string{},
+		},
+		{
+			name: "mixed params - TLS and non-TLS",
+			input: map[string]string{
+				"useSslForAllConnections":      "true",
+				"sslTrustedServerCertificates": "/some/path/ca.pem",
+				"sslClientCertificate":         "/some/path/cert.pem",
+				"someOtherParam":               "someValue",
+				"anotherParam":                 "anotherValue",
+			},
+			expectedOutput: map[string]string{
+				"someOtherParam": "someValue",
+				"anotherParam":   "anotherValue",
+			},
+		},
+		{
+			name: "only non-TLS params",
+			input: map[string]string{
+				"someOtherParam": "someValue",
+				"anotherParam":   "anotherValue",
+			},
+			expectedOutput: map[string]string{
+				"someOtherParam": "someValue",
+				"anotherParam":   "anotherValue",
+			},
+		},
+		{
+			name: "partial TLS params",
+			input: map[string]string{
+				"useSslForAllConnections": "true",
+				"someOtherParam":          "someValue",
+			},
+			expectedOutput: map[string]string{
+				"someOtherParam": "someValue",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			clearTLSParamsFromAdditionalParams(tt.input)
+			assert.Equal(t, tt.expectedOutput, tt.input)
+		})
+	}
+}

controllers/operator/common_controller.go

@@ -1075,7 +1075,7 @@ func ReconcileReplicaSetAC(ctx context.Context, d om.Deployment, spec mdbv1.DbCo
 	}
 
 	d.MergeReplicaSet(rs, spec.GetAdditionalMongodConfig().ToMap(), lastMongodConfig, log)
-	d.AddMonitoringAndBackup(log, spec.GetSecurity().IsTLSEnabled(), caFilePath)
+	d.ConfigureMonitoringAndBackup(log, spec.GetSecurity().IsTLSEnabled(), caFilePath)
 	d.ConfigureTLS(spec.GetSecurity(), caFilePath)
 	d.ConfigureInternalClusterAuthentication(rs.GetProcessNames(), spec.GetSecurity().GetInternalClusterAuthenticationMode(), internalClusterPath)
 

controllers/operator/mongodbshardedcluster_controller.go

@@ -2006,7 +2006,7 @@ func (r *ShardedClusterReconcileHelper) publishDeployment(ctx context.Context, c
 				return err
 			}
 
-			d.AddMonitoringAndBackup(log, sc.Spec.GetSecurity().IsTLSEnabled(), opts.caFilePath)
+			d.ConfigureMonitoringAndBackup(log, sc.Spec.GetSecurity().IsTLSEnabled(), opts.caFilePath)
 			d.ConfigureTLS(sc.Spec.GetSecurity(), opts.caFilePath)
 
 			setupInternalClusterAuth(d, sc.Name, sc.GetSecurity().GetInternalClusterAuthenticationMode(),

controllers/operator/mongodbshardedcluster_controller_test.go

@@ -1676,7 +1676,7 @@ func createDeploymentFromShardedCluster(t *testing.T, updatable v1.CustomResourc
 		Finalizing:      false,
 	})
 	assert.NoError(t, err)
-	d.AddMonitoringAndBackup(zap.S(), sh.Spec.GetSecurity().IsTLSEnabled(), util.CAFilePathInContainer)
+	d.ConfigureMonitoringAndBackup(zap.S(), sh.Spec.GetSecurity().IsTLSEnabled(), util.CAFilePathInContainer)
 	return d
 }