-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
Expose MQTT and STOMP endpoints #292
Conversation
servicePortsMap["mqtt"] = corev1.ServicePort{ | ||
Protocol: corev1.ProtocolTCP, | ||
Port: 1883, | ||
Name: "mqtt", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should STOMP-over-WebSockets and MQTT-over-WebSockets be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I think it makes sense.
However, I have been hesitant to include them because currently we hard-code the port numbers.
For example we hard code the MQTT port to its default 1883.
A user could override the MQTT listener port in the additional config which does not change the hard coded 1883 port in this K8s service object.
There are multiple solutions around this problem:
- We could prevent the user from overriding the listener port (e.g. within a K8s validating webhook)
- We could allow the user to override the listener port but make also sure that the user also overrides the client service with the same port
- We shouldn't hard code the port within the service definition. Instead, we should read the additional config, and if MQTT plugin is enabled + MQTT listener port is set in the additional config, then we set that port in this service definition (otherwise the default port)
I guess option 3 makes most sense. What do you think @michaelklishin ?
I'm just mentioning this issue because it seems very common to change the MQTT over WebSockets port:
When no configuration is specified the Web MQTT plugin will listen on all interfaces on port 15675 and have a default user login and password of guest/guest. Note that this user is only allowed to connect from localhost by default. We highly recommend creating a separate user production systems.
To change this, edit your Configuration file, to contain a port variable for the rabbitmq_web_mqtt application.
Therefore, it doesn't make sense to simply add another ServicePort
in the code snippet above with the hard coded default MQTT-over-WebSockets port 15675.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operator can provide a default. Option 3 makes most sense to me, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I suggest we can still merge this PR as-is.
I created #294 to respect ports in the config and to expose STOMP over WebSockets and MQTT over WebSockets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STOMP-over-WebSockets and MQTT-over-WebSockets are implemented by different plugins - rabbitmq_web_stomp
and rabbitmq_web_mqtt
. Given that in our current logic, we add the default port on the service and the container for a given additionalPlugin, why should it be any different for the web stomp and web mqtt plugins? They are just different plugins with a different default port. As for configuring a user, would the admin user we created not work here?
A potential problem is if the web versions of these plugins enable the non-web versions(rabbitmq_web_stomp
enabling rabbitmq_stomp
) and we would need to open the non-web version default port too in our container. If this is the case, we can encode this logic in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading above docs for Web MQTT my understanding was one has to change the listener port to revoke the guest/guest
access. I guess my understanding was wrong and / or the docs not clear. Created rabbitmq/rabbitmq-website#1044.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of override is to provide flexibility in our CRD for any uncommon use cases that we don't foresee. If someone were to override a default port, whether it's a plugin related port like STOMP, or AMQP, I assume that they know their use cases and know that they need to use both the sts override and the client service override. If we continue to see customizing certain ports as a common pattern across users, we could then consider adding support of customizing ports at the top level of CRD spec, or to read the additionalConfig like you suggested @ansd
Which plugins related ports we expose on our client service should be determined by how popular the plugins are. If we know that most of the users would likely to enable STOMP, I think adding special login to open a STOMP port makes sense. Furthermore, we can even consider enabling such plugins as a default plugins, like what we are already doing for management and prometheus.
I think this PR is fine for now. We can wait and see how many people are using MQTT and STOMP with customized ports, and only optimize it if it's a popular pattern. If the MQTT and STOMP websockets plugins are just as popular as MQTT and STOMP, I agree with @ferozjilla that we can add similar logic to set their port in our client service if enabled.
@michaelklishin how popular are MQTT and STOMP websockets plugins? Are you aware of any other popular plugins that requires additional port?
They are fairly popular but more importantly, they enable certain types of apps — the SPAs — to use RabbitMQ. Prometheus is the only non-protocol plugin (other than |
eb6399b
to
1498b9a
Compare
I rebased onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This closes #271 .