From a19c4221041c89cf84cb10ca418b384606333485 Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Tue, 19 Dec 2023 19:58:27 +0100 Subject: [PATCH 1/5] (*artifact.Config).Unpack preserve not overridden velues (*artifact.Config).Unpack was creating a new instance of Config and manually coping all values from the original to the new one. New values added to config, RetrySleepInitDuration in this case, were neither maintaining its default value nor being updated with the new value. Now it uses reflection to ensure all values are correctly copied. --- .../application/upgrade/artifact/config.go | 94 ++++++++++++++----- .../upgrade/artifact/config_test.go | 64 +++++++++++++ 2 files changed, 136 insertions(+), 22 deletions(-) diff --git a/internal/pkg/agent/application/upgrade/artifact/config.go b/internal/pkg/agent/application/upgrade/artifact/config.go index 79c6f73b804..475dc0eb76e 100644 --- a/internal/pkg/agent/application/upgrade/artifact/config.go +++ b/internal/pkg/agent/application/upgrade/artifact/config.go @@ -5,6 +5,7 @@ package artifact import ( + "reflect" "runtime" "strings" "time" @@ -30,6 +31,38 @@ type ConfigReloader interface { Reload(*Config) error } +// configWithoutHTTPTransportSettings is a copy of Config without +// httpcommon.HTTPTransportSettings so we can handle the HTTPTransportSettings +// config separately during *Config.Unpack +type configWithoutHTTPTransportSettings struct { + // OperatingSystem: operating system [linux, windows, darwin] + OperatingSystem string `json:"-" config:",ignore"` + + // Architecture: target architecture [32, 64] + Architecture string `json:"-" config:",ignore"` + + // SourceURI: source of the artifacts, e.g https://artifacts.elastic.co/downloads/ + SourceURI string `json:"sourceURI" config:"sourceURI"` + + // TargetDirectory: path to the directory containing downloaded packages + TargetDirectory string `json:"targetDirectory" config:"target_directory"` + + // InstallPath: path to the directory containing installed packages + InstallPath string `yaml:"installPath" config:"install_path"` + + // DropPath: path where elastic-agent can find installation files for download. + // Difference between this and TargetDirectory is that when fetching packages (from web or fs) they are stored in TargetDirectory + // DropPath specifies where Filesystem downloader can find packages which will then be placed in TargetDirectory. This can be + // local or network disk. + // If not provided FileSystem Downloader will fallback to /beats subfolder of elastic-agent directory. + DropPath string `yaml:"dropPath" config:"drop_path"` + + // RetrySleepInitDuration: the duration to sleep for before the first retry attempt. This duration + // will increase for subsequent retry attempts in a randomized exponential backoff manner. + // This key is, for some reason, problematic + RetrySleepInitDuration time.Duration `yaml:"retry_sleep_init_duration" config:"retry_sleep_init_duration"` +} + // Config is a configuration used for verifier and downloader type Config struct { // OperatingSystem: operating system [linux, windows, darwin] @@ -206,20 +239,30 @@ func (c *Config) Arch() string { // Unpack reads a config object into the settings. func (c *Config) Unpack(cfg *c.C) error { - tmp := struct { - OperatingSystem string `json:"-" config:",ignore"` - Architecture string `json:"-" config:",ignore"` - SourceURI string `json:"sourceURI" config:"sourceURI"` - TargetDirectory string `json:"targetDirectory" config:"target_directory"` - InstallPath string `yaml:"installPath" config:"install_path"` - DropPath string `yaml:"dropPath" config:"drop_path"` - }{ - OperatingSystem: c.OperatingSystem, - Architecture: c.Architecture, - SourceURI: c.SourceURI, - TargetDirectory: c.TargetDirectory, - InstallPath: c.InstallPath, - DropPath: c.DropPath, + // c.HTTPTransportSettings need to be unpacked separately (see + // https://github.com/elastic/elastic-agent/pull/776). Therefore, to ensure + // we don't miss out any value, we'll use reflection to copy them all. + // see https://go.dev/blog/laws-of-reflection to understand the reflection part. + + tmp := configWithoutHTTPTransportSettings{} + + cValue := reflect.ValueOf(c).Elem() + tmpType := reflect.TypeOf(tmp) + tmpValue := reflect.ValueOf(&tmp).Elem() + + // Copy all values of c to tmp. As Config has more fields than + // configWithoutHTTPTransportSettings, we iterate over the fields of + // configWithoutHTTPTransportSettings and for each field we get the value + // of the field in c and set in tmp. + // There is a test to ensure Config will always be a superset of + // configWithoutHTTPTransportSettings. + for i := 0; i < tmpType.NumField(); i++ { + name := tmpType.Field(i).Name + tmpField := tmpValue.FieldByName(name) + + if tmpField.IsValid() && tmpField.CanSet() { + tmpField.Set(cValue.Field(i)) + } } if err := cfg.Unpack(&tmp); err != nil { @@ -230,15 +273,22 @@ func (c *Config) Unpack(cfg *c.C) error { if err := cfg.Unpack(&transport); err != nil { return err } + // HTTPTransportSettings.Proxy.Headers defaults to empty. If absent in cfg, + // Unpack will set it to nil. To ensure consistency, we reset it to empty. + if transport.Proxy.Headers == nil { + transport.Proxy.Headers = map[string]string{} + } + + // Now copy back all updated values of tmp to c. + for i := 0; i < tmpType.NumField(); i++ { + name := tmpType.Field(i).Name + cField := cValue.FieldByName(name) - *c = Config{ - OperatingSystem: tmp.OperatingSystem, - Architecture: tmp.Architecture, - SourceURI: tmp.SourceURI, - TargetDirectory: tmp.TargetDirectory, - InstallPath: tmp.InstallPath, - DropPath: tmp.DropPath, - HTTPTransportSettings: transport, + if cField.IsValid() && cField.CanSet() { + cField.Set(tmpValue.Field(i)) + } } + c.HTTPTransportSettings = transport + return nil } diff --git a/internal/pkg/agent/application/upgrade/artifact/config_test.go b/internal/pkg/agent/application/upgrade/artifact/config_test.go index 803154e465f..736c9a02d55 100644 --- a/internal/pkg/agent/application/upgrade/artifact/config_test.go +++ b/internal/pkg/agent/application/upgrade/artifact/config_test.go @@ -5,11 +5,16 @@ package artifact import ( + "reflect" + "slices" "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + agentlibsconfig "github.com/elastic/elastic-agent-libs/config" + "github.com/elastic/elastic-agent-libs/transport/httpcommon" "github.com/elastic/elastic-agent/internal/pkg/config" "github.com/elastic/elastic-agent/pkg/core/logger" ) @@ -246,3 +251,62 @@ func TestReload(t *testing.T) { } } } + +// TestConfigWithoutHTTPTransportSettings ensures configWithoutHTTPTransportSettings +// and Config stay aligned. +func TestConfigWithoutHTTPTransportSettings(t *testing.T) { + cfg := reflect.TypeOf(Config{}) + cfgWithout := reflect.TypeOf(configWithoutHTTPTransportSettings{}) + transportSettings := reflect.TypeOf(httpcommon.HTTPTransportSettings{}) + + var missing []string + + // get all the fields of httpcommon.HTTPTransportSettings + // check configWithoutHTTPTransportSettings has all the fields of Config, but + // the ones from httpcommon.HTTPTransportSettings + + // get all the fields of httpcommon.HTTPTransportSettings + transportSettingsFields := []string{transportSettings.Name()} + for i := 0; i < transportSettings.NumField(); i++ { + transportSettingsFields = append( + transportSettingsFields, + transportSettings.Field(i).Name) + } + + // check configWithoutHTTPTransportSettings has got all the fields Config + // has, except the fields of httpcommon.HTTPTransportSettings. + for i := 0; i < cfg.NumField(); i++ { + field := cfg.Field(i).Name + if slices.Contains(transportSettingsFields, field) { + // configWithoutHTTPTransportSettings should not have this field + continue + } + + _, has := cfgWithout.FieldByName(field) + if !has { + missing = append(missing, field) + } + } + + if len(missing) != 0 { + t.Errorf("type %s should have the same fields as Config, "+ + "except the httpcommon.HTTPTransportSettings fields. However it's "+ + "missing the fields %v", + cfgWithout.Name(), missing) + } +} + +// TestConfig_Unpack takes a shortcut as testing every possible config would be +// hard to maintain, a new config would be added to Config and the test would not +// be updated. Instead, this test ensures the default config is preserved if an +// empty config is unpacked into it. +func TestConfig_Unpack(t *testing.T) { + defaultcfg := DefaultConfig() + + emptycgf, err := agentlibsconfig.NewConfigFrom("") + require.NoError(t, err, "could not create config from empty string") + + err = defaultcfg.Unpack(emptycgf) + require.NoError(t, err, "Unpack failed") + assert.Equal(t, DefaultConfig(), defaultcfg) +} From d71dd61fefc386e48055c4332698e3d59b00615d Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Wed, 20 Dec 2023 06:37:35 +0100 Subject: [PATCH 2/5] use golang.org/x/exp/slices instead of std lib --- internal/pkg/agent/application/upgrade/artifact/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/artifact/config_test.go b/internal/pkg/agent/application/upgrade/artifact/config_test.go index 736c9a02d55..e2cb20dd67d 100644 --- a/internal/pkg/agent/application/upgrade/artifact/config_test.go +++ b/internal/pkg/agent/application/upgrade/artifact/config_test.go @@ -6,12 +6,12 @@ package artifact import ( "reflect" - "slices" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" agentlibsconfig "github.com/elastic/elastic-agent-libs/config" "github.com/elastic/elastic-agent-libs/transport/httpcommon" From f4cb56629368c8b26a899fe5f5e83437d88470e2 Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Wed, 20 Dec 2023 06:41:53 +0100 Subject: [PATCH 3/5] go mod tidy && mage notice --- NOTICE.txt | 74 +++++++++++++++++++++++++++--------------------------- go.mod | 2 +- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/NOTICE.txt b/NOTICE.txt index c6764491202..d9edae9d582 100644 --- a/NOTICE.txt +++ b/NOTICE.txt @@ -6277,6 +6277,43 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +-------------------------------------------------------------------------------- +Dependency : golang.org/x/exp +Version: v0.0.0-20220722155223-a9213eeb770e +Licence type (autodetected): BSD-3-Clause +-------------------------------------------------------------------------------- + +Contents of probable licence file $GOMODCACHE/golang.org/x/exp@v0.0.0-20220722155223-a9213eeb770e/LICENSE: + +Copyright (c) 2009 The Go Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + + -------------------------------------------------------------------------------- Dependency : golang.org/x/lint Version: v0.0.0-20210508222113-6edffad5e616 @@ -17517,43 +17554,6 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. --------------------------------------------------------------------------------- -Dependency : golang.org/x/exp -Version: v0.0.0-20220722155223-a9213eeb770e -Licence type (autodetected): BSD-3-Clause --------------------------------------------------------------------------------- - -Contents of probable licence file $GOMODCACHE/golang.org/x/exp@v0.0.0-20220722155223-a9213eeb770e/LICENSE: - -Copyright (c) 2009 The Go Authors. All rights reserved. - -Redistribution and use in source and binary forms, with or without -modification, are permitted provided that the following conditions are -met: - - * Redistributions of source code must retain the above copyright -notice, this list of conditions and the following disclaimer. - * Redistributions in binary form must reproduce the above -copyright notice, this list of conditions and the following disclaimer -in the documentation and/or other materials provided with the -distribution. - * Neither the name of Google Inc. nor the names of its -contributors may be used to endorse or promote products derived from -this software without specific prior written permission. - -THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - - -------------------------------------------------------------------------------- Dependency : golang.org/x/mod Version: v0.9.0 diff --git a/go.mod b/go.mod index 43853026954..b4d6ed9bbad 100644 --- a/go.mod +++ b/go.mod @@ -56,6 +56,7 @@ require ( go.elastic.co/go-licence-detector v0.5.0 go.uber.org/zap v1.25.0 golang.org/x/crypto v0.14.0 + golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 golang.org/x/sync v0.3.0 golang.org/x/sys v0.13.0 @@ -147,7 +148,6 @@ require ( go.elastic.co/apm/v2 v2.0.0 // indirect go.elastic.co/fastjson v1.1.0 // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect golang.org/x/mod v0.9.0 // indirect golang.org/x/net v0.17.0 // indirect golang.org/x/oauth2 v0.10.0 // indirect From 1afaa1029232eb415200bdb8911aeab60901d154 Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Wed, 20 Dec 2023 11:10:24 +0100 Subject: [PATCH 4/5] PR change --- internal/pkg/agent/application/upgrade/artifact/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/artifact/config.go b/internal/pkg/agent/application/upgrade/artifact/config.go index 475dc0eb76e..88bfc931b4b 100644 --- a/internal/pkg/agent/application/upgrade/artifact/config.go +++ b/internal/pkg/agent/application/upgrade/artifact/config.go @@ -261,7 +261,7 @@ func (c *Config) Unpack(cfg *c.C) error { tmpField := tmpValue.FieldByName(name) if tmpField.IsValid() && tmpField.CanSet() { - tmpField.Set(cValue.Field(i)) + tmpField.Set(cValue.FieldByName(name)) } } From 7d9610ec8030a8198b59022e30d715208faafd62 Mon Sep 17 00:00:00 2001 From: Anderson Queiroz Date: Wed, 20 Dec 2023 11:11:56 +0100 Subject: [PATCH 5/5] pr change --- internal/pkg/agent/application/upgrade/artifact/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/agent/application/upgrade/artifact/config.go b/internal/pkg/agent/application/upgrade/artifact/config.go index 88bfc931b4b..148e4610d91 100644 --- a/internal/pkg/agent/application/upgrade/artifact/config.go +++ b/internal/pkg/agent/application/upgrade/artifact/config.go @@ -285,7 +285,7 @@ func (c *Config) Unpack(cfg *c.C) error { cField := cValue.FieldByName(name) if cField.IsValid() && cField.CanSet() { - cField.Set(tmpValue.Field(i)) + cField.Set(tmpValue.FieldByName(name)) } } c.HTTPTransportSettings = transport