From 5d4df54296299bb002069aed7c8b98f91eceec8f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 7 Aug 2020 21:08:43 -0400 Subject: [PATCH 1/2] agent: extract dependency creation from New With this change, Agent.New() accepts many of the dependencies instead of creating them in New. Accepting fully constructed dependencies from a constructor makes the type easier to test, and easier to change. There are still a number of dependencies created in Start() which can be addressed in a follow up. --- agent/acl.go | 11 +- agent/acl_test.go | 49 ++++---- agent/agent.go | 265 +++------------------------------------- agent/config/builder.go | 6 +- agent/setup.go | 163 ++++++++++++++++++++++++ agent/testagent.go | 44 ++++--- command/agent/agent.go | 35 +++--- logging/grpc.go | 4 +- logging/grpc_test.go | 4 +- logging/logger.go | 1 + 10 files changed, 257 insertions(+), 325 deletions(-) create mode 100644 agent/setup.go diff --git a/agent/acl.go b/agent/acl.go index a603304d2b..5463fbf1f1 100644 --- a/agent/acl.go +++ b/agent/acl.go @@ -59,7 +59,7 @@ func (a *Agent) aclAccessorID(secretID string) string { return ident.ID() } -func (a *Agent) initializeACLs() error { +func initializeACLs(nodeName string) (acl.Authorizer, error) { // Build a policy for the agent master token. // The builtin agent master policy allows reading any node information // and allows writes to the agent with the node name of the running agent @@ -69,7 +69,7 @@ func (a *Agent) initializeACLs() error { PolicyRules: acl.PolicyRules{ Agents: []*acl.AgentRule{ { - Node: a.config.NodeName, + Node: nodeName, Policy: acl.PolicyWrite, }, }, @@ -81,12 +81,7 @@ func (a *Agent) initializeACLs() error { }, }, } - master, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - if err != nil { - return err - } - a.aclMasterAuthorizer = master - return nil + return acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) } // vetServiceRegister makes sure the service registration action is allowed by diff --git a/agent/acl_test.go b/agent/acl_test.go index 303c893374..63525f0397 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -35,37 +35,38 @@ type TestACLAgent struct { // Basically it needs a local state for some of the vet* functions, a logger and a delegate. // The key is that we are the delegate so we can control the ResolveToken responses func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzResolver, resolveIdent identResolver) *TestACLAgent { + t.Helper() + a := &TestACLAgent{resolveAuthzFn: resolveAuthz, resolveIdentFn: resolveIdent} dataDir := testutil.TempDir(t, "acl-agent") - logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ - Name: name, - Level: hclog.Debug, - Output: testutil.NewLogBuffer(t), - }) - opts := []AgentOption{ - WithLogger(logger), - WithBuilderOpts(config.BuilderOpts{ - HCL: []string{ - TestConfigHCL(NodeID()), - hcl, - fmt.Sprintf(`data_dir = "%s"`, dataDir), - }, - }), + logBuffer := testutil.NewLogBuffer(t) + loader := func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) { + dataDir := fmt.Sprintf(`data_dir = "%s"`, dataDir) + opts := config.BuilderOpts{ + HCL: []string{TestConfigHCL(NodeID()), hcl, dataDir}, + } + return config.Load(opts, source) } - - agent, err := New(opts...) + bd, err := NewBaseDeps(loader, logBuffer) require.NoError(t, err) - cfg := agent.GetConfig() + + bd.Logger = hclog.NewInterceptLogger(&hclog.LoggerOptions{ + Name: name, + Level: hclog.Debug, + Output: logBuffer, + TimeFormat: "04:05.000", + }) + bd.TelemetrySink = metrics.NewInmemSink(1*time.Second, time.Minute) + + agent, err := New(bd) + require.NoError(t, err) + + agent.delegate = a + agent.State = local.NewState(LocalConfig(bd.RuntimeConfig), bd.Logger, bd.Tokens) + agent.State.TriggerSyncChanges = func() {} a.Agent = agent - - agent.logger = logger - agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) - - a.Agent.delegate = a - a.Agent.State = local.NewState(LocalConfig(cfg), logger, a.Agent.tokens) - a.Agent.State.TriggerSyncChanges = func() {} return a } diff --git a/agent/agent.go b/agent/agent.go index fc41059372..7af0f5d15a 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -21,7 +21,6 @@ import ( "github.com/hashicorp/go-connlimit" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" - "github.com/mitchellh/cli" "google.golang.org/grpc" "google.golang.org/grpc/grpclog" @@ -312,85 +311,6 @@ type Agent struct { enterpriseAgent } -type agentOptions struct { - logger hclog.InterceptLogger - builderOpts config.BuilderOpts - ui cli.Ui - config *config.RuntimeConfig - overrides []config.Source - writers []io.Writer - initTelemetry bool -} - -type AgentOption func(opt *agentOptions) - -// WithTelemetry is used to control whether the agent will -// set up metrics. -func WithTelemetry(initTelemetry bool) AgentOption { - return func(opt *agentOptions) { - opt.initTelemetry = initTelemetry - } -} - -// WithLogger is used to override any automatic logger creation -// and provide one already built instead. This is mostly useful -// for testing. -func WithLogger(logger hclog.InterceptLogger) AgentOption { - return func(opt *agentOptions) { - opt.logger = logger - } -} - -// WithBuilderOpts specifies the command line config.BuilderOpts to use that the agent -// is being started with -func WithBuilderOpts(builderOpts config.BuilderOpts) AgentOption { - return func(opt *agentOptions) { - opt.builderOpts = builderOpts - } -} - -// WithCLI provides a cli.Ui instance to use when emitting configuration -// warnings during the first configuration parsing. -func WithCLI(ui cli.Ui) AgentOption { - return func(opt *agentOptions) { - opt.ui = ui - } -} - -// WithLogWriter will add an additional log output to the logger that gets -// configured after configuration parsing -func WithLogWriter(writer io.Writer) AgentOption { - return func(opt *agentOptions) { - opt.writers = append(opt.writers, writer) - } -} - -// WithOverrides is used to provide a config source to append to the tail sources -// during config building. It is really only useful for testing to tune non-user -// configurable tunables to make various tests converge more quickly than they -// could otherwise. -func WithOverrides(overrides ...config.Source) AgentOption { - return func(opt *agentOptions) { - opt.overrides = overrides - } -} - -// WithConfig provides an already parsed configuration to the Agent -// Deprecated: Should allow the agent to parse the configuration. -func WithConfig(config *config.RuntimeConfig) AgentOption { - return func(opt *agentOptions) { - opt.config = config - } -} - -func flattenAgentOptions(options []AgentOption) agentOptions { - var flat agentOptions - for _, opt := range options { - opt(&flat) - } - return flat -} - // New process the desired options and creates a new Agent. // This process will // * parse the config given the config Flags @@ -406,10 +326,7 @@ func flattenAgentOptions(options []AgentOption) agentOptions { // * setup the NodeID if one isn't provided in the configuration // * create the AutoConfig object for future use in fully // resolving the configuration -func New(options ...AgentOption) (*Agent, error) { - flat := flattenAgentOptions(options) - - // Create most of the agent +func New(bd BaseDeps) (*Agent, error) { a := Agent{ checkReapAfter: make(map[structs.CheckID]time.Duration), checkMonitors: make(map[structs.CheckID]*checks.CheckMonitor), @@ -425,94 +342,28 @@ func New(options ...AgentOption) (*Agent, error) { retryJoinCh: make(chan error), shutdownCh: make(chan struct{}), endpoints: make(map[string]string), - tokens: new(token.Store), - logger: flat.logger, + + // TODO: store the BaseDeps instead of copying them over to Agent + tokens: bd.Tokens, + logger: bd.Logger, + tlsConfigurator: bd.TLSConfigurator, + config: bd.RuntimeConfig, + cache: bd.Cache, + MemSink: bd.TelemetrySink, + connPool: bd.ConnPool, + autoConf: bd.AutoConfig, } - // parse the configuration and handle the error/warnings - cfg, warnings, err := config.Load(flat.builderOpts, nil, flat.overrides...) - if err != nil { - return nil, err - } - for _, w := range warnings { - if a.logger != nil { - a.logger.Warn(w) - } else if flat.ui != nil { - flat.ui.Warn(w) - } else { - fmt.Fprint(os.Stderr, w) - } - } - - // set the config in the agent, this is just the preliminary configuration as we haven't - // loaded any auto-config sources yet. - a.config = cfg - - // create the cache using the rate limiting settings from the config. Note that this means - // that these limits are not reloadable. - a.cache = cache.New(a.config.Cache) - - if flat.logger == nil { - logConf := &logging.Config{ - LogLevel: cfg.LogLevel, - LogJSON: cfg.LogJSON, - Name: logging.Agent, - EnableSyslog: cfg.EnableSyslog, - SyslogFacility: cfg.SyslogFacility, - LogFilePath: cfg.LogFile, - LogRotateDuration: cfg.LogRotateDuration, - LogRotateBytes: cfg.LogRotateBytes, - LogRotateMaxFiles: cfg.LogRotateMaxFiles, - } - - a.logger, err = logging.Setup(logConf, flat.writers) - if err != nil { - return nil, err - } - - grpclog.SetLoggerV2(logging.NewGRPCLogger(logConf, a.logger)) - } - - if flat.initTelemetry { - memSink, err := lib.InitTelemetry(cfg.Telemetry) - if err != nil { - return nil, fmt.Errorf("Failed to initialize telemetry: %w", err) - } - a.MemSink = memSink - } - - // TODO (autoconf) figure out how to let this setting be pushed down via autoconf - // right now it gets defaulted if unset so this check actually doesn't do much - // for a normal running agent. - if a.config.Datacenter == "" { - return nil, fmt.Errorf("Must configure a Datacenter") - } - if a.config.DataDir == "" && !a.config.DevMode { - return nil, fmt.Errorf("Must configure a DataDir") - } - - tlsConfigurator, err := tlsutil.NewConfigurator(a.config.ToTLSUtilConfig(), a.logger) - if err != nil { - return nil, err - } - a.tlsConfigurator = tlsConfigurator - - err = a.initializeConnectionPool() - if err != nil { - return nil, fmt.Errorf("Failed to initialize the connection pool: %w", err) - } + // TODO: set globals somewhere else, not Agent.New + grpclog.SetLoggerV2(logging.NewGRPCLogger(bd.RuntimeConfig.LogLevel, bd.Logger)) a.serviceManager = NewServiceManager(&a) - if err := a.initializeACLs(); err != nil { - return nil, err - } - - // Retrieve or generate the node ID before setting up the rest of the - // agent, which depends on it. - cfg.NodeID, err = newNodeIDFromConfig(a.config, a.logger) + // TODO: do this somewhere else, maybe move to newBaseDeps + var err error + a.aclMasterAuthorizer, err = initializeACLs(bd.RuntimeConfig.NodeName) if err != nil { - return nil, fmt.Errorf("failed to setup node ID: %w", err) + return nil, err } // We used to do this in the Start method. However it doesn't need to go @@ -521,48 +372,9 @@ func New(options ...AgentOption) (*Agent, error) { // pass the agent itself so its safe to move here. a.registerCache() - cmConf := new(certmon.Config). - WithCache(a.cache). - WithTLSConfigurator(a.tlsConfigurator). - WithDNSSANs(a.config.AutoConfig.DNSSANs). - WithIPSANs(a.config.AutoConfig.IPSANs). - WithDatacenter(a.config.Datacenter). - WithNodeName(a.config.NodeName). - WithFallback(a.autoConfigFallbackTLS). - WithLogger(a.logger.Named(logging.AutoConfig)). - WithTokens(a.tokens). - WithPersistence(a.autoConfigPersist) - acCertMon, err := certmon.New(cmConf) - if err != nil { - return nil, err - } - - acConf := autoconf.Config{ - DirectRPC: a.connPool, - Logger: a.logger, - CertMonitor: acCertMon, - Loader: func(source config.Source) (*config.RuntimeConfig, []string, error) { - return config.Load(flat.builderOpts, source, flat.overrides...) - }, - } - ac, err := autoconf.New(acConf) - if err != nil { - return nil, err - } - - a.autoConf = ac - return &a, nil } -// GetLogger retrieves the agents logger -// TODO make export the logger field and get rid of this method -// This is here for now to simplify the work I am doing and make -// reviewing the final PR easier. -func (a *Agent) GetLogger() hclog.InterceptLogger { - return a.logger -} - // GetConfig retrieves the agents config // TODO make export the config field and get rid of this method // This is here for now to simplify the work I am doing and make @@ -573,31 +385,6 @@ func (a *Agent) GetConfig() *config.RuntimeConfig { return a.config } -func (a *Agent) initializeConnectionPool() error { - var rpcSrcAddr *net.TCPAddr - if !ipaddr.IsAny(a.config.RPCBindAddr) { - rpcSrcAddr = &net.TCPAddr{IP: a.config.RPCBindAddr.IP} - } - - pool := &pool.ConnPool{ - Server: a.config.ServerMode, - SrcAddr: rpcSrcAddr, - Logger: a.logger.StandardLogger(&hclog.StandardLoggerOptions{InferLevels: true}), - TLSConfigurator: a.tlsConfigurator, - Datacenter: a.config.Datacenter, - } - if a.config.ServerMode { - pool.MaxTime = 2 * time.Minute - pool.MaxStreams = 64 - } else { - pool.MaxTime = 127 * time.Second - pool.MaxStreams = 32 - } - - a.connPool = pool - return nil -} - // LocalConfig takes a config.RuntimeConfig and maps the fields to a local.Config func LocalConfig(cfg *config.RuntimeConfig) local.Config { lc := local.Config{ @@ -638,8 +425,8 @@ func (a *Agent) Start(ctx context.Context) error { return fmt.Errorf("Failed to load TLS configurations after applying auto-config settings: %w", err) } - // load the tokens - this requires the logger to be setup - // which is why we can't do this in New + // TODO: move to newBaseDeps + // TODO: handle error a.loadTokens(a.config) a.loadEnterpriseTokens(a.config) @@ -858,20 +645,6 @@ func (a *Agent) autoEncryptInitialCertificate(ctx context.Context) (*structs.Sig return client.RequestAutoEncryptCerts(ctx, addrs, a.config.ServerPort, a.tokens.AgentToken(), a.config.AutoEncryptDNSSAN, a.config.AutoEncryptIPSAN) } -func (a *Agent) autoConfigFallbackTLS(ctx context.Context) (*structs.SignedResponse, error) { - if a.autoConf == nil { - return nil, fmt.Errorf("AutoConfig manager has not been created yet") - } - return a.autoConf.FallbackTLS(ctx) -} - -func (a *Agent) autoConfigPersist(resp *structs.SignedResponse) error { - if a.autoConf == nil { - return fmt.Errorf("AutoConfig manager has not been created yet") - } - return a.autoConf.RecordUpdatedCerts(resp) -} - func (a *Agent) listenAndServeGRPC() error { if len(a.config.GRPCAddrs) < 1 { return nil diff --git a/agent/config/builder.go b/agent/config/builder.go index 216237e6f0..cf63780273 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -35,11 +35,11 @@ import ( "golang.org/x/time/rate" ) -// LoadConfig will build the configuration including the extraHead source injected +// Load will build the configuration including the extraHead source injected // after all other defaults but before any user supplied configuration and the overrides // source injected as the final source in the configuration parsing chain. -func Load(builderOpts BuilderOpts, extraHead Source, overrides ...Source) (*RuntimeConfig, []string, error) { - b, err := NewBuilder(builderOpts) +func Load(opts BuilderOpts, extraHead Source, overrides ...Source) (*RuntimeConfig, []string, error) { + b, err := NewBuilder(opts) if err != nil { return nil, nil, err } diff --git a/agent/setup.go b/agent/setup.go new file mode 100644 index 0000000000..9feb9d6f8e --- /dev/null +++ b/agent/setup.go @@ -0,0 +1,163 @@ +package agent + +import ( + "context" + "fmt" + "io" + "net" + "time" + + "github.com/armon/go-metrics" + autoconf "github.com/hashicorp/consul/agent/auto-config" + "github.com/hashicorp/consul/agent/cache" + certmon "github.com/hashicorp/consul/agent/cert-monitor" + "github.com/hashicorp/consul/agent/config" + "github.com/hashicorp/consul/agent/pool" + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/agent/token" + "github.com/hashicorp/consul/ipaddr" + "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/logging" + "github.com/hashicorp/consul/tlsutil" + "github.com/hashicorp/go-hclog" +) + +// TODO: BaseDeps should be renamed in the future once more of Agent.Start +// has been moved out in front of Agent.New, and we can better see the setup +// dependencies. +type BaseDeps struct { + Logger hclog.InterceptLogger + TLSConfigurator *tlsutil.Configurator // TODO: use an interface + TelemetrySink *metrics.InmemSink // TODO: use an interface + RuntimeConfig *config.RuntimeConfig + Tokens *token.Store + Cache *cache.Cache + AutoConfig *autoconf.AutoConfig // TODO: use an interface + ConnPool *pool.ConnPool // TODO: use an interface +} + +type ConfigLoader func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) + +func NewBaseDeps(configLoader ConfigLoader, logOut io.Writer) (BaseDeps, error) { + d := BaseDeps{} + cfg, warnings, err := configLoader(nil) + if err != nil { + return d, err + } + + // TODO: use logging.Config in RuntimeConfig instead of separate fields + logConf := &logging.Config{ + LogLevel: cfg.LogLevel, + LogJSON: cfg.LogJSON, + Name: logging.Agent, + EnableSyslog: cfg.EnableSyslog, + SyslogFacility: cfg.SyslogFacility, + LogFilePath: cfg.LogFile, + LogRotateDuration: cfg.LogRotateDuration, + LogRotateBytes: cfg.LogRotateBytes, + LogRotateMaxFiles: cfg.LogRotateMaxFiles, + } + d.Logger, err = logging.Setup(logConf, []io.Writer{logOut}) + if err != nil { + return d, err + } + + for _, w := range warnings { + d.Logger.Warn(w) + } + + cfg.NodeID, err = newNodeIDFromConfig(cfg, d.Logger) + if err != nil { + return d, fmt.Errorf("failed to setup node ID: %w", err) + } + + d.TelemetrySink, err = lib.InitTelemetry(cfg.Telemetry) + if err != nil { + return d, fmt.Errorf("failed to initialize telemetry: %w", err) + } + + d.TLSConfigurator, err = tlsutil.NewConfigurator(cfg.ToTLSUtilConfig(), d.Logger) + if err != nil { + return d, err + } + + d.RuntimeConfig = cfg + d.Tokens = new(token.Store) + // cache-types are not registered yet, but they won't be used until the components are started. + d.Cache = cache.New(cfg.Cache) + d.ConnPool = newConnPool(cfg, d.Logger, d.TLSConfigurator) + + deferredAC := &deferredAutoConfig{} + + cmConf := new(certmon.Config). + WithCache(d.Cache). + WithTLSConfigurator(d.TLSConfigurator). + WithDNSSANs(cfg.AutoConfig.DNSSANs). + WithIPSANs(cfg.AutoConfig.IPSANs). + WithDatacenter(cfg.Datacenter). + WithNodeName(cfg.NodeName). + WithFallback(deferredAC.autoConfigFallbackTLS). + WithLogger(d.Logger.Named(logging.AutoConfig)). + WithTokens(d.Tokens). + WithPersistence(deferredAC.autoConfigPersist) + acCertMon, err := certmon.New(cmConf) + if err != nil { + return d, err + } + + acConf := autoconf.Config{ + DirectRPC: d.ConnPool, + Logger: d.Logger, + CertMonitor: acCertMon, + Loader: configLoader, + } + d.AutoConfig, err = autoconf.New(acConf) + if err != nil { + return d, err + } + // TODO: can this cyclic dependency be un-cycled? + deferredAC.autoConf = d.AutoConfig + + return d, nil +} + +func newConnPool(config *config.RuntimeConfig, logger hclog.Logger, tls *tlsutil.Configurator) *pool.ConnPool { + var rpcSrcAddr *net.TCPAddr + if !ipaddr.IsAny(config.RPCBindAddr) { + rpcSrcAddr = &net.TCPAddr{IP: config.RPCBindAddr.IP} + } + + pool := &pool.ConnPool{ + Server: config.ServerMode, + SrcAddr: rpcSrcAddr, + Logger: logger.StandardLogger(&hclog.StandardLoggerOptions{InferLevels: true}), + TLSConfigurator: tls, + Datacenter: config.Datacenter, + } + if config.ServerMode { + pool.MaxTime = 2 * time.Minute + pool.MaxStreams = 64 + } else { + pool.MaxTime = 127 * time.Second + pool.MaxStreams = 32 + } + return pool +} + +type deferredAutoConfig struct { + autoConf *autoconf.AutoConfig // TODO: use an interface +} + +func (a *deferredAutoConfig) autoConfigFallbackTLS(ctx context.Context) (*structs.SignedResponse, error) { + if a.autoConf == nil { + return nil, fmt.Errorf("AutoConfig manager has not been created yet") + } + return a.autoConf.FallbackTLS(ctx) +} + +func (a *deferredAutoConfig) autoConfigPersist(resp *structs.SignedResponse) error { + if a.autoConf == nil { + return fmt.Errorf("AutoConfig manager has not been created yet") + } + return a.autoConf.RecordUpdatedCerts(resp) +} diff --git a/agent/testagent.go b/agent/testagent.go index a946b32c46..0cf69fb9e4 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/go-hclog" uuid "github.com/hashicorp/go-uuid" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/config" @@ -167,38 +168,35 @@ func (a *TestAgent) Start(t *testing.T) (err error) { portsConfig, returnPortsFn := randomPortsSource(a.UseTLS) t.Cleanup(returnPortsFn) - nodeID := NodeID() - - opts := []AgentOption{ - WithLogger(logger), - WithBuilderOpts(config.BuilderOpts{ - HCL: []string{ - TestConfigHCL(nodeID), - portsConfig, - a.HCL, - hclDataDir, - }, - }), - WithOverrides(config.FileSource{ - Name: "test-overrides", - Format: "hcl", - Data: a.Overrides}, + // Create NodeID outside the closure, so that it does not change + testHCLConfig := TestConfigHCL(NodeID()) + loader := func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) { + opts := config.BuilderOpts{ + HCL: []string{testHCLConfig, portsConfig, a.HCL, hclDataDir}, + } + overrides := []config.Source{ + config.FileSource{ + Name: "test-overrides", + Format: "hcl", + Data: a.Overrides}, config.DefaultConsulSource(), config.DevConsulSource(), - ), + } + return config.Load(opts, source, overrides...) } + bd, err := NewBaseDeps(loader, logOutput) + require.NoError(t, err) - agent, err := New(opts...) + bd.Logger = logger + bd.TelemetrySink = metrics.NewInmemSink(1*time.Second, time.Minute) + a.Config = bd.RuntimeConfig + + agent, err := New(bd) if err != nil { return fmt.Errorf("Error creating agent: %s", err) } - a.Config = agent.GetConfig() - - agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) - id := string(a.Config.NodeID) - if err := agent.Start(context.Background()); err != nil { agent.ShutdownAgent() agent.ShutdownEndpoints() diff --git a/command/agent/agent.go b/command/agent/agent.go index 995b5529e1..60d493585f 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -60,11 +60,6 @@ type cmd struct { logger hclog.InterceptLogger } -type GatedUi struct { - JSONoutput bool - ui cli.Ui -} - func (c *cmd) init() { c.flags = flag.NewFlagSet("", flag.ContinueOnError) config.AddFlags(c.flags, &c.flagArgs) @@ -165,25 +160,26 @@ func (c *cmd) run(args []string) int { return 1 } - logGate := logging.GatedWriter{Writer: &cli.UiWriter{Ui: c.UI}} - - agentOptions := []agent.AgentOption{ - agent.WithBuilderOpts(c.flagArgs), - agent.WithCLI(c.UI), - agent.WithLogWriter(&logGate), - agent.WithTelemetry(true), + logGate := &logging.GatedWriter{Writer: &cli.UiWriter{Ui: c.UI}} + loader := func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) { + return config.Load(c.flagArgs, source) } - - agent, err := agent.New(agentOptions...) + bd, err := agent.NewBaseDeps(loader, logGate) if err != nil { c.UI.Error(err.Error()) return 1 } - config := agent.GetConfig() - c.logger = agent.GetLogger() + c.logger = bd.Logger + agent, err := agent.New(bd) + if err != nil { + c.UI.Error(err.Error()) + return 1 + } - //Setup gate to check if we should output CLI information + config := bd.RuntimeConfig + + // Setup gate to check if we should output CLI information cli := GatedUi{ JSONoutput: config.LogJSON, ui: c.UI, @@ -344,6 +340,11 @@ func (c *cmd) run(args []string) int { } } +type GatedUi struct { + JSONoutput bool + ui cli.Ui +} + func (g *GatedUi) output(s string) { if !g.JSONoutput { g.ui.Output(s) diff --git a/logging/grpc.go b/logging/grpc.go index eddc3cb50d..27264c3826 100644 --- a/logging/grpc.go +++ b/logging/grpc.go @@ -19,9 +19,9 @@ type GRPCLogger struct { // Note that grpclog has Info, Warning, Error, Fatal severity levels AND integer // verbosity levels for additional info. Verbose logs in glog are always INFO // severity so we map Info,V0 to INFO, Info,V1 to DEBUG, and Info,V>1 to TRACE. -func NewGRPCLogger(config *Config, logger hclog.Logger) *GRPCLogger { +func NewGRPCLogger(logLevel string, logger hclog.Logger) *GRPCLogger { return &GRPCLogger{ - level: config.LogLevel, + level: logLevel, logger: logger, } } diff --git a/logging/grpc_test.go b/logging/grpc_test.go index a97b2e590a..bd19f958e6 100644 --- a/logging/grpc_test.go +++ b/logging/grpc_test.go @@ -20,7 +20,7 @@ func TestGRPCLogger(t *testing.T) { Output: &out, TimeFormat: "timeformat", }) - grpclog.SetLoggerV2(NewGRPCLogger(&Config{LogLevel: "TRACE"}, logger)) + grpclog.SetLoggerV2(NewGRPCLogger("TRACE", logger)) // All of these should output something grpclog.Info("Info,") @@ -92,7 +92,7 @@ func TestGRPCLogger_V(t *testing.T) { Level: hclog.Trace, Output: &out, }) - grpclog.SetLoggerV2(NewGRPCLogger(&Config{LogLevel: tt.level}, logger)) + grpclog.SetLoggerV2(NewGRPCLogger(tt.level, logger)) assert.Equal(t, tt.want, grpclog.V(tt.v)) }) diff --git a/logging/logger.go b/logging/logger.go index bd530581cc..e9988ffb07 100644 --- a/logging/logger.go +++ b/logging/logger.go @@ -63,6 +63,7 @@ type LogSetupErrorFn func(string) // The provided ui object will get any log messages related to setting up // logging itself, and will also be hooked up to the gated logger. The final bool // parameter indicates if logging was set up successfully. +// TODO: accept a single io.Writer func Setup(config *Config, writers []io.Writer) (hclog.InterceptLogger, error) { if !ValidateLogLevel(config.LogLevel) { return nil, fmt.Errorf("Invalid log level: %s. Valid log levels are: %v", From 63bad36de70acdc1e0a8ba5d7de207ab1ecbba50 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 17 Aug 2020 14:12:04 -0400 Subject: [PATCH 2/2] testing: disable global metrics sink in tests This might be better handled by allowing configuration for the InMemSink interval and retail, and disabling the global. For now this is a smaller change to remove the goroutine leak caused by tests because go-metrics does not provide any way of shutting down the global goroutine. --- agent/acl_test.go | 10 +++++++--- agent/agent.go | 4 ++-- agent/config/runtime_test.go | 1 + agent/setup.go | 11 ++++++++--- agent/testagent.go | 10 +++++++--- lib/telemetry.go | 7 +++++++ 6 files changed, 32 insertions(+), 11 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index 63525f0397..a88d89273f 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -42,12 +42,16 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe dataDir := testutil.TempDir(t, "acl-agent") logBuffer := testutil.NewLogBuffer(t) - loader := func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) { + loader := func(source config.Source) (*config.RuntimeConfig, []string, error) { dataDir := fmt.Sprintf(`data_dir = "%s"`, dataDir) opts := config.BuilderOpts{ HCL: []string{TestConfigHCL(NodeID()), hcl, dataDir}, } - return config.Load(opts, source) + cfg, warnings, err := config.Load(opts, source) + if cfg != nil { + cfg.Telemetry.Disable = true + } + return cfg, warnings, err } bd, err := NewBaseDeps(loader, logBuffer) require.NoError(t, err) @@ -58,7 +62,7 @@ func NewTestACLAgent(t *testing.T, name string, hcl string, resolveAuthz authzRe Output: logBuffer, TimeFormat: "04:05.000", }) - bd.TelemetrySink = metrics.NewInmemSink(1*time.Second, time.Minute) + bd.MetricsHandler = metrics.NewInmemSink(1*time.Second, time.Minute) agent, err := New(bd) require.NoError(t, err) diff --git a/agent/agent.go b/agent/agent.go index 7af0f5d15a..bc5af6abcb 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -171,7 +171,7 @@ type Agent struct { logger hclog.InterceptLogger // In-memory sink used for collecting metrics - MemSink *metrics.InmemSink + MemSink MetricsHandler // delegate is either a *consul.Server or *consul.Client // depending on the configuration @@ -349,7 +349,7 @@ func New(bd BaseDeps) (*Agent, error) { tlsConfigurator: bd.TLSConfigurator, config: bd.RuntimeConfig, cache: bd.Cache, - MemSink: bd.TelemetrySink, + MemSink: bd.MetricsHandler, connPool: bd.ConnPool, autoConf: bd.AutoConfig, } diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index dee77376cc..3f51c6d9af 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -7100,6 +7100,7 @@ func TestSanitize(t *testing.T) { "CirconusCheckTags": "", "CirconusSubmissionInterval": "", "CirconusSubmissionURL": "", + "Disable": false, "DisableHostname": false, "DogstatsdAddr": "", "DogstatsdTags": [], diff --git a/agent/setup.go b/agent/setup.go index 9feb9d6f8e..9554ef7b5a 100644 --- a/agent/setup.go +++ b/agent/setup.go @@ -5,9 +5,9 @@ import ( "fmt" "io" "net" + "net/http" "time" - "github.com/armon/go-metrics" autoconf "github.com/hashicorp/consul/agent/auto-config" "github.com/hashicorp/consul/agent/cache" certmon "github.com/hashicorp/consul/agent/cert-monitor" @@ -28,7 +28,7 @@ import ( type BaseDeps struct { Logger hclog.InterceptLogger TLSConfigurator *tlsutil.Configurator // TODO: use an interface - TelemetrySink *metrics.InmemSink // TODO: use an interface + MetricsHandler MetricsHandler RuntimeConfig *config.RuntimeConfig Tokens *token.Store Cache *cache.Cache @@ -36,6 +36,11 @@ type BaseDeps struct { ConnPool *pool.ConnPool // TODO: use an interface } +// MetricsHandler provides an http.Handler for displaying metrics. +type MetricsHandler interface { + DisplayMetrics(resp http.ResponseWriter, req *http.Request) (interface{}, error) +} + type ConfigLoader func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) func NewBaseDeps(configLoader ConfigLoader, logOut io.Writer) (BaseDeps, error) { @@ -71,7 +76,7 @@ func NewBaseDeps(configLoader ConfigLoader, logOut io.Writer) (BaseDeps, error) return d, fmt.Errorf("failed to setup node ID: %w", err) } - d.TelemetrySink, err = lib.InitTelemetry(cfg.Telemetry) + d.MetricsHandler, err = lib.InitTelemetry(cfg.Telemetry) if err != nil { return d, fmt.Errorf("failed to initialize telemetry: %w", err) } diff --git a/agent/testagent.go b/agent/testagent.go index 0cf69fb9e4..8f05b6ed47 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -170,7 +170,7 @@ func (a *TestAgent) Start(t *testing.T) (err error) { // Create NodeID outside the closure, so that it does not change testHCLConfig := TestConfigHCL(NodeID()) - loader := func(source config.Source) (cfg *config.RuntimeConfig, warnings []string, err error) { + loader := func(source config.Source) (*config.RuntimeConfig, []string, error) { opts := config.BuilderOpts{ HCL: []string{testHCLConfig, portsConfig, a.HCL, hclDataDir}, } @@ -182,13 +182,17 @@ func (a *TestAgent) Start(t *testing.T) (err error) { config.DefaultConsulSource(), config.DevConsulSource(), } - return config.Load(opts, source, overrides...) + cfg, warnings, err := config.Load(opts, source, overrides...) + if cfg != nil { + cfg.Telemetry.Disable = true + } + return cfg, warnings, err } bd, err := NewBaseDeps(loader, logOutput) require.NoError(t, err) bd.Logger = logger - bd.TelemetrySink = metrics.NewInmemSink(1*time.Second, time.Minute) + bd.MetricsHandler = metrics.NewInmemSink(1*time.Second, time.Minute) a.Config = bd.RuntimeConfig agent, err := New(bd) diff --git a/lib/telemetry.go b/lib/telemetry.go index 8933c0c301..595e6005e8 100644 --- a/lib/telemetry.go +++ b/lib/telemetry.go @@ -19,6 +19,10 @@ import ( // the shared InitTelemetry functions below, but we can't import agent/config // due to a dependency cycle. type TelemetryConfig struct { + // Disable may be set to true to have InitTelemetry to skip initialization + // and return a nil MetricsSink. + Disable bool + // Circonus*: see https://github.com/circonus-labs/circonus-gometrics // for more details on the various configuration options. // Valid configuration combinations: @@ -326,6 +330,9 @@ func circonusSink(cfg TelemetryConfig, hostname string) (metrics.MetricSink, err // InitTelemetry configures go-metrics based on map of telemetry config // values as returned by Runtimecfg.Config(). func InitTelemetry(cfg TelemetryConfig) (*metrics.InmemSink, error) { + if cfg.Disable { + return nil, nil + } // Setup telemetry // Aggregate on 10 second intervals for 1 minute. Expose the // metrics over stderr when there is a SIGUSR1 received.