bridge: Migrate /var/lib/cockpit/machines.json to /etc

doc/guide/feature-machines.xml

@@ -15,15 +15,95 @@
   <para>Using SSH keys is only supported when the system has the
     necessary APIs in libssh.</para>
 
-  <para>There is currently no command line interface for adding and/or removing
-    machines from the dashboard. The machine data is stored in
-    <filename>/etc/cockpit/machines.d/*.json</filename>. Settings in
+  <para>SSH host keys are stored in
+    <filename>/var/lib/cockpit/known_hosts</filename>.</para>
+
+  <para>The machine data is stored in
+    <filename>/etc/cockpit/machines.d/*.json</filename>.  Settings in
     lexicographically later files amend or override settings in earlier ones.
-    Cockpit itself writes into <filename>99-webui.json</filename>;
-    packages or admins who want to pre-configure machines should ship files
-    like <filename>05-mymachine.json</filename> so that changes from the web
+    Cockpit itself writes into <filename>99-webui.json</filename>; packages or
+    admins who want to pre-configure machines should ship files like
+    <filename>05-mymachine.json</filename> so that changes from the web
     interface override the pre-configured files.</para>
 
-  <para>SSH host keys are stored in
-     <filename>/var/lib/cockpit/known_hosts</filename>.</para>
+  <para>Each JSON file contains an object that maps machine IDs to objects that
+    define the properties of that machine. The ID can be a human readable name
+    or an IP address or any other unique value, and is shown in the web
+    interface until conneting to it the first time, at which point the web
+    interface will show the machine's host name.</para>
+
+  <para>The following properties are recognized:</para>
+
+  <variablelist>
+    <varlistentry>
+      <term><code>"address"</code></term>
+      <listitem><para><emphasis>(string, mandatory)</emphasis> IP address or
+          DNS name of the machine</para></listitem>
+    </varlistentry>
+
+    <varlistentry>
+      <term><code>"visible"</code></term>
+      <listitem><para><emphasis>(boolean, optional)</emphasis> If
+          <code>true</code>, the machine will be displayed and
+          available for managing with Cockpit. If <code>false</code> (the
+          default), it will not be displayed, but still taken into account for
+          type-ahead search when adding new machines in the web
+          interface.</para></listitem>
+    </varlistentry>
+
+    <varlistentry>
+      <term><code>"user"</code></term>
+      <listitem><para><emphasis>(string, optional)</emphasis> User name on the remote machine.
+          When not given, Cockpit will default to the user name that was being
+          used to log into Cockpit itself.</para></listitem>
+    </varlistentry>
+
+    <varlistentry>
+      <term><code>"port"</code></term>
+      <listitem><para><emphasis>(integer, optional)</emphasis> ssh port of the
+          remote machine. When not given, the default port 22 is used.
+          </para></listitem>
+    </varlistentry>
+
+    <varlistentry>
+      <term><code>"color"</code></term>
+      <listitem><para><emphasis>(string, optional)</emphasis> Color to
+          assign to the machine label in the web interface. This can be either given as
+          <code>rgb(r_value, g_value, b_value)</code> with each value being an
+          integer between 0 and 255, or as a color name like <code>yellow</code>.
+          When not given, Cockpit will assign an unused color automatically.
+          </para></listitem>
+    </varlistentry>
+
+    <!-- TODO: This cannot sensibly be used right now, as this neither accepts
+         a full (foreign) URL nor a relative path in the dist/ directory
+    <varlistentry>
+      <term><code>"avatar"</code></term>
+      <listitem><para><emphasis>(string, optional)</emphasis> Path to an image
+          file that will be shown as an icon for that machine in the web
+          interface. If not given, a generic "computer" icon is used.
+          </para></listitem>
+    </varlistentry>
+    -->
+  </variablelist>
+
+  <para>Example:</para>
+  <programlisting>
+{
+    "web server": {
+        "address": "192.168.2.4",
+        "visible": true,
+        "color": "rgb(100, 200, 0)",
+        "user": "admin"
+    },
+    "192.168.2.1": {
+        "address": "192.168.2.1",
+        "port": 2222,
+        "visible": true,
+        "color": "green"
+    }
+}</programlisting>
+
+  <para>There is currently no command line interface for adding and/or removing
+    machines from the dashboard.</para>
 </chapter>

src/bridge/cockpitdbusmachines.c

@@ -22,6 +22,7 @@
 #include <glob.h>
 #include <string.h>
 #include <errno.h>
+#include <glib/gstdio.h>
 #include <json-glib/json-glib.h>
 
 #include "cockpitdbusinternal.h"
@@ -47,7 +48,7 @@ static int
 glob_err_func (const char *epath,
                int eerrno)
 {
-  /* just log the error for debugging */
+  /* Should Not Happen™ -- log the error for debugging */
   if (eerrno != ENOENT)
     g_warning ("%s: cannot read: %s", epath, g_strerror (eerrno));
   return 0;
@@ -99,6 +100,20 @@ parse_json_file (const char *path)
   return result;
 }
 
+static gboolean
+write_json_file (JsonNode *config, const char *path, GError **error)
+{
+  JsonGenerator *json_gen;
+  gboolean res;
+
+  json_gen = json_generator_new ();
+  json_generator_set_root (json_gen, config);
+  json_generator_set_pretty (json_gen, TRUE); /* bikeshed zone */
+  res = json_generator_to_file (json_gen, path, error);
+  g_object_unref (json_gen);
+  return res;
+}
+
 static void
 merge_config (JsonObject *machines,
               JsonObject *delta,
@@ -163,8 +178,9 @@ read_machines_json (void)
       return NULL;
     }
 
-  /* also read /var/lib/cockpit/machines.json for backwards compat */
-  conf_glob.gl_pathv[0] = "/var/lib/cockpit/machines.json";
+  /* also read /var/lib/cockpit/machines.json for backwards compat; except when
+   * running unit tests, then disable this (this is covered by an integration test) */
+  conf_glob.gl_pathv[0] = g_getenv ("COCKPIT_TEST_CONFIG_DIR") ? "/dev/null" : "/var/lib/cockpit/machines.json";
 
   /* start with an empty object */
   machines = new_object_node ();
@@ -219,7 +235,6 @@ update_machine (const char *filename,
 {
   gchar *path;
   JsonNode *cur_config;
-  JsonGenerator *json_gen;
   JsonObject *cur_config_obj;
   JsonNode *cur_props;
   gboolean res;
@@ -245,13 +260,8 @@ update_machine (const char *filename,
       json_object_set_member (cur_config_obj, hostname, json_node_copy (info));
     }
 
-  /* write back json file */
-  json_gen = json_generator_new ();
-  json_generator_set_root (json_gen, cur_config);
-  json_generator_set_pretty (json_gen, TRUE); /* bikeshed zone */
-  res = json_generator_to_file (json_gen, path, error);
+  res = write_json_file (cur_config, path, error);
   g_free (path);
-  g_object_unref (json_gen);
   json_node_free (cur_config);
   return res;
 }
@@ -375,6 +385,69 @@ on_machines_changed (GFileMonitor *monitor,
   g_free (path);
 }
 
+static void
+migrate_var_config (void)
+{
+  const gchar *var_path = "/var/lib/cockpit/machines.json";
+  GError *error = NULL;
+
+  /* TOCTOU, but if we really miss this, we'll migrate it the next time */
+  if (!g_file_test (var_path, G_FILE_TEST_IS_REGULAR))
+    {
+      g_debug ("%s does not exist, nothing to migrate", var_path);
+      return;
+    }
+
+  /* the directory should already exist (shipped by the package), but let's make sure */
+  if (g_mkdir_with_parents (machines_json_dir (), 0755) < 0)
+    {
+      g_message ("failed to create %s, Cockpit will not work properly: %m", machines_json_dir ());
+      return;
+    }
+
+  /* common case is to move it to 99-webui.json */
+  gchar *etc_path = g_build_filename (machines_json_dir (), "99-webui.json", NULL);
+  GFile *var_file = g_file_new_for_path (var_path);
+  GFile *etc_file = g_file_new_for_path (etc_path);
+  if (g_file_move (var_file, etc_file, G_FILE_COPY_NONE, NULL, NULL, NULL, &error))
+    {
+      g_info ("migrated %s to %s", var_path, etc_path);
+    }
+  else
+    {
+      if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_EXISTS))
+        {
+          GError *error2 = NULL;
+
+          /* most likely an interrupted/failed previous transition attempt;
+           * don't clobber the existing file but move it to 98-migrated.json
+           * instead */
+          g_free (etc_path);
+          g_object_unref (etc_file);
+          etc_path = g_build_filename (machines_json_dir (), "98-migrated.json", NULL);
+          etc_file = g_file_new_for_path (etc_path);
+          if (g_file_move (var_file, etc_file, G_FILE_COPY_NONE, NULL, NULL, NULL, &error2))
+            {
+              g_info ("migrated %s to %s (99-webui.json already exists)", var_path, etc_path);
+            }
+          else
+            {
+              g_message ("moving of %s to %s failed: %s", var_path, etc_path, error2->message);
+              g_error_free (error2);
+            }
+        }
+      else /* different g_file_move() error than EXISTS */
+        {
+          g_message ("migration of %s to %s failed: %s", var_path, etc_path, error->message);
+        }
+      g_error_free (error);
+    }
+
+  g_object_unref (etc_file);
+  g_object_unref (var_file);
+  g_free (etc_path);
+}
+
 
 static GDBusInterfaceVTable machines_vtable = {
   .method_call = machines_method_call,
@@ -442,6 +515,10 @@ cockpit_dbus_machines_startup (void)
       return;
     }
 
+  /* only attempt this in a privileged bridge, otherwise we get confusing failure messages */
+  if (g_access ("/etc/cockpit", W_OK) >= 0)
+    migrate_var_config ();
+
   /* watch for file changes and send D-Bus signal for it */
   machines_monitor_file = g_file_new_for_path (machines_json_dir ());
   machines_monitor = g_file_monitor (machines_monitor_file, G_FILE_MONITOR_NONE, NULL, &error);

test/verify/check-dashboard

@@ -274,15 +274,39 @@ class TestDashboardSetup(MachineCase, DashBoardHelpers):
         m2.execute("ps aux | grep cockpit-bridge | grep admin")
         self.allow_hostkey_messages()
 
-    def testMachinesJsonVar(self):
+    def testMachinesJsonVarMigration(self):
         b = self.browser
         m = self.machine
 
-        # create obsolete machines.json in /var, which should still be respected
+        # create obsolete machines.json in /var
         blue_conf = '{"blue": {"address": "1.2.3.4", "visible": true}}'
-        m.execute("""echo '%s' > /var/lib/cockpit/machines.json""" % blue_conf)
+        m.execute("echo '%s' > /var/lib/cockpit/machines.json" % blue_conf)
+        # prevent migration due to permission error; should not crash and keep/respect the old file
+        m.execute("""chattr +i /etc/cockpit/machines.d""")
         self.login_and_go("/dashboard")
         self.wait_dashboard_addresses (b, [ "localhost", "1.2.3.4" ])
+        b.logout()
+        # should not have written anything and left the original file untouched
+        m.execute('chattr -i /etc/cockpit/machines.d && [ "$(ls /etc/cockpit/machines.d)" = "" ]')
+        self.assertEqual(m.execute("cat /var/lib/cockpit/machines.json").strip(), blue_conf)
+        self.allow_journal_messages(".*migration of /var/lib/cockpit/machines.json to /etc/cockpit/machines.d/99-webui.json failed:.*")
+
+        # now machines.d/ is writable, should migrate
+        self.login_and_go("/dashboard")
+        self.wait_dashboard_addresses (b, [ "localhost", "1.2.3.4" ])
+        b.logout()
+        m.execute('test ! -e /var/lib/cockpit/machines.json')
+        self.assertEqual(m.execute('cat /etc/cockpit/machines.d/99-webui.json').strip(), blue_conf)
+
+        # old /var file should not clobber existing 99-webui.json but move to 98-migrated.json and merge its data
+        green_conf = '{"green": {"address": "9.8.7.6", "visible": true}}'
+        m.execute("echo '%s' > /var/lib/cockpit/machines.json" % green_conf)
+        self.login_and_go("/dashboard")
+        self.wait_dashboard_addresses (b, [ "localhost", "1.2.3.4", "9.8.7.6" ])
+        b.logout()
+        m.execute('test ! -e /var/lib/cockpit/machines.json')
+        self.assertEqual(m.execute('cat /etc/cockpit/machines.d/99-webui.json').strip(), blue_conf)
+        self.assertEqual(m.execute('cat /etc/cockpit/machines.d/98-migrated.json').strip(), green_conf)
 
 
 @skipImage("No dashboard on Atomic", "continuous-atomic", "fedora-atomic", "rhel-atomic")