Fix (*artifact.Config).Unpack not preserving not overridden values

NOTICE.txt

@@ -9661,6 +9661,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-20230711023510-fffb14384f22
+Licence type (autodetected): BSD-3-Clause
+--------------------------------------------------------------------------------
+
+Contents of probable licence file $GOMODCACHE/golang.org/x/exp@v0.0.0-20230711023510-fffb14384f22/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
@@ -35818,43 +35855,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-20230711023510-fffb14384f22
-Licence type (autodetected): BSD-3-Clause
---------------------------------------------------------------------------------
-
-Contents of probable licence file $GOMODCACHE/golang.org/x/exp@v0.0.0-20230711023510-fffb14384f22/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.14.0

go.mod

@@ -72,6 +72,7 @@ require (
 	go.opentelemetry.io/collector/receiver/otlpreceiver v0.89.0
 	go.uber.org/zap v1.26.0
 	golang.org/x/crypto v0.15.0
+	golang.org/x/exp v0.0.0-20230711023510-fffb14384f22
 	golang.org/x/lint v0.0.0-20210508222113-6edffad5e616
 	golang.org/x/sync v0.5.0
 	golang.org/x/sys v0.14.0
@@ -238,7 +239,6 @@ require (
 	go.opentelemetry.io/otel/trace v1.20.0 // indirect
 	go.opentelemetry.io/proto/otlp v1.0.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
-	golang.org/x/exp v0.0.0-20230711023510-fffb14384f22 // indirect
 	golang.org/x/mod v0.14.0 // indirect
 	golang.org/x/net v0.18.0 // indirect
 	golang.org/x/oauth2 v0.12.0 // indirect

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.FieldByName(name))
+		}
 	}
 
 	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.FieldByName(name))
+		}
 	}
+	c.HTTPTransportSettings = transport
+
 	return nil
 }

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

@@ -5,11 +5,16 @@
 package artifact
 
 import (
+	"reflect"
 	"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"
 	"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)
+}