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

Add gladys assistant #1044

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add gladys assistant #1044

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f6b250
feat(gladys): Add Gladys Assistant
cicoub13 Apr 12, 2024
dcc73dd
feat(gladys): Fix Gladys Assistant
cicoub13 Apr 12, 2024
00f40b5
feat(gladys): Fix PR number Gladys Assistant
cicoub13 Apr 12, 2024
3669552
tweak main data bind mount
nmfretz May 20, 2024
9b4af0a
add gladysassistant data subdirectory
nmfretz May 20, 2024
c20b585
add multiarch image digest
nmfretz May 20, 2024
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
Empty file added gladys-assistant/data/.gitkeep
View file
Open in desktop
Empty file.
20 changes: 20 additions & 0 deletions gladys-assistant/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
version: "3.7"

services:
web:
image: gladysassistant/gladys:v4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
image: gladysassistant/gladys:v4
image: gladysassistant/gladys:v4@sha256:f694d49bf57426cfc37590eb71a19b0e183cd3e8cbae966ef093bb3d199d318b

It would be better to add a digest, so that the exact Dockerfile contents are pinned, not just the tag name.

Copy link
Contributor

@nmfretz nmfretz May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @highghlow 🙏.

@cicoub13 the main reason to do this is that tags can be "overwritten" on docker hub by just pushing another image to the same tag (e.g., v4). If for some reason a newer image that breaks something gets accidentally or maliciously pushed to the same tag, then Docker will download this new image when new users install the app or when existing users restart their app.

Docker gives the highest priority to the digest, which locks-in the exact image referenced by the digest. So even if the v4 tag changes at some point, Docker will still download the image associated with the exact digest we have tested here.

Copy link
Author

Choose a reason for hiding this comment

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

If the recommendation is to fix the image with a hash, then do Pull Requests to update the image, I will follow it.

Copy link
Contributor

@nmfretz nmfretz May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @cicoub13. I'll add this for you and do this one as well to make things easier: #1044 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

multi-arch image digest added here: c20b585

An easy way to grab the digest when you know your image is multi-architecture is to run

docker pull <image>:<tag>

and you will see the multi-arch digest in the output. For example:

$ sudo docker pull gladysassistant/gladys:v4
v4: Pulling from gladysassistant/gladys
...
Digest: sha256:f694d49bf57426cfc37590eb71a19b0e183cd3e8cbae966ef093bb3d199d318b
Status: Downloaded newer image for gladysassistant/gladys:v4
docker.io/gladysassistant/gladys:v4

restart: on-failure
stop_grace_period: 1m
privileged: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need it to be privileged?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cicoub13 do you happen to know if all of the following are required for Gladys functionality:
priveleged: true
cgroup: host
- /var/run/docker.sock:/var/run/docker.sock

Copy link
Contributor

Choose a reason for hiding this comment

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

In particular, I'm wondering how important binding the Docker socket is:
/var/run/docker.sock:/var/run/docker.sock

This gives the Gladys container complete control of the Docker daemon, which we can't allow because Gladys can then issue Docker commands to the host's Docker daemon, where other app containers are running.

Copy link
Author

@cicoub13 cicoub13 May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the Docker daemon binding, it's needed from our side to create, restart other containers when a user adds an integration like Zigbee2Mqtt / MQTT / Node-Red.
Gladys will automatically pull, configure and start a new container
We try to be as smooth for the users but still managed integrations cleanly by using docker each time it's needed.

For the cgroup and privileged, let me confirm it with the community/developers.

Copy link

Choose a reason for hiding this comment

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

We use the cgroup in Gladys to get Gladys current containerId in Docker ( https://github.com/GladysAssistant/Gladys/blob/master/server/lib/system/system.getGladysContainerId.js#L32 )

We use this to check if the container is in the correct state (network_mode = host for example)

privileged is used for some integration like Bluetooth to be able to scan for Bluetooth devices

Home Assistant Umbrel integration uses it too: https://github.com/getumbrel/umbrel-apps/blob/master/home-assistant/docker-compose.yml#L8

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks very much for the explanations @cicoub13 and @Pierre-Gilles. Let me run this by Luke to figure out the best path forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cicoub13 @Pierre-Gilles - understood regarding cgroup and priveleged: true. These are fine to keep to maintain app functionality. Thanks for confirming their necessity.

I have added some thoughts on the docker socket mount here: #1044 (comment)

network_mode: host
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider specifying only the ports you need to be accessible or using app-auth

Copy link
Contributor

@nmfretz nmfretz May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I think for now the Gladys container will need to run in host network mode. This is because Gladys needs to have network device discovery capabilities and access to multicast UDP traffic (e.g., for UPnP) is that correct @cicoub13?

We have some ideas on how to get this to work with bridge networks in the future, but there isn't really a way to do this on umbrelOS right now.

Copy link
Author

@cicoub13 cicoub13 May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, exactly. We need host for because we are doing network discovery in local network https://demo.gladysassistant.com/dashboard/integration/device/lan-manager/config
Home Assistant is working the same way. If you find a way to work differently, we could adapt yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👌

cgroup: host
volumes:
- ${APP_DATA_DIR}/gladysassistant:/var/lib/gladysassistant
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ${APP_DATA_DIR}/gladysassistant:/var/lib/gladysassistant
- ${APP_DATA_DIR}/data/gladysassistant:/var/lib/gladysassistant

This is where app data is usually located

Copy link
Contributor

Choose a reason for hiding this comment

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

${APP_DATA_DIR} points to the gladys-assistant parent directory shown here:
image

The data directory under gladysassistant is at ${APP_DATA_DIR}/data/ as @highghlow shows.

If we want to bind /var/lib/gladysassistant to the host filesystem at ${APP_DATA_DIR}/data/gladysassistant (which I think is a good idea), then you will need to add a gladysassistant directory under the data directory and add a .gitkeep file to it.

This will future proof the data directory in the event that you add other containers to this app in the future, or you want to bind other directories in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Let me check and fix that

Copy link
Contributor

Choose a reason for hiding this comment

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

I can do this and push a commit @cicoub13

Copy link
Contributor

@nmfretz nmfretz May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added in 3669552 and 9b4af0a

- /var/run/docker.sock:/var/run/docker.sock
- /dev:/dev
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Binding the entire /dev directory is something we probably have to do here so that Gladys Assistant can communicate with any USB that may need to be plugged into the user's device (e.g., a USB dongle for zigbee support).

We can't know in advance what devices a user does or does not have plugged in, so we need to mount the entire /dev directory. We're definitely open to suggestions on how to get around this though, as the goal should always be to restrict a container's access and permissions as much as possible.

This device issue is pretty tricky though. For example, let's just assume that the only device a user ever needs to plug in to work with Gladys gets connected as /dev/ttyACM0. The logical solution in that case would seem to be to use this bind mount:
/dev/ttyACM0:/dev/ttyACM0

But there is a huge problem with this in Docker, which is that if the user does not have a device plugged in at /dev/ttyACM0 then Docker will error out and exit when trying to bring up the container because the /dev/ttyACM0 file doesn't exist. This means that users who install Gladys without a usb installed at that file will get a broken install (Gladys won't even start), and any user that starts up Gladys after removing that USB device will also have a broken installation.

Copy link
Author

Choose a reason for hiding this comment

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

Exactly. We need to connect to existing or new plugged usb devices and there is no other way to do it.

Home Assistant is doing the same here https://github.com/getumbrel/umbrel-apps/blob/master/home-assistant/docker-compose.yml#L11

- /run/udev:/run/udev:ro
environment:
NODE_ENV: production
SQLITE_FILE_PATH: /var/lib/gladysassistant/gladys-production.db
SERVER_PORT: 5081
TZ: Europe/Paris
25 changes: 25 additions & 0 deletions gladys-assistant/umbrel-app.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
manifestVersion: 1
id: gladys-assistant
category: automation
name: Gladys Assistant
version: "v4"
tagline: A privacy-first, open-source home assistant
description: >-
Gladys Assistant is a privacy-first, open-source home assistant that
runs on any Linux machine: a Raspberry Pi, a NAS, a VPS, or a server at home.
developer: Pierre-Gilles Leymarie
website: https://gladysassistant.com/
repo: https://github.com/GladysAssistant/Gladys
support: https://en-community.gladysassistant.com
dependencies: []
port: 5081
gallery:
- 1.jpg
- 2.jpg
- 3.jpg
path: ""
defaultUsername: ""
defaultPassword: ""
releaseNotes: ""
submitter: Cyril Beslay
submission: https://github.com/getumbrel/umbrel/pull/1044