Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CHANGED] MQTT no longer requires server name to be set #4679

Merged
merged 4 commits into from
Oct 25, 2023
Merged

[CHANGED] MQTT no longer requires server name to be set #4679

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1543e69
[CHANGED] MQTT no longer requires server name to be set
levb Oct 19, 2023
bf263d1
Back to requiring the name, in cluster mode only
levb Oct 20, 2023
18dd159
nit: removed debugging leftover
levb Oct 20, 2023
3e71a18
require a name for gateways
levb Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions server/mqtt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ var (
errMQTTTopicFilterCannotBeEmpty = errors.New("topic filter cannot be empty")
errMQTTMalformedVarInt = errors.New("malformed variable int")
errMQTTSecondConnectPacket = errors.New("received a second CONNECT packet")
errMQTTServerNameMustBeSet = errors.New("mqtt requires server name to be explicitly set")
errMQTTUserMixWithUsersNKeys = errors.New("mqtt authentication username not compatible with presence of users/nkeys")
errMQTTTokenMixWIthUsersNKeys = errors.New("mqtt authentication token not compatible with presence of users/nkeys")
errMQTTAckWaitMustBePositive = errors.New("ack wait must be a positive value")
Expand Down Expand Up @@ -598,11 +597,6 @@ func validateMQTTOptions(o *Options) error {
if mo.Port == 0 {
return nil
}
// We have to force the server name to be explicitly set. There are conditions
// where we need a unique, repeatable name.
if o.ServerName == _EMPTY_ {
Copy link
Member

@derekcollison derekcollison Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be ok for single server mode, but not clustered mode. In clustered mode this is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought @bruth's point was that a (unique?) name will be auto-defaulted, not? I don't see any direct references to it in the MQTT code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Peer names, which need to be consistent, are a derived hash from server names. So in clustered mode this condition has to hold. So we need to check if we are in single server mode, or not using JetStream, although I thought it was required for MQTT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS is required for MQTT; I am 0/5 if it makes any sense to remove the name requirement for single server only. @bruth?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a js helper function to test if in single server mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, unfortunately it just checks that the run-time object is not nil, that it has initialized. This one, https://github.com/nats-io/nats-server/blob/main/server/jetstream_cluster.go#L724?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// Determines if this server is in standalone mode, meaning no routes or gateways.
func (s *Server) standAloneMode() bool {
	opts := s.getOpts()
	return opts.Cluster.Port == 0 && opts.Gateway.Port == 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekcollison Can you please confirm

return errMQTTServerNameMustBeSet
}
// If there is a NoAuthUser, we need to have Users defined and
// the user to be present.
if mo.NoAuthUser != _EMPTY_ {
Expand Down
15 changes: 0 additions & 15 deletions server/mqtt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,21 +373,6 @@ func testMQTTDefaultTLSOptions(t *testing.T, verify bool) *Options {
return o
}

func TestMQTTServerNameRequired(t *testing.T) {
conf := createConfFile(t, []byte(`
mqtt {
port: -1
}
`))
o, err := ProcessConfigFile(conf)
if err != nil {
t.Fatalf("Error processing config file: %v", err)
}
if _, err := NewServer(o); err == nil || err.Error() != errMQTTServerNameMustBeSet.Error() {
t.Fatalf("Expected error about requiring server name to be set, got %v", err)
}
}

func TestMQTTStandaloneRequiresJetStream(t *testing.T) {
conf := createConfFile(t, []byte(`
server_name: mqtt
Expand Down
Loading