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

App submission mosquitto #1789

Merged
merged 32 commits into from
Dec 18, 2024
Merged

App submission mosquitto #1789

merged 32 commits into from
Dec 18, 2024

Conversation

dirstel
Copy link
Contributor

@dirstel dirstel commented Nov 11, 2024

App Submission

App name

Eclipse Mosquitto

256x256 SVG icon

logo

Gallery images

image
(not really much to screenshot)

I have tested my app on:

  • umbrelOS on a Raspberry Pi
  • umbrelOS on an Umbrel Home
  • umbrelOS on Linux VM

@nmfretz
Copy link
Contributor

nmfretz commented Nov 13, 2024

Thanks for another submission @dirstel! We'll try to get to this early next week. Thanks for your patience.

Copy link
Contributor

@nmfretz nmfretz left a comment

Choose a reason for hiding this comment

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

Nice work repurposing the Samba/Snowflake index.html to give the app a web UI with instructions @dirstel 👌

Can mosquitto even serve the html page via it's websocket port though? I think that port is just so that mqtt clients can connect using websockets. I tried quickly installing this on my umbrel and nothing shows up.

Regardless, I've got some suggestions below on how to serve the UI and make your life much easier when updating this app in the future.

Package the UI as a separate Docker image

The way umbrelOS currently works is that on app install, everything from the app's repo is copied over to a users app-data directory. But on app updates only a whitelist of files is copied over so that user data is not overwritten:
https://github.com/getumbrel/umbrel/blob/49d37581c8233f808fc5a79c2cd38810c6cd1b0b/packages/umbreld/source/modules/apps/legacy-compat/app-script#L594-L595

What that means for you here is that if you need to make changes to your UI (e.g., changing the instructions in the html), only new installs would receive the changes. Existing installs that update won't get the changes. You would need to resort to some really hacky things like using a hook that runs on app update and makes modifications to the html/image files. It would end up adding a lot of complexity and overhead for you.

Instead though, you can just package the UI as a super simple Docker image that serves your html and then point the app_proxy service to it. That way if you ever need to update the UI, you just build and push a new Docker image to your repo and then change it in the docker-compose.yml when submitting an app update.

I have not tested this at all, but something as simple as this could work for a Dockerfile:

# Use the joseluisq/static-web-server as the base image
FROM joseluisq/static-web-server:2.33.1

# Copy the directory with your ui web assets to the /var/public directory which is set by the SERVER_ROOT env var.
# e.g., if your directory is called `public`:
COPY public /var/public

You'll then need to make sure to build a multi-architecture image for amd64 and arm64. You'll run something like this:

docker buildx build --platform linux/amd64,linux/arm64 -t your-docker-hub-username/static-web-server-mosquitto:version-tag .

Comment on lines 7 to 10
# websockets listener (including webserver)
listener 9001
protocol websockets
http_dir /mosquitto/www
Copy link
Contributor

Choose a reason for hiding this comment

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

If the webserver ends up as its own Docker service, this needs to be modified.

But I'm not sure it works like this anyways. Really sorry if I'm totally wrong here!

http_dir /mosquitto/www

# security
allow_anonymous true
Copy link
Contributor

Choose a reason for hiding this comment

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

For this use-case, are we good to allow this? Or should this require authentication?

@@ -0,0 +1,18 @@
services:
Copy link
Contributor

Choose a reason for hiding this comment

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

This docker-compose file and the umbrel-app manifest file need to have the .yml suffix instead of .yaml ,otherwise they won't render in the app store and you won't be able to install them via the UI or CLI. Were you still able to install it somehow?

environment:
APP_HOST: mosquitto_broker_1
APP_PORT: 9001
container_name: mosquitto_app_proxy_1
Copy link
Contributor

Choose a reason for hiding this comment

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

The container_name for the app_proxy and broker services can be removed.

Comment on lines 10 to 12
ports:
- '1883:1883'
- '9001:9001'
Copy link
Contributor

Choose a reason for hiding this comment

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

Port 1883 is free, but 9001 is used by Penpot. You could change to 9991:9001 and update the instructions accordingly.

Comment on lines 13 to 17
volumes:
- ${APP_DATA_DIR}/config:/mosquitto/config:rw
- ${APP_DATA_DIR}/data:/mosquitto/data:rw
- ${APP_DATA_DIR}/log:/mosquitto/log:rw
- ${APP_DATA_DIR}/www:/mosquitto/www:r
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep everything under one data directory like other apps I'd suggest going with something like this:

data
├── mosquitto-data
│   └── .gitkeep
├── config
│   └── mosquitto.conf.default
└── log
    └── .gitkeep

And then if serving the UI from a Docker container as described above, just get rid of - ${APP_DATA_DIR}/www:/mosquitto/www:r

Comment on lines 14 to 15
# set correct owner
chown umbrel:umbrel "${CONFIG_FILE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice hook 🫡

Looking at the mosquitto Dockerfile, we're actually stuck running the container as user:group 1883:1883 since they have hardcoded this into the image unfortunately. I think this should still work as-is though.

You can see that all data directories for mosquitto end up owned by 1883

:~$ ls -l ~/umbrel/app-data/mosquitto/
total 20
drwxr-xr-x 1   1883   1883   72 Nov 28 05:14 config
drwxr-xr-x 1   1883   1883    0 Nov 28 02:04 data
-rw-r--r-- 1 umbrel umbrel  574 Nov 28 04:59 docker-compose.yaml
-rw-r--r-- 1 umbrel umbrel  583 Nov 28 05:14 docker-compose.yml
drwxr-xr-x 1 umbrel umbrel   18 Nov 28 05:14 hooks
drwxr-xr-x 1   1883   1883    0 Nov 28 02:04 log
-rw-r--r-- 1 root   root     17 Nov 28 05:14 settings.yml
-rw-r--r-- 1 umbrel umbrel 1008 Nov 28 04:59 umbrel-app.yaml
-rw-r--r-- 1 umbrel umbrel 1008 Nov 28 04:59 umbrel-app.yml
drwxr-xr-x 1   1883   1883   58 Nov 28 02:04 www

id: mosquitto
name: Mosquitto
tagline: An open source MQTT broker
icon: https://raw.githubusercontent.com/eclipse-mosquitto/mosquitto/refs/heads/master/logo/mosquitto-logo-only.svg
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire icon key-value pair can be deleted. We'll host the icon at https://github.com/getumbrel/umbrel-apps-gallery

<!DOCTYPE html>
<!-- saved from url=(0024)http://server.home:9445/ -->
<html><head><meta http-equiv="Content-Type" content="text/html; charset=windows-1252">
<title>Samba</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can change to Mosquitto

…default

renamed:    data/.gitkeep -> data/mosquitto/.gitkeep
renamed:    log/.gitkeep -> data/log/.gitkeep
renamed:    www/favicon.png -> data/www/favicon.png
renamed:    www/index.html -> data/www/index.html
renamed:    www/logo.png -> data/www/logo.png
renamed:    docker-compose.yaml -> docker-compose.yml
renamed:    umbrel-app.yaml -> umbrel-app.yml
modified:   data/www/index.html
modified:   docker-compose.yml
modified:   hooks/pre-start
modified:   umbrel-app.yml
@dirstel
Copy link
Contributor Author

dirstel commented Nov 28, 2024

Addressed the following concerns:

  • ports: 9991 is used for MQTT via WebService / Webserver
  • app-port changed to 9021 (9011 is also used - is there a list of all used ports so far?)
  • fixed directories
  • fixed file-suffixes
  • fixed user by explizit setting in docker-compose.yml

Allow anonymous MQTT-access
The MQTT-Service is usuable without permissions. When exposing the service to the internet this should definetly be changed. as there are lots of possibilities (one user, user per client/sensor, acl, aditonal plugins, ...) I think this should not be set in default installation. Maybe a hint or two in the webpage can be added.

Serving webpage by dedicated container
First, Mosquitto can serve a webpage on its own.
Second the availability of the webpage is a decent indicator of Moswuitto really running.
I agree with you in terms of updates. Maybe there should be a detailed description of what is done by an update? As far as I can see, the hook used is not needed too. In general, headless services should be given the opportunity to display a webpage by umbrel or the app-proxy. In my opinion it is somewhat cumbersome to build a dedicated container for each of this app.

added zigbee2mqtt/data/configuration.yaml.default
added zigbee2mqtt/docker-compose.yml
added zigbee2mqtt/hooks/pre-start
added zigbee2mqtt/umbrel-app.yml
@nmfretz
Copy link
Contributor

nmfretz commented Dec 10, 2024

Thanks for making those changes @dirstel.

First, Mosquitto can serve a webpage on its own.
Second the availability of the webpage is a decent indicator of Moswuitto really running.

Thanks for the clarification. With your changes the webpage is now being served correctly for me by mosquitto on app install.

The MQTT-Service is usuable without permissions. When exposing the service to the internet this should definetly be changed. as there are lots of possibilities (one user, user per client/sensor, acl, aditonal plugins, ...) I think this should not be set in default installation. Maybe a hint or two in the webpage can be added.

Adding a hint to the UI is a great idea. While you're doing that you may want to also change the following:

  1. accessinformation --> access information
  2. Settings > Advanced Settings > Terminal --> Settings > Advanced settings > Terminal > umbrelOS
  3. nano umbrel/app-data/... --> sudo nano ~/umbrel/app-data/...

I agree with you in terms of updates. Maybe there should be a detailed description of what is done by an update?

I agree with you. We are redoing the apps framework and will make sure to have detailed descriptions for this new framework.

Under the current framework, an app update copies over the following dirs/files:
https://github.com/getumbrel/umbrel/blob/49d37581c8233f808fc5a79c2cd38810c6cd1b0b/packages/umbreld/source/modules/apps/legacy-compat/app-script#L594-L595
docker-compose.yml *.template exports.sh torrc hooks/ umbrel-app.yml

Any other files/dirs are not copied over on update. As an example, the data directory in this mosquitto submission won't be copied over meaning that a users data/settings won't get clobbered when they update.

As far as I can see, the hook used is not needed too.

That's correct. If all you are doing in the hook is creating the config, then you can just delete the entire hooks/ directory and rename the default config file to mosquitto.conf.default.

In general, headless services should be given the opportunity to display a webpage by umbrel or the app-proxy. In my opinion it is somewhat cumbersome to build a dedicated container for each of this app.

We're redoing the apps framework, so will take your suggestion under advisement 🤝.

The idea with the current framework is that an app should not be tied to umbrelOS. The manifest (umbrel-app.yml) is umbrelOS-specific, but the actual app containers themselves are not. The app_proxy is providing transparent proxying and auth (password, 2fa). If the app_proxy were to provide a way for headless services to include a webUI then we would be dictating how an app needs to be developed (umbrelOS-specific) instead of just having services be containerized and thus doing anything they want to create a UI.

App functionality

Can you please do some additional testing to make sure this is working correctly for you? Unfortunately my time is limited for the next while, so I can't spend too much time troubleshooting.

I just quickly tested mosquitto as of your commit ac6824c. The webUI works for me, but I cannot connect via MQTTX Web using port 9991. This is because your compose file is exposing 9991 on the host and binding to internal port 9001 in the container. So when you try to connect to something like http://umbrel.local:9991 you'll hit the internal container port 9001 which isn't associated with a listener in your mosquitto.conf.

What you can do instead is simplify the exposed ports since what the app_proxy container does is proxy requests directly to a port INSIDE the broker container. There is no need to expose 9991. The app proxy will upgrade the request to ws automatically. See my screen-recording below for the changes to make:

mosquitto.mp4

We may need to whitelist certain routes in the app proxy like /mqtt if external (to umbrelOS) applications need to access.

Before considering implementing this though, can you please first test using your commit ac6824c to make sure that I have not made a mistake while briefly playing around.

renamed:    data/config/mosquitto.conf.default -> data/config/mosquitto.conf
modified:   data/config/mosquitto.conf
modified:   data/www/index.html
modified:   docker-compose.yml
modified:   umbrel-app.yml
	modified:   data/www/index.html
	modified:   docker-compose.yml
	modified:   umbrel-app.yml
removed unnecessary port-mapping
@dirstel
Copy link
Contributor Author

dirstel commented Dec 10, 2024

Sorry for the confusion about the port(mapping)s - I'm glad, the app_proxy is able to serve the web page AND the mqtt-via-webservice as well at the same time. This takes away some confusing entries.

  • removed hook as it is not needed + downgraded manifestVersion to 1 as no hooks needed

  • removed port mapping in docker container: webpage and mqtt via webservice is handled and exposed by app_proxy

  • added information about configuring authentication + corrected typos in web page

  • successfully tested access to web page

  • successfully tested access via Port 1883 and mqtt-protocol

  • successfully tested access via Port 9021 and mqtt-via-webservice

Copy link

⚠️   Linting finished with 1 warning   ⚠️

Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store.

Please review the linting results below and make any necessary changes to your submission.

Linting Results

Severity File Description
ℹ️ mosquitto/docker-compose.yml External port mapping "1883:1883":
Port mappings may be unnecessary for the app to function correctly. Docker's internal DNS resolves container names to IP addresses within the same network. External access to the web interface is handled by the app_proxy container. Port mappings are only needed if external access is required to a port not proxied by the app_proxy, or if an app needs to expose multiple ports for its functionality (e.g., DHCP, DNS, P2P, etc.).
⚠️ mosquitto/umbrel-app.yml "icon" and "gallery" needs to be empty for new app submissions:
The "icon" and "gallery" fields must be empty for new app submissions as it is being created by the Umbrel team.

Legend

Symbol Description
Error: This must be resolved before this PR can be merged.
⚠️ Warning: This is highly encouraged to be resolved, but is not strictly mandatory.
ℹ️ Info: This is just for your information.

@nmfretz
Copy link
Contributor

nmfretz commented Dec 18, 2024

Excellent, thanks for making those changes and thoroughly testing @dirstel 🤝. I have tested as well and everything is working nicely.

The gallery assets are complete and we're going live. Thanks for all your efforts bringing this second app to the store!

image

@nmfretz nmfretz merged commit 7547975 into getumbrel:master Dec 18, 2024
1 check passed
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.

2 participants