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.

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

Support Unix Domain Socket connections #146

Merged

Conversation

jpgrayson
Copy link
Contributor

The approach used for setting-up an MQTTClient to use a unix socket is similar to the approach used by mosquitto to configure listening on a unix socket; namely, setting the port to 0 indicates that the host string should be interpreted as a path to a socket.

From a semver perspective, this approach enables the unix domain sockets feature without breaking or otherwise changing an MQTTNIO interfaces. Prior to this change, specifying port 0 would have been an error, now it has meaning.

The docker-compose.yml files are updated to setup a shared "mosquitto-socket" volume. The mosquitto container will listen on a socket file in this volume. The "test"/"app" container also mounts this volume and thus can use the socket file found there to connect to mosquitto. The new unix socket configuration in the example mosquitto.conf is setup such that the socket path will work both with the docker-compose.yml setups as well as with a local (non-container) mosquitto instance.

The mosquitto/socket directory must be present in order to run a local mosquitto service with mosquitto/conf/mosquitto.conf since that configuration will listen on a socket in that directory. The readme.md
in that directory ensures its existence in git workspaces.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6c41888) 78.92% compared to head (205fc8e) 79.07%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   78.92%   79.07%   +0.15%     
==========================================
  Files          22       22              
  Lines        2500     2518      +18     
==========================================
+ Hits         1973     1991      +18     
  Misses        527      527              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Code changes look ok, there are a few issues with testing though.

  • Tests are not working when I run mosquitto via the shell script scripts/mosquitto.sh
  • For Github CI to work I need to publish a docker file to kick off mosquitto. The Dockerfile for that scripts/Dockerfile.ci will need updated. Before then CI will continue to fail.

docker-compose.yml Show resolved Hide resolved
Sources/MQTTNIO/MQTTConnection.swift Show resolved Hide resolved
mosquitto/socket/readme.md Outdated Show resolved Hide resolved
@jpgrayson
Copy link
Contributor Author

jpgrayson commented Nov 20, 2023

Code changes look ok, there are a few issues with testing though.

Thank you for taking a look at this PR. I really appreciate mqtt-nio.

Tests are not working when I run mosquitto via the shell script scripts/mosquitto.sh

I'm taking a look at this now. Sorry I missed it in the first go around.

The obstacle I'm running into is that it does not seem possible for a (linux) container to create a unix socket in a MacOS host's filesystem, which is what we would be asking the mosquitto.sh script to do. This makes intuitive sense because how could a linux unix socket exist in a MacOS host?

mosquitto reports:

14:53:04: Opening unix listen socket on path ./mosquitto/socket/mosquitto.sock.
14:53:04: Error binding unix socket: Not supported

For Github CI to work I need to publish a docker file to kick off mosquitto. The Dockerfile for that scripts/Dockerfile.ci will need updated. Before then CI will continue to fail.

For the CI case, the mosquitto container should be able to share the socket with the Linux host. I will try to get this working.


So there is one current test environment that will not work and several that do:

  • mosquitto.sh on MacOS host
  • mosquitto.sh on Linux host
  • docker-compose.yml on MacOS host
  • docker-compose.yml on Linux host
  • native mosquitto on MacOS host
  • native mosquitto on Linux host

Do you have a preferred approach for how to address the unix socket sharing problem with mosquitto.sh on MacOS?

One possibility would be to have mosquitto.sh run a native mosquitto instance instead of the current containerized approach. This may be palatable since docker-compose.yml provides a fully containerized test environment.

@jpgrayson jpgrayson force-pushed the unix-domain-sockets branch 2 times, most recently from ee3ba5f to 3d5faf8 Compare November 20, 2023 17:03
@jpgrayson
Copy link
Contributor Author

I've submitted #147 to help solve CI with unix sockets. That PR modifies the Ci workflow to restart the mosquitto service container after mqtt-nio is checked-out. This allows the upstream eclipse-mosquitto image to be used. And for unix sockets, the container restart allows the mosquitto socket to be available at the expected mqtt-nio/mosquitto/socket path via a volume mount.

I'm hoping #147 will be approved and merged. I'll then rebase and update this PR so that CI works with the unix domain socket feature.

@jpgrayson jpgrayson force-pushed the unix-domain-sockets branch 2 times, most recently from 085bc4e to 205fc8e Compare November 22, 2023 17:31
@jpgrayson
Copy link
Contributor Author

I think this PR is in pretty good shape at this point. Please let me know if you see any remaining obstacles. I appreciate the time you've taken to work with me on this, @adam-fowler.

@adam-fowler
Copy link
Collaborator

Do you want to rebase your other changes in and I think we can merge this after that

This is now the default behavior of `swift test`. And since
docker-compose.yml specifies the latest swift:5.9 image, there is not a
backword compatibility concern.

Signed-off-by: Peter Grayson <[email protected]>
 The approach used for setting-up an MQTTClient to use a unix socket is
 similar to the approach used by mosquitto to configure listening on a
 unix socket; namely, setting the port to 0 indicates that the `host`
 string should be interpreted as a path to a socket.

 From a semver perspective, this approach enables the unix domain sockets
 feature without breaking or otherwise changing an MQTTNIO interfaces.
 Prior to this change, specifying port 0 would have been an error, now it
 has meaning.

 The docker-compose.yml files are updated to setup a shared
 "mosquitto-socket" volume. The mosquitto container will listen on a
 socket file in this volume. The "test"/"app" container also mounts this
 volume and thus can use the socket file found there to connect to
 mosquitto. The new unix socket configuration in the example
 mosquitto.conf is setup such that the socket path will work both with
 the docker-compose.yml setups as well as with a local (non-container)
 mosquitto instance.

 The mosquitto/socket directory must be present in order to run a local
 mosquitto service with mosquitto/conf/mosquitto.conf since that
 configuration will listen on a socket in that directory. The .gitkeep
 in that directory ensures its existence in git workspaces.

Signed-off-by: Peter Grayson <[email protected]>
This new convenience initializer has a `unixSocketPath` parameter
instead of `host` and `port` to make setting up an MQTTClient with a
unix socket somewhat less icky.

N.B. to maintain compatibility, the public MQTTClient.host and
MQTTClient.port properties still must be the unix socket path and port
0, respectively.

Signed-off-by: Peter Grayson <[email protected]>
@adam-fowler
Copy link
Collaborator

So the homebrew version of mosquitto doesn't work out of the box
It appears you have to create a link to mosquitto manually as homebrew doesn't do it for you.

Copy link
Collaborator

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

I think this can go in now.
I'll add a comment to add the symbolic link for mosquitto. Unless you have some other method to ensure it is in the path

@jpgrayson
Copy link
Contributor Author

So the homebrew version of mosquitto doesn't work out of the box It appears you have to create a link to mosquitto manually as homebrew doesn't do it for you.

The mosquitto executable lands in /opt/homebrew/sbin, so it works out of the box if that path is in $PATH. I'm using an M1 Macbook. Ostensibly that would be /usr/local/sbin on an x86-64 Macbook.

The brew shellenv command, which the installer indicates to use to setup paths, does include both the bin and sbin paths.

So it seems like mosquitto should be available if the homebrew paths were setup correctly.

If we were to add a comment somewhere, we may want to reference brew shellenv for getting $PATH setup correctly instead of adding a symlink.

My vote is to merge this PR as-is and add any comments/docs for this mosquitto path issue in another change.

@adam-fowler adam-fowler merged commit a370d9e into swift-server-community:main Nov 28, 2023
8 checks passed
@jpgrayson jpgrayson deleted the unix-domain-sockets branch November 28, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants