Review fixes: Windows, remove IsEnabled, Configure defaults

cmd/agent/common/common_windows.go

@@ -49,7 +49,7 @@ func CheckAndUpgradeConfig() error {
 		return nil
 	}
 	config.Datadog.AddConfigPath(path.DefaultConfPath)
-	_, err := config.Load()
+	_, err := config.LoadWithoutSecret()
 	if err == nil {
 		// was able to read config, check for api key
 		if config.Datadog.GetString("api_key") != "" {

cmd/agent/common/helpers.go

@@ -6,59 +6,13 @@
 package common
 
 import (
-	"errors"
-	"fmt"
-	"io/fs"
-	"runtime"
-	"strings"
-
-	"github.com/DataDog/datadog-agent/cmd/agent/common/path"
-	"github.com/DataDog/datadog-agent/comp/core/secrets"
 	"github.com/DataDog/datadog-agent/pkg/autodiscovery/integration"
 	"github.com/DataDog/datadog-agent/pkg/config"
 	"github.com/DataDog/datadog-agent/pkg/config/model"
 	"github.com/DataDog/datadog-agent/pkg/config/settings"
 	"github.com/DataDog/datadog-agent/pkg/util/log"
-	"github.com/DataDog/datadog-agent/pkg/util/optional"
-
-	"github.com/DataDog/viper"
 )
 
-// SetupConfigForTest fires up the configuration system and returns warnings if any.
-func SetupConfigForTest(confFilePath string) (*config.Warnings, error) {
-	cfg := config.Datadog
-	origin := "datadog.yaml"
-	// set the paths where a config file is expected
-	if len(confFilePath) != 0 {
-		// if the configuration file path was supplied on the command line,
-		// add that first so it's first in line
-		cfg.AddConfigPath(confFilePath)
-		// If they set a config file directly, let's try to honor that
-		if strings.HasSuffix(confFilePath, ".yaml") {
-			cfg.SetConfigFile(confFilePath)
-		}
-	}
-	cfg.AddConfigPath(path.DefaultConfPath)
-	// load the configuration
-	warnings, err := config.LoadDatadogCustom(cfg, origin, optional.NewNoneOption[secrets.Component](), nil)
-	// If `!failOnMissingFile`, do not issue an error if we cannot find the default config file.
-	var e viper.ConfigFileNotFoundError
-	if err != nil && (!errors.As(err, &e) || confFilePath != "") {
-		// special-case permission-denied with a clearer error message
-		if errors.Is(err, fs.ErrPermission) {
-			if runtime.GOOS == "windows" {
-				err = fmt.Errorf(`cannot access the Datadog config file (%w); try running the command in an Administrator shell"`, err)
-			} else {
-				err = fmt.Errorf("cannot access the Datadog config file (%w); try running the command under the same user as the Datadog Agent", err)
-			}
-		} else {
-			err = fmt.Errorf("unable to load Datadog config file: %w", err)
-		}
-		return warnings, err
-	}
-	return warnings, nil
-}
-
 // SelectedCheckMatcherBuilder returns a function that returns true if the number of configs found for the
 // check name is more or equal to min instances
 func SelectedCheckMatcherBuilder(checkNames []string, minInstances uint) func(configs []integration.Config) bool {

cmd/agent/common/test_helpers.go

@@ -0,0 +1,54 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016-present Datadog, Inc.
+
+//go:build test
+
+package common
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"runtime"
+	"strings"
+
+	"github.com/DataDog/datadog-agent/cmd/agent/common/path"
+	"github.com/DataDog/datadog-agent/comp/core/secrets"
+	"github.com/DataDog/datadog-agent/pkg/config"
+	"github.com/DataDog/datadog-agent/pkg/util/optional"
+)
+
+// SetupConfigForTest fires up the configuration system and returns warnings if any.
+func SetupConfigForTest(confFilePath string) (*config.Warnings, error) {
+	cfg := config.Datadog
+	origin := "datadog.yaml"
+	// set the paths where a config file is expected
+	if len(confFilePath) != 0 {
+		// if the configuration file path was supplied on the command line,
+		// add that first so it's first in line
+		cfg.AddConfigPath(confFilePath)
+		// If they set a config file directly, let's try to honor that
+		if strings.HasSuffix(confFilePath, ".yaml") {
+			cfg.SetConfigFile(confFilePath)
+		}
+	}
+	cfg.AddConfigPath(path.DefaultConfPath)
+	// load the configuration
+	warnings, err := config.LoadDatadogCustom(cfg, origin, optional.NewNoneOption[secrets.Component](), nil)
+	if err != nil {
+		// special-case permission-denied with a clearer error message
+		if errors.Is(err, fs.ErrPermission) {
+			if runtime.GOOS == "windows" {
+				err = fmt.Errorf(`cannot access the Datadog config file (%w); try running the command in an Administrator shell"`, err)
+			} else {
+				err = fmt.Errorf("cannot access the Datadog config file (%w); try running the command under the same user as the Datadog Agent", err)
+			}
+		} else {
+			err = fmt.Errorf("unable to load Datadog config file: %w", err)
+		}
+		return warnings, err
+	}
+	return warnings, nil
+}

cmd/agent/subcommands/run/command_windows.go

@@ -33,6 +33,7 @@ import (
 	"github.com/DataDog/datadog-agent/comp/core/config"
 	"github.com/DataDog/datadog-agent/comp/core/flare"
 	"github.com/DataDog/datadog-agent/comp/core/log"
+	"github.com/DataDog/datadog-agent/comp/core/secrets"
 	"github.com/DataDog/datadog-agent/comp/core/sysprobeconfig"
 	"github.com/DataDog/datadog-agent/comp/core/sysprobeconfig/sysprobeconfigimpl"
 	"github.com/DataDog/datadog-agent/comp/core/telemetry"
@@ -88,6 +89,7 @@ func StartAgentWithDefaults(ctxChan <-chan context.Context) (<-chan error, error
 			hostMetadata host.Component,
 			invAgent inventoryagent.Component,
 			invHost inventoryhost.Component,
+			secretResolver secrets.Component,
 			_ netflowServer.Component,
 		) error {
 
@@ -113,6 +115,7 @@ func StartAgentWithDefaults(ctxChan <-chan context.Context) (<-chan error, error
 				invAgent,
 				invHost,
 			)
+				secretResolver)
 			if err != nil {
 				return err
 			}

cmd/serverless/main_test.go

@@ -45,7 +45,7 @@ func TestProxyLoadedFromEnvVars(t *testing.T) {
 	t.Setenv("DD_PROXY_HTTP", proxyHTTP)
 	t.Setenv("DD_PROXY_HTTPS", proxyHTTPS)
 
-	config.Load()
+	config.LoadWithoutSecret()
 	proxyHTTPConfig := config.Datadog.GetString("proxy.http")
 	proxyHTTPSConfig := config.Datadog.GetString("proxy.https")
 
@@ -61,7 +61,7 @@ func TestProxyLoadedFromConfigFile(t *testing.T) {
 	os.WriteFile(configTest, []byte("proxy:\n  http: \"c:1\"\n  https: \"c:2\""), 0644)
 
 	config.Datadog.AddConfigPath(tempDir)
-	config.Load()
+	config.LoadWithoutSecret()
 	proxyHTTPConfig := config.Datadog.GetString("proxy.http")
 	proxyHTTPSConfig := config.Datadog.GetString("proxy.https")
 
@@ -82,7 +82,7 @@ func TestProxyLoadedFromConfigFileAndEnvVars(t *testing.T) {
 	os.WriteFile(configTest, []byte("proxy:\n  http: \"e:1\"\n  https: \"e:2\""), 0644)
 
 	config.Datadog.AddConfigPath(tempDir)
-	config.Load()
+	config.LoadWithoutSecret()
 	proxyHTTPConfig := config.Datadog.GetString("proxy.http")
 	proxyHTTPSConfig := config.Datadog.GetString("proxy.https")
 

comp/core/config/config.go

@@ -29,7 +29,7 @@ type cfg struct {
 }
 
 // configDependencies is an interface that mimics the fx-oriented dependencies struct
-// TODO: investigate whether this interrface is worth keeping, otherwise delete it and just use dependencies
+// TODO: (components) investigate whether this interface is worth keeping, otherwise delete it and just use dependencies
 type configDependencies interface {
 	getParams() *Params
 	getSecretResolver() secrets.Component

comp/core/config/setup.go

@@ -48,7 +48,7 @@ func setupConfig(deps configDependencies) (*config.Warnings, error) {
 	var err error
 	var warnings *config.Warnings
 	resolver := deps.getSecretResolver()
-	if resolver == nil || !resolver.IsEnabled() {
+	if resolver == nil {
 		warnings, err = config.LoadWithoutSecret()
 	} else {
 		warnings, err = config.LoadWithSecret(resolver)

comp/core/secrets/component.go

@@ -18,8 +18,6 @@ type Component interface {
 	Configure(command string, arguments []string, timeout, maxSize int, groupExecPerm, removeLinebreak bool)
 	// Get debug information and write it to the parameter
 	GetDebugInfo(w io.Writer)
-	// Whether this component is enabled, if disabled other methods will only log and error and return
-	IsEnabled() bool
 	// Decrypt the given handle and return the corresponding secret value
 	Decrypt(data []byte, origin string) ([]byte, error)
 }

comp/core/secrets/secretsimpl/fetch_secret.go

@@ -38,20 +38,13 @@ func (b *limitBuffer) Write(p []byte) (n int, err error) {
 }
 
 func (r *secretResolver) execCommand(inputPayload string) ([]byte, error) {
+	// hook used only for tests
 	if r.commandHookFunc != nil {
 		return r.commandHookFunc(inputPayload)
 	}
-	commandTimeout := r.backendTimeout
-	if commandTimeout == 0 {
-		commandTimeout = SecretBackendTimeoutDefault
-	}
-	responseMaxSize := r.responseMaxSize
-	if responseMaxSize == 0 {
-		responseMaxSize = SecretBackendOutputMaxSizeDefault
-	}
 
 	ctx, cancel := context.WithTimeout(context.Background(),
-		time.Duration(commandTimeout)*time.Second)
+		time.Duration(r.backendTimeout)*time.Second)
 	defer cancel()
 
 	cmd, done, err := commandContext(ctx, r.backendCommand, r.backendArguments...)
@@ -68,11 +61,11 @@ func (r *secretResolver) execCommand(inputPayload string) ([]byte, error) {
 
 	stdout := limitBuffer{
 		buf: &bytes.Buffer{},
-		max: responseMaxSize,
+		max: r.responseMaxSize,
 	}
 	stderr := limitBuffer{
 		buf: &bytes.Buffer{},
-		max: responseMaxSize,
+		max: r.responseMaxSize,
 	}
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr

comp/core/secrets/secretsimpl/fetch_secret_test.go

@@ -101,7 +101,7 @@ func TestExecCommandError(t *testing.T) {
 
 	t.Run("No Error", func(t *testing.T) {
 		resolver := newEnabledSecretResolver()
-		resolver.backendCommand = "./test/simple/simple" + binExtension
+		resolver.Configure("./test/simple/simple"+binExtension, nil, 0, 0, false, false)
 		setCorrectRight(resolver.backendCommand)
 		resp, err := resolver.execCommand(inputPayload)
 		require.NoError(t, err)
@@ -118,7 +118,7 @@ func TestExecCommandError(t *testing.T) {
 
 	t.Run("argument", func(t *testing.T) {
 		resolver := newEnabledSecretResolver()
-		resolver.backendCommand = "./test/argument/argument" + binExtension
+		resolver.Configure("./test/argument/argument"+binExtension, nil, 0, 0, false, false)
 		setCorrectRight(resolver.backendCommand)
 		resolver.backendArguments = []string{"arg1"}
 		_, err := resolver.execCommand(inputPayload)
@@ -131,7 +131,7 @@ func TestExecCommandError(t *testing.T) {
 
 	t.Run("input", func(t *testing.T) {
 		resolver := newEnabledSecretResolver()
-		resolver.backendCommand = "./test/input/input" + binExtension
+		resolver.Configure("./test/input/input"+binExtension, nil, 0, 0, false, false)
 		setCorrectRight(resolver.backendCommand)
 		resp, err := resolver.execCommand(inputPayload)
 		require.NoError(t, err)
@@ -140,7 +140,7 @@ func TestExecCommandError(t *testing.T) {
 
 	t.Run("buffer limit", func(t *testing.T) {
 		resolver := newEnabledSecretResolver()
-		resolver.backendCommand = "./test/response_too_long/response_too_long" + binExtension
+		resolver.Configure("./test/response_too_long/response_too_long"+binExtension, nil, 0, 0, false, false)
 		setCorrectRight(resolver.backendCommand)
 		resolver.responseMaxSize = 20
 		_, err := resolver.execCommand(inputPayload)

comp/core/secrets/secretsimpl/info_nix.go

@@ -24,10 +24,10 @@ type permissionsDetails struct {
 	Group    string
 }
 
-func getExecutablePermissions(secret *secretResolver) (interface{}, error) {
+func (r *secretResolver) getExecutablePermissions() (interface{}, error) {
 	var stat syscall.Stat_t
-	if err := syscall.Stat(secret.backendCommand, &stat); err != nil {
-		return nil, fmt.Errorf("Could not stat %s: %s", secret.backendCommand, err)
+	if err := syscall.Stat(r.backendCommand, &stat); err != nil {
+		return nil, fmt.Errorf("Could not stat %s: %s", r.backendCommand, err)
 	}
 
 	details := permissionsDetails{

comp/core/secrets/secretsimpl/info_nix_test.go

@@ -30,7 +30,7 @@ func TestGetExecutablePermissionsError(t *testing.T) {
 	resolver := newEnabledSecretResolver()
 	resolver.backendCommand = "some_command"
 
-	_, err := getExecutablePermissions(resolver)
+	_, err := resolver.getExecutablePermissions()
 	assert.Error(t, err, "getExecutablePermissions should fail when secretBackendCommand file does not exists")
 }
 
@@ -57,7 +57,7 @@ func TestGetExecutablePermissionsSuccess(t *testing.T) {
 	resolver := newEnabledSecretResolver()
 	currentUser, currentGroup := setupSecretCommand(t, resolver)
 
-	res, err := getExecutablePermissions(resolver)
+	res, err := resolver.getExecutablePermissions()
 	require.NoError(t, err)
 	require.IsType(t, permissionsDetails{}, res)
 	details := res.(permissionsDetails)

comp/core/secrets/secretsimpl/info_windows.go

@@ -24,8 +24,8 @@ type permissionsDetails struct {
 	Stderr string
 }
 
-func getExecutablePermissions() (interface{}, error) {
-	execPath := fmt.Sprintf("\"%s\"", strings.TrimSpace(secretBackendCommand))
+func (r *secretResolver) getExecutablePermissions() (interface{}, error) {
+	execPath := fmt.Sprintf("\"%s\"", strings.TrimSpace(r.backendCommand))
 	ps, err := exec.LookPath("powershell.exe")
 	if err != nil {
 		return nil, fmt.Errorf("Could not find executable powershell.exe: %s", err)

comp/core/secrets/secretsimpl/info_windows_test.go

@@ -19,10 +19,10 @@ import (
 )
 
 func TestGetExecutablePermissionsError(t *testing.T) {
-	secretBackendCommand = "some_command"
-	t.Cleanup(resetPackageVars)
+	resolver := newEnabledSecretResolver()
+	resolver.backendCommand = "some_command"
 
-	res, err := getExecutablePermissions()
+	res, err := resolver.getExecutablePermissions()
 	require.NoError(t, err)
 	require.IsType(t, permissionsDetails{}, res)
 	details := res.(permissionsDetails)
@@ -31,27 +31,27 @@ func TestGetExecutablePermissionsError(t *testing.T) {
 	assert.NotEqual(t, "", details.Stderr)
 }
 
-func setupSecretCommmand(t *testing.T) {
+func setupSecretCommmand(t *testing.T, resolver *secretResolver) {
 	dir := t.TempDir()
-	t.Cleanup(resetPackageVars)
 
-	secretBackendCommand = filepath.Join(dir, "an executable with space")
-	f, err := os.Create(secretBackendCommand)
+	resolver.backendCommand = filepath.Join(dir, "an executable with space")
+	f, err := os.Create(resolver.backendCommand)
 	require.NoError(t, err)
 	f.Close()
 
 	exec.Command("powershell", "test/setAcl.ps1",
-		"-file", fmt.Sprintf("\"%s\"", secretBackendCommand),
+		"-file", fmt.Sprintf("\"%s\"", resolver.backendCommand),
 		"-removeAllUser", "0",
 		"-removeAdmin", "0",
 		"-removeLocalSystem", "0",
 		"-addDDuser", "1").Run()
 }
 
 func TestGetExecutablePermissionsSuccess(t *testing.T) {
-	setupSecretCommmand(t)
+	resolver := newEnabledSecretResolver()
+	setupSecretCommmand(t, resolver)
 
-	res, err := getExecutablePermissions()
+	res, err := resolver.getExecutablePermissions()
 	require.NoError(t, err)
 	require.IsType(t, permissionsDetails{}, res)
 	details := res.(permissionsDetails)