Merge branch 'main' into dependabot/go_modules/github.com/elastic/go-…

…sysinfo-1.11.2

changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# Change summary; a 80ish characters long description of the change.
+summary: custom-yaml-marshal-for-component
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+description: Create a custom MarshalYAML() method to properly handle error fields
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: component
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/3835
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/2940

internal/pkg/agent/application/coordinator/diagnostics_test.go

@@ -383,12 +383,10 @@ func TestDiagnosticComponentsActual(t *testing.T) {
 		},
 	}
 
-	// The error values here shouldn't really be empty, this is a known bug, see
-	// https://github.com/elastic/elastic-agent/issues/2940
 	expected := `
 components:
   - id: component-1
-    error: {}
+    error: "component error"
     input_type: "test-input"
     output_type: "test-output"
     units:

internal/pkg/agent/application/upgrade/marker_watcher.go

@@ -91,18 +91,18 @@ func (mfw *MarkerFileWatcher) Run(ctx context.Context) error {
 				case e.Op&(fsnotify.Create|fsnotify.Write) != 0:
 					// Upgrade marker file was created or updated; read its contents
 					// and send them over the update channel.
-					mfw.processMarker(version.GetAgentPackageVersion())
+					mfw.processMarker(version.GetAgentPackageVersion(), version.Commit())
 				}
 			case <-doInitialRead:
-				mfw.processMarker(version.GetAgentPackageVersion())
+				mfw.processMarker(version.GetAgentPackageVersion(), version.Commit())
 			}
 		}
 	}()
 
 	return nil
 }
 
-func (mfw *MarkerFileWatcher) processMarker(currentVersion string) {
+func (mfw *MarkerFileWatcher) processMarker(currentVersion string, commit string) {
 	marker, err := loadMarker(mfw.markerFilePath)
 	if err != nil {
 		mfw.logger.Error(err)
@@ -120,7 +120,7 @@ func (mfw *MarkerFileWatcher) processMarker(currentVersion string) {
 	// been recorded in the marker's upgrade details field but, in case it
 	// isn't for some reason, we fallback to explicitly setting that state as
 	// part of the upgrade details in the marker.
-	if marker.PrevVersion == currentVersion {
+	if marker.PrevVersion == currentVersion && marker.PrevHash == commit {
 		if marker.Details == nil {
 			marker.Details = details.NewDetails("unknown", details.StateRollback, marker.GetActionID())
 		} else {

internal/pkg/agent/application/upgrade/marker_watcher_test.go

@@ -84,6 +84,9 @@ func TestProcessMarker(t *testing.T) {
 	cases := map[string]struct {
 		markerFileContents string
 
+		currentAgentVersion string
+		currentAgentHash    string
+
 		expectedErrLogMsg bool
 		expectedDetails   *details.Details
 	}{
@@ -146,6 +149,51 @@ details:
 				State:         details.StateWatching,
 			},
 		},
+		"same_version_different_hash": {
+			markerFileContents: `
+prev_version: 8.9.2
+prev_hash: aaaaaa
+details:
+  target_version: 8.9.2
+  state: UPG_WATCHING
+`,
+			currentAgentVersion: "8.9.2",
+			currentAgentHash:    "bbbbbb",
+			expectedErrLogMsg:   false,
+			expectedDetails: &details.Details{
+				TargetVersion: "8.9.2",
+				State:         details.StateWatching,
+			},
+		},
+		"same_version_same_hash": {
+			markerFileContents: `
+prev_version: 8.9.2
+prev_hash: aaaaaa
+details:
+  target_version: 8.9.2
+  state: UPG_WATCHING
+`,
+			currentAgentVersion: "8.9.2",
+			currentAgentHash:    "aaaaaa",
+			expectedErrLogMsg:   false,
+			expectedDetails: &details.Details{
+				TargetVersion: "8.9.2",
+				State:         details.StateRollback,
+			},
+		},
+		"same_version_same_hash_no_details": {
+			markerFileContents: `
+prev_version: 8.9.2
+prev_hash: aaaaaa
+`,
+			currentAgentVersion: "8.9.2",
+			currentAgentHash:    "aaaaaa",
+			expectedErrLogMsg:   false,
+			expectedDetails: &details.Details{
+				TargetVersion: "unknown",
+				State:         details.StateRollback,
+			},
+		},
 	}
 
 	for name, test := range cases {
@@ -182,7 +230,19 @@ details:
 				}
 			}()
 
-			mfw.processMarker("8.9.2")
+			// default values for version and hash
+			currentVersion := "8.9.2"
+			currentCommit := ""
+
+			// apply overrides from testcase
+			if test.currentAgentVersion != "" {
+				currentVersion = test.currentAgentVersion
+			}
+			if test.currentAgentHash != "" {
+				currentCommit = test.currentAgentHash
+			}
+
+			mfw.processMarker(currentVersion, currentCommit)
 
 			// error loading marker
 			if test.expectedErrLogMsg {

pkg/component/component.go

@@ -156,7 +156,11 @@ type Component struct {
 
 	// Err used when there is an error with running this input. Used by the runtime to alert
 	// the reason that all of these units are failed.
-	Err error `yaml:"error,omitempty"`
+	Err error `yaml:"-"`
+	// the YAML marshaller won't handle `error` values, since they don't implement MarshalYAML()
+	// the Component's own MarshalYAML method needs to handle this, and place any error values here instead of `Err`,
+	// so they can properly be rendered as a string
+	ErrMsg string `yaml:"error,omitempty"`
 
 	// InputSpec on how the input should run. (not set when ShipperSpec set)
 	InputSpec *InputRuntimeSpec `yaml:"input_spec,omitempty"`
@@ -187,6 +191,13 @@ type Component struct {
 	ShipperRef *ShipperReference `yaml:"shipper,omitempty"`
 }
 
+func (c Component) MarshalYAML() (interface{}, error) {
+	if c.Err != nil {
+		c.ErrMsg = c.Err.Error()
+	}
+	return c, nil
+}
+
 // Type returns the type of the component.
 func (c *Component) Type() string {
 	if c.InputSpec != nil {

pkg/component/component_test.go

@@ -36,6 +36,31 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// fake error type used for the test below
+type testErr struct {
+	data string
+}
+
+func (t testErr) Error() string {
+	return t.data
+}
+
+func TestComponentMarshalError(t *testing.T) {
+	testComponent := Component{
+		ID:  "test-device",
+		Err: testErr{data: "test error value"},
+	}
+	componentConfigs := []Component{testComponent}
+
+	outData, err := yaml.Marshal(struct {
+		Components []Component `yaml:"components"`
+	}{
+		Components: componentConfigs,
+	})
+	require.NoError(t, err)
+	require.Contains(t, string(outData), "test error value")
+}
+
 func TestToComponents(t *testing.T) {
 	linuxAMD64Platform := PlatformDetail{
 		Platform: Platform{