fix: preserve env var references in manager.yaml during OpAMP config updates (#3205)

When the server pushes config updates (e.g. measurements_interval), the
managerReload function was marshaling the in-memory config (which has
resolved env var values) back to YAML, replacing ${env:...} references
with plaintext secrets. This changes to a read-modify-write approach:
read the raw on-disk file (which preserves env var strings), selectively
update only the updatable fields, and write back.

Co-authored-by: Ian Adams <ian.sam.adams@gmail.com>

Author: DanQUlXOTE

opamp/observiq/reload_funcs.go

@@ -88,9 +88,59 @@ func managerReload(client *Client, managerConfigPath string) opamp.ReloadFunc {
 		client.ident.agentName = newConfig.AgentName
 		client.ident.labels = newConfig.Labels
 
-		// Write out new config file
-		// Marshal back into bytes
-		newContents, err := yaml.Marshal(client.currentConfig)
+		// Read the raw on-disk file (preserves ${env:...} references)
+		rawContents, err := os.ReadFile(filepath.Clean(managerConfigPath))
+		if err != nil {
+			// Rollback file
+			if rollbackErr := rollbackFunc(); rollbackErr != nil {
+				client.logger.Error("Rollback failed for manager config", zap.Error(rollbackErr))
+			}
+			client.ident = rollbackIdent
+			client.currentConfig = *rollBackCfg
+			return false, fmt.Errorf("failed to read manager config for update: %w", err)
+		}
+
+		// Unmarshal into a generic map (yaml.v3 does NOT resolve env vars)
+		var rawMap map[string]any
+		if err := yaml.Unmarshal(rawContents, &rawMap); err != nil {
+			// Rollback file
+			if rollbackErr := rollbackFunc(); rollbackErr != nil {
+				client.logger.Error("Rollback failed for manager config", zap.Error(rollbackErr))
+			}
+			client.ident = rollbackIdent
+			client.currentConfig = *rollBackCfg
+			return false, fmt.Errorf("failed to parse manager config for update: %w", err)
+		}
+
+		// Update only the fields that are allowed to change via OpAMP
+		if client.currentConfig.AgentName != nil {
+			rawMap["agent_name"] = *client.currentConfig.AgentName
+		} else {
+			delete(rawMap, "agent_name")
+		}
+		if client.currentConfig.Labels != nil {
+			rawMap["labels"] = *client.currentConfig.Labels
+		} else {
+			delete(rawMap, "labels")
+		}
+		if client.currentConfig.MeasurementsInterval != 0 {
+			rawMap["measurements_interval"] = client.currentConfig.MeasurementsInterval
+		} else {
+			delete(rawMap, "measurements_interval")
+		}
+		if client.currentConfig.ExtraMeasurementsAttributes != nil {
+			rawMap["extra_measurements_attributes"] = client.currentConfig.ExtraMeasurementsAttributes
+		} else {
+			delete(rawMap, "extra_measurements_attributes")
+		}
+		if client.currentConfig.TopologyInterval != nil {
+			rawMap["topology_interval"] = *client.currentConfig.TopologyInterval
+		} else {
+			delete(rawMap, "topology_interval")
+		}
+
+		// Marshal the map back (env var references in untouched fields are preserved)
+		newContents, err := yaml.Marshal(rawMap)
 		if err != nil {
 			// Rollback file
 			if rollbackErr := rollbackFunc(); rollbackErr != nil {

opamp/observiq/reload_funcs_test.go

@@ -132,10 +132,15 @@ func Test_managerReload(t *testing.T) {
 				assert.Equal(t, newConfig.AgentName, client.ident.agentName)
 				assert.Equal(t, newConfig.AgentName, client.currentConfig.AgentName)
 
-				// Verify new file was written
+				// Verify new file was written with correct values
 				data, err := os.ReadFile(managerFilePath)
 				assert.NoError(t, err)
-				assert.Equal(t, newContents, data)
+
+				var writtenConfig opamp.Config
+				assert.NoError(t, yaml.Unmarshal(data, &writtenConfig))
+				assert.Equal(t, newConfig.Endpoint, writtenConfig.Endpoint)
+				assert.Equal(t, newConfig.AgentID.String(), writtenConfig.AgentID.String())
+				assert.Equal(t, *newConfig.AgentName, *writtenConfig.AgentName)
 			},
 		},
 		{
@@ -196,6 +201,76 @@ func Test_managerReload(t *testing.T) {
 				assert.Equal(t, currContents, data)
 			},
 		},
+		{
+			desc: "Env var references preserved in non-updatable fields",
+			testFunc: func(*testing.T) {
+				tmpDir := t.TempDir()
+
+				managerFilePath := filepath.Join(tmpDir, ManagerConfigName)
+
+				// Write a manager.yaml with env var references in non-updatable fields
+				rawYAML := []byte(`endpoint: wss://example.com
+secret_key: ${env:OPAMP_SECRET_KEY}
+agent_id: ` + testAgentID.String() + `
+labels: env=prod
+`)
+				err := os.WriteFile(managerFilePath, rawYAML, 0600)
+				require.NoError(t, err)
+
+				labels := "env=prod"
+				currConfig := &opamp.Config{
+					Endpoint: "wss://example.com",
+					SecretKey: func() *string {
+						s := "resolved-secret-value"
+						return &s
+					}(),
+					AgentID: testAgentID,
+					Labels:  &labels,
+				}
+
+				mockOpAmpClient := mocks.NewMockOpAMPClient(t)
+				mockOpAmpClient.On("SetAgentDescription", mock.Anything).Return(nil)
+
+				client := &Client{
+					logger:             zap.NewNop(),
+					opampClient:        mockOpAmpClient,
+					ident:              newIdentity(zap.NewNop(), *currConfig, "0.0.0"),
+					currentConfig:      *currConfig,
+					measurementsSender: newMeasurementsSender(zap.NewNop(), nil, mockOpAmpClient, 0, nil),
+					topologySender:     newTopologySender(zap.NewNop(), nil, mockOpAmpClient, nil),
+				}
+				reloadFunc := managerReload(client, managerFilePath)
+
+				// Create a new config with changed measurements_interval
+				agentName := "new-name"
+				newConfig := &opamp.Config{
+					Endpoint:             "wss://example.com",
+					AgentID:              testAgentID,
+					Labels:               &labels,
+					AgentName:            &agentName,
+					MeasurementsInterval: 30 * time.Second,
+				}
+
+				newContents, err := yaml.Marshal(newConfig)
+				require.NoError(t, err)
+
+				changed, err := reloadFunc(newContents)
+				assert.NoError(t, err)
+				assert.True(t, changed)
+
+				// Read back the file and verify env var reference is preserved
+				data, err := os.ReadFile(managerFilePath)
+				require.NoError(t, err)
+
+				dataStr := string(data)
+				assert.Contains(t, dataStr, "${env:OPAMP_SECRET_KEY}", "env var reference should be preserved")
+				assert.NotContains(t, dataStr, "resolved-secret-value", "resolved secret should not appear in file")
+
+				// Verify the updatable fields were updated
+				assert.Contains(t, dataStr, "new-name")
+				assert.Contains(t, dataStr, "30s")
+			},
+		},
 	}
 
 	for _, tc := range testCases {