From 5517946d2a6426389a7105a49ffc627b3e37af04 Mon Sep 17 00:00:00 2001 From: Maxime mouial Date: Mon, 16 Dec 2024 15:36:43 +0100 Subject: [PATCH] Remove optional from configsync in favor of an internal enabled/disabled state The caller should not care if configsync is enabled or not. There is no need to expose the internal state of the component to the use since it has no public method. --- cmd/otel-agent/subcommands/run/command.go | 10 ++++---- cmd/process-agent/command/main_common.go | 5 ++-- cmd/security-agent/main_windows.go | 5 ++-- .../subcommands/start/command.go | 5 ++-- cmd/trace-agent/subcommands/run/command.go | 4 ++-- comp/core/configsync/configsyncimpl/module.go | 24 +++++++++---------- .../configsyncimpl/module_integration_test.go | 5 ++-- .../configsync/configsyncimpl/module_test.go | 8 +++---- 8 files changed, 31 insertions(+), 35 deletions(-) diff --git a/cmd/otel-agent/subcommands/run/command.go b/cmd/otel-agent/subcommands/run/command.go index a414674620e44b..e4c29bfd56764b 100644 --- a/cmd/otel-agent/subcommands/run/command.go +++ b/cmd/otel-agent/subcommands/run/command.go @@ -178,10 +178,10 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams, fx.Invoke(func(_ collectordef.Component, _ defaultforwarder.Forwarder, _ optional.Option[logsagentpipeline.Component]) { }), - // TODO: don't rely on this pattern; remove this `OptionalModuleWithParams` thing + // TODO: don't rely on this pattern; remove this `ModuleWithParams` thing // and instead adapt OptionalModule to allow parameter passing naturally. // See: https://github.com/DataDog/datadog-agent/pull/28386 - configsyncimpl.OptionalModuleWithParams(), + configsyncimpl.ModuleWithParams(), fx.Provide(func() configsyncimpl.Params { return configsyncimpl.NewParams(params.SyncTimeout, params.SyncDelay, true) }), @@ -205,7 +205,7 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams, // TODO: consider adding configsync.Component as an explicit dependency for traceconfig // to avoid this sort of dependency tree hack. - fx.Provide(func(deps traceconfig.Dependencies, _ optional.Option[configsync.Component]) (traceconfig.Component, error) { + fx.Provide(func(deps traceconfig.Dependencies, _ configsync.Component) (traceconfig.Component, error) { // TODO: this would be much better if we could leverage traceconfig.Module // Must add a new parameter to traconfig.Module to handle this. return traceconfig.NewConfig(deps) @@ -223,13 +223,13 @@ func runOTelAgentCommand(ctx context.Context, params *subcommands.GlobalParams, // ForwarderBundle returns the fx.Option for the forwarder bundle. // TODO: cleanup the forwarder instantiation with fx. -// This is a bit of a hack because we need to enforce optional.Option[configsync.Component] +// This is a bit of a hack because we need to enforce configsync.Component // is passed to newForwarder to enforce the correct instantiation order. Currently, the // new forwarder.BundleWithProvider makes a few assumptions in its generic prototype, and // this is the current workaround to leverage it. func ForwarderBundle() fx.Option { return defaultforwarder.ModulWithOptionTMP( - fx.Provide(func(_ optional.Option[configsync.Component]) defaultforwarder.Params { + fx.Provide(func(_ configsync.Component) defaultforwarder.Params { return defaultforwarder.NewParams() })) } diff --git a/cmd/process-agent/command/main_common.go b/cmd/process-agent/command/main_common.go index 188c9684fd30bf..8bc59bc3f45280 100644 --- a/cmd/process-agent/command/main_common.go +++ b/cmd/process-agent/command/main_common.go @@ -65,7 +65,6 @@ import ( "github.com/DataDog/datadog-agent/pkg/util/fxutil" "github.com/DataDog/datadog-agent/pkg/util/fxutil/logging" "github.com/DataDog/datadog-agent/pkg/util/log" - "github.com/DataDog/datadog-agent/pkg/util/optional" "github.com/DataDog/datadog-agent/pkg/version" ) @@ -156,7 +155,7 @@ func runApp(ctx context.Context, globalParams *GlobalParams) error { fetchonlyimpl.Module(), // Provide configsync module - configsyncimpl.OptionalModule(), + configsyncimpl.Module(), // Provide autoexit module autoexitimpl.Module(), @@ -215,7 +214,7 @@ func runApp(ctx context.Context, globalParams *GlobalParams) error { _ expvars.Component, _ apiserver.Component, cfg config.Component, - _ optional.Option[configsync.Component], + _ configsync.Component, // TODO: This is needed by the container-provider which is not currently a component. // We should ensure the tagger is a dependency when converting to a component. _ tagger.Component, diff --git a/cmd/security-agent/main_windows.go b/cmd/security-agent/main_windows.go index b81223e6aebc17..d0c4c009721a6e 100644 --- a/cmd/security-agent/main_windows.go +++ b/cmd/security-agent/main_windows.go @@ -54,7 +54,6 @@ import ( "github.com/DataDog/datadog-agent/pkg/util/defaultpaths" "github.com/DataDog/datadog-agent/pkg/util/fxutil" - "github.com/DataDog/datadog-agent/pkg/util/optional" "github.com/DataDog/datadog-agent/pkg/util/startstop" "github.com/DataDog/datadog-agent/pkg/util/winutil/servicemain" ) @@ -164,9 +163,9 @@ func (s *service) Run(svcctx context.Context) error { statusimpl.Module(), fetchonlyimpl.Module(), - configsyncimpl.OptionalModule(), + configsyncimpl.Module(), // Force the instantiation of the component - fx.Invoke(func(_ optional.Option[configsync.Component]) {}), + fx.Invoke(func(_ configsync.Component) {}), autoexitimpl.Module(), fx.Provide(func(c config.Component) settings.Params { return settings.Params{ diff --git a/cmd/security-agent/subcommands/start/command.go b/cmd/security-agent/subcommands/start/command.go index 8386100c0031ba..f80523ab00a943 100644 --- a/cmd/security-agent/subcommands/start/command.go +++ b/cmd/security-agent/subcommands/start/command.go @@ -65,7 +65,6 @@ import ( "github.com/DataDog/datadog-agent/pkg/status/health" "github.com/DataDog/datadog-agent/pkg/util" "github.com/DataDog/datadog-agent/pkg/util/fxutil" - "github.com/DataDog/datadog-agent/pkg/util/optional" "github.com/DataDog/datadog-agent/pkg/util/profiling" "github.com/DataDog/datadog-agent/pkg/util/startstop" "github.com/DataDog/datadog-agent/pkg/version" @@ -173,9 +172,9 @@ func Commands(globalParams *command.GlobalParams) []*cobra.Command { }), statusimpl.Module(), fetchonlyimpl.Module(), - configsyncimpl.OptionalModule(), + configsyncimpl.Module(), // Force the instantiation of the component - fx.Invoke(func(_ optional.Option[configsync.Component]) {}), + fx.Invoke(func(_ configsync.Component) {}), autoexitimpl.Module(), fx.Supply(pidimpl.NewParams(params.pidfilePath)), fx.Provide(func(c config.Component) settings.Params { diff --git a/cmd/trace-agent/subcommands/run/command.go b/cmd/trace-agent/subcommands/run/command.go index 1d0788501ffae2..4526101362de1d 100644 --- a/cmd/trace-agent/subcommands/run/command.go +++ b/cmd/trace-agent/subcommands/run/command.go @@ -113,9 +113,9 @@ func runTraceAgentProcess(ctx context.Context, cliParams *Params, defaultConfPat zstdfx.Module(), trace.Bundle(), fetchonlyimpl.Module(), - configsyncimpl.OptionalModule(), + configsyncimpl.Module(), // Force the instantiation of the components - fx.Invoke(func(_ traceagent.Component, _ optional.Option[configsync.Component], _ autoexit.Component) {}), + fx.Invoke(func(_ traceagent.Component, _ configsync.Component, _ autoexit.Component) {}), ) if err != nil && errors.Is(err, traceagentimpl.ErrAgentDisabled) { return nil diff --git a/comp/core/configsync/configsyncimpl/module.go b/comp/core/configsync/configsyncimpl/module.go index 0b77c1370de204..547a68c6c0439d 100644 --- a/comp/core/configsync/configsyncimpl/module.go +++ b/comp/core/configsync/configsyncimpl/module.go @@ -22,7 +22,6 @@ import ( log "github.com/DataDog/datadog-agent/comp/core/log/def" apiutil "github.com/DataDog/datadog-agent/pkg/api/util" "github.com/DataDog/datadog-agent/pkg/util/fxutil" - "github.com/DataDog/datadog-agent/pkg/util/optional" ) type dependencies struct { @@ -35,20 +34,20 @@ type dependencies struct { SyncParams Params } -// OptionalModule defines the fx options for this component. -func OptionalModule() fxutil.Module { +// Module defines the fx options for this component. +func Module() fxutil.Module { return fxutil.Component( - fx.Provide(newOptionalConfigSync), + fx.Provide(newConfigSync), fx.Supply(Params{}), ) } -// OptionalModuleWithParams defines the fx options for this component, but +// ModuleWithParams defines the fx options for this component, but // requires additionally specifying custom Params from the fx App, to be // passed to the constructor. -func OptionalModuleWithParams() fxutil.Module { +func ModuleWithParams() fxutil.Module { return fxutil.Component( - fx.Provide(newOptionalConfigSync), + fx.Provide(newConfigSync), ) } @@ -63,17 +62,18 @@ type configSync struct { ctx context.Context } -// newOptionalConfigSync checks if the component was enabled as per the config, and returns an optional.Option -func newOptionalConfigSync(deps dependencies) optional.Option[configsync.Component] { +// newConfigSync checks if the component was enabled as per the config and return a enable/disabled configsync +func newConfigSync(deps dependencies) configsync.Component { agentIPCPort := deps.Config.GetInt("agent_ipc.port") configRefreshIntervalSec := deps.Config.GetInt("agent_ipc.config_refresh_interval") if agentIPCPort <= 0 || configRefreshIntervalSec <= 0 { - return optional.NewNoneOption[configsync.Component]() + deps.Log.Infof("configsync disabled (agent_ipc.port: %d | agent_ipc.config_refresh_interval: %d)", agentIPCPort, configRefreshIntervalSec) + return configsync{} } - configSync := newConfigSync(deps, agentIPCPort, configRefreshIntervalSec) - return optional.NewOption(configSync) + deps.Log.Infof("configsync enabled (agent_ipc '%s:%d' | agent_ipc.config_refresh_interval: %d)", deps.Config.GetString("agent_ipc.host"), agentIPCPort, configRefreshIntervalSec) + return newConfigSync(deps, agentIPCPort, configRefreshIntervalSec) } // newConfigSync creates a new configSync component. diff --git a/comp/core/configsync/configsyncimpl/module_integration_test.go b/comp/core/configsync/configsyncimpl/module_integration_test.go index e4cfa8ec1ba637..3584bede676d73 100644 --- a/comp/core/configsync/configsyncimpl/module_integration_test.go +++ b/comp/core/configsync/configsyncimpl/module_integration_test.go @@ -22,7 +22,6 @@ import ( "github.com/DataDog/datadog-agent/comp/core/config" "github.com/DataDog/datadog-agent/comp/core/configsync" "github.com/DataDog/datadog-agent/pkg/util/fxutil" - "github.com/DataDog/datadog-agent/pkg/util/optional" ) func TestOptionalModule(t *testing.T) { @@ -44,10 +43,10 @@ func TestOptionalModule(t *testing.T) { "agent_ipc.port": port, "agent_ipc.config_refresh_interval": 1, } - csopt := fxutil.Test[optional.Option[configsync.Component]](t, fx.Options( + csopt := fxutil.Test[configsync.Component](t, fx.Options( core.MockBundle(), fetchonlyimpl.Module(), - OptionalModule(), + Module(), fx.Populate(&cfg), fx.Replace(config.MockParams{Overrides: overrides}), )) diff --git a/comp/core/configsync/configsyncimpl/module_test.go b/comp/core/configsync/configsyncimpl/module_test.go index 076288737a65ef..f6cd85e446172a 100644 --- a/comp/core/configsync/configsyncimpl/module_test.go +++ b/comp/core/configsync/configsyncimpl/module_test.go @@ -13,12 +13,12 @@ import ( pkgconfigmodel "github.com/DataDog/datadog-agent/pkg/config/model" ) -func TestNewOptionalConfigSync(t *testing.T) { +func TestNewConfigSync(t *testing.T) { t.Run("enabled", func(t *testing.T) { deps := makeDeps(t) deps.Config.Set("agent_ipc.port", 1234, pkgconfigmodel.SourceFile) deps.Config.Set("agent_ipc.config_refresh_interval", 30, pkgconfigmodel.SourceFile) - optConfigSync := newOptionalConfigSync(deps) + optConfigSync := newConfigSync(deps) _, ok := optConfigSync.Get() require.True(t, ok) }) @@ -26,7 +26,7 @@ func TestNewOptionalConfigSync(t *testing.T) { t.Run("disabled ipc port zero", func(t *testing.T) { deps := makeDeps(t) deps.Config.Set("agent_ipc.port", 0, pkgconfigmodel.SourceFile) - optConfigSync := newOptionalConfigSync(deps) + optConfigSync := newConfigSync(deps) _, ok := optConfigSync.Get() require.False(t, ok) }) @@ -34,7 +34,7 @@ func TestNewOptionalConfigSync(t *testing.T) { t.Run("disabled config refresh interval zero", func(t *testing.T) { deps := makeDeps(t) deps.Config.Set("agent_ipc.config_refresh_interval", 0, pkgconfigmodel.SourceFile) - optConfigSync := newOptionalConfigSync(deps) + optConfigSync := newConfigSync(deps) _, ok := optConfigSync.Get() require.False(t, ok) })