Piotr Kliczewski has uploaded a new change for review.
Change subject: ssl: runtime config to choose implementation ......................................................................
ssl: runtime config to choose implementation
During build process we can choose to use standard ssl module or both m2crypto and ssl module. First option is dedicated enable debian support whereas second options make it possible to choose an implementation by setting ssl_impl config value to m2c or ssl.
Change-Id: I9881d11e30ced9c34bfe602bba3d968f57e0fe15 Signed-off-by: pkliczewski piotr.kliczewski@gmail.com --- M lib/vdsm/config.py.in M lib/vdsm/jsonrpcvdscli.py M lib/vdsm/vdscli.py M lib/yajsonrpc/stompreactor.py M vdsm.spec.in M vdsm/clientIF.py M vdsm/kaxmlrpclib.py M vdsm/protocoldetector.py M vdsm/virt/migration.py 9 files changed, 37 insertions(+), 9 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/89/44689/1
diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in index 8f2e519..6f5a719 100644 --- a/lib/vdsm/config.py.in +++ b/lib/vdsm/config.py.in @@ -203,6 +203,9 @@
('connection_stats_timeout', '3600', 'Time in seconds defining how frequently we log transport stats'), + + ('ssl_impl', 'm2c', + 'Specifies which ssl impl should be used. m2c by default'), ]),
# Section: [mom] diff --git a/lib/vdsm/jsonrpcvdscli.py b/lib/vdsm/jsonrpcvdscli.py index 8d8aa0a..ae8965a 100644 --- a/lib/vdsm/jsonrpcvdscli.py +++ b/lib/vdsm/jsonrpcvdscli.py @@ -30,7 +30,10 @@ from vdsm import response from .config import config try: - from . import m2cutils as sslutils + if config.get('vars', 'ssl_impl') == 'm2c': + from . import m2cutils as sslutils + else: + from . import sslutils except ImportError: from . import sslutils
diff --git a/lib/vdsm/vdscli.py b/lib/vdsm/vdscli.py index cf083e3..e655c0d 100644 --- a/lib/vdsm/vdscli.py +++ b/lib/vdsm/vdscli.py @@ -26,8 +26,12 @@ import re import sys from xml.parsers.expat import ExpatError +from .config import config try: - from . import m2cutils as sslutils + if config.get('vars', 'ssl_impl') == 'm2c': + from . import m2cutils as sslutils + else: + from . import sslutils except ImportError: from . import sslutils
diff --git a/lib/yajsonrpc/stompreactor.py b/lib/yajsonrpc/stompreactor.py index b1eda30..4cab4aa 100644 --- a/lib/yajsonrpc/stompreactor.py +++ b/lib/yajsonrpc/stompreactor.py @@ -27,7 +27,10 @@ from . import stomp from .betterAsyncore import Dispatcher, Reactor try: - from vdsm.m2cutils import SSLSocket + if config.get('vars', 'ssl_impl') == 'm2c': + from vdsm.m2cutils import SSLSocket + else: + from ssl import SSLSocket except ImportError: from ssl import SSLSocket
diff --git a/vdsm.spec.in b/vdsm.spec.in index 33e89e1..851c0d3 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -42,7 +42,8 @@ %global _polkitdir %{_localstatedir}/lib/polkit-1/localauthority/10-vendor.d %endif
-# enable m2crypto by default +# 0 - enabled ssl +# 1 - enabled both implementations by default %global with_m2c 1
# Gluster should not be shipped with RHEV @@ -1071,7 +1072,7 @@ %{python_sitelib}/%{vdsm_name}/schedule.py* %if %{with_m2c} %{python_sitelib}/%{vdsm_name}/m2cutils.py* -%exclude %{python_sitelib}/%{vdsm_name}/sslutils.py* +%{python_sitelib}/%{vdsm_name}/sslutils.py* %else %exclude %{python_sitelib}/%{vdsm_name}/m2cutils.py* %{python_sitelib}/%{vdsm_name}/sslutils.py* diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py index 043383a..0020a1c 100644 --- a/vdsm/clientIF.py +++ b/vdsm/clientIF.py @@ -55,7 +55,10 @@ from virt.vmdevices import hwclass from virt.utils import isVdsmImage try: - from vdsm import m2cutils as sslutils + if config.get('vars', 'ssl_impl') == 'm2c': + from vdsm import m2cutils as sslutils + else: + from vdsm import sslutils except ImportError: from vdsm import sslutils try: diff --git a/vdsm/kaxmlrpclib.py b/vdsm/kaxmlrpclib.py index b0640e8..d43230f 100644 --- a/vdsm/kaxmlrpclib.py +++ b/vdsm/kaxmlrpclib.py @@ -109,9 +109,13 @@ _connection_class = TcpkeepHTTPConnection
################### +from vdsm.config import config # the same, for ssl try: - from vdsm import m2cutils as sslutils + if config.get('vars', 'ssl_impl') == 'm2c': + from vdsm import m2cutils as sslutils + else: + from vdsm import sslutils except ImportError: from vdsm import sslutils import ssl diff --git a/vdsm/protocoldetector.py b/vdsm/protocoldetector.py index dbdc730..d06fb3d 100644 --- a/vdsm/protocoldetector.py +++ b/vdsm/protocoldetector.py @@ -23,9 +23,13 @@
import vdsm.infra.filecontrol as filecontrol
+from vdsm.config import config from vdsm.utils import monotonic_time try: - from vdsm.m2cutils import SSLHandshakeDispatcher + if config.get('vars', 'ssl_impl') == 'm2c': + from vdsm.m2cutils import SSLHandshakeDispatcher + else: + from vdsm.sslutils import SSLHandshakeDispatcher except ImportError: from vdsm.sslutils import SSLHandshakeDispatcher
diff --git a/vdsm/virt/migration.py b/vdsm/virt/migration.py index 797002c..2de8fd5 100644 --- a/vdsm/virt/migration.py +++ b/vdsm/virt/migration.py @@ -40,7 +40,10 @@ from . import vmstatus
try: - from vdsm import m2cutils as sslutils + if config.get('vars', 'ssl_impl') == 'm2c': + from vdsm import m2cutils as sslutils + else: + from vdsm import sslutils except ImportError: from vdsm import sslutils
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2:
Verified all 3 patches together by building with_m2c both with 0 and 1. Installed both build and seeing that communication is OK. For option 1 I tested both implementations by setting both implementation in config py and restarting vdsm.
CI failures are not related to these patches.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2: Verified+1
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/2/vdsm.spec.in File vdsm.spec.in:
Line 42 Line 43 Line 44 Line 45 Line 46 This is not clear - how about two flags:
with_m2c - enable m2crypto with_ssl - enable builtin ssl
Both can be 1 by default, allowing runtime configuration. If someone like to pack only one option, it is very easy.
On Debian we will not enable m2c since we cannot support it.
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/2/lib/vdsm/jsonrpcvdscli.py File lib/vdsm/jsonrpcvdscli.py:
Line 31 Line 32 Line 33 Line 34 Line 35 This boilerplate should appear once in compat.py.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2:
(2 comments)
https://gerrit.ovirt.org/#/c/44689/2/lib/vdsm/jsonrpcvdscli.py File lib/vdsm/jsonrpcvdscli.py:
Line 31 Line 32 Line 33 Line 34 Line 35
This boilerplate should appear once in compat.py.
Done
https://gerrit.ovirt.org/#/c/44689/2/vdsm.spec.in File vdsm.spec.in:
Line 42 Line 43 Line 44 Line 45 Line 46
This is not clear - how about two flags:
Done
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 2: Verified-1
When I try to create a vm using ssl standard module I loose connectivity. This can be related to the perf issue. For visibility giving -1.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 3: Verified+1
All three patches verified together. Vdsm was built with both options set to 1 and tested with both configurations m2c and ssl. For both cases handshake was successful.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 4: Verified+1
Maunual rebase, no code changes. No conflicts.
Rebuilt to check packaging for both with_ssl set to 0 and 1
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 4: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/4/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 205: ('connection_stats_timeout', '3600', Line 206: 'Time in seconds defining how frequently we log transport stats'), Line 207: Line 208: ('ssl_impl', 'm2c', Line 209: 'Specifies which ssl impl should be used. m2c by default'), Please specify which other option the user can use. /s/m2c by default/ m2c or ssl. Line 210: ]), Line 211: Line 212: # Section: [mom] Line 213: ('mom', [
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://gerrit.ovirt.org/#/c/44689/4/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 205: ('connection_stats_timeout', '3600', Line 206: 'Time in seconds defining how frequently we log transport stats'), Line 207: Line 208: ('ssl_impl', 'm2c', Line 209: 'Specifies which ssl impl should be used. m2c by default'),
Please specify which other option the user can use.
Done Line 210: ]), Line 211: Line 212: # Section: [mom] Line 213: ('mom', [
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 4: -Code-Review
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 5: Verified+1
Fixed comments and rebased. Verified by building with and without m2c and tested communication with m2c by host deploying vdsm and seeing that the connection was OK.
Dan Kenigsberg has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 5: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/5/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 205: ('connection_stats_timeout', '3600', Line 206: 'Time in seconds defining how frequently we log transport stats'), Line 207: Line 208: ('ssl_impl', 'm2c', Line 209: 'Specifies which ssl impl should be used. m2c by default or ssl'), please make the text clearer:
"m2c" by default, or "ssl" for python-native ssl implementation. Line 210: ]), Line 211: Line 212: # Section: [mom] Line 213: ('mom', [
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 5:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/5/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 205: ('connection_stats_timeout', '3600', Line 206: 'Time in seconds defining how frequently we log transport stats'), Line 207: Line 208: ('ssl_impl', 'm2c', Line 209: 'Specifies which ssl impl should be used. m2c by default or ssl'),
please make the text clearer:
Done Line 210: ]), Line 211: Line 212: # Section: [mom] Line 213: ('mom', [
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6: Verified+1
Updated description in config.py, no code changes. Copying verification flag from previous patch set.
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket Line 30: except ImportError: Could you try and do it more nicely? maybe use a True/False variable, and set it in case of ImportError or the 'else' clause.. and only then import ssl instead of importing twice..? Line 31: from . import sslutils Line 32: from .sslutils import SSLHandshakeDispatcher Line 33: from ssl import SSLSocket Line 34:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket Line 30: except ImportError:
Could you try and do it more nicely?
We may have contradicting settings during the build and in runtime. I am open for suggestions but I think that there is no nicer way to do it. Line 31: from . import sslutils Line 32: from .sslutils import SSLHandshakeDispatcher Line 33: from ssl import SSLSocket Line 34:
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket Line 30: except ImportError:
We may have contradicting settings during the build and in runtime. I am op
maybe use ssl=False before importing, and set it in case you need import. and only import it if needed... Line 31: from . import sslutils Line 32: from .sslutils import SSLHandshakeDispatcher Line 33: from ssl import SSLSocket Line 34:
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6: Code-Review-1
(5 comments)
-1 for visibility minor documentation comments, one suggestion
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 207: Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_impl', 'm2c', oh dear, an old comment of mine got lost... Let me repaste: no need to be so verbose here, let's change this to 'ssl_implementation' (or whaterver else you feel more appropriate, just be verbose) Line 212: 'Specifies which ssl impl should be used. There are 2 options: ' Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'), Line 215: ]),
Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_impl', 'm2c', Line 212: 'Specifies which ssl impl should be used. There are 2 options: ' same, we can spare 'ssl implementation', no need to use abbreviation s in documentation. Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'), Line 215: ]), Line 216:
Line 210: Line 211: ('ssl_impl', 'm2c', Line 212: 'Specifies which ssl impl should be used. There are 2 options: ' Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'), I'd write:
'There are two options: ' '"m2c" to use the m2crypto module (default) ' '"ssl" to use the standard python ssl module) '
I'm not a native english speaker, so feel free to add further corrections. Line 215: ]), Line 216: Line 217: # Section: [mom] Line 218: ('mom', [
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: from .config import config Line 21: try: Line 22: if config.get('vars', 'ssl_impl') == 'm2c': ssl_implementation - or whatever long name you want to use Line 23: from . import m2cutils as sslutils Line 24: from .m2cutils import SSLSocket Line 25: from .m2cutils import SSLHandshakeDispatcher Line 26: else:
Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket Line 30: except ImportError:
maybe use ssl=False before importing, and set it in case you need import.
Maybe something like
from .config import config
USED_IMPLEMENTATION = None
if config.get('vars', 'ssl_impl') == 'm2c': try: from . import m2cutils as sslutils from .m2cutils import SSLSocket from .m2cutils import SSLHandshakeDispatcher except ImportError: # m2crpyto was request but it is unavailable. # worth a log line? pass else: USED_IMPLEMENTATION = 'm2c'
if USED_IMPLEMENTATION is None: from . import sslutils from .sslutils import SSLHandshakeDispatcher from ssl import SSLSocket USED_IMPLEMENTATION = 'ssl'
# either way, we need it to satisfy pyflakes sslutils SSLHandshakeDispatcher SSLSocket Line 31: from . import sslutils Line 32: from .sslutils import SSLHandshakeDispatcher Line 33: from ssl import SSLSocket Line 34:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 6:
(5 comments)
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 207: Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_impl', 'm2c',
oh dear, an old comment of mine got lost...
I will change the name of this property. Line 212: 'Specifies which ssl impl should be used. There are 2 options: ' Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'), Line 215: ]),
Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_impl', 'm2c', Line 212: 'Specifies which ssl impl should be used. There are 2 options: '
same, we can spare 'ssl implementation', no need to use abbreviation s in d
Done Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'), Line 215: ]), Line 216:
Line 210: Line 211: ('ssl_impl', 'm2c', Line 212: 'Specifies which ssl impl should be used. There are 2 options: ' Line 213: 'm2crypto (by default) defined as m2c and python standard ssl ' Line 214: 'module defined as ssl'),
I'd write:
It looks good to me. Will update. Line 215: ]), Line 216: Line 217: # Section: [mom] Line 218: ('mom', [
https://gerrit.ovirt.org/#/c/44689/6/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: from .config import config Line 21: try: Line 22: if config.get('vars', 'ssl_impl') == 'm2c':
ssl_implementation - or whatever long name you want to use
Done Line 23: from . import m2cutils as sslutils Line 24: from .m2cutils import SSLSocket Line 25: from .m2cutils import SSLHandshakeDispatcher Line 26: else:
Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket Line 30: except ImportError:
Maybe something like
It is not less code but at least we have ssl imports in one place so I will apply your suggestion, thanks Line 31: from . import sslutils Line 32: from .sslutils import SSLHandshakeDispatcher Line 33: from ssl import SSLSocket Line 34:
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7: Verified+1
Verified by building only with ssl and testing runtime settings with m2c and ssl. As part of the test vdsm was built with m2c and verified that communication works.
Patch rebased, changed property name and description in config.py. Sslcompat import logic was reworked as suggested.
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7: Code-Review+1
(2 comments)
good enough for me.
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 37: Line 38: # we need it to satisfy pyflakes Line 39: sslutils Line 40: SSLHandshakeDispatcher Line 41: SSLSocket does pyflakes complain if you remove this... Line 42: Line 43: if _USED_IMPLEMENTATION is None: Line 44: from . import sslutils Line 45: from .sslutils import SSLHandshakeDispatcher
Line 47: Line 48: # we need it to satisfy pyflakes Line 49: sslutils Line 50: SSLHandshakeDispatcher Line 51: SSLSocket ...and deindent this?
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
(2 comments)
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 37: Line 38: # we need it to satisfy pyflakes Line 39: sslutils Line 40: SSLHandshakeDispatcher Line 41: SSLSocket
does pyflakes complain if you remove this...
Yes, that is way I had to add it here. Line 42: Line 43: if _USED_IMPLEMENTATION is None: Line 44: from . import sslutils Line 45: from .sslutils import SSLHandshakeDispatcher
Line 47: Line 48: # we need it to satisfy pyflakes Line 49: sslutils Line 50: SSLHandshakeDispatcher Line 51: SSLSocket
...and deindent this?
It is required as well. Without it the build fails....
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 47: Line 48: # we need it to satisfy pyflakes Line 49: sslutils Line 50: SSLHandshakeDispatcher Line 51: SSLSocket
It is required as well. Without it the build fails....
OK, no big deal.
Sandro Bonazzola has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 47: Line 48: # we need it to satisfy pyflakes Line 49: sslutils Line 50: SSLHandshakeDispatcher Line 51: SSLSocket
It is required as well. Without it the build fails....
and does it actually work after the build?
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 47: Line 48: # we need it to satisfy pyflakes Line 49: sslutils Line 50: SSLHandshakeDispatcher Line 51: SSLSocket
and does it actually work after the build?
I tested following scenarios as described in the comment when I gave +1 for verification. If you miss any test you would like to make sure that it works please let me know and I will verify.
Sandro Bonazzola has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7: Code-Review+1
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7: Verified-1
(1 comment)
As in the comment I tested different combination of runtime and build settings and the code fails. Giving -1 for visibility.
https://gerrit.ovirt.org/#/c/44689/7/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 35: else: Line 36: _USED_IMPLEMENTATION = 'm2c' Line 37: Line 38: # we need it to satisfy pyflakes Line 39: sslutils When building with m2c and ssl and running it with ssl setting the code fails in line #39. Need to be fixed. Line 40: SSLHandshakeDispatcher Line 41: SSLSocket Line 42: Line 43: if _USED_IMPLEMENTATION is None:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 7:
Previous tests for ssl where performed when m2crypto was removed and it passed.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 8:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 8:
Broken scenario tested and I can see that sslcompat works OK.
I am testing hand compiled python with ssl module but I see that it is not working so for now I won't mark verify flag.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 8: Verified+1
Verified failed scenario. No issues with communication.
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 8: Code-Review-1
(3 comments)
https://gerrit.ovirt.org/#/c/44689/8/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 207: Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_implementation', 'm2c', m2crypto Line 212: 'Specifies which ssl implementation should be used.' Line 213: 'There are 2 options:' Line 214: '"m2c" to use the m2crypto module (default)' Line 215: '"ssl" to use the standard python ssl module'),
https://gerrit.ovirt.org/#/c/44689/8/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 38: if _USED_IMPLEMENTATION == 'm2c': Line 39: sslutils Line 40: SSLHandshakeDispatcher Line 41: SSLSocket Line 42: whaaa? :/ please do
use_default_ssl = False if config..==m2crypto (stop shorting things..): try: imports except ImportError: log no m2crypto , using sslutils use_default_ssl=True
if use_default_ssl: from . import sslutils ...
#we need satisfy pyflakes (always..) sslutils .. Line 43: if _USED_IMPLEMENTATION is None: Line 44: from . import sslutils Line 45: from .sslutils import SSLHandshakeDispatcher Line 46: from ssl import SSLSocket
https://gerrit.ovirt.org/#/c/44689/8/vdsm.spec.in File vdsm.spec.in:
Line 1077: %endif Line 1078: %if %{with_ssl} Line 1079: %{python_sitelib}/%{vdsm_name}/sslutils.py* Line 1080: %else Line 1081: %exclude %{python_sitelib}/%{vdsm_name}/sslutils.py* why do you need to exclude the file? Line 1082: %endif Line 1083: %{python_sitelib}/%{vdsm_name}/supervdsm.py* Line 1084: %{python_sitelib}/%{vdsm_name}/sysctl.py* Line 1085: %{python_sitelib}/%{vdsm_name}/udevadm.py*
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 8:
(3 comments)
https://gerrit.ovirt.org/#/c/44689/8/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 207: Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('ssl_implementation', 'm2c',
m2crypto
We already use m2c in different part of the code base. In my opinion we should be consistent. Line 212: 'Specifies which ssl implementation should be used.' Line 213: 'There are 2 options:' Line 214: '"m2c" to use the m2crypto module (default)' Line 215: '"ssl" to use the standard python ssl module'),
https://gerrit.ovirt.org/#/c/44689/8/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 38: if _USED_IMPLEMENTATION == 'm2c': Line 39: sslutils Line 40: SSLHandshakeDispatcher Line 41: SSLSocket Line 42:
whaaa? :/
You suggest something which won't work.
pyflakes needs to have it in two places or it complains Line 43: if _USED_IMPLEMENTATION is None: Line 44: from . import sslutils Line 45: from .sslutils import SSLHandshakeDispatcher Line 46: from ssl import SSLSocket
https://gerrit.ovirt.org/#/c/44689/8/vdsm.spec.in File vdsm.spec.in:
Line 1077: %endif Line 1078: %if %{with_ssl} Line 1079: %{python_sitelib}/%{vdsm_name}/sslutils.py* Line 1080: %else Line 1081: %exclude %{python_sitelib}/%{vdsm_name}/sslutils.py*
why do you need to exclude the file?
Please check previous patch.
In general we need to be able to set it during build and runtime. Line 1082: %endif Line 1083: %{python_sitelib}/%{vdsm_name}/supervdsm.py* Line 1084: %{python_sitelib}/%{vdsm_name}/sysctl.py* Line 1085: %{python_sitelib}/%{vdsm_name}/udevadm.py*
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9: Verified+1
Verified all combinations of config settings m2c and ssl with and without m2crypto rpm installed. Every time no issues with communication.
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9: Code-Review+1
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9: Code-Review-1
(5 comments)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 27: if config.get('vars', 'ssl_implementation') == 'm2c': Line 28: try: Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Please keep the same order of names everywhere. Line 32: _USED_M2C = True Line 33: except ImportError: Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl')
Line 28: try: Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Not needed if we fail here. Line 33: except ImportError: Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError: If you configure m2c, and m2c is not available, vdsm should fail to start, not fallback magically to some other implementation.
Lets keep it simple. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Line 36: else: Line 37: # we need it to satisfy pyflakes Line 38: sslutils Line 39: SSLHandshakeDispatcher Line 40: SSLSocket Can we satisfy pyflakes by mentioning these once at the end? Line 41: Line 42: if not _USED_M2C: Line 43: from . import sslutils Line 44: from .sslutils import SSLHandshakeDispatcher
Line 38: sslutils Line 39: SSLHandshakeDispatcher Line 40: SSLSocket Line 41: Line 42: if not _USED_M2C: Can be just else: Line 43: from . import sslutils Line 44: from .sslutils import SSLHandshakeDispatcher Line 45: from ssl import SSLSocket Line 46:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
(5 comments)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 27: if config.get('vars', 'ssl_implementation') == 'm2c': Line 28: try: Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher
Please keep the same order of names everywhere.
Done Line 32: _USED_M2C = True Line 33: except ImportError: Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl')
Line 28: try: Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True
Not needed if we fail here.
We need it as described below. Line 33: except ImportError: Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
If you configure m2c, and m2c is not available, vdsm should fail to start,
There can be difference in how we built vdsm and how we configured.
When we built without m2c and still have this configuration here we would fail. Whereas we should gracefully handle misconfiguration. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Line 36: else: Line 37: # we need it to satisfy pyflakes Line 38: sslutils Line 39: SSLHandshakeDispatcher Line 40: SSLSocket
Can we satisfy pyflakes by mentioning these once at the end?
I tried but it does not like it. Line 41: Line 42: if not _USED_M2C: Line 43: from . import sslutils Line 44: from .sslutils import SSLHandshakeDispatcher
Line 38: sslutils Line 39: SSLHandshakeDispatcher Line 40: SSLSocket Line 41: Line 42: if not _USED_M2C:
Can be just else:
We need it as described above Line 43: from . import sslutils Line 44: from .sslutils import SSLHandshakeDispatcher Line 45: from ssl import SSLSocket Line 46:
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
There can be difference in how we built vdsm and how we configured.
That's exactly the reason why I don't like this direction. its messed up - how the user should know if he installed rpm with m2c or not.. :/ Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
There can be difference in how we built vdsm and how we configured.
Piotr, if we build without m2c, our configuration must be ssl, and we should not have magic fallbacks for configuration. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
Piotr, if we build without m2c, our configuration must be ssl, and we shoul
Sure, This approach was to solve this particular issue. If we can use vdsm_tool or any other approach to change it during the installation or even during build process it would be better to do it.
We need one more patch for it. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
Sure, This approach was to solve this particular issue. If we can use vdsm_
use m2crypto as default and add a check in vdsm/vdsm that if we run over debian and the config to use ssl is not exist there, exit with an error that suggest the user to configure the ssl value - "add to vdsm.conf ssl_implementation=ssl" - I don't think we need to add it for the user by vdsm-tool. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 9:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/9/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 29: from . import m2cutils as sslutils Line 30: from .m2cutils import SSLSocket Line 31: from .m2cutils import SSLHandshakeDispatcher Line 32: _USED_M2C = True Line 33: except ImportError:
use m2crypto as default and add a check in vdsm/vdsm that if we run over de
OK. it would be better than falling back like we did. Line 34: _log.warning('Configured m2crypto but not available falling back' Line 35: ' to ssl') Line 36: else: Line 37: # we need it to satisfy pyflakes
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 11:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 11: Verified+1
Verified by building vdsm for both implementation and tested with both implementation settings. There was separate test by building vdsm for ssl module only and verified runtime with ssl setting. Both building and runtime machines had no m2crypto rpm installed.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12: Verified+1
Verified by building vdsm for both implementation and tested with both implementation settings. There was separate test by building vdsm for ssl module only and verified runtime with ssl setting. Both building and runtime machines had no m2crypto rpm installed.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12:
CI failure is not related to this change
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12: Code-Review+1
I like this approach more.
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/12/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 21: Line 22: if config.get('vars', 'ssl_implementation') == 'm2c': Line 23: from . import m2cutils as sslutils Line 24: from .m2cutils import SSLHandshakeDispatcher Line 25: from .m2cutils import SSLSocket this specific change (import order swap) belongs in the parent patch. Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 12:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/12/lib/vdsm/sslcompat.py File lib/vdsm/sslcompat.py:
Line 21: Line 22: if config.get('vars', 'ssl_implementation') == 'm2c': Line 23: from . import m2cutils as sslutils Line 24: from .m2cutils import SSLHandshakeDispatcher Line 25: from .m2cutils import SSLSocket
this specific change (import order swap) belongs in the parent patch.
Done Line 26: else: Line 27: from . import sslutils Line 28: from .sslutils import SSLHandshakeDispatcher Line 29: from ssl import SSLSocket
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 13:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 13: Verified+1
Moved sorting order of imports to previous patch as Francesco suggested. No other code changes and verification was done in previous patch set.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 14:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 14: Verified+1
Moved fixed tests from previous patch. Verified by running build in mock locally.
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 14:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/14/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py This should done via autoconf, modifying config.py.in.
We should have a way to configure ssl implemenation during configure, and the value you configure should be use in the code.
./configure --ssl-implemenation m2c should fail on debian and succeed on el/fc.
The configured value should be the default in config.py.in.
Users on el/fc will be able to change the default.
This is how all other configuration is done, why not use the same way to configure everything? Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 14:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/14/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
This should done via autoconf, modifying config.py.in.
Done Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15: Verified+1
Verified by building vdsm with m2c enabled and disabled both builds configured vdsm properly.
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: do we really want this? users should know what they want to configure in vdsm.conf. we can alert somehow about it over debian installation that default m2crypto implementation might not work and its recommanded to set it to work with default ssl..
Nir, do you agree with setting it in build time? not that its so important.. I just think that this config value is simple enough to be familiar with. if user knows to run ./configure with --enable-m2c=no , he also can do "vi vdsm.conf" and add whatever he needs.
and if not, it can be done by vdsm-tool configure in some shape. I just think its prettier Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168:
do we really want this? users should know what they want to configure in vd
I tend to agree with you Yaniv. This is what we have vdsm.conf for. Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168:
I tend to agree with you Yaniv.
We still have config.py and we want to have it there. Here we decide whether we want to build with m2c or not. For debian we use disable-m2c.
We need both options and run and build time. Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(2 comments)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 161: [--enable-m2c], Line 162: [build using m2crypto @<:@default=yes@:>@] Line 163: )], Line 164: , Line 165: [enable_m2c="yes"] This does not work on debian - we need a check for the platform, and set the value to "no" when configuring on debian. Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: Line 169: # Users and groups
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168:
We still have config.py and we want to have it there. Here we decide whethe
I agree with Piotr - we need both options.
The purpose of this configuration is configure the system so m2c option is available. We cannot do this in runtime since m2c is not available on all platforms.
This setting can also set the default ssl impl. On rpm based systems we want to have both ssl and m2c and use m2c by default. On debian based systems we want to configure only ssl and it should be the default. Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(3 comments)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 161: [--enable-m2c], Line 162: [build using m2crypto @<:@default=yes@:>@] Line 163: )], Line 164: , Line 165: [enable_m2c="yes"]
This does not work on debian - we need a check for the platform, and set th
Nir, could it be that the debain rules are overriding this configuration? Which makes this ok..? Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: Line 169: # Users and groups
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168:
I agree with Piotr - we need both options.
Nir and Piotr, good point. Fine by me. Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
https://gerrit.ovirt.org/#/c/44689/15/tests/integration/jsonRpcHelper.py File tests/integration/jsonRpcHelper.py:
Line 38: from vdsm.config import config Line 39: from vdsm import utils Line 40: Line 41: if config.get('vars', 'ssl_implementation') == 'm2c': Line 42: from integration.m2chelper import DEAFAULT_SSL_CONTEXT Piotr, Can it happen that we'll be on debian with config.py ssl_implementation=='m2c' and then we'll fail the import? Line 43: else: Line 44: from integration.sslhelper import DEAFAULT_SSL_CONTEXT Line 45: Line 46: PERMUTATIONS = tuple(product((True, False), ("xml", "stomp")))
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(2 comments)
do you verify it over debian?
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 164: , Line 165: [enable_m2c="yes"] Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168:
Nir and Piotr, good point. Fine by me.
still no. m2crypto is available on system vdsm runs on - rpm based and debian based. user need to modify vdsm.conf and that's it. I don't see any advantages for this logic - which relevant only to rpm anyway Line 169: # Users and groups Line 170: AC_SUBST([VDSMUSER], [vdsm]) Line 171: AC_SUBST([VDSMGROUP], [kvm]) Line 172: AC_SUBST([METADATAUSER], [vdsm])
https://gerrit.ovirt.org/#/c/44689/15/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py don't you need to do it in debian as well? Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Ondřej Svoboda has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15: Code-Review-1
(1 comment)
Description of vars.ssl_implementation lacks spaces.
https://gerrit.ovirt.org/#/c/44689/15/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 220: ' Please note that between parts of this string literal there is no implicit whitespace. Here, it hurts the most but a space at the end of each line would do.
Nir Soffer has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 161: [--enable-m2c], Line 162: [build using m2crypto @<:@default=yes@:>@] Line 163: )], Line 164: , Line 165: [enable_m2c="yes"]
Nir, could it be that the debain rules are overriding this configuration? W
I don't know about debian rules, but It is unlikely that they will modify the configuration. Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: Line 169: # Users and groups
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 161: [--enable-m2c], Line 162: [build using m2crypto @<:@default=yes@:>@] Line 163: )], Line 164: , Line 165: [enable_m2c="yes"]
I don't know about debian rules, but It is unlikely that they will modify t
as the code is right now - nothing will change in debian by this flag Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: Line 169: # Users and groups
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 15:
(4 comments)
https://gerrit.ovirt.org/#/c/44689/15/configure.ac File configure.ac:
Line 161: [--enable-m2c], Line 162: [build using m2crypto @<:@default=yes@:>@] Line 163: )], Line 164: , Line 165: [enable_m2c="yes"]
as the code is right now - nothing will change in debian by this flag
It will change. We call dh_auto_configure with --disable-m2c so it will set it config.py and it will be using ssl during build and runtime. Line 166: ) Line 167: AM_CONDITIONAL([M2C], [test "${enable_m2c}" = "yes"]) Line 168: Line 169: # Users and groups
https://gerrit.ovirt.org/#/c/44689/15/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 220: '
Please note that between parts of this string literal there is no implicit
Done
https://gerrit.ovirt.org/#/c/44689/15/tests/integration/jsonRpcHelper.py File tests/integration/jsonRpcHelper.py:
Line 38: from vdsm.config import config Line 39: from vdsm import utils Line 40: Line 41: if config.get('vars', 'ssl_implementation') == 'm2c': Line 42: from integration.m2chelper import DEAFAULT_SSL_CONTEXT
Piotr,
Yes, it someone by mistake will change setting in config.py it will fail Line 43: else: Line 44: from integration.sslhelper import DEAFAULT_SSL_CONTEXT Line 45: Line 46: PERMUTATIONS = tuple(product((True, False), ("xml", "stomp")))
https://gerrit.ovirt.org/#/c/44689/15/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
don't you need to do it in debian as well?
I need to do it just missed it. Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16: Verified+1
Added debian related changes so we can set config.py value for ssl standard module. I attempted to verify that vdsm is building on debian but failed due to cpopen.
I cherry-picked http://gerrit.ovirt.org/#/c/37737/ and attempted to build but configure failed on cpopen dependency. I cherry-picked cpopen project containing http://gerrit.ovirt.org/#/c/37926/ but cpopen build failed due to KeyError: 'CPOPEN_VERSIONi in UserDict.py line 23.
Simone I need your help to verify debian side of things. There were no changes to other parts of this patch so copying verification flag.
Ondřej Svoboda has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/44689/16/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 217: I believe this one shoulnd't be here.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/16/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 217:
I believe this one shoulnd't be here.
Done
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16: Code-Review-1
(4 comments)
https://gerrit.ovirt.org/#/c/44689/16/debian/control File debian/control:
Line 19 Line 20 Line 21 Line 22 Line 23 leave it..
https://gerrit.ovirt.org/#/c/44689/16/debian/rules File debian/rules:
Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py Line 37: dh_auto_build if you always do that why do you need the configure flag?? for fedora\rhel the default will be m2c and here ssl. Line 38: Line 39: destdir = debian/tmp Line 40: Line 41: override_dh_auto_install:
https://gerrit.ovirt.org/#/c/44689/16/tests/integration/jsonRpcHelper.py File tests/integration/jsonRpcHelper.py:
Line 41: if config.get('vars', 'ssl_implementation') == 'm2c': Line 42: from integration.m2chelper import DEAFAULT_SSL_CONTEXT Line 43: else: Line 44: from integration.sslhelper import DEAFAULT_SSL_CONTEXT Line 45: can't you use the sslcompact and all places to avoid this if else clause? Line 46: PERMUTATIONS = tuple(product((True, False), ("xml", "stomp"))) Line 47: Line 48: TIMEOUT = 3 Line 49:
https://gerrit.ovirt.org/#/c/44689/16/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py just put m2c as default. user will modify it manually if required. the global is redundant Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
(3 comments)
https://gerrit.ovirt.org/#/c/44689/16/debian/rules File debian/rules:
Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py Line 37: dh_auto_build
if you always do that why do you need the configure flag?? for fedora\rhel
For debian we are going to use ssl only due to issue in m2crypto so we do not need the flag. Line 38: Line 39: destdir = debian/tmp Line 40: Line 41: override_dh_auto_install:
https://gerrit.ovirt.org/#/c/44689/16/tests/integration/jsonRpcHelper.py File tests/integration/jsonRpcHelper.py:
Line 41: if config.get('vars', 'ssl_implementation') == 'm2c': Line 42: from integration.m2chelper import DEAFAULT_SSL_CONTEXT Line 43: else: Line 44: from integration.sslhelper import DEAFAULT_SSL_CONTEXT Line 45:
can't you use the sslcompact and all places to avoid this if else clause?
We are using different contexts for testing so we do not want to use sslcompat here. Please notice that we are using specific module here. Line 46: PERMUTATIONS = tuple(product((True, False), ("xml", "stomp"))) Line 47: Line 48: TIMEOUT = 3 Line 49:
https://gerrit.ovirt.org/#/c/44689/16/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
just put m2c as default. user will modify it manually if required. the glob
When we provide --enable-m2c or --disable-m2c we update config.py which seems to be better than manual update. Every manual step is error prone and it can confuse people. Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
(3 comments)
https://gerrit.ovirt.org/#/c/44689/16/debian/rules File debian/rules:
Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py Line 37: dh_auto_build
For debian we are going to use ssl only due to issue in m2crypto so we do n
so why to have the flag at all? Line 38: Line 39: destdir = debian/tmp Line 40: Line 41: override_dh_auto_install:
https://gerrit.ovirt.org/#/c/44689/16/tests/integration/jsonRpcHelper.py File tests/integration/jsonRpcHelper.py:
Line 41: if config.get('vars', 'ssl_implementation') == 'm2c': Line 42: from integration.m2chelper import DEAFAULT_SSL_CONTEXT Line 43: else: Line 44: from integration.sslhelper import DEAFAULT_SSL_CONTEXT Line 45:
We are using different contexts for testing so we do not want to use sslcom
ok Line 46: PERMUTATIONS = tuple(product((True, False), ("xml", "stomp"))) Line 47: Line 48: TIMEOUT = 3 Line 49:
https://gerrit.ovirt.org/#/c/44689/16/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
When we provide --enable-m2c or --disable-m2c we update config.py which see
also any additional flag that user can not be aware about- just keep fedora and rhel default to m2c and debian to ssl. that's it.. Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
(2 comments)
https://gerrit.ovirt.org/#/c/44689/16/debian/rules File debian/rules:
Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py Line 37: dh_auto_build
so why to have the flag at all?
The flag is here to help us. We can use configure to define which implementation we want to use for fedora/centos. Once the flag is set during build process we test our ssl code using one or the other ssl implementation. Without the flag we would need to make sure that when we build vdsm we test using both implementations which would complicate test code significantly. Line 38: Line 39: destdir = debian/tmp Line 40: Line 41: override_dh_auto_install:
https://gerrit.ovirt.org/#/c/44689/16/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
also any additional flag that user can not be aware about- just keep fedora
As you can see m2c is default for fedora but you can choose during the build/runtime to use ssl. Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 16:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/16/vdsm.spec.in File vdsm.spec.in:
Line 628: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 629: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 630: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 631: Line 632: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py
As you can see m2c is default for fedora but you can choose during the buil
I understand and I still insist that there is no need for that.. sorry Line 633: Line 634: %install Line 635: rm -rf %{buildroot} Line 636: make DESTDIR=%{buildroot} install
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 17:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 17: Verified+1
Fixed space but still we remove m2c as required. No other code changes.
Francesco Romani has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 17: Code-Review+1
I'm not infra/build expert, but from my perspective looks OK.
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 17: Code-Review-1
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 18:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 18: Verified+1
Verified by build vdsm with --enable-m2c and with --disable-m2c to set default implementation in config.py. Both options set to test communication with the engine.
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 18:
(1 comment)
Yaniv's comments fixed. Dan supports this approach.
https://gerrit.ovirt.org/#/c/44689/18/lib/yajsonrpc/betterAsyncore.py File lib/yajsonrpc/betterAsyncore.py:
Line 20: import asyncore Line 21: import socket Line 22: from errno import EWOULDBLOCK Line 23: Line 24: from vdsm.sslcompat import sslutils This changed due to patch https://gerrit.ovirt.org/#/c/46625/ which was merged recently. Line 25: from vdsm.infra.eventfd import EventFD Line 26: Line 27: Line 28: class Dispatcher(asyncore.dispatcher):
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 18: Code-Review-1
(5 comments)
https://gerrit.ovirt.org/#/c/44689/18//COMMIT_MSG Commit Message:
Line 9: During build process we can choose to set default ssl implementation Line 10: which will be used during runtime. We can change the value which Line 11: implementation is used in runtime by updating config.py with: Line 12: - m2c - m2crypto is used Line 13: - ssl - standard ssl module is used user should modify vdsm.conf and restart vdsm - config.py just declares the default values. now during build process over debian we set default to ssl and over fedora to m2c (mention it) and you need to remove the new --enable-m2c flag. user should work only with vdsm.conf to change the values and the default value is hard-coded set during the build Line 14: Line 15: This setting do not apply for debian where we use ssl only. Line 16: Line 17:
Line 11: implementation is used in runtime by updating config.py with: Line 12: - m2c - m2crypto is used Line 13: - ssl - standard ssl module is used Line 14: Line 15: This setting do not apply for debian where we use ssl only. I don't understand this statement Line 16: Line 17: Line 18: Change-Id: I9881d11e30ced9c34bfe602bba3d968f57e0fe15
https://gerrit.ovirt.org/#/c/44689/18/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 216: Line 217: ('ssl_implementation', '@SSl_IMPLEMENTATION@', Line 218: 'Specifies which ssl implementation should be used. ' Line 219: 'There are 2 options: ' Line 220: '"m2c" to use the m2crypto module (default) ' I don't see a reason to state that its a default, as the default changes between distributions.. Line 221: '"ssl" to use the standard python ssl module'), Line 222: ]), Line 223: Line 224: # Section: [mom]
https://gerrit.ovirt.org/#/c/44689/18/vdsm.spec.in File vdsm.spec.in:
Line 45: # M2C build - overrideable using rpmbuild --define "with_m2c 1" Line 46: %{!?with_m2c: %global with_m2c 0} Line 47: Line 48: %if %{with_m2c} Line 49: %global ssl_implementation 'm2c' in fedora always use m2c as default. if user want ssl he modified vdsm.conf Line 50: %else Line 51: %global ssl_implementation 'ssl' Line 52: %endif Line 53:
Line 620: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 621: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 622: Line 623: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py Line 624: why do we need it? over fedora (rpm installation) make it always m2c as default (as you do in debian with ssl) - if user wants to change the default he\she just need to add to vdsm.conf "ssl_implementation=ssl" forget about this build flag.. we don't need it Line 625: %install Line 626: rm -rf %{buildroot} Line 627: make DESTDIR=%{buildroot} install Line 628:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 18:
(5 comments)
https://gerrit.ovirt.org/#/c/44689/18//COMMIT_MSG Commit Message:
Line 9: During build process we can choose to set default ssl implementation Line 10: which will be used during runtime. We can change the value which Line 11: implementation is used in runtime by updating config.py with: Line 12: - m2c - m2crypto is used Line 13: - ssl - standard ssl module is used
user should modify vdsm.conf and restart vdsm - config.py just declares the
Done Line 14: Line 15: This setting do not apply for debian where we use ssl only. Line 16: Line 17:
Line 11: implementation is used in runtime by updating config.py with: Line 12: - m2c - m2crypto is used Line 13: - ssl - standard ssl module is used Line 14: Line 15: This setting do not apply for debian where we use ssl only.
I don't understand this statement
Will update Line 16: Line 17: Line 18: Change-Id: I9881d11e30ced9c34bfe602bba3d968f57e0fe15
https://gerrit.ovirt.org/#/c/44689/18/lib/vdsm/config.py.in File lib/vdsm/config.py.in:
Line 216: Line 217: ('ssl_implementation', '@SSl_IMPLEMENTATION@', Line 218: 'Specifies which ssl implementation should be used. ' Line 219: 'There are 2 options: ' Line 220: '"m2c" to use the m2crypto module (default) '
I don't see a reason to state that its a default, as the default changes be
Done Line 221: '"ssl" to use the standard python ssl module'), Line 222: ]), Line 223: Line 224: # Section: [mom]
https://gerrit.ovirt.org/#/c/44689/18/vdsm.spec.in File vdsm.spec.in:
Line 45: # M2C build - overrideable using rpmbuild --define "with_m2c 1" Line 46: %{!?with_m2c: %global with_m2c 0} Line 47: Line 48: %if %{with_m2c} Line 49: %global ssl_implementation 'm2c'
in fedora always use m2c as default. if user want ssl he modified vdsm.conf
Done Line 50: %else Line 51: %global ssl_implementation 'ssl' Line 52: %endif Line 53:
Line 620: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 621: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 622: Line 623: sed -i -e 's/@SSl_IMPLEMENTATION@/%{ssl_implementation}/g' lib/vdsm/config.py Line 624:
why do we need it? over fedora (rpm installation) make it always m2c as def
I will update. Line 625: %install Line 626: rm -rf %{buildroot} Line 627: make DESTDIR=%{buildroot} install Line 628:
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 19:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 19: Verified+1
Patch updated with removing build time configuration. No code changes. Verified during vdsm build and checking that ssl configuration works OK.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 20:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 21:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 21: Code-Review+1
you could say somewhere that default value that will be install in fedora is m2c and for debian is ssl
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 22:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 22: Verified+1
Fixed log issue.
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23: Code-Review+1
Yeela Kaplan has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23:
(1 comment)
One minor comment, other than that looks ok...
https://gerrit.ovirt.org/#/c/44689/23/debian/rules File debian/rules:
Line 32: REBOOT_PATH=/sbin/reboot \ Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py why is '@SSl_IMPLEMENTATION@' not all capital letters? Line 37: dh_auto_build Line 38: Line 39: destdir = debian/tmp Line 40:
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23: Code-Review-1
(1 comment)
please fix the constant name
https://gerrit.ovirt.org/#/c/44689/23/debian/rules File debian/rules:
Line 32: REBOOT_PATH=/sbin/reboot \ Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py
why is '@SSl_IMPLEMENTATION@' not all capital letters?
nice catch :) Line 37: dh_auto_build Line 38: Line 39: destdir = debian/tmp Line 40:
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/23/debian/rules File debian/rules:
Line 32: REBOOT_PATH=/sbin/reboot \ Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py
nice catch :)
and btw, having @something@ is quite misleading.. configure.ac replaces @x@ templates. here you do the magic trick yourself. just sayin... Line 37: dh_auto_build Line 38: Line 39: destdir = debian/tmp Line 40:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/23/debian/rules File debian/rules:
Line 32: REBOOT_PATH=/sbin/reboot \ Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py
and btw, having @something@ is quite misleading.. configure.ac replaces @x@
Will update Line 37: dh_auto_build Line 38: Line 39: destdir = debian/tmp Line 40:
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 23:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/23/debian/rules File debian/rules:
Line 32: REBOOT_PATH=/sbin/reboot \ Line 33: SERVICE_PATH=/usr/sbin/service Line 34: Line 35: override_dh_auto_build: Line 36: sed -i -e 's/@SSl_IMPLEMENTATION@/ssl/g' lib/vdsm/config.py
Will update
I wanted to be consistent with what other parts of the code are doing. Line 37: dh_auto_build Line 38: Line 39: destdir = debian/tmp Line 40:
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 24:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 24: Verified+1
Fixing token to use only capital letters. No code changes. Verified by running the build and seeing that configuration was properly updated.
Yaniv Bronhaim has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 24: Code-Review+1
Acked it - but still consider to use different format than @something@ which is used by the .in files.. lets see what others will say about it
Dan Kenigsberg has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 24: Code-Review+2
(1 comment)
https://gerrit.ovirt.org/#/c/44689/24/vdsm.spec.in File vdsm.spec.in:
Line 614: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 615: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 616: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 617: Line 618: sed -i -e 's/@SSL_IMPLEMENTATION@/m2c/g' lib/vdsm/config.py I don't mind doing this for the mean while. A future patch can make it a pretty flag to ./configure. Line 619: Line 620: %install Line 621: rm -rf %{buildroot} Line 622: make DESTDIR=%{buildroot} install
Piotr Kliczewski has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 24:
(1 comment)
https://gerrit.ovirt.org/#/c/44689/24/vdsm.spec.in File vdsm.spec.in:
Line 614: sed -i -e 's/^software_version =.*/software_version = "'"${baseversion}"'"/' \ Line 615: -e 's/^raw_version_revision =.*/raw_version_revision = "'"${rawversion}"'"/' \ Line 616: -e 's/^software_revision =.*/software_revision = "'"${baserelease}"'"/' vdsm/dsaversion.py Line 617: Line 618: sed -i -e 's/@SSL_IMPLEMENTATION@/m2c/g' lib/vdsm/config.py
I don't mind doing this for the mean while. A future patch can make it a pr
Please take a look at previous patch set. Yaniv asked to remove this flag. Line 619: Line 620: %install Line 621: rm -rf %{buildroot} Line 622: make DESTDIR=%{buildroot} install
Dan Kenigsberg has submitted this change and it was merged.
Change subject: ssl: runtime config to choose implementation ......................................................................
ssl: runtime config to choose implementation
During build process we provide which ssl implementation is set as default depending on distribution we are building for. We can change the configuration value used in runtime by updating config.py. Vdsm reboot is required after the update. There are two options supported - m2c - m2crypto is used - ssl - standard ssl module is used
For debian we only support standard ssl module.
Change-Id: I9881d11e30ced9c34bfe602bba3d968f57e0fe15 Signed-off-by: pkliczewski piotr.kliczewski@gmail.com Reviewed-on: https://gerrit.ovirt.org/44689 Continuous-Integration: Jenkins CI Reviewed-by: Yaniv Bronhaim ybronhei@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M debian/rules M debian/vdsm-python.install M lib/vdsm/Makefile.am M lib/vdsm/config.py.in M lib/vdsm/jsonrpcvdscli.py M lib/vdsm/m2cutils.py A lib/vdsm/sslcompat.py A lib/vdsm/sslutils.py M lib/vdsm/utils.py M lib/vdsm/vdscli.py M lib/yajsonrpc/betterAsyncore.py M lib/yajsonrpc/stompreactor.py M tests/crossImportsTests.py.in M tests/integration/Makefile.am M tests/integration/jsonRpcHelper.py M tests/integration/m2chelper.py A tests/integration/sslhelper.py M tests/protocoldetectorTests.py M tests/sslTests.py M tests/stompTests.py M tests/vdscliTests.py M vdsm.spec.in M vdsm/clientIF.py M vdsm/kaxmlrpclib.py M vdsm/protocoldetector.py M vdsm/virt/migration.py 26 files changed, 590 insertions(+), 98 deletions(-)
Approvals: Piotr Kliczewski: Verified Yaniv Bronhaim: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved
automation@ovirt.org has posted comments on this change.
Change subject: ssl: runtime config to choose implementation ......................................................................
Patch Set 25:
* Update tracker::IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Dan Kenigsberg -
fromani＠redhat.com -
Jenkins CI RO -
Nir Soffer -
osvoboda＠redhat.com -
Piotr Kliczewski -
Sandro Bonazzola -
ybronhei＠redhat.com -
ykaplan＠redhat.com