[WIP] Basic File Configuration

[dgduncan]

File Based Configuration

Add basic file based configuration along with the boilerplate to add future methods of configuration (config server anybody?) This included the following enhancements:

• "Smart Defaults" (TM)
• Allow-All hook
• TCP
• Websocket
• Stats

@mochi-co As part of this PR, I would also like to update the Docker README with some configuration instructions along with ways mount files. Is DockerHub parsing the README or is it currently set to manual and thusly must be updated on DockerHub?

@mochi-co @werbenhu @thedevop With this change, I updated how defaults for the server are set. Previously, when any Capability would be defined, the default values were not used. Instead, the server would use the defined Capabilities as prescribed and use the zero values of everything not explicitly defined. This caused things such as MaximumMessageExpiryInterval and ReceiveMaximum to be set to 0 instead of their defaults for example. This change to properly use the default makes sense to me; however, I want to make sure this change makes sense as per the MQTT spec. This change breaks two tests and I do not want to go changing how those tests work until I know this change makes sense as per the spec and is wanted by other maintainers.

[werbenhu]

@dgduncan Thanks. You have done a great job, and I quickly reviewed the code. I have a small suggestion: shouldn't it be possible to bind multiple ports for the same type of listener? It might be a good idea to expose the listenerID for configuration in a file. For example, I have two TCP listeners: one with a Listener ID of "t1," bound to port 1883, and another with a Listener ID of "t2," bound to port 8885. "t2" also supports TLS. In my use case, this is a very common scenario.

[dgduncan]

@dgduncan Thanks. You have done a great job, and I quickly reviewed the code. I have a small suggestion: shouldn't it be possible to bind multiple ports for the same type of listener? It might be a good idea to expose the listenerID for configuration in a file. For example, I have two TCP listeners: one with a Listener ID of "t1," bound to port 1883, and another with a Listener ID of "t2," bound to port 8885. "t2" also supports TLS. In my use case, this is a very common scenario.

Do you mean have one endpoint that is non-TLS and another that is TLS? Such as 1883/8883? If so, this initial one I only have non-tls. I have a branch up as well that uses TLS and allows you to have a non-tls and a tls port.

[mochi-co]

@mochi-co As part of this PR, I would also like to update the Docker README with some configuration instructions along with ways mount files. Is DockerHub parsing the README or is it currently set to manual and thusly must be updated on DockerHub?

I haven't been able to find a way to get DockerHub to parse anything additional, it's possible this is an upgrade option. Feel welcome to write any instructions in Discussions or an issue and I will update the docker repo 🙂

For your second point, I will need to investigate a bit 👍🏻

[mochi-co]

@dgduncan Thanks. You have done a great job, and I quickly reviewed the code. I have a small suggestion: shouldn't it be possible to bind multiple ports for the same type of listener? It might be a good idea to expose the listenerID for configuration in a file. For example, I have two TCP listeners: one with a Listener ID of "t1," bound to port 1883, and another with a Listener ID of "t2," bound to port 8885. "t2" also supports TLS. In my use case, this is a very common scenario.

I agree with this also. Listeners could be an array, something like:

tcp:
  - id: t1
     port: 1882
  - id: t2
     port: 1885
     tls: <some tls config data>
ws: 
  - id: ws1
     port: 1883
etc.

[mochi-co]

Here's a potentially controversial idea. It's not necessarily a good idea, but I want to throw it out here for some discussion.

• Listeners are always set before server start. There is currently no facility for new listeners to be added or removed after the server has started (although this could be added if someone actually wanted it).
• Listeners are all configured using the Listeners.Config struct, and are somewhat predictable, they all take an id, an address, and a Config (containing TLS). HTTPStats also has a *system.Info pointer.
• Listeners could, in fact, be incorporated into the server.Options struct and configured on server start.

// Options contains configurable options for the server.
type Options struct {
	Listeners []listeners.Config
	Capabilities *Capabilities
	ClientNetWriteBufferSize int
	ClientNetReadBufferSize int
	Logger *slog.Logger
	SysTopicResendInterval int64
	InlineClient bool
}

func (s *Server) AddListenersFromConfig(configs []listeners.Config) error {
	for _, conf := range configs {
		var l listeners.Listener
		switch conf.Type {
			case listeners.TypeTCP:
			l = listeners.NewTCP(conf.ID, conf.Address, conf.Config)
			case listeners.TypeSysInfo:
			l = listeners.NewHTTPSysInfo(conf.ID, conf.Address, conf.Config, s.Info)
			// etc...
		}
		if err := s.AddListener(l); err != nil {
			return err
		}
	}
	return nil
}

And then we add a new DefaultServerOptions which includes all of the values required above. In theory it would also allow us to continue using the existing AddListener system too, which would allow non-core listeners to continue being used. There are some other bits we'd need to do as well, but what do you all think about this as a concept?

[mochi-co]

Edit：here's a runnable patch which implements the above. It would need a bit of finessing if we went in this direction, but it illustrates the concept. We would be able to translate yaml listener configs detailed above directly into mochi listeners.

diff --git forkSrcPrefix/examples/configlisteners/main.go forkDstPrefix/examples/configlisteners/main.go
new file mode 100644
index 0000000000000000000000000000000000000000..c2028bf144f1c79c36769e34e30e28d8de117cd8
--- /dev/null
+++ forkDstPrefix/examples/configlisteners/main.go
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: MIT
+// SPDX-FileCopyrightText: 2022 mochi-mqtt, mochi-co
+// SPDX-FileContributor: mochi-co
+
+package main
+
+import (
+	"log"
+	"os"
+	"os/signal"
+	"syscall"
+
+	mqtt "github.com/mochi-mqtt/server/v2"
+	"github.com/mochi-mqtt/server/v2/hooks/auth"
+	"github.com/mochi-mqtt/server/v2/listeners"
+)
+
+func main() {
+	sigs := make(chan os.Signal, 1)
+	done := make(chan bool, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
+	go func() {
+		<-sigs
+		done <- true
+	}()
+
+	options := &mqtt.Options{
+		Listeners: []listeners.Config{
+			{
+				Type:    listeners.TypeTCP,
+				ID:      "t1config",
+				Address: ":1883",
+			},
+		},
+	}
+
+	server := mqtt.New(options)
+	err := server.AddHook(new(auth.AllowHook), nil)
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	go func() {
+		err := server.Serve()
+		if err != nil {
+			log.Fatal(err)
+		}
+	}()
+
+	<-done
+	server.Log.Warn("caught signal, stopping...")
+	_ = server.Close()
+	server.Log.Info("main.go finished")
+}
diff --git forkSrcPrefix/listeners/tcp.go forkDstPrefix/listeners/tcp.go
index 1682734ace84553b83a97298902b69ca3483ac9c..ce58c2c287d2d4e8a29fb0540eeb0a14b4bf74d4 100644
--- forkSrcPrefix/listeners/tcp.go
+++ forkDstPrefix/listeners/tcp.go
@@ -13,27 +13,25 @@ import (
 	"log/slog"
 )
 
+const TypeTCP = "TCP"
+
 // TCP is a listener for establishing client connections on basic TCP protocol.
 type TCP struct { // [MQTT-4.2.0-1]
 	sync.RWMutex
 	id      string       // the internal id of the listener
 	address string       // the network address to bind to
 	listen  net.Listener // a net.Listener which will listen for new clients
-	config  *Config      // configuration values for the listener
+	config  Config       // configuration values for the listener
 	log     *slog.Logger // server logger
 	end     uint32       // ensure the close methods are only called once
 }
 
 // NewTCP initialises and returns a new TCP listener, listening on an address.
-func NewTCP(id, address string, config *Config) *TCP {
-	if config == nil {
-		config = new(Config)
-	}
-
+func NewTCP(config Config) *TCP {
 	return &TCP{
-		id:      id,
-		address: address,
 		config:  config,
+		id:      config.ID,
+		address: config.Address,
 	}
 }
 
diff --git forkSrcPrefix/listeners/listeners.go forkDstPrefix/listeners/listeners.go
index 301dd56ed209eba8c5e8e1287ecc77751eec74af..0ba3423e7559e7bcd155bcb566e08f497900da60 100644
--- forkSrcPrefix/listeners/listeners.go
+++ forkDstPrefix/listeners/listeners.go
@@ -14,6 +14,10 @@ import (
 
 // Config contains configuration values for a listener.
 type Config struct {
+	Type    string
+	ID      string
+	Address string
+
 	// TLSConfig is a tls.Config configuration to be used with the listener.
 	// See examples folder for basic and mutual-tls use.
 	TLSConfig *tls.Config
diff --git forkSrcPrefix/server.go forkDstPrefix/server.go
index 8b7b46da77734338be58126635a96576dd270909..705163b7dfa32bf857fdbf0ac037d7f2644c11f6 100644
--- forkSrcPrefix/server.go
+++ forkDstPrefix/server.go
@@ -83,6 +83,8 @@ type Compatibilities struct {
 
 // Options contains configurable options for the server.
 type Options struct {
+	Listeners []listeners.Config
+
 	// Capabilities defines the server features and behaviour. If you only wish to modify
 	// several of these values, set them explicitly - e.g.
 	// 	server.Options.Capabilities.MaximumClientWritesPending = 16 * 1024
@@ -240,6 +242,20 @@ func (s *Server) NewClient(c net.Conn, listener string, id string, inline bool)
 	return cl
 }
 
+func (s *Server) AddListenersFromConfig(configs []listeners.Config) error {
+	for _, conf := range configs {
+		var l listeners.Listener
+		switch conf.Type {
+		case listeners.TypeTCP:
+			l = listeners.NewTCP(conf)
+		}
+		if err := s.AddListener(l); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // AddHook attaches a new Hook to the server. Ideally, this should be called
 // before the server is started with s.Serve().
 func (s *Server) AddHook(hook Hook, config any) error {
@@ -276,6 +292,13 @@ func (s *Server) Serve() error {
 	s.Log.Info("mochi mqtt starting", "version", Version)
 	defer s.Log.Info("mochi mqtt server started")
 
+	if len(s.Options.Listeners) > 0 {
+		err := s.AddListenersFromConfig(s.Options.Listeners)
+		if err != nil {
+			return err
+		}
+	}
+
 	if s.hooks.Provides(
 		StoredClients,
 		StoredInflightMessages,

[dgduncan]

@dgduncan Thanks. You have done a great job, and I quickly reviewed the code. I have a small suggestion: shouldn't it be possible to bind multiple ports for the same type of listener? It might be a good idea to expose the listenerID for configuration in a file. For example, I have two TCP listeners: one with a Listener ID of "t1," bound to port 1883, and another with a Listener ID of "t2," bound to port 8885. "t2" also supports TLS. In my use case, this is a very common scenario.

I agree with this also. Listeners could be an array, something like:

tcp:
  - id: t1
     port: 1882
  - id: t2
     port: 1885
     tls: <some tls config data>
ws: 
  - id: ws1
     port: 1883
etc.

I really like this idea. A much cleaner implementation.

[dgduncan]

I've made a few suggestions, but don't worry, they're mostly only minor things and some style issues that I encountered before reaching an assessment.

Generally there's a lot to like, and on the whole I think what you've done here makes sense. I like that the yaml tags are directly on the server capabilities and options, that helps keep a single point of truth. The changes to ensureDefaults are somewhat unavoidable, but there's precedent for this sort of thing already in packets/packets.go and packets/properties.go. I also really like the logging configure method, conceptually.

Having said that, I am a little uneasy about the idea of listener configuration in the configs/default.go - it feels like we're adding multiple layers of abstraction. My other concern is that we are making what is effectively struct based configuration a second-class citizen by locking it behind a server, err := file.Configure() call. The reason this feels weird is because we already configure the server with a pointer to an Options struct, which now already has yaml tags on it directly.

At the same time, I can see why you did what you did - I think it makes sense in context, and it's well structured and thought out - and as I write this, I haven't got an alternative proposal or solution in mind.

My gut feeling is that we should try to avoid abstracting the server configuration. If you'll allow me, I'd like to think about this a bit and see if we can look a the problem from a different angle.

It's funny I actually originally had it all outside of config as I felt the same as you. I'll pull that out and bring it back to the main.go of the docker main.

[coveralls]

Pull Request Test Coverage Report for Build 6569741509

• 160 of 164 (97.56%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.04%) to 98.869%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
configs/file/file.go	91	95	95.79%
<!--	Total:	160	164

Totals
Change from base Build 6536886378:	-0.04%
Covered Lines:	5594
Relevant Lines:	5658

---

💛 - Coveralls

[mochi-co]

Changes implemented in #351 following this investigations - thank you very much!