Repository: open-telemetry/opamp-go
Pull request #73 (2022-05-13 15:54:22)

Make Hash field calculations internal responsibility
The Hash is now calculated internally by the client instead of requiring
the caller to use the helper function to calculate the Hash before
passing the message to the client.

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#73](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (3ba00f4) into [main](https://codecov.io/gh/open-telemetry/opamp-go/commit/6e8043342f6a1ec6f21d3174c446e1da57badf1b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (6e80433) will **increase** coverage by `0.31%`.
> The diff coverage is `71.42%`.

```diff
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   77.70%   78.02%   +0.31%     
==========================================
  Files          21       20       -1     
  Lines        1301     1297       -4     
==========================================
+ Hits         1011     1012       +1     
+ Misses        218      215       -3     
+ Partials       72       70       -2     
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [client/types/callbacks.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/73/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L3R5cGVzL2NhbGxiYWNrcy5nbw==) | `65.00% <ø> (ø)` | |
| [client/internal/clientstate.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/73/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2NsaWVudHN0YXRlLmdv) | `69.86% <57.57%> (-2.48%)` | :arrow_down: |
| [client/internal/clientcommon.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/73/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2NsaWVudGNvbW1vbi5nbw==) | `84.61% <100.00%> (+5.92%)` | :arrow_up: |
| [client/internal/receivedprocessor.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/73/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL3JlY2VpdmVkcHJvY2Vzc29yLmdv) | `83.90% <100.00%> (-0.10%)` | :arrow_down: |
| [client/internal/wssender.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/73/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL3dzc2VuZGVyLmdv) | `81.81% <0.00%> (-9.10%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Last update [6e80433...3ba00f4](https://codecov.io/gh/open-telemetry/opamp-go/pull/73?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).

andykellr: This looks good, but should we keep helpers or provide something similar for the server to use? Particularly with the json encode and the issue mentioned in #70, it may not be obvious how the server should compute a hash to compare with the client hash.
andykellr: It should be possible to internalize the hash computation on the server side as well, but I haven't had time to think though the changes required.
tigrannajaryan: > This looks good, but should we keep helpers or provide something similar for the server to use? Particularly with the json encode and the issue mentioned in #70, it may not be obvious how the server should compute a hash to compare with the client hash.

The server should not need to computer the hash. It only needs to store and compare. See example implementation: https://github.com/open-telemetry/opamp-go/blob/dfbe616ffb2211c2e75870f461e4521f1419de94/internal/examples/server/data/agent.go#L113

However if we find that for some other cases it is needed we can refactor again to make them publicly available helpers.
tigrannajaryan: Rebased and conflicts resolved.

Also need to fix https://github.com/open-telemetry/opamp-go/issues/70 but will do in a separate PR.
andykellr: > > This looks good, but should we keep helpers or provide something similar for the server to use? Particularly with the json encode and the issue mentioned in #70, it may not be obvious how the server should compute a hash to compare with the client hash.
> 
> The server should not need to computer the hash. It only needs to store and compare. See example implementation:
> 
> https://github.com/open-telemetry/opamp-go/blob/dfbe616ffb2211c2e75870f461e4521f1419de94/internal/examples/server/data/agent.go#L113
> 
> However if we find that for some other cases it is needed we can refactor again to make them publicly available helpers.

I was thinking of EffectiveConfig, not AgentDescription, but it's a similar concern.

https://github.com/open-telemetry/opamp-go/blob/dfbe616ffb2211c2e75870f461e4521f1419de94/internal/examples/server/data/agent.go#L254


tigrannajaryan: > I was thinking of EffectiveConfig, not AgentDescription, but it's a similar concern.

Right. I think the server doesn't need to compute the hashes.

Patch:
diff --git a/client/client.go b/client/client.go
index 73f14cf..5b0174c 100644
--- a/client/client.go
+++ b/client/client.go
@@ -47,6 +47,10 @@ type OpAMPClient interface {
 	// May be also called after Start(), in which case the attributes will be included
 	// in the next outgoing status report. This is typically used by Agents which allow
 	// their AgentDescription to change dynamically while the OpAMPClient is started.
+	//
+	// The Hash field will be calculated and updated from the content of the rest of
+	// the fields.
+	//
 	// nil values are not allowed and will return an error.
 	SetAgentDescription(descr *protobufs.AgentDescription) error
 
diff --git a/client/helpers.go b/client/helpers.go
deleted file mode 100644
index 6fa567c..0000000
--- a/client/helpers.go
+++ /dev/null
@@ -1,60 +0,0 @@
-package client
-
-import (
-	"crypto/sha256"
-	"hash"
-
-	"github.com/open-telemetry/opamp-go/protobufs"
-	"google.golang.org/protobuf/encoding/protojson"
-)
-
-func writeHashKV(hash hash.Hash, kv *protobufs.KeyValue) error {
-	// To keep the implementation simple we convert the data to an equivalent JSON
-	// string and calculate the hash from the string bytes.
-	b, err := protojson.Marshal(kv)
-	if err != nil {
-		return err
-	}
-	hash.Write(b)
-	return nil
-}
-
-func writeHashKVList(hash hash.Hash, kvl []*protobufs.KeyValue) error {
-	for _, kv := range kvl {
-		err := writeHashKV(hash, kv)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// CalcHashAgentDescription calculates and sets the Hash field from the rest of the
-// fields in the message.
-func CalcHashAgentDescription(msg *protobufs.AgentDescription) error {
-	h := sha256.New()
-	err := writeHashKVList(h, msg.IdentifyingAttributes)
-	if err != nil {
-		return err
-	}
-	err = writeHashKVList(h, msg.NonIdentifyingAttributes)
-	if err != nil {
-		return err
-	}
-	msg.Hash = h.Sum(nil)
-	return nil
-}
-
-// CalcHashEffectiveConfig calculates and sets the Hash field from the rest of the
-// fields in the message.
-func CalcHashEffectiveConfig(msg *protobufs.EffectiveConfig) {
-	h := sha256.New()
-	if msg.ConfigMap != nil {
-		for k, v := range msg.ConfigMap.ConfigMap {
-			h.Write([]byte(k))
-			h.Write(v.Body)
-			h.Write([]byte(v.ContentType))
-		}
-	}
-	msg.Hash = h.Sum(nil)
-}
diff --git a/client/internal/clientcommon.go b/client/internal/clientcommon.go
index a0f7bf2..1f08865 100644
--- a/client/internal/clientcommon.go
+++ b/client/internal/clientcommon.go
@@ -2,6 +2,7 @@ package internal
 
 import (
 	"context"
+	"crypto/sha256"
 	"errors"
 	"fmt"
 	"sync"
@@ -62,7 +63,6 @@ func (c *ClientCommon) PrepareStart(_ context.Context, settings types.StartSetti
 		settings.RemoteConfigStatus = &protobufs.RemoteConfigStatus{
 			Status: protobufs.RemoteConfigStatus_UNSET,
 		}
-		calcHashRemoteConfigStatus(settings.RemoteConfigStatus)
 	}
 
 	if err := c.ClientSyncedState.SetRemoteConfigStatus(settings.RemoteConfigStatus); err != nil {
@@ -143,8 +143,8 @@ func (c *ClientCommon) PrepareFirstMessage(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
-	if cfg != nil && len(cfg.Hash) == 0 {
-		return errors.New("EffectiveConfig hash is empty")
+	if cfg != nil {
+		calcHashEffectiveConfig(cfg)
 	}
 
 	c.sender.NextMessage().Update(
@@ -188,6 +188,20 @@ func (c *ClientCommon) SetAgentDescription(descr *protobufs.AgentDescription) er
 	return nil
 }
 
+// calcHashEffectiveConfig calculates and sets the Hash field from the rest of the
+// fields in the message.
+func calcHashEffectiveConfig(msg *protobufs.EffectiveConfig) {
+	h := sha256.New()
+	if msg.ConfigMap != nil {
+		for k, v := range msg.ConfigMap.ConfigMap {
+			h.Write([]byte(k))
+			h.Write(v.Body)
+			h.Write([]byte(v.ContentType))
+		}
+	}
+	msg.Hash = h.Sum(nil)
+}
+
 // UpdateEffectiveConfig fetches the current local effective config using
 // GetEffectiveConfig callback and sends it to the Server using provided Sender.
 func (c *ClientCommon) UpdateEffectiveConfig(ctx context.Context) error {
@@ -196,8 +210,8 @@ func (c *ClientCommon) UpdateEffectiveConfig(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("GetEffectiveConfig failed: %w", err)
 	}
-	if cfg != nil && len(cfg.Hash) == 0 {
-		return errors.New("hash field must be set, use CalcHashEffectiveConfig")
+	if cfg != nil {
+		calcHashEffectiveConfig(cfg)
 	}
 	// Send it to the Server.
 	c.sender.NextMessage().UpdateStatus(func(statusReport *protobufs.StatusReport) {
diff --git a/client/internal/clientstate.go b/client/internal/clientstate.go
index 0aa9fa1..b6c718f 100644
--- a/client/internal/clientstate.go
+++ b/client/internal/clientstate.go
@@ -2,10 +2,11 @@ package internal
 
 import (
 	"crypto/sha256"
-	"errors"
+	"hash"
 	"sync"
 
 	"github.com/open-telemetry/opamp-go/protobufs"
+	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -50,7 +51,43 @@ func (s *ClientSyncedState) PackageStatuses() *protobufs.PackageStatuses {
 	return s.packageStatuses
 }
 
+func writeHashKV(hash hash.Hash, kv *protobufs.KeyValue) error {
+	// To keep the implementation simple we convert the data to an equivalent JSON
+	// string and calculate the hash from the string bytes.
+	b, err := protojson.Marshal(kv)
+	if err != nil {
+		return err
+	}
+	hash.Write(b)
+	return nil
+}
+
+func writeHashKVList(hash hash.Hash, kvl []*protobufs.KeyValue) error {
+	for _, kv := range kvl {
+		err := writeHashKV(hash, kv)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func calcHashAgentDescription(msg *protobufs.AgentDescription) error {
+	h := sha256.New()
+	err := writeHashKVList(h, msg.IdentifyingAttributes)
+	if err != nil {
+		return err
+	}
+	err = writeHashKVList(h, msg.NonIdentifyingAttributes)
+	if err != nil {
+		return err
+	}
+	msg.Hash = h.Sum(nil)
+	return nil
+}
+
 // SetAgentDescription sets the AgentDescription in the state.
+// Will calculate the Hash from the content of the other fields.
 func (s *ClientSyncedState) SetAgentDescription(descr *protobufs.AgentDescription) error {
 	if descr == nil {
 		return ErrAgentDescriptionMissing
@@ -60,12 +97,12 @@ func (s *ClientSyncedState) SetAgentDescription(descr *protobufs.AgentDescriptio
 		return ErrAgentDescriptionNoAttributes
 	}
 
-	clone := proto.Clone(descr).(*protobufs.AgentDescription)
-
-	if len(descr.Hash) == 0 {
-		return errors.New("hash field must be set, use CalcHashAgentDescription")
+	if err := calcHashAgentDescription(descr); err != nil {
+		return err
 	}
 
+	clone := proto.Clone(descr).(*protobufs.AgentDescription)
+
 	defer s.mutex.Unlock()
 	s.mutex.Lock()
 	s.agentDescription = clone
@@ -74,7 +111,6 @@ func (s *ClientSyncedState) SetAgentDescription(descr *protobufs.AgentDescriptio
 }
 
 func calcHashRemoteConfigStatus(status *protobufs.RemoteConfigStatus) {
-	// Calculate and set the Hash field from the rest of the fields in the message.
 	h := sha256.New()
 	h.Write(status.LastRemoteConfigHash)
 	h.Write([]byte(status.Status.String()))
@@ -82,14 +118,14 @@ func calcHashRemoteConfigStatus(status *protobufs.RemoteConfigStatus) {
 	status.Hash = h.Sum(nil)
 }
 
+// SetRemoteConfigStatus sets the RemoteConfigStatus in the state.
+// Will calculate the Hash from the content of the other fields.
 func (s *ClientSyncedState) SetRemoteConfigStatus(status *protobufs.RemoteConfigStatus) error {
 	if status == nil {
 		return errRemoteConfigStatusMissing
 	}
 
-	if len(status.Hash) == 0 {
-		return errors.New("hash field must be set, use CalcHashRemoteConfigStatus")
-	}
+	calcHashRemoteConfigStatus(status)
 
 	clone := proto.Clone(status).(*protobufs.RemoteConfigStatus)
 
diff --git a/client/internal/receivedprocessor.go b/client/internal/receivedprocessor.go
index efb6716..f97a6ca 100644
--- a/client/internal/receivedprocessor.go
+++ b/client/internal/receivedprocessor.go
@@ -142,20 +142,19 @@ func (r *receivedProcessor) rcvRemoteConfig(
 
 	// Respond back with the hash of the config received from the Server.
 	cfgStatus.LastRemoteConfigHash = remoteCfg.ConfigHash
-	calcHashRemoteConfigStatus(cfgStatus)
 
 	// Get the hash of the status before we update it.
 	prevStatus := r.clientSyncedState.RemoteConfigStatus()
-	var cfgStatusHash []byte
+	var prevCfgStatusHash []byte
 	if prevStatus != nil {
-		cfgStatusHash = prevStatus.Hash
+		prevCfgStatusHash = prevStatus.Hash
 	}
 
 	// Remember the status for the future use.
 	_ = r.clientSyncedState.SetRemoteConfigStatus(cfgStatus)
 
 	// Check if the status has changed by comparing the hashes.
-	if !bytes.Equal(cfgStatusHash, cfgStatus.Hash) {
+	if !bytes.Equal(prevCfgStatusHash, cfgStatus.Hash) {
 		// Let the Agent know about the status.
 		r.callbacks.SaveRemoteConfigStatus(ctx, cfgStatus)
 
diff --git a/client/types/callbacks.go b/client/types/callbacks.go
index 538b538..4969a6f 100644
--- a/client/types/callbacks.go
+++ b/client/types/callbacks.go
@@ -45,6 +45,9 @@ type Callbacks interface {
 	// the Server while OnRemoteConfig call has not returned will be remembered.
 	// Once OnRemoteConfig call returns it will be called again with the most recent
 	// remote config received.
+	//
+	// The EffectiveConfig.Hash field will be calculated and updated from the content
+	// of the rest of the fields.
 	OnRemoteConfig(
 		ctx context.Context,
 		remoteConfig *protobufs.AgentRemoteConfig,
@@ -60,6 +63,9 @@ type Callbacks interface {
 	// GetEffectiveConfig returns the current effective config. Only one
 	// GetEffectiveConfig call can be active at any time. Until GetEffectiveConfig
 	// returns it will not be called again.
+	//
+	// The Hash field in the returned EffectiveConfig will be calculated and updated
+	// by the caller from the content of the rest of the fields.
 	GetEffectiveConfig(ctx context.Context) (*protobufs.EffectiveConfig, error)
 
 	// OnOpampConnectionSettings is called when the Agent receives an OpAMP
diff --git a/client/types/startsettings.go b/client/types/startsettings.go
index 851a3ba..f2fe95c 100644
--- a/client/types/startsettings.go
+++ b/client/types/startsettings.go
@@ -30,6 +30,8 @@ type StartSettings struct {
 	// The remote config status. If nil is passed it will force
 	// the Server to send a remote config back. It is not required to set the Hash
 	// field, it will be calculated by Start() function.
+	// The Hash field will be calculated and updated from the content of the rest of
+	// the fields.
 	RemoteConfigStatus *protobufs.RemoteConfigStatus
 
 	LastConnectionSettingsHash []byte
diff --git a/internal/examples/agent/agent/agent.go b/internal/examples/agent/agent/agent.go
index dea0acb..70625ba 100644
--- a/internal/examples/agent/agent/agent.go
+++ b/internal/examples/agent/agent/agent.go
@@ -167,9 +167,6 @@ func (agent *Agent) createAgentIdentity() {
 			},
 		},
 	}
-	if err := client.CalcHashAgentDescription(agent.agentDescription); err != nil {
-		agent.logger.Errorf("Cannot calculate the hash: %v", err)
-	}
 }
 
 func (agent *Agent) updateAgentIdentity(instanceId ulid.ULID) {


Test patch:
diff --git a/client/clientimpl_test.go b/client/clientimpl_test.go
index 4f53842..837d710 100644
--- a/client/clientimpl_test.go
+++ b/client/clientimpl_test.go
@@ -3,7 +3,6 @@ package client
 import (
 	"context"
 	"errors"
-	"log"
 	"math/rand"
 	"net/http"
 	"net/url"
@@ -33,10 +32,6 @@ func createAgentDescr() *protobufs.AgentDescription {
 			},
 		},
 	}
-	err := CalcHashAgentDescription(agentDescr)
-	if err != nil {
-		log.Fatalln(err)
-	}
 	return agentDescr
 }
 
@@ -414,7 +409,6 @@ func createEffectiveConfig() *protobufs.EffectiveConfig {
 			},
 		},
 	}
-	CalcHashEffectiveConfig(cfg)
 	return cfg
 }
 
@@ -523,7 +517,6 @@ func TestSetAgentDescription(t *testing.T) {
 				Value: &protobufs.AnyValue{Value: &protobufs.AnyValue_StringValue{StringValue: "linux"}},
 			},
 		}
-		assert.NoError(t, CalcHashAgentDescription(clientAgentDescr))
 		assert.NoError(t, client.SetAgentDescription(clientAgentDescr))
 
 		// Verify change is delivered.