Pass the parsed version to all downloaders (elastic#3824)

* Pass the parsed version to all downloaders * add changelog * Add fs downloader unit tests * add unit tests for http downloader * add tests for snapshot downloader --------- Co-authored-by: Craig MacKenzie <[email protected]>
belimawr · Dec 5, 2023 · fdce58b · fdce58b
1 parent 098fd47
commit fdce58b
Show file tree

Hide file tree

Showing 22 changed files with 847 additions and 73 deletions.
diff --git a/changelog/fragments/1701445320-preserve-build-metadata-in-upgrade-version-strings.yaml b/changelog/fragments/1701445320-preserve-build-metadata-in-upgrade-version-strings.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: Preserve build metadata in upgrade version strings
+
+# 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:
+
+# Affected component; a word indicating the component this changeset affects.
+component: agent
+
+# 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/owner/repo/1234
+
+# 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/owner/repo/1234
diff --git a/internal/pkg/agent/application/upgrade/artifact/artifact.go b/internal/pkg/agent/application/upgrade/artifact/artifact.go
@@ -9,6 +9,7 @@ import (
 	"path/filepath"
 
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 )
 
 var packageArchMap = map[string]string{
@@ -31,18 +32,18 @@ type Artifact struct {
 }
 
 // GetArtifactName constructs a path to a downloaded artifact
-func GetArtifactName(a Artifact, version, operatingSystem, arch string) (string, error) {
+func GetArtifactName(a Artifact, version agtversion.ParsedSemVer, operatingSystem, arch string) (string, error) {
 	key := fmt.Sprintf("%s-binary-%s", operatingSystem, arch)
 	suffix, found := packageArchMap[key]
 	if !found {
 		return "", errors.New(fmt.Sprintf("'%s' is not a valid combination for a package", key), errors.TypeConfig)
 	}
 
-	return fmt.Sprintf("%s-%s-%s", a.Cmd, version, suffix), nil
+	return fmt.Sprintf("%s-%s-%s", a.Cmd, version.String(), suffix), nil
 }
 
 // GetArtifactPath returns a full path of artifact for a program in specific version
-func GetArtifactPath(a Artifact, version, operatingSystem, arch, targetDir string) (string, error) {
+func GetArtifactPath(a Artifact, version agtversion.ParsedSemVer, operatingSystem, arch, targetDir string) (string, error) {
 	artifactName, err := GetArtifactName(a, version, operatingSystem, arch)
 	if err != nil {
 		return "", err

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/downloader.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/downloader.go
@@ -13,6 +13,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
+	"github.com/elastic/elastic-agent/pkg/version"
 )
 
 // Downloader is a downloader with a predefined set of downloaders.
@@ -35,7 +36,7 @@ func NewDownloader(downloaders ...download.Downloader) *Downloader {
 
 // Download fetches the package from configured source.
 // Returns absolute path to downloaded package and an error.
-func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version string) (string, error) {
+func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version *version.ParsedSemVer) (string, error) {
 	var err error
 	span, ctx := apm.StartSpan(ctx, "download", "app.internal")
 	defer span.End()

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/downloader_test.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/downloader_test.go
@@ -9,8 +9,11 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -23,7 +26,7 @@ type FailingDownloader struct {
 	called bool
 }
 
-func (d *FailingDownloader) Download(ctx context.Context, _ artifact.Artifact, _ string) (string, error) {
+func (d *FailingDownloader) Download(context.Context, artifact.Artifact, *agtversion.ParsedSemVer) (string, error) {
 	d.called = true
 	return "", errors.New("failing")
 }
@@ -34,7 +37,7 @@ type SuccDownloader struct {
 	called bool
 }
 
-func (d *SuccDownloader) Download(ctx context.Context, _ artifact.Artifact, _ string) (string, error) {
+func (d *SuccDownloader) Download(context.Context, artifact.Artifact, *agtversion.ParsedSemVer) (string, error) {
 	d.called = true
 	return succ, nil
 }
@@ -61,9 +64,11 @@ func TestComposed(t *testing.T) {
 		},
 	}
 
+	parseVersion, err := agtversion.ParseVersion("1.2.3")
+	require.NoError(t, err)
 	for _, tc := range testCases {
 		d := NewDownloader(tc.downloaders[0], tc.downloaders[1])
-		r, _ := d.Download(context.TODO(), artifact.Artifact{Name: "a"}, "b")
+		r, _ := d.Download(context.TODO(), artifact.Artifact{Name: "a"}, parseVersion)
 
 		assert.Equal(t, tc.expectedResult, r == succ)
 

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier.go
@@ -11,6 +11,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 )
 
 // Verifier is a verifier with a predefined set of verifiers.
@@ -38,7 +39,7 @@ func NewVerifier(log *logger.Logger, verifiers ...download.Verifier) *Verifier {
 }
 
 // Verify checks the package from configured source.
-func (v *Verifier) Verify(a artifact.Artifact, version string, skipDefaultPgp bool, pgpBytes ...string) error {
+func (v *Verifier) Verify(a artifact.Artifact, version agtversion.ParsedSemVer, skipDefaultPgp bool, pgpBytes ...string) error {
 	var err error
 
 	for _, verifier := range v.vv {

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go b/internal/pkg/agent/application/upgrade/artifact/download/composed/verifier_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -23,7 +24,7 @@ func (d *ErrorVerifier) Name() string {
 	return "error"
 }
 
-func (d *ErrorVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
+func (d *ErrorVerifier) Verify(artifact.Artifact, agtversion.ParsedSemVer, bool, ...string) error {
 	d.called = true
 	return errors.New("failing")
 }
@@ -38,7 +39,7 @@ func (d *FailVerifier) Name() string {
 	return "fail"
 }
 
-func (d *FailVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
+func (d *FailVerifier) Verify(artifact.Artifact, agtversion.ParsedSemVer, bool, ...string) error {
 	d.called = true
 	return &download.InvalidSignatureError{File: "", Err: errors.New("invalid signature")}
 }
@@ -53,7 +54,7 @@ func (d *SuccVerifier) Name() string {
 	return "succ"
 }
 
-func (d *SuccVerifier) Verify(a artifact.Artifact, version string, _ bool, _ ...string) error {
+func (d *SuccVerifier) Verify(artifact.Artifact, agtversion.ParsedSemVer, bool, ...string) error {
 	d.called = true
 	return nil
 }
@@ -86,9 +87,10 @@ func TestVerifier(t *testing.T) {
 		},
 	}
 
+	testVersion := agtversion.NewParsedSemVer(1, 2, 3, "", "")
 	for _, tc := range testCases {
 		d := NewVerifier(log, tc.verifiers[0], tc.verifiers[1], tc.verifiers[2])
-		err := d.Verify(artifact.Artifact{Name: "a", Cmd: "a", Artifact: "a/a"}, "b", false)
+		err := d.Verify(artifact.Artifact{Name: "a", Cmd: "a", Artifact: "a/a"}, *testVersion, false)
 
 		assert.Equal(t, tc.expectedResult, err == nil)
 

diff --git a/internal/pkg/agent/application/upgrade/artifact/download/downloader.go b/internal/pkg/agent/application/upgrade/artifact/download/downloader.go
@@ -8,9 +8,10 @@ import (
 	"context"
 
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
+	"github.com/elastic/elastic-agent/pkg/version"
 )
 
 // Downloader is an interface allowing download of an artifact
 type Downloader interface {
-	Download(ctx context.Context, a artifact.Artifact, version string) (string, error)
+	Download(ctx context.Context, a artifact.Artifact, version *version.ParsedSemVer) (string, error)
 }
diff --git a/internal/pkg/agent/application/upgrade/artifact/download/fs/downloader.go b/internal/pkg/agent/application/upgrade/artifact/download/fs/downloader.go
@@ -16,6 +16,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
+	agtversion "github.com/elastic/elastic-agent/pkg/version"
 )
 
 const (
@@ -38,7 +39,7 @@ func NewDownloader(config *artifact.Config) *Downloader {
 
 // Download fetches the package from configured source.
 // Returns absolute path to downloaded package and an error.
-func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version string) (_ string, err error) {
+func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version *agtversion.ParsedSemVer) (_ string, err error) {
 	span, ctx := apm.StartSpan(ctx, "download", "app.internal")
 	defer span.End()
 	downloadedFiles := make([]string, 0, 2)
@@ -52,20 +53,20 @@ func (e *Downloader) Download(ctx context.Context, a artifact.Artifact, version
 	}()
 
 	// download from source to dest
-	path, err := e.download(e.config.OS(), a, version, "")
+	path, err := e.download(e.config.OS(), a, *version, "")
 	downloadedFiles = append(downloadedFiles, path)
 	if err != nil {
 		return "", err
 	}
 
-	hashPath, err := e.download(e.config.OS(), a, version, ".sha512")
+	hashPath, err := e.download(e.config.OS(), a, *version, ".sha512")
 	downloadedFiles = append(downloadedFiles, hashPath)
 	return path, err
 }
 
 // DownloadAsc downloads the package .asc file from configured source.
 // It returns absolute path to the downloaded file and a no-nil error if any occurs.
-func (e *Downloader) DownloadAsc(_ context.Context, a artifact.Artifact, version string) (string, error) {
+func (e *Downloader) DownloadAsc(_ context.Context, a artifact.Artifact, version agtversion.ParsedSemVer) (string, error) {
 	path, err := e.download(e.config.OS(), a, version, ".asc")
 	if err != nil {
 		os.Remove(path)
@@ -78,7 +79,7 @@ func (e *Downloader) DownloadAsc(_ context.Context, a artifact.Artifact, version
 func (e *Downloader) download(
 	operatingSystem string,
 	a artifact.Artifact,
-	version,
+	version agtversion.ParsedSemVer,
 	extension string) (string, error) {
 	filename, err := artifact.GetArtifactName(a, version, operatingSystem, e.config.Arch())
 	if err != nil {