[cmd/opampsupervisor] Fix supervisor ignoring RemoteConfig messages

.chloggen/fix_dakotapaasman-config-unchanged-report-rcs.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: "bug_fix"
+
+# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
+component: "opampsupervisor"
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Fix supervisor ignoring RemoteConfig messages from server"
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [42474]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: []

cmd/opampsupervisor/supervisor/supervisor.go

@@ -1950,10 +1950,14 @@ func (s *Supervisor) processRemoteConfigMessage(ctx context.Context, msg *protob
 		span.SetStatus(codes.Error, fmt.Sprintf("Error composing merged config. Reporting failed remote config status: %s", err.Error()))
 		s.telemetrySettings.Logger.Error("Error composing merged config. Reporting failed remote config status.", zap.Error(err))
 		s.saveAndReportConfigStatus(protobufs.RemoteConfigStatuses_RemoteConfigStatuses_FAILED, err.Error())
+		return false
 	}
 	if configChanged {
 		// only report applying if the config has changed and will run agent with new config
 		s.saveAndReportConfigStatus(protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLYING, "")
+	} else {
+		// if the config has not changed report applied status, we should still report a status to the server in this case
+		s.saveAndReportConfigStatus(protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED, "")
 	}
 
 	span.SetStatus(codes.Ok, "")

cmd/opampsupervisor/supervisor/supervisor_test.go

@@ -5,6 +5,7 @@ package supervisor
 
 import (
 	"context"
+	"crypto/sha256"
 	"errors"
 	"fmt"
 	"io"
@@ -799,11 +800,20 @@ service:
 		assert.Nil(t, s.cfgState.Load())
 		assert.True(t, remoteConfigStatusUpdated)
 	})
-	t.Run("RemoteConfig - Don't report status if config is not changed", func(t *testing.T) {
-		const testConfigMessage = `receivers:
-  debug:`
-
-		const expectedMergedConfig = `extensions:
+	t.Run("RemoteConfig - Report applied status if config is not changed", func(t *testing.T) {
+		const initialConfigMessage = `receivers:
+  debug:
+  exporters:
+    nop:`
+		const remoteConfigMessage = `exporters:
+  nop:
+receivers:
+  debug:
+`
+		// mergedConfig should be the result of creating the config for both the initial and remote config messages
+		const mergedConfig = `exporters:
+    nop: null
+extensions:
     opamp:
         capabilities:
             reports_available_components: false
@@ -830,28 +840,48 @@ service:
         resource: null
 `
 
+		// store the initial remote config message so the supervisor is initialized with it
+		configStorageDir := t.TempDir()
+		err := os.WriteFile(filepath.Join(configStorageDir, lastRecvRemoteConfigFile), []byte(initialConfigMessage), 0o600)
+		require.NoError(t, err)
+
 		// the remote config message we will send that will get merged and compared with the initial config
+		remoteConfigHash := sha256.Sum256([]byte(remoteConfigMessage))
 		remoteConfig := &protobufs.AgentRemoteConfig{
 			Config: &protobufs.AgentConfigMap{
 				ConfigMap: map[string]*protobufs.AgentConfigFile{
 					"": {
-						Body: []byte(testConfigMessage),
+						Body: []byte(remoteConfigMessage),
 					},
 				},
 			},
-			ConfigHash: []byte("hash"),
+			ConfigHash: remoteConfigHash[:],
 		}
 
 		remoteConfigStatusUpdated := false
 		mc := &mockOpAMPClient{
-			setRemoteConfigStatusFunc: func(*protobufs.RemoteConfigStatus) error {
+			setRemoteConfigStatusFunc: func(rcs *protobufs.RemoteConfigStatus) error {
 				remoteConfigStatusUpdated = true
+				// assert the
+				assert.Equal(t, remoteConfig.ConfigHash, rcs.LastRemoteConfigHash)
+				assert.Equal(t, protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED, rcs.Status)
+				assert.Empty(t, rcs.ErrorMessage)
+				return nil
+			},
+			updateEffectiveConfigFunc: func(context.Context) error {
 				return nil
 			},
 		}
 
+		// initial persistent state should be the result of the initial config message
 		testUUID := uuid.MustParse("018fee23-4a51-7303-a441-73faed7d9deb")
-		configStorageDir := t.TempDir()
+		initialRemoteConfigHash := sha256.Sum256([]byte(initialConfigMessage))
+		startingPersistentState := &persistentState{InstanceID: testUUID, LastRemoteConfigStatus: &RemoteConfigStatus{
+			LastRemoteConfigHash: string(initialRemoteConfigHash[:]),
+			Status:               protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED,
+			ErrorMessage:         "",
+		}}
+
 		s := Supervisor{
 			telemetrySettings: newNopTelemetrySettings(),
 			pidProvider:       staticPIDProvider(88888),
@@ -862,7 +892,7 @@ service:
 				},
 			},
 			hasNewConfig:                   make(chan struct{}, 1),
-			persistentState:                &persistentState{InstanceID: testUUID},
+			persistentState:                startingPersistentState,
 			agentConfigOwnTelemetrySection: &atomic.Value{},
 			effectiveConfig:                &atomic.Value{},
 			opampClient:                    mc,
@@ -880,26 +910,24 @@ service:
 			NonIdentifyingAttributes: []*protobufs.KeyValue{},
 		})
 
-		// initially write & store config so that we have the same config when we send the remote config message
-		err := os.WriteFile(filepath.Join(configStorageDir, lastRecvRemoteConfigFile), []byte(testConfigMessage), 0o600)
-		require.NoError(t, err)
-
+		// store the initial merged config
 		s.cfgState.Store(&configState{
-			mergedConfig:     expectedMergedConfig,
+			mergedConfig:     mergedConfig,
 			configMapIsEmpty: false,
 		})
 
 		s.onMessage(t.Context(), &types.MessageData{
 			RemoteConfig: remoteConfig,
 		})
 
-		// assert the remote config status callback was not called
-		assert.False(t, remoteConfigStatusUpdated)
-		// assert the config file and stored data are still the same
+		// assert the remote config status callback was called
+		assert.True(t, remoteConfigStatusUpdated)
+
+		// assert the config file and stored data are updated
 		fileContent, err := os.ReadFile(filepath.Join(configStorageDir, lastRecvRemoteConfigFile))
 		require.NoError(t, err)
-		assert.Contains(t, string(fileContent), testConfigMessage)
-		assert.Equal(t, expectedMergedConfig, s.cfgState.Load().(*configState).mergedConfig)
+		assert.Contains(t, string(fileContent), remoteConfigMessage)
+		assert.Equal(t, mergedConfig, s.cfgState.Load().(*configState).mergedConfig)
 	})
 
 	t.Run("RemoteConfig - do nothing if not capable of accepting remote config", func(t *testing.T) {