[extension/apmconfig] save RemoteConfigStatus.LastRemoteConfigHash regardless of status (#609)

* [extension/apmconfig] save RemoteConfigStatus.LastRemoteConfigHash regardless of status

Before this change the OpAMP server would only store the LastRemoteConfigHash
provided in an AgentToServer message if the RemoteConfigStatus.Status
was "APPLIED".

An agent may want to (a) acknowledge that a particular config was
received (via LastRemoteConfigHash) *and* (b) provide some other status
(APPLYING or FAILED). In my particular case, for EDOT Node.js, I want to
be able to support the case where the server might be sending new config
names that an older EDOT Node.js might not know about. My
RemoteConfigStatus in this case is:
  LastRemoteConfigHash: ...
  Status=FAILED
  ErrorMessage="there were issues applying remote config: config name 'some_new_var' is unsupported"

The downside of the current behaviour is that the server will ignore the
provided LastRemoteConfigHash and continue to re-send the same remote
config in every ServerToAgent. Re-sending is not going to fix the issue
the agent had with applying the config.

My interpretation of the OpAMP spec and proto is that the OpAMP server
should use *only* the LastRemoteConfigHash field for determining if
RemoteConfig should be sent. The proto says:

    // The hash of the remote config that was last received by this Agent in the
    // AgentRemoteConfig.config_hash field.
    // The Server SHOULD compare this hash with the config hash
    // it has for the Agent and if the hashes are different the Server MUST include
    // the remote_config field in the response in the ServerToAgent message.

* fix test case

* ensure we save RemoteConfigStatus before a possible early return

* add a test case for the RemoteConfigStatuses_FAILED case

---------

Co-authored-by: Roger Coll <[email protected]>

Author: trentm

extension/apmconfigextension/opamp_callbacks.go

@@ -119,6 +119,12 @@ func (rc *remoteConfigCallbacks) onMessage(ctx context.Context, conn types.Conne
 		rc.logger.Warn("unexpected type in agentState cache", agentUidField)
 		return rc.serverError("internal error: invalid agent state", &serverToAgent)
 	}
+	remoteConfigStatus := message.GetRemoteConfigStatus()
+	if remoteConfigStatus != nil {
+		agent.lastConfigHash = remoteConfigStatus.GetLastRemoteConfigHash()
+		rc.logger.Info("Remote config status", agentUidField, zap.String("lastRemoteConfigHash", hex.EncodeToString(agent.lastConfigHash)), zap.String("status", remoteConfigStatus.GetStatus().String()), zap.String("errorMessage", remoteConfigStatus.ErrorMessage))
+		rc.agentState.Store(agentUid, agent)
+	}
 
 	remoteConfig, err := rc.configClient.RemoteConfig(ctx, agent.agentUid, agent.identifyingAttributes)
 	if err != nil {
@@ -132,11 +138,7 @@ func (rc *remoteConfigCallbacks) onMessage(ctx context.Context, conn types.Conne
 		return &serverToAgent
 	}
 
-	if message.GetRemoteConfigStatus() != nil && message.GetRemoteConfigStatus().Status == protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED && bytes.Equal(remoteConfig.ConfigHash, message.RemoteConfigStatus.GetLastRemoteConfigHash()) {
-		rc.logger.Info("Remote config applied", zap.String("hash", hex.EncodeToString(remoteConfig.ConfigHash)), agentUidField)
-		agent.lastConfigHash = message.GetRemoteConfigStatus().GetLastRemoteConfigHash()
-		rc.agentState.Store(agentUid, agent)
-	} else if !bytes.Equal(agent.lastConfigHash, remoteConfig.ConfigHash) {
+	if !bytes.Equal(agent.lastConfigHash, remoteConfig.ConfigHash) {
 		rc.logger.Info("Sending new remote configuration", agentUidField, zap.String("hash", hex.EncodeToString(remoteConfig.ConfigHash)))
 		serverToAgent.RemoteConfig = remoteConfig
 	}

extension/apmconfigextension/opamp_callbacks_test.go

@@ -177,17 +177,39 @@ func TestOnMessage(t *testing.T) {
 					expectedServerToAgent: &protobufs.ServerToAgent{
 						InstanceUid:  []byte("test"),
 						Capabilities: uint64(protobufs.ServerCapabilities_ServerCapabilities_OffersRemoteConfig),
-						RemoteConfig: &protobufs.AgentRemoteConfig{
-							ConfigHash: []byte("abcd"),
-							Config: &protobufs.AgentConfigMap{
-								ConfigMap: map[string]*protobufs.AgentConfigFile{
-									"elastic": {
-										Body:        []byte(`{"test":"aaa"}`),
-										ContentType: "application/json",
-									},
+					},
+				},
+			},
+			callbacks: newRemoteConfigCallbacks(&remoteConfigMock{
+				remoteConfigFn: func(context.Context, apmconfig.InstanceUid, apmconfig.IdentifyingAttributes) (*protobufs.AgentRemoteConfig, error) {
+					return &protobufs.AgentRemoteConfig{
+						ConfigHash: []byte("abcd"),
+						Config: &protobufs.AgentConfigMap{
+							ConfigMap: map[string]*protobufs.AgentConfigFile{
+								"elastic": {
+									ContentType: "application/json",
+									Body:        []byte(`{"test":"aaa"}`),
 								},
 							},
 						},
+					}, nil
+				},
+			}, zap.NewNop()),
+		},
+		"agent failed to apply remote config": {
+			opampMessages: []inOutOpamp{
+				{
+					agentToServer: &protobufs.AgentToServer{
+						InstanceUid: []byte("test"),
+						RemoteConfigStatus: &protobufs.RemoteConfigStatus{
+							LastRemoteConfigHash: []byte("abcd"),
+							Status:               protobufs.RemoteConfigStatuses_RemoteConfigStatuses_FAILED,
+							ErrorMessage:         "oh noes!",
+						},
+					},
+					expectedServerToAgent: &protobufs.ServerToAgent{
+						InstanceUid:  []byte("test"),
+						Capabilities: uint64(protobufs.ServerCapabilities_ServerCapabilities_OffersRemoteConfig),
 					},
 				},
 			},