Upgrader applies agent.download.proxy_url on policy update (elastic#3803)

The Upgrader now applies the whole artifact.Config on Upgrader.Reload

Author: AndersonQ

changelog/fragments/1700678892-Fixes-the-Elastic-Agent-ignoring-agent.download.proxy_url-on-policy-update.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: Fixes the Elastic Agent ignoring agent.download.proxy_url on policy update
+
+# 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: Upgrader
+
+# 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/elastic/elastic-agent/issues/3560

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

@@ -257,7 +257,8 @@ func (u *Upgrader) downloadWithRetries(
 	}
 
 	opFailureNotificationFn := func(err error, retryAfter time.Duration) {
-		u.log.Warnf("%s; retrying (will be retry %d) in %s.", err.Error(), attempt, retryAfter)
+		u.log.Warnf("download attempt %d failed: %s; retrying in %s.",
+			attempt, err.Error(), retryAfter)
 	}
 
 	if err := backoff.RetryNotify(opFn, boCtx, opFailureNotificationFn); err != nil {

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

@@ -139,7 +139,7 @@ func TestDownloadWithRetries(t *testing.T) {
 		logs := obs.TakeAll()
 		require.Len(t, logs, 3)
 		require.Equal(t, "download attempt 1", logs[0].Message)
-		require.Contains(t, logs[1].Message, "unable to create fetcher: failed to construct downloader; retrying (will be retry 1)")
+		require.Contains(t, logs[1].Message, "unable to create fetcher: failed to construct downloader")
 		require.Equal(t, "download attempt 2", logs[2].Message)
 	})
 
@@ -178,7 +178,7 @@ func TestDownloadWithRetries(t *testing.T) {
 		logs := obs.TakeAll()
 		require.Len(t, logs, 3)
 		require.Equal(t, "download attempt 1", logs[0].Message)
-		require.Contains(t, logs[1].Message, "unable to download package: download failed; retrying (will be retry 1)")
+		require.Contains(t, logs[1].Message, "unable to download package: download failed; retrying")
 		require.Equal(t, "download attempt 2", logs[2].Message)
 	})
 
@@ -207,7 +207,7 @@ func TestDownloadWithRetries(t *testing.T) {
 		require.GreaterOrEqual(t, len(logs), minNmExpectedAttempts*2)
 		for i := 0; i < minNmExpectedAttempts; i++ {
 			require.Equal(t, fmt.Sprintf("download attempt %d", i+1), logs[(2*i)].Message)
-			require.Contains(t, logs[(2*i+1)].Message, fmt.Sprintf("unable to download package: download failed; retrying (will be retry %d)", i+1))
+			require.Contains(t, logs[(2*i+1)].Message, "unable to download package: download failed; retrying")
 		}
 	})
 }

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

@@ -20,6 +20,7 @@ import (
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/reexec"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details"
+	"github.com/elastic/elastic-agent/internal/pkg/agent/configuration"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/install"
 	"github.com/elastic/elastic-agent/internal/pkg/config"
@@ -89,35 +90,39 @@ func (u *Upgrader) SetClient(c fleetclient.Sender) {
 
 // Reload reloads the artifact configuration for the upgrader.
 func (u *Upgrader) Reload(rawConfig *config.Config) error {
-	type reloadConfig struct {
-		// SourceURI: source of the artifacts, e.g https://artifacts.elastic.co/downloads/
-		SourceURI string `json:"agent.download.sourceURI" config:"agent.download.sourceURI"`
+	cfg, err := configuration.NewFromConfig(rawConfig)
+	if err != nil {
+		return fmt.Errorf("invalid config: %w", err)
+	}
 
-		// FleetSourceURI: source of the artifacts, e.g https://artifacts.elastic.co/downloads/ coming from fleet which uses
-		// different naming.
+	// the source URI coming from fleet which uses a different naming.
+	type fleetCfg struct {
+		// FleetSourceURI: source of the artifacts, e.g https://artifacts.elastic.co/downloads/
 		FleetSourceURI string `json:"agent.download.source_uri" config:"agent.download.source_uri"`
 	}
-	cfg := &reloadConfig{}
-	if err := rawConfig.Unpack(&cfg); err != nil {
+	fleetSourceURI := &fleetCfg{}
+	if err := rawConfig.Unpack(&fleetSourceURI); err != nil {
 		return errors.New(err, "failed to unpack config during reload")
 	}
 
-	var newSourceURI string
-	if cfg.FleetSourceURI != "" {
-		// fleet configuration takes precedence
-		newSourceURI = cfg.FleetSourceURI
-	} else if cfg.SourceURI != "" {
-		newSourceURI = cfg.SourceURI
+	// fleet configuration takes precedence
+	if fleetSourceURI.FleetSourceURI != "" {
+		cfg.Settings.DownloadConfig.SourceURI = fleetSourceURI.FleetSourceURI
 	}
 
-	if newSourceURI != "" {
-		u.log.Infof("Source URI changed from %q to %q", u.settings.SourceURI, newSourceURI)
-		u.settings.SourceURI = newSourceURI
+	if cfg.Settings.DownloadConfig.SourceURI != "" {
+		u.log.Infof("Source URI changed from %q to %q",
+			u.settings.SourceURI,
+			cfg.Settings.DownloadConfig.SourceURI)
 	} else {
 		// source uri unset, reset to default
-		u.log.Infof("Source URI reset from %q to %q", u.settings.SourceURI, artifact.DefaultSourceURI)
-		u.settings.SourceURI = artifact.DefaultSourceURI
+		u.log.Infof("Source URI reset from %q to %q",
+			u.settings.SourceURI,
+			artifact.DefaultSourceURI)
+		cfg.Settings.DownloadConfig.SourceURI = artifact.DefaultSourceURI
 	}
+
+	u.settings = cfg.Settings.DownloadConfig
 	return nil
 }
 

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

@@ -14,15 +14,17 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/elastic/elastic-agent/pkg/control/v2/client"
-	"github.com/elastic/elastic-agent/pkg/control/v2/client/mocks"
-	"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
-
 	"github.com/gofrs/flock"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
 	"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
+	"github.com/elastic/elastic-agent/internal/pkg/config"
 	"github.com/elastic/elastic-agent/internal/pkg/release"
+	"github.com/elastic/elastic-agent/pkg/control/v2/client"
+	"github.com/elastic/elastic-agent/pkg/control/v2/client/mocks"
+	"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
 	"github.com/elastic/elastic-agent/pkg/core/logger"
 )
 
@@ -209,3 +211,67 @@ func TestIsInProgress(t *testing.T) {
 		})
 	}
 }
+
+func TestUpgraderReload(t *testing.T) {
+	defaultCfg := artifact.DefaultConfig()
+	tcs := []struct {
+		name      string
+		sourceURL string
+		proxyURL  string
+		cfg       string
+	}{
+		{
+			name:      "proxy_url is applied",
+			sourceURL: defaultCfg.SourceURI,
+			proxyURL:  "http://someBrokenURL/",
+			cfg: `
+agent.download:
+  proxy_url: http://someBrokenURL/
+`,
+		},
+		{
+			name:      "source_uri has precedence over sourceURI",
+			sourceURL: "https://this.sourceURI.co/downloads/beats/",
+			cfg: `
+agent.download:
+  source_uri: "https://this.sourceURI.co/downloads/beats/"
+  sourceURI: "https://NOT.sourceURI.co/downloads/beats/"
+`}, {
+			name:      "only sourceURI",
+			sourceURL: "https://this.sourceURI.co/downloads/beats/",
+			cfg: `
+agent.download:
+  sourceURI: "https://this.sourceURI.co/downloads/beats/"
+`}, {
+			name:      "only source_uri",
+			sourceURL: "https://this.sourceURI.co/downloads/beats/",
+			cfg: `
+agent.download:
+  source_uri: "https://this.sourceURI.co/downloads/beats/"
+`},
+	}
+
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			log, _ := logger.NewTesting("")
+
+			u := Upgrader{
+				log:      log,
+				settings: artifact.DefaultConfig(),
+			}
+
+			cfg, err := config.NewConfigFrom(tc.cfg)
+			require.NoError(t, err, "failed to create new config")
+
+			err = u.Reload(cfg)
+			require.NoError(t, err, "error reloading config")
+
+			assert.Equal(t, tc.sourceURL, u.settings.SourceURI)
+			if tc.proxyURL != "" {
+				require.NotNilf(t, u.settings.Proxy.URL,
+					"ProxyURI should not be nil, want %s", tc.proxyURL)
+				assert.Equal(t, tc.proxyURL, u.settings.Proxy.URL.String())
+			}
+		})
+	}
+}