diff --git a/changelog/fragments/1700678892-Fixes-the-Elastic-Agent-ignoring-agent.download.proxy_url-on-policy-update.yaml b/changelog/fragments/1700678892-Fixes-the-Elastic-Agent-ignoring-agent.download.proxy_url-on-policy-update.yaml new file mode 100644 index 00000000000..cc09836a0b5 --- /dev/null +++ b/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 diff --git a/internal/pkg/agent/application/upgrade/step_download.go b/internal/pkg/agent/application/upgrade/step_download.go index a273460f337..0a45f6bd785 100644 --- a/internal/pkg/agent/application/upgrade/step_download.go +++ b/internal/pkg/agent/application/upgrade/step_download.go @@ -204,7 +204,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 { diff --git a/internal/pkg/agent/application/upgrade/step_download_test.go b/internal/pkg/agent/application/upgrade/step_download_test.go index 330a60f5288..db0447380a3 100644 --- a/internal/pkg/agent/application/upgrade/step_download_test.go +++ b/internal/pkg/agent/application/upgrade/step_download_test.go @@ -132,7 +132,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) }) @@ -168,7 +168,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) }) @@ -194,7 +194,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") } }) } diff --git a/internal/pkg/agent/application/upgrade/upgrade.go b/internal/pkg/agent/application/upgrade/upgrade.go index d477537a6f1..0ed295ff317 100644 --- a/internal/pkg/agent/application/upgrade/upgrade.go +++ b/internal/pkg/agent/application/upgrade/upgrade.go @@ -23,6 +23,7 @@ import ( "github.com/elastic/elastic-agent/internal/pkg/agent/application/paths" "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/configuration" "github.com/elastic/elastic-agent/internal/pkg/agent/errors" "github.com/elastic/elastic-agent/internal/pkg/fleetapi" "github.com/elastic/elastic-agent/internal/pkg/fleetapi/acker" @@ -86,35 +87,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 } diff --git a/internal/pkg/agent/application/upgrade/upgrade_test.go b/internal/pkg/agent/application/upgrade/upgrade_test.go index d8de6923cfe..66967de4135 100644 --- a/internal/pkg/agent/application/upgrade/upgrade_test.go +++ b/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()) + } + }) + } +}