From 4a8661d003d422a34f87e65c2d29152050343cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paolo=20Chil=C3=A0?= Date: Thu, 30 Nov 2023 17:36:11 +0100 Subject: [PATCH 1/2] Add agent hash to check for detecting upgrade rollbacks (#3842) * Add agent hash to check for detecting upgrade rollbacks --- .../application/upgrade/marker_watcher.go | 8 +-- .../upgrade/marker_watcher_test.go | 62 ++++++++++++++++++- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/marker_watcher.go b/internal/pkg/agent/application/upgrade/marker_watcher.go index faefc3e64b2..8ce6e3af842 100644 --- a/internal/pkg/agent/application/upgrade/marker_watcher.go +++ b/internal/pkg/agent/application/upgrade/marker_watcher.go @@ -91,10 +91,10 @@ 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()) } } }() @@ -102,7 +102,7 @@ func (mfw *MarkerFileWatcher) Run(ctx context.Context) error { 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 { diff --git a/internal/pkg/agent/application/upgrade/marker_watcher_test.go b/internal/pkg/agent/application/upgrade/marker_watcher_test.go index 9d54a9bbcae..c2b42b7533f 100644 --- a/internal/pkg/agent/application/upgrade/marker_watcher_test.go +++ b/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 { From 93f159ac9c84c47a0517d59cd1d1e389f9dc85bd Mon Sep 17 00:00:00 2001 From: Alex K <8418476+fearful-symmetry@users.noreply.github.com> Date: Thu, 30 Nov 2023 08:48:33 -0800 Subject: [PATCH 2/2] Add custom MarshalYAML() method to component to fix YAML marshaling of error values (#3835) * add custom MarshalYAML to component * add changelog * add nil check * update tests --------- Co-authored-by: Pierre HILBERT --- ...315-custom-yaml-marshal-for-component.yaml | 32 +++++++++++++++++++ .../coordinator/diagnostics_test.go | 4 +-- pkg/component/component.go | 13 +++++++- pkg/component/component_test.go | 25 +++++++++++++++ 4 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml diff --git a/changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml b/changelog/fragments/1701208315-custom-yaml-marshal-for-component.yaml new file mode 100644 index 00000000000..d6c6834ced5 --- /dev/null +++ b/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 diff --git a/internal/pkg/agent/application/coordinator/diagnostics_test.go b/internal/pkg/agent/application/coordinator/diagnostics_test.go index 1e12dd38141..58cadb9c22a 100644 --- a/internal/pkg/agent/application/coordinator/diagnostics_test.go +++ b/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: diff --git a/pkg/component/component.go b/pkg/component/component.go index 18060b645fd..d9100a6dd98 100644 --- a/pkg/component/component.go +++ b/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 { diff --git a/pkg/component/component_test.go b/pkg/component/component_test.go index 00c4d1c63cb..8d42ab64427 100644 --- a/pkg/component/component_test.go +++ b/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{