kvm: keep the Ceph key out of the logs and process list of the RBD Host HA checks, and harden the parsing

Follow-up to the review of the Host HA on Ceph RBD support:

- LibvirtStoragePool: do not add a port to Ceph monitors which have one
  already, and only wrap bare IPv6 addresses in square brackets; mask the
  Ceph key in the logged command
- kvmheartbeat_rbd.sh, kvmvmactivity_rbd.sh: give the Ceph key to rados and
  rbd in a temporary keyfile instead of --key
- kvmheartbeat_rbd.sh: require -t to be a number when checking a heartbeat
- kvmvmactivity_rbd.sh: read the list of volumes without word splitting or
  globbing, skipping empty entries, and check if the activity object exists
  by the exit status of rados stat
- add unit tests for monitors with a port and for the masked key

Author: weizhouapache

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java

@@ -352,8 +352,7 @@ private String findKvmHaScript(String scriptName) {
 
     /**
      * Returns the Ceph monitors as expected by "--mon-host": the comma-separated monitors of the pool, trimmed
-     * and without empty entries, each with the pool's monitor port if one is set (IPv6 addresses are enclosed
-     * in square brackets then).
+     * and without empty entries. If the pool has a monitor port, it is added to each monitor which has none yet.
      */
     protected String getRbdMonitors() {
         List<String> monitors = new ArrayList<>();
@@ -362,17 +361,28 @@ protected String getRbdMonitors() {
             if (monitor.isEmpty()) {
                 continue;
             }
-            if (sourcePort > 0) {
-                if (monitor.contains(":") && !monitor.startsWith("[")) {
-                    monitor = "[" + monitor + "]";
-                }
-                monitor = monitor + ":" + sourcePort;
-            }
-            monitors.add(monitor);
+            monitors.add(sourcePort > 0 ? addPortToRbdMonitor(monitor) : monitor);
         }
         return String.join(",", monitors);
     }
 
+    private String addPortToRbdMonitor(String monitor) {
+        if (monitor.startsWith("[")) {
+            // IPv6 address in square brackets, which has a port if followed by ":<port>"
+            return monitor.contains("]:") ? monitor : monitor + ":" + sourcePort;
+        }
+        int colons = StringUtils.countMatches(monitor, ":");
+        if (colons == 0) {
+            return monitor + ":" + sourcePort;
+        }
+        if (colons == 1) {
+            // IPv4 address or host name, with a port
+            return monitor;
+        }
+        // IPv6 address without square brackets, so without a port
+        return "[" + monitor + "]:" + sourcePort;
+    }
+
     /**
      * Adds the Ceph cluster connection details (monitors, pool and, if cephx is enabled, credentials)
      * to a heartbeat/VM-activity check {@link Script} for a RBD storage pool. Mirrors the "mon_host"/"id"/"key"
@@ -388,7 +398,8 @@ protected void addRbdConnectionArgs(Script cmd) {
         cmd.add("-o", sourceDir);
         if (hasUser) {
             cmd.add("-n", authUsername);
-            cmd.add("-k", authSecret);
+            cmd.add("-k");
+            cmd.addSensitive(authSecret);
         }
     }
 

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java

@@ -195,7 +195,8 @@ public void testAddRbdConnectionArgsWithCephx() {
         Mockito.verify(cmd).add("-s", "10.0.0.1:6789,[fd00::1]:6789");
         Mockito.verify(cmd).add("-o", "rbdpool");
         Mockito.verify(cmd).add("-n", "cephuser");
-        Mockito.verify(cmd).add("-k", "cephkey");
+        Mockito.verify(cmd).add("-k");
+        Mockito.verify(cmd).addSensitive("cephkey");
         Mockito.verifyNoMoreInteractions(cmd);
     }
 
@@ -222,4 +223,19 @@ public void testAddRbdConnectionArgsWithCephUserOnlyOrKeyOnly() {
             }
         }
     }
+
+    @Test
+    public void testRbdMonitorsWhichHaveAPortAlready() {
+        // the monitors keep their port, with or without a port of the pool
+        String withPorts = "10.0.0.1:6789,mon.example.com:3300,[fd00::1]:6789";
+        assertEquals(withPorts, getRbdMonitors(withPorts, 0));
+        assertEquals(withPorts, getRbdMonitors(withPorts, 6789));
+        assertEquals(withPorts, getRbdMonitors(withPorts, 3300));
+    }
+
+    @Test
+    public void testRbdMonitorsWithAndWithoutAPortAreMixed() {
+        assertEquals("10.0.0.1:6789,10.0.0.2:3300,mon.example.com:3300,[fd00::1]:3300,[fd00::2]:6789,[fd00::3]:3300",
+                getRbdMonitors("10.0.0.1:6789,10.0.0.2,mon.example.com,fd00::1,[fd00::2]:6789,[fd00::3]", 3300));
+    }
 }

scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh

@@ -95,10 +95,20 @@ then
    exit 1
 fi
 
+# -t is the maximum age of the heartbeat, which is compared in a check
+if [ "$rflag" == "1" ] && ! [[ "$interval" =~ ^[0-9]+$ ]]
+then
+   exit 1
+fi
+
 RadosOpts=(--mon-host "$MonHosts")
-if [ -n "$CephUser" ]
+if [ -n "$CephUser" ] && [ "$cflag" != "1" ]
 then
-   RadosOpts+=(--id "$CephUser" --key "$CephKey")
+   # the key is given to rados in a file, to keep it out of the process list
+   KeyFile=$(mktemp)
+   trap 'rm -f "$KeyFile"' EXIT
+   printf '%s' "$CephKey" > "$KeyFile"
+   RadosOpts+=(--id "$CephUser" --keyfile "$KeyFile")
 fi
 
 hbObject="KVMHA-hb-$HostIP"

scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh

@@ -107,8 +107,12 @@ RadosOpts=(--mon-host "$MonHosts")
 RbdOpts=(--mon-host "$MonHosts")
 if [ -n "$CephUser" ]
 then
-   RadosOpts+=(--id "$CephUser" --key "$CephKey")
-   RbdOpts+=(--id "$CephUser" --key "$CephKey")
+   # the key is given to rados and rbd in a file, to keep it out of the process list
+   KeyFile=$(mktemp)
+   trap 'rm -f "$KeyFile"' EXIT
+   printf '%s' "$CephKey" > "$KeyFile"
+   RadosOpts+=(--id "$CephUser" --keyfile "$KeyFile")
+   RbdOpts+=(--id "$CephUser" --keyfile "$KeyFile")
 fi
 
 hbObject="KVMHA-hb-$HostIP"
@@ -137,8 +141,14 @@ fi
 # If any of the host's volumes still has a live watcher, something (most
 # likely qemu on the host being checked) is actively using it right now.
 latestUpdateTime=0
-for image in ${UUIDList//,/ }
+IFS=',' read -ra images <<< "$UUIDList"
+for image in "${images[@]}"
 do
+  image=${image//[[:space:]]/}
+  if [ -z "$image" ]
+  then
+    continue
+  fi
   watcherCount=$(rbd status "$PoolName/$image" "${RbdOpts[@]}" --format json 2> /dev/null | \
     python3 -c 'import json,sys
 try:
@@ -152,7 +162,7 @@ except Exception:
   fi
 done
 
-if [ ! -z "$(rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" 2> /dev/null)" ]
+if rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" &> /dev/null
 then
   acTime=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$acObject" - 2> /dev/null)
 else