Change in vdsm[ovirt-3.4]: Fixing ssl config validation

Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/24500

to review the following change.

Change subject: Fixing ssl config validation
......................................................................

Fixing ssl config validation

Currently we verify only the config of livbirtd.conf and qemu.conf
when ssl=False in vdsm.conf. If ssl=True we assumed the validation
passes. This patch add a check also to ssl=True.

Except that, the validation function is not general and checks only ssl
configuration. This patch splits to specific test_conflict functions and
allow adding more test_conflict functions in the future.

Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Signed-off-by: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/24425
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M lib/vdsm/tool/libvirt_configure.sh.in
1 file changed, 32 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/24500/1

diff --git a/lib/vdsm/tool/libvirt_configure.sh.in
b/lib/vdsm/tool/libvirt_configure.sh.in
index 8a70be5..f1b70bd 100755
--- a/lib/vdsm/tool/libvirt_configure.sh.in
+++ b/lib/vdsm/tool/libvirt_configure.sh.in
@@ -47,11 +47,16 @@
 get_libvirt_conf_item() {
     local cfile="$1"
     local key="$2"
-    @GREP_PATH@ "^\s*"${key}"\s*=" "${cfile}" | \
+    @GREP_PATH@ "^\s*"${key}"\s*=" "${cfile}" >/dev/null
2>&1 | \
             tail -1 | @SED_PATH@ "s/\s*$key\s*=\s*//;s/\s*\(#.*\)\?$//"
 }
 
 test_conflicting_conf() {
+    test_ssl_conflict_conf
+    # add here additional conf checks
+}
+
+test_ssl_conflict_conf() {
     local lconf="$1"
     local qconf="$2"
 
@@ -61,26 +66,38 @@
         return 3
     fi
 
-    if [ "${ssl}" = "true" ]; then
-        echo "SUCCESS: ssl configured to true. No conflicts"
-        return 0
-    fi
-
     local listen_tcp="$(get_libvirt_conf_item "${lconf}"
listen_tcp)"
     local auth_tcp="$(get_libvirt_conf_item "${lconf}" auth_tcp)"
     local spice_tls="$(get_libvirt_conf_item "${qconf}" spice_tls)"
 
-    if [ "${listen_tcp}" = "1" -a \
-         "${auth_tcp}" = '"none"' -a \
-         "${spice_tls}" = "0" ]; then
-        echo "SUCCESS: No conflicts between configuration files"
+    if [ "${ssl}" = "true" ]; then
+        if [ "${listen_tcp}" != "1" -a \
+             "${auth_tcp}" != '"none"' -a \
+             "${spice_tls}" != "0" ]; then
+            echo "SUCCESS: ssl configured to true. No conflicts"
+            return 0
+        else
+            echo "FAILED: conflicting vdsm and libvirt-qemu tls
configuration."
+            echo "vdsm.conf with ssl=True requires the following changed: "
+            echo "libvirtd.conf: listen_tcp=0, auth_tcp=\"sasl\", "
+            echo "qemu.conf: spice_tls=1."
+            return 1
+        fi
+
         return 0
     else
-        echo "FAILED: conflicting vdsm and libvirt-qemu tls configuration."
-        echo "vdsm.conf with ssl=False requires the following changed: "
-        echo "libvirtd.conf: listen_tcp=1, auth_tcp=\"none\", "
-        echo "qemu.conf: spice_tls=0."
-        return 1
+        if [ "${listen_tcp}" = "1" -a \
+             "${auth_tcp}" = '"none"' -a \
+             "${spice_tls}" = "0" ]; then
+            echo "SUCCESS: ssl configured to false. No conflicts."
+            return 0
+        else
+            echo "FAILED: conflicting vdsm and libvirt-qemu tls
configuration."
+            echo "vdsm.conf with ssl=False requires the following changed: "
+            echo "libvirtd.conf: listen_tcp=1, auth_tcp=\"none\", "
+            echo "qemu.conf: spice_tls=0."
+            return 1
+        fi
     fi
 }
 


-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Yaniv Bronhaim has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3:

I'm consider if to squash also http://gerrit.ovirt.org/#/c/24741/ in. what do you say?
one patch for that as it should have been ?

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3: Verified+1

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3: Code-Review+2

Understanding that moving from ssl=True to False and vice versa was verified, I'm
giving my ack.

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

Yaniv Bronhaim has submitted this change and it was merged.

Change subject: Fixing ssl config validation
......................................................................


Fixing ssl config validation

Currently we verify only the config of livbirtd.conf and qemu.conf
when ssl=False in vdsm.conf. If ssl=True we assumed the validation
passes. This patch add a check also to ssl=True.

Except that, the validation function is not general and checks only ssl
configuration. This patch splits to specific test_conflict functions and
allow adding more test_conflict functions in the future.

Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Signed-off-by: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/24425
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/24500
---
M lib/vdsm/tool/libvirt_configure.sh.in
1 file changed, 31 insertions(+), 14 deletions(-)

Approvals:
  Yaniv Bronhaim: Verified
  Dan Kenigsberg: Looks good to me, approved



-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

Yaniv Bronhaim has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3:

verified over rhel and fedora. please ack

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3: -Code-Review

Why squash? how is the dubious grep issue related to our own bug?

P.S have you really tried flipping between ssl=True to False on the same host? It seems
that you verify that auth_tcp is not "none", but you never reset it to
"sasl".

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fixing ssl config validation
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/24500/1/lib/vdsm/tool/libvirt_configure.sh.in
File lib/vdsm/tool/libvirt_configure.sh.in:

Line 46: 
Line 47: get_libvirt_conf_item() {
Line 48:     local cfile="$1"
Line 49:     local key="$2"
Line 50:     @GREP_PATH@ "^\s*"${key}"\s*=" "${cfile}"
>/dev/null 2>&1 | \
please squash Ic4cab6f3346f9b1ad492d40ea3d3c645484570da into this patch!
Line 51:             tail -1 | @SED_PATH@
"s/\s*$key\s*=\s*//;s/\s*\(#.*\)\?$//"
Line 52: }
Line 53: 
Line 54: test_conflicting_conf() {


-- 
To view, visit http://gerrit.ovirt.org/24500
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3d9ada1b81d275b8cac9391c15ab903ec9552184
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-HasComments: Yes