From ad13d026ec9030bead61cfdba6d3d8c9c492595a Mon Sep 17 00:00:00 2001 From: "fox.cpp" Date: Sat, 26 Oct 2019 21:05:46 +0300 Subject: [PATCH] Make endpoint modules special To support unusual configuration syntax, endpoint modules (imap, smtp, etc) relied on rather awkward code using modName+instName+aliases as arguments. This commit replaces old handling with use of special signature similar to inlineArgs introduced in 1edd031. Endpoint modules are placed in a separate 'registry' and use different initialization callback signature for simplicity. This makes them inaccessible for other modules, though they are not supposed to be anyway. Endpoint modules are initialized before other modules. This allows detecting unused configuration blocks by checking for modules that were not lazily initalized after endpoint initialization. This relies on endpoint modules being essentially "roots" of instances dependency tree. Idea of "semantical module names" is completely dropped now and so HACKING.md is updated to not mention it. --- HACKING.md | 23 ++++----- config/module/modconfig.go | 4 -- endpoint/imap/imap.go | 25 ++++------ endpoint/smtp/smtp.go | 24 ++++----- maddy.go | 69 +++---------------------- module.go | 100 +++++++++++++++++++++++++++++++++++++ module/instances.go | 3 -- module/module.go | 14 ++++++ module/registry.go | 34 ++++++++++++- signal.go | 10 +++- signal_nonposix.go | 2 +- 11 files changed, 190 insertions(+), 118 deletions(-) create mode 100644 module.go diff --git a/HACKING.md b/HACKING.md index 5a84cf8..dc2f49b 100644 --- a/HACKING.md +++ b/HACKING.md @@ -67,19 +67,14 @@ A single module instance can have one or more names. The first name is called called "aliases" and only used by module.GetInstance (e.g. module instance can be fetched by any name). -Some modules attach additional meaning to names. This is generally accepted -since it is better to have only a single instance managing one resource. For -example, module instance implementing forwarding to the downstream server can not -reasonably enforce any limitations unless it is only one instance "controlling" -that downstream. Unique names requirement helps a lot here. - -"Semantical names" idea explained above is not applied when modules instances -are defined "inline" (in place they are used in). These instances have no -instance names and are not added to the global map so they can not be accessed -by modules other than one that used ConfigFromNode on the corresponding config -block. All arguments after the module name in an inline definition represent -"inline arguments". They are passed to the module instance directly and not -used anyhow by other code (i.e. they are not guaranteed to be unique). +Some module instances are defined "inline" in user configuration. That is. +their configuration block is placed right where they are used. These instances +have no instance names and are not added to the global map so they can not be +accessed by modules other than one that used ConfigFromNode on the +corresponding config block. All arguments after the module name in an inline +definition represent "inline arguments". They are passed to the module instance +directly and not used anyhow by other code (i.e. they are not guaranteed to be +unique). ### A word on error logging @@ -99,4 +94,4 @@ together, remote module will provide logs about concrete errors happened and queue module will provide information about tries made and scheduled to be made in the future. -[1]: https://github.com/foxcpp/maddy/wiki/Dev:-Comments-on-design \ No newline at end of file +[1]: https://github.com/foxcpp/maddy/wiki/Dev:-Comments-on-design diff --git a/config/module/modconfig.go b/config/module/modconfig.go index 55d6f7d..a1e887a 100644 --- a/config/module/modconfig.go +++ b/config/module/modconfig.go @@ -14,7 +14,6 @@ import ( "github.com/foxcpp/maddy/config" "github.com/foxcpp/maddy/config/parser" - "github.com/foxcpp/maddy/log" "github.com/foxcpp/maddy/module" ) @@ -25,8 +24,6 @@ func createInlineModule(modName string, args []string) (module.Module, error) { return nil, fmt.Errorf("unknown module: %s", modName) } - log.Debugln("module create", modName, args, "(inline)") - return newMod(modName, "", nil, args) } @@ -35,7 +32,6 @@ func createInlineModule(modName string, args []string) (module.Module, error) { // // args must contain at least one argument, otherwise initInlineModule panics. func initInlineModule(modObj module.Module, globals map[string]interface{}, block *config.Node) error { - log.Debugln("module init", modObj.Name(), modObj.InstanceName(), "(inline)") return modObj.Init(config.NewMap(globals, block)) } diff --git a/endpoint/imap/imap.go b/endpoint/imap/imap.go index e66dce3..65abd0f 100644 --- a/endpoint/imap/imap.go +++ b/endpoint/imap/imap.go @@ -27,8 +27,7 @@ import ( ) type Endpoint struct { - name string - aliases []string + addrs []string serv *imapserver.Server listeners []net.Listener Auth module.AuthProvider @@ -41,16 +40,11 @@ type Endpoint struct { Log log.Logger } -func New(_, instName string, aliases, inlineArgs []string) (module.Module, error) { - if len(inlineArgs) != 0 { - return nil, errors.New("imap: inline arguments are not used") - } +func New(modName string, addrs []string) (module.Module, error) { endp := &Endpoint{ - name: instName, - aliases: aliases, - Log: log.Logger{Name: "imap"}, + addrs: addrs, + Log: log.Logger{Name: "imap"}, } - endp.name = instName return endp, nil } @@ -83,12 +77,11 @@ func (endp *Endpoint) Init(cfg *config.Map) error { return fmt.Errorf("imap: failed to init backend: nil update channel") } - args := append([]string{endp.name}, endp.aliases...) - addresses := make([]config.Endpoint, 0, len(args)) - for _, addr := range args { + addresses := make([]config.Endpoint, 0, len(endp.addrs)) + for _, addr := range endp.addrs { saddr, err := config.ParseEndpoint(addr) if err != nil { - return fmt.Errorf("imap: invalid address: %s", endp.name) + return fmt.Errorf("imap: invalid address: %s", addr) } addresses = append(addresses, saddr) } @@ -154,7 +147,7 @@ func (endp *Endpoint) Name() string { } func (endp *Endpoint) InstanceName() string { - return endp.name + return "imap" } func (endp *Endpoint) Close() error { @@ -204,7 +197,7 @@ func (endp *Endpoint) enableExtensions() error { } func init() { - module.Register("imap", New) + module.RegisterEndpoint("imap", New) imap.CharsetReader = message.CharsetReader } diff --git a/endpoint/smtp/smtp.go b/endpoint/smtp/smtp.go index 7e8e3e5..1307026 100644 --- a/endpoint/smtp/smtp.go +++ b/endpoint/smtp/smtp.go @@ -216,7 +216,7 @@ type Endpoint struct { Auth module.AuthProvider serv *smtp.Server name string - aliases []string + addrs []string listeners []net.Listener pipeline *msgpipeline.MsgPipeline resolver dns.Resolver @@ -232,20 +232,17 @@ type Endpoint struct { } func (endp *Endpoint) Name() string { - return "smtp" + return endp.name } func (endp *Endpoint) InstanceName() string { return endp.name } -func New(modName, instName string, aliases, inlineArgs []string) (module.Module, error) { - if len(inlineArgs) != 0 { - return nil, fmt.Errorf("%s: inline arguments are not used", modName) - } +func New(modName string, addrs []string) (module.Module, error) { endp := &Endpoint{ - name: instName, - aliases: aliases, + name: modName, + addrs: addrs, submission: modName == "submission", lmtp: modName == "lmtp", resolver: net.DefaultResolver, @@ -264,9 +261,8 @@ func (endp *Endpoint) Init(cfg *config.Map) error { endp.Log.Debugf("authentication provider: %s %s", endp.Auth.(module.Module).Name(), endp.Auth.(module.Module).InstanceName()) } - args := append([]string{endp.name}, endp.aliases...) - addresses := make([]config.Endpoint, 0, len(args)) - for _, addr := range args { + addresses := make([]config.Endpoint, 0, len(endp.addrs)) + for _, addr := range endp.addrs { saddr, err := config.ParseEndpoint(addr) if err != nil { return fmt.Errorf("smtp: invalid address: %s", addr) @@ -441,9 +437,9 @@ func (endp *Endpoint) Close() error { } func init() { - module.Register("smtp", New) - module.Register("submission", New) - module.Register("lmtp", New) + module.RegisterEndpoint("smtp", New) + module.RegisterEndpoint("submission", New) + module.RegisterEndpoint("lmtp", New) rand.Seed(time.Now().UnixNano()) } diff --git a/maddy.go b/maddy.go index b5b9c60..4746320 100644 --- a/maddy.go +++ b/maddy.go @@ -5,7 +5,6 @@ import ( "github.com/foxcpp/maddy/config" "github.com/foxcpp/maddy/log" - "github.com/foxcpp/maddy/module" // Import packages for side-effect of module registration. _ "github.com/foxcpp/maddy/auth/external" @@ -22,13 +21,7 @@ import ( _ "github.com/foxcpp/maddy/target/smtp_downstream" ) -type modInfo struct { - instance module.Module - cfg config.Node -} - func Start(cfg []config.Node) error { - instances := make(map[string]modInfo) globals := config.NewMap(nil, &config.Node{Children: cfg}) globals.String("hostname", false, false, "", nil) globals.String("autogenerated_msg_domain", false, false, "", nil) @@ -45,65 +38,17 @@ func Start(cfg []config.Node) error { defer log.DefaultLogger.Out.Close() - for _, block := range unmatched { - var instName string - var modAliases []string - if len(block.Args) == 0 { - instName = block.Name - } else { - instName = block.Args[0] - modAliases = block.Args[1:] - } - - modName := block.Name - - factory := module.Get(modName) - if factory == nil { - return config.NodeErr(&block, "unknown module: %s", modName) - } - - if module.HasInstance(instName) { - return config.NodeErr(&block, "config block named %s already exists", instName) - } - - log.Debugln("module create", modName, instName) - inst, err := factory(modName, instName, modAliases, nil) - if err != nil { - return err - } - - block := block - module.RegisterInstance(inst, config.NewMap(globals.Values, &block)) - for _, alias := range modAliases { - if module.HasInstance(alias) { - return config.NodeErr(&block, "config block named %s already exists", alias) - } - module.RegisterAlias(alias, instName) - log.Debugln("module alias", alias, "->", instName) - } - instances[instName] = modInfo{instance: inst, cfg: block} + insts, err := instancesFromConfig(globals.Values, unmatched) + if err != nil { + return err } - for _, inst := range instances { - if module.Initialized[inst.instance.InstanceName()] { - log.Debugln("module init", inst.instance.Name(), inst.instance.InstanceName(), "skipped because it was lazily initialized before") - continue - } + handleSignals() - module.Initialized[inst.instance.InstanceName()] = true - log.Debugln("module init", inst.instance.Name(), inst.instance.InstanceName()) - if err := inst.instance.Init(config.NewMap(globals.Values, &inst.cfg)); err != nil { - return err - } - } - - waitForSignal() - - for _, inst := range instances { - if closer, ok := inst.instance.(io.Closer); ok { - log.Debugln("clean-up for module", inst.instance.Name(), inst.instance.InstanceName()) + for _, inst := range insts { + if closer, ok := inst.(io.Closer); ok { if err := closer.Close(); err != nil { - log.Printf("module %s (%s) close failed: %v", inst.instance.Name(), inst.instance.InstanceName(), err) + log.Printf("module %s (%s) close failed: %v", inst.Name(), inst.InstanceName(), err) } } } diff --git a/module.go b/module.go new file mode 100644 index 0000000..9b770cb --- /dev/null +++ b/module.go @@ -0,0 +1,100 @@ +package maddy + +import ( + "github.com/foxcpp/maddy/config" + "github.com/foxcpp/maddy/log" + "github.com/foxcpp/maddy/module" +) + +type modInfo struct { + instance module.Module + cfg config.Node +} + +func instancesFromConfig(globals map[string]interface{}, nodes []config.Node) ([]module.Module, error) { + var ( + endpoints []modInfo + mods []modInfo + ) + + for _, block := range nodes { + var instName string + var modAliases []string + if len(block.Args) == 0 { + instName = block.Name + } else { + instName = block.Args[0] + modAliases = block.Args[1:] + } + + modName := block.Name + + endpFactory := module.GetEndpoint(modName) + if endpFactory != nil { + inst, err := endpFactory(modName, block.Args) + if err != nil { + return nil, err + } + + endpoints = append(endpoints, modInfo{instance: inst, cfg: block}) + continue + } + + factory := module.Get(modName) + if factory == nil { + return nil, config.NodeErr(&block, "unknown module: %s", modName) + } + + if module.HasInstance(instName) { + return nil, config.NodeErr(&block, "config block named %s already exists", instName) + } + + inst, err := factory(modName, instName, modAliases, nil) + if err != nil { + return nil, err + } + + block := block + module.RegisterInstance(inst, config.NewMap(globals, &block)) + for _, alias := range modAliases { + if module.HasInstance(alias) { + return nil, config.NodeErr(&block, "config block named %s already exists", alias) + } + module.RegisterAlias(alias, instName) + } + mods = append(mods, modInfo{instance: inst, cfg: block}) + } + + for _, endp := range endpoints { + if err := endp.instance.Init(config.NewMap(globals, &endp.cfg)); err != nil { + return nil, err + } + } + + // We initialize all non-endpoint modules defined at top-level + // just for purpose of checking that they have a valid configuration. + // + // Modules that are actually used will be pulled by lazy initialization + // logic during endpoint initialization. + for _, inst := range mods { + if module.Initialized[inst.instance.InstanceName()] { + continue + } + + log.Printf("%s (%s) is not used anywhere", inst.instance.InstanceName(), inst.instance.Name()) + + module.Initialized[inst.instance.InstanceName()] = true + if err := inst.instance.Init(config.NewMap(globals, &inst.cfg)); err != nil { + return nil, err + } + } + + res := make([]module.Module, 0, len(mods)+len(endpoints)) + for _, endp := range endpoints { + res = append(res, endp.instance) + } + for _, mod := range mods { + res = append(res, mod.instance) + } + return res, nil +} diff --git a/module/instances.go b/module/instances.go index 260b69f..9a30d2f 100644 --- a/module/instances.go +++ b/module/instances.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/foxcpp/maddy/config" - "github.com/foxcpp/maddy/log" ) var ( @@ -64,11 +63,9 @@ func GetInstance(name string) (Module, error) { // Break circular dependencies. if Initialized[name] { - log.Debugln("module init", mod.mod.Name(), name, "(lazy init skipped)") return mod.mod, nil } - log.Debugln("module init", mod.mod.Name(), name, "(lazy)") Initialized[name] = true if err := mod.mod.Init(mod.cfg); err != nil { return mod.mod, err diff --git a/module/module.go b/module/module.go index 90fb2de..82f86c9 100644 --- a/module/module.go +++ b/module/module.go @@ -55,3 +55,17 @@ type Module interface { // If module is defined inline, instName will be empty and all values // specified after module name in configuration will be in inlineArgs. type FuncNewModule func(modName, instName string, aliases, inlineArgs []string) (Module, error) + +// FuncNewEndpoint is a function that creates new instance of endpoint +// module. +// +// Compared to regular modules, endpoint module instances are: +// - Not registered in the global registry. +// - Can't be defined inline. +// - Don't have an unique name +// - All config arguments are always passed as an 'addrs' slice and not used as +// names. +// +// As a consequence of having no per-instance name, InstanceName of the module +// object always returns the same value as Name. +type FuncNewEndpoint func(modName string, addrs []string) (Module, error) diff --git a/module/registry.go b/module/registry.go index 7d1e280..31229f9 100644 --- a/module/registry.go +++ b/module/registry.go @@ -4,8 +4,11 @@ import ( "sync" ) -var modules = make(map[string]FuncNewModule) -var modulesLock sync.RWMutex +var ( + modules = make(map[string]FuncNewModule) + endpoints = make(map[string]FuncNewEndpoint) + modulesLock sync.RWMutex +) // Register adds module factory function to global registry. // @@ -26,6 +29,8 @@ func Register(name string, factory FuncNewModule) { // Get returns module from global registry. // +// This function does not return endpoint-type modules, use GetEndpoint for +// that. // Nil is returned if no module with specified name is registered. func Get(name string) FuncNewModule { modulesLock.RLock() @@ -33,3 +38,28 @@ func Get(name string) FuncNewModule { return modules[name] } + +// GetEndpoints returns an endpoint module from global registry. +// +// Nil is returned if no module with specified name is registered. +func GetEndpoint(name string) FuncNewEndpoint { + modulesLock.RLock() + defer modulesLock.RUnlock() + + return endpoints[name] +} + +// RegisterEndpoint registers an endpoint module. +// +// See FuncNewEndpoint for information about +// differences of endpoint modules from regular modules. +func RegisterEndpoint(name string, factory FuncNewEndpoint) { + modulesLock.Lock() + defer modulesLock.Unlock() + + if _, ok := endpoints[name]; ok { + panic("Register: module with specified name is already registered: " + name) + } + + endpoints[name] = factory +} diff --git a/signal.go b/signal.go index a7a1f7e..37ae5f0 100644 --- a/signal.go +++ b/signal.go @@ -10,7 +10,13 @@ import ( "github.com/foxcpp/maddy/log" ) -func waitForSignal() os.Signal { +// handleSignals function creates and listens on OS signals channel. +// +// OS-specific signals that correspond to the program termination +// (SIGTERM, SIGHUP, SIGINT) will cause this function to return. +// +// SIGUSR1 will call reinitLogging without returning. +func handleSignals() os.Signal { sig := make(chan os.Signal, 5) signal.Notify(sig, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGINT, syscall.SIGUSR1) @@ -21,7 +27,7 @@ func waitForSignal() os.Signal { reinitLogging() default: go func() { - s := waitForSignal() + s := handleSignals() log.Printf("forced shutdown due to signal (%v)!", s) os.Exit(1) }() diff --git a/signal_nonposix.go b/signal_nonposix.go index 8a96c06..45da4d2 100644 --- a/signal_nonposix.go +++ b/signal_nonposix.go @@ -10,7 +10,7 @@ import ( "github.com/foxcpp/maddy/log" ) -func waitForSignal() os.Signal { +func handleSignals() os.Signal { sig := make(chan os.Signal, 5) signal.Notify(sig, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGINT)