Skip to content

Commit

Permalink
[cmd/opampsupervisor,extension/opamp] Update opamp-go v0.15.0 (#33416)
Browse files Browse the repository at this point in the history
**Description:**
* Updates opamp-go to v0.15.0

This change is breaking, in that an opamp server using v0.14.0 may be
incompatible with the changes introduced in v0.15.0 (this is due to
invalid UTF-8 sequences now being allowed in the agent's instance ID).

As part of this update, the preferred format for IDs in the opamp
extension's config has changed to UUID (any UUID is allowed, but
specifically v7 is preferred). I've allowed ULIDs to still be specified,
so older configurations should still work.

For the supervisor, I've changed the ULID generated to be a UUID. This
is a breaking change for the persistent state, but this component is in
developmen status, and breaking changes are expected.

**Testing:** 
Unit tests.
Tested connecting the supervisor to a management server using v0.15.0 of
OpAMP.

**Documentation:**
* Modified documentation to switch references to ULID to UUID

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Co-authored-by: Yang Song <songy23@users.noreply.github.com>
  • Loading branch information
4 people committed Jun 7, 2024
1 parent 39a7773 commit 595c44b
Show file tree
Hide file tree
Showing 19 changed files with 215 additions and 114 deletions.
21 changes: 21 additions & 0 deletions .chloggen/deps_update-opamp-go-v0.15.0.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: cmd/opampsupervisor,extension/opamp

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Upgrade the opamp-go library to v0.15.0

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [33416]

# (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: |
With this change, UUIDv7 is recommended for the OpAMP extension's instance_uid field instead of ULID. ULIDs will continue to work, but may be displayed as UUIDs.
The supervisor's persistent state (${storage_dir}/persistent_state.yaml) will need to be cleared to generate a new UUIDv7 instead of a ULID.
This change may be incompatible with management servers using v0.14.0 of opamp-go.
25 changes: 13 additions & 12 deletions cmd/opampsupervisor/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"text/template"
"time"

"github.com/google/uuid"
"github.com/knadh/koanf/parsers/yaml"
"github.com/knadh/koanf/providers/file"
"github.com/knadh/koanf/providers/rawbytes"
Expand Down Expand Up @@ -480,7 +481,7 @@ func TestSupervisorAgentDescriptionConfigApplies(t *testing.T) {
expectedDescription := &protobufs.AgentDescription{
IdentifyingAttributes: []*protobufs.KeyValue{
stringKeyValue("client.id", "my-client-id"),
stringKeyValue(semconv.AttributeServiceInstanceID, ad.InstanceUid),
stringKeyValue(semconv.AttributeServiceInstanceID, uuid.UUID(ad.InstanceUid).String()),
stringKeyValue(semconv.AttributeServiceName, command),
stringKeyValue(semconv.AttributeServiceVersion, version),
},
Expand Down Expand Up @@ -788,7 +789,7 @@ func TestSupervisorPersistsInstanceID(t *testing.T) {
// persist and re-use the same instance ID.
storageDir := t.TempDir()

agentIDChan := make(chan string, 1)
agentIDChan := make(chan []byte, 1)
server := newOpAMPServer(
t,
defaultConnectingHandler,
Expand All @@ -813,14 +814,14 @@ func TestSupervisorPersistsInstanceID(t *testing.T) {

t.Logf("Supervisor connected")

var firstAgentID string
var firstAgentID []byte
select {
case firstAgentID = <-agentIDChan:
case <-time.After(1 * time.Second):
t.Fatalf("failed to get first agent ID")
}

t.Logf("Got agent ID %s, shutting down supervisor", firstAgentID)
t.Logf("Got agent ID %s, shutting down supervisor", uuid.UUID(firstAgentID))

s.Shutdown()

Expand All @@ -844,7 +845,7 @@ func TestSupervisorPersistsInstanceID(t *testing.T) {

t.Logf("Supervisor connected")

var secondAgentID string
var secondAgentID []byte
select {
case secondAgentID = <-agentIDChan:
case <-time.After(1 * time.Second):
Expand All @@ -859,9 +860,9 @@ func TestSupervisorPersistsNewInstanceID(t *testing.T) {
// is properly persisted.
storageDir := t.TempDir()

newID := "01HW3GS9NWD840C5C2BZS3KYPW"
newID := uuid.MustParse("018fee23-4a51-7303-a441-73faed7d9deb")

agentIDChan := make(chan string, 1)
agentIDChan := make(chan []byte, 1)
server := newOpAMPServer(
t,
defaultConnectingHandler,
Expand All @@ -873,11 +874,11 @@ func TestSupervisorPersistsNewInstanceID(t *testing.T) {
default:
}

if message.InstanceUid != newID {
if !bytes.Equal(message.InstanceUid, newID[:]) {
return &protobufs.ServerToAgent{
InstanceUid: message.InstanceUid,
AgentIdentification: &protobufs.AgentIdentification{
NewInstanceUid: newID,
NewInstanceUid: newID[:],
},
}
}
Expand All @@ -896,7 +897,7 @@ func TestSupervisorPersistsNewInstanceID(t *testing.T) {
t.Logf("Supervisor connected")

for id := range agentIDChan {
if id == newID {
if bytes.Equal(id, newID[:]) {
t.Logf("Agent ID was changed to new ID")
break
}
Expand Down Expand Up @@ -924,12 +925,12 @@ func TestSupervisorPersistsNewInstanceID(t *testing.T) {

t.Logf("Supervisor connected")

var newRecievedAgentID string
var newRecievedAgentID []byte
select {
case newRecievedAgentID = <-agentIDChan:
case <-time.After(1 * time.Second):
t.Fatalf("failed to get second agent ID")
}

require.Equal(t, newID, newRecievedAgentID)
require.Equal(t, newID, uuid.UUID(newRecievedAgentID))
}
4 changes: 2 additions & 2 deletions cmd/opampsupervisor/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ go 1.21.0

require (
github.com/cenkalti/backoff/v4 v4.3.0
github.com/google/uuid v1.6.0
github.com/knadh/koanf/parsers/yaml v0.1.0
github.com/knadh/koanf/providers/file v0.1.0
github.com/knadh/koanf/providers/rawbytes v0.1.0
github.com/knadh/koanf/v2 v2.1.1
github.com/oklog/ulid/v2 v2.1.0
github.com/open-telemetry/opamp-go v0.14.0
github.com/open-telemetry/opamp-go v0.15.0
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/config/configopaque v1.9.1-0.20240605145924-86ee482e5b49
go.opentelemetry.io/collector/config/configtls v0.102.1
Expand Down
9 changes: 4 additions & 5 deletions cmd/opampsupervisor/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions cmd/opampsupervisor/specification/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ Collector.
### Collector Instance UID
The Supervisor maintains a Collector instance_uid (a
[ULID](https://github.com/open-telemetry/opamp-spec/blob/main/specification.md#agenttoserverinstance_uid)).
[UUIDv7](https://www.rfc-editor.org/rfc/rfc9562#section-5.7)).
The instance_uid is generated by the Supervisor on the first run or
during the Supervisor installation and remains unchanged thereafter. The
instance_uid will be used in OpAMP communication.
Expand Down Expand Up @@ -430,7 +430,7 @@ extensions:
# to an extension that implements the Authentication interface.
endpoint:

# ULID formatted as a 26 character string in canonical
# UUID formatted as a 36 character string in canonical
# representation. Auto-generated on start if missing.
# Injected by Supervisor.
# Note: can be deprecated after Collector issue #6599
Expand Down
20 changes: 4 additions & 16 deletions cmd/opampsupervisor/supervisor/persistence.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,23 @@
package supervisor

import (
"crypto/rand"
"errors"
"os"
"time"

"github.com/oklog/ulid/v2"
"github.com/google/uuid"
"gopkg.in/yaml.v3"
)

// persistentState represents persistent state for the supervisor
type persistentState struct {
InstanceID ulid.ULID `yaml:"instance_id"`
InstanceID uuid.UUID `yaml:"instance_id"`

// Path to the config file that the state should be saved to.
// This is not marshaled.
configPath string `yaml:"-"`
}

func (p *persistentState) SetInstanceID(id ulid.ULID) error {
func (p *persistentState) SetInstanceID(id uuid.UUID) error {
p.InstanceID = id
return p.writeState()
}
Expand Down Expand Up @@ -68,7 +66,7 @@ func loadPersistentState(file string) (*persistentState, error) {
}

func createNewPersistentState(file string) (*persistentState, error) {
id, err := generateNewULID()
id, err := uuid.NewV7()
if err != nil {
return nil, err
}
Expand All @@ -80,13 +78,3 @@ func createNewPersistentState(file string) (*persistentState, error) {

return p, p.writeState()
}

func generateNewULID() (ulid.ULID, error) {
entropy := ulid.Monotonic(rand.Reader, 0)
id, err := ulid.New(ulid.Timestamp(time.Now()), entropy)
if err != nil {
return ulid.ULID{}, err
}

return id, nil
}
29 changes: 9 additions & 20 deletions cmd/opampsupervisor/supervisor/persistence_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"path/filepath"
"testing"

"github.com/oklog/ulid/v2"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
)

Expand All @@ -19,21 +19,21 @@ func TestCreateOrLoadPersistentState(t *testing.T) {
require.NoError(t, err)

// instance ID should be populated
require.NotEqual(t, ulid.ULID{}, state.InstanceID)
require.NotEqual(t, uuid.Nil, state.InstanceID)
require.FileExists(t, f)
})

t.Run("loads state from file if it exists", func(t *testing.T) {
f := filepath.Join(t.TempDir(), "state.yaml")

err := os.WriteFile(f, []byte(`instance_id: "01HW3GS9NWD840C5C2BZS3KYPW"`), 0600)
err := os.WriteFile(f, []byte(`instance_id: "018feed6-905b-7aa6-ba37-b0eec565de03"`), 0600)
require.NoError(t, err)

state, err := loadOrCreatePersistentState(f)
require.NoError(t, err)

// instance ID should be populated with value from file
require.Equal(t, ulid.MustParse("01HW3GS9NWD840C5C2BZS3KYPW"), state.InstanceID)
require.Equal(t, uuid.MustParse("018feed6-905b-7aa6-ba37-b0eec565de03"), state.InstanceID)
require.FileExists(t, f)
})

Expand All @@ -45,29 +45,18 @@ func TestPersistentState_SetInstanceID(t *testing.T) {
require.NoError(t, err)

// instance ID should be populated
require.NotEqual(t, ulid.ULID{}, state.InstanceID)
require.NotEqual(t, uuid.Nil, state.InstanceID)
require.FileExists(t, f)

newULID := ulid.MustParse("01HW3GS9NWD840C5C2BZS3KYPW")
err = state.SetInstanceID(newULID)
newUUID := uuid.MustParse("018fee1f-871a-7d82-b22f-478085b3a1d6")
err = state.SetInstanceID(newUUID)
require.NoError(t, err)

require.Equal(t, newULID, state.InstanceID)
require.Equal(t, newUUID, state.InstanceID)

// Test that loading the state after setting the instance ID has the new instance ID
loadedState, err := loadPersistentState(f)
require.NoError(t, err)

require.Equal(t, newULID, loadedState.InstanceID)
}

func TestGenerateNewULID(t *testing.T) {
// Test generating a new ULID twice returns 2 different results
id1, err := generateNewULID()
require.NoError(t, err)

id2, err := generateNewULID()
require.NoError(t, err)

require.NotEqual(t, id1, id2)
require.Equal(t, newUUID, loadedState.InstanceID)
}
34 changes: 17 additions & 17 deletions cmd/opampsupervisor/supervisor/supervisor.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ import (
"time"

"github.com/cenkalti/backoff/v4"
"github.com/google/uuid"
"github.com/knadh/koanf/parsers/yaml"
"github.com/knadh/koanf/providers/file"
"github.com/knadh/koanf/providers/rawbytes"
"github.com/knadh/koanf/v2"
"github.com/oklog/ulid/v2"
"github.com/open-telemetry/opamp-go/client"
"github.com/open-telemetry/opamp-go/client/types"
"github.com/open-telemetry/opamp-go/protobufs"
Expand Down Expand Up @@ -388,7 +388,7 @@ func (s *Supervisor) startOpAMP() error {
OpAMPServerURL: s.config.Server.Endpoint,
Header: s.config.Server.Headers,
TLSConfig: tlsConfig,
InstanceUid: s.persistentState.InstanceID.String(),
InstanceUid: types.InstanceUid(s.persistentState.InstanceID),
Callbacks: types.CallbacksStruct{
OnConnectFunc: func(_ context.Context) {
s.connectedToOpAMPServer <- struct{}{}
Expand Down Expand Up @@ -1065,26 +1065,26 @@ func (s *Supervisor) onMessage(ctx context.Context, msg *types.MessageData) {
}

if msg.AgentIdentification != nil {
newInstanceID, err := ulid.Parse(msg.AgentIdentification.NewInstanceUid)
newInstanceID, err := uuid.FromBytes(msg.AgentIdentification.NewInstanceUid)
if err != nil {
s.logger.Error("Failed to parse instance ULID", zap.Error(err))
}
s.logger.Error("Failed to parse instance UUID", zap.Error(err))
} else {
s.logger.Debug("Agent identity is changing",
zap.String("old_id", s.persistentState.InstanceID.String()),
zap.String("new_id", newInstanceID.String()))

s.logger.Debug("Agent identity is changing",
zap.String("old_id", s.persistentState.InstanceID.String()),
zap.String("new_id", newInstanceID.String()))
err = s.persistentState.SetInstanceID(newInstanceID)
if err != nil {
s.logger.Error("Failed to persist new instance ID, instance ID will revert on restart.", zap.String("new_id", newInstanceID.String()), zap.Error(err))
}

err = s.persistentState.SetInstanceID(newInstanceID)
if err != nil {
s.logger.Error("Failed to persist new instance ID, instance ID will revert on restart.", zap.String("new_id", newInstanceID.String()), zap.Error(err))
}
err = s.opampClient.SetAgentDescription(s.agentDescription)
if err != nil {
s.logger.Error("Failed to send agent description to OpAMP server")
}

err = s.opampClient.SetAgentDescription(s.agentDescription)
if err != nil {
s.logger.Error("Failed to send agent description to OpAMP server")
configChanged = true
}

configChanged = true
}

if configChanged {
Expand Down
Loading

0 comments on commit 595c44b

Please sign in to comment.