Use a separate D-Bus server by imobachgs · Pull Request #384 · agama-project/agama

imobachgs · 2022-12-30T09:05:40Z

Problem

D-Installer uses the D-Bus "system" service. When running as a container, the backend container shares its "system" bus, so the web UI can access the services.

However, this approach does not work with Iguana. In that scenario, the web UI needs to access the D-Installer system bus (which is shared as it was the "system" D-Bus) and the host system bus (which cannot be accessed at this point) at the same time.

Our first attempt was to attach D-Installer to the host D-Bus server, but that's not possible because we need to change the server's configuration (to set the permissions).

Solution

This PR changes D-Installer to use its own D-Bus server. It has two main advantages:

We do not need to change the system D-Bus configuration anymore.
We can use our own D-Bus server for internal communication and use the system service to access components like NetworkManager.

We have not tested this approach in Iguana yet.

Testing

Added a new unit test
Tested manually
Tested with Iguana (see this comment)

To do

(optional) Allow setting the D-Bus server using an environment variable and/or the configuration file
Improve the process handling through bin/d-installer
Make the address discoverable to avoid using a hardcoded value in the web UI
Extend the documentation describing the approach

* Replace DBus::SystemBus instances with DInstaller::DBus::Bus.

coveralls · 2022-12-30T09:10:15Z

Coverage: 76.337% (+0.1%) from 76.218% when pulling 144c60c on use-own-bus into 8a94c2c on master.

web/src/client/dbus.js

service/lib/dinstaller/dbus/server_manager.rb

service/bin/d-installer

service/lib/dinstaller/dbus/bus.rb

service/share/dbus.conf

mvidner · 2023-01-02T12:59:48Z

service/share/dbus.conf

+
+  <standard_session_servicedirs />
+
+  <policy context="default">


This makes the policy wide open, overriding the root restrictions below.

I think it is wrong: for a regular session bus it works out because the socket used is /run/user/$UID/bus, in a user-specific tmpdir, and the access rights of the directory are 0700, locking other users out.

Oh, I see. So I guess we should drop this whole section, right?

I have updated the file and I wonder whether it makes sense to put it under /usr/share/dbus-1/. We can read that configuration just from the gem path. WDYT?

PS: if we still decide to keep the file under /usr/share/dbus-1, we should at least move it out of the system.d folder.

I have moved that file to /usr/share/dbus-1/d-installer and the services to /usr/share/dbus-1/d-installer-services. I found out that other packages are doing the same:

/usr/share/dbus-1/accessibility-services /usr/share/dbus-1/system-services

See the servicedir element.

Co-authored-by: Martin Vidner <mvidner@suse.cz>

imobachgs · 2023-01-03T08:55:08Z

I tried this PR and it works: I could interact with NetworkManager from D-Installer's UI. The list of Iguana volumes looks like this:

volumes:
    - dbus_run:/run/d-installer
    - /var/run/dbus/system_bus_socket:/var/run/dbus/system_bus_socket

However, I found a different problem with the storage layer. In my setup (I do not know whether it might be a problem in other Iguana-based scenarios), D-Installer cannot probe one of the file systems (~~I think it corresponds to the ramdisk~~ it is a disk corresponding to an old installation):

Minor issues were detected:
* Probing file system with UUID 794acf8c-0b66-4931-8804-9dcd6ace0160 failed

Given that the YastProbe callbacks are not implemented, the storage probing fails. See https://github.com/yast/yast-storage-ng/blob/3d1cadddfa8072ad1f074f3f52774812097fe75d/src/lib/y2storage/callbacks/yast_probe.rb#L46-L51.

PS: I used a PXE-based approach so I will write a small "howto" (in a different PR).

imobachgs · 2023-01-03T10:10:12Z

However, I found a different problem with the storage layer. In my setup (I do not know whether it might be a problem in other Iguana-based scenarios), D-Installer cannot probe one of the file systems (~~I think it corresponds to the ramdisk~~ it is a disk corresponding to an old installation):

Trying to mount the /dev/sda2 (a Btrfs file system) results in the following message (from the container or the host):

mount: /mnt: /dev/sda2 already mounted or mount point busy.

However, mounting the DVD (/dev/sr0) works just fine.

* The createClient is not async anymore. * The responsability of reading the bus address is moved to the installer context.

service/lib/dinstaller/dbus/server_manager.rb

jreidinger · 2023-01-10T20:41:36Z

service/lib/dinstaller/dbus/server_manager.rb

+        pid = find_server
+        return unless pid
+
+        Process.kill("KILL", pid.to_i)


NP: note that usually preferred way for killing is something like SIGTERM wait 5 seconds and then SIGKILL if it does not help. Here you do not allow process to finish anything.

Good point. Let's postpone this change a bit, anyway.

web/src/client/dbus.js

jreidinger · 2023-01-10T20:48:36Z

web/src/client/manager.js

 import cockpit from "../lib/cockpit";

-const MANAGER_SERVICE = "org.opensuse.DInstaller.Manager";
+const MANAGER_SERVICE = "org.opensuse.DInstaller";


hmm, now I am curious why previously it works when service const was wrong?

That service name was used as a fallback:

https://github.com/yast/d-installer/blob/8a94c2c940957d56f8b0f3eb32962bcaafaf3deb/web/src/client/manager.js#L41-L43

However, we were injecting the correct D-Bus client:

https://github.com/yast/d-installer/blob/8a94c2c940957d56f8b0f3eb32962bcaafaf3deb/web/src/client/index.js#L54-L67

Hence the MANAGER_SERVICE constant was not used at all.

jreidinger · 2023-01-10T20:50:50Z

In general change looks good, just please document exception at #384 (comment)

imobachgs added 5 commits December 30, 2022 08:52

[service] Use a separate D-Bus server

6f48466

[service] Stop the D-Bus server on "d-installer -k"

a90d3c1

[service] Use the own D-Bus server

6c5de31

* Replace DBus::SystemBus instances with DInstaller::DBus::Bus.

[cli] Update Gemfile.lock

18a9590

[web] Connect to the D-Installer D-Bus server

d8c9e05

imobachgs self-assigned this Jan 2, 2023

imobachgs added the enhancement New feature or request label Jan 2, 2023

mvidner reviewed Jan 2, 2023

View reviewed changes

imobachgs and others added 3 commits January 3, 2023 07:47

[service] Fix several documentation issues

3fd005f

Co-authored-by: Martin Vidner <mvidner@suse.cz>

[service] Remove the START_CMD constant

cbbf944

[service] Fix the path to the D-Bus configuration file

36970b7

[service] Fix and document the D-Bus configuration

c8e358d

imobachgs added 10 commits January 9, 2023 12:07

[service] Write the D-Bus address to a file

3eff53b

[web] Read the D-Bus address from /run/d-installer

0277449

[service] Fix D-Bus permissions

4b97d02

[service]

3fbe599

[service] Improve d-installer -s and -k handling

7446abf

Merge branch 'master' into use-own-bus

ca49315

[service] Avoid running the D-Bus servers on tests

a199091

Merge branch 'master' into use-own-bus

77def21

[service] createClient receives the address

e9d2897

* The createClient is not async anymore. * The responsability of reading the bus address is moved to the installer context.

[web] Make ESLint happy

c7fdcf5

imobachgs changed the title ~~[WIP] Use a separate D-Bus server~~ Use a separate D-Bus server Jan 10, 2023

imobachgs added 4 commits January 10, 2023 09:03

[doc] Mention that D-Installer uses a dedicated D-Bus server

3eafc10

[service] Adjust the D-Bus configuration file paths

3070588

Update the changes files

47d0f73

[service] Fix D-Bus activation

6f41e60

imobachgs marked this pull request as ready for review January 10, 2023 12:16

imobachgs added 2 commits January 10, 2023 15:42

[service] Move some logic from bin/d-installer to ServerManager

0a8b4ae

[service] Make RuboCop happy

8dddfd3

imobachgs mentioned this pull request Jan 10, 2023

Expose the system bus to D-Installer's web UI openSUSE/iguana#17

Merged

jreidinger reviewed Jan 10, 2023

View reviewed changes

service/lib/dinstaller/dbus/server_manager.rb Outdated Show resolved Hide resolved

jreidinger reviewed Jan 10, 2023

View reviewed changes

web/src/client/dbus.js Show resolved Hide resolved

jreidinger reviewed Jan 10, 2023

View reviewed changes

Update from code review

144c60c

jreidinger approved these changes Jan 11, 2023

View reviewed changes

imobachgs merged commit 4de1d34 into master Jan 11, 2023

imobachgs deleted the use-own-bus branch January 11, 2023 10:16

imobachgs mentioned this pull request Feb 13, 2023

Release 0.7 #423

Merged

Comments

Conversation

imobachgs commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

To do

Uh oh!

coveralls commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imobachgs Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imobachgs commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imobachgs commented Jan 3, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreidinger commented Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imobachgs commented Dec 30, 2022 •

edited

Loading

coveralls commented Dec 30, 2022 •

edited

Loading

imobachgs Jan 3, 2023 •

edited

Loading

imobachgs commented Jan 3, 2023 •

edited

Loading