| Message ID | 80d50f66d4311415dad5d9852cd3af2493028fad.camel@cloudandheat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] test-stream: Add ssl tests for stream open block | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Thu, 2023-04-27 at 18:27 +0200, Stefan Hoffmann wrote: > This tests stream.c and stream.py with ssl connection at > CHECK_STREAM_OPEN_BLOCK. > For the tests, ovsdb needs to be build with libssl. > > Signed-off-by: Stefan Hoffmann <stefan.hoffmann@cloudandheat.com> Some questions from myself: Are there other places, that should also get ssl or python checks and should be included here? Maybe something (python) client related. Besides that I add some comments to discuss my change below. > --- > tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- > tests/test-stream.c | 12 +++++++++++- > tests/test-stream.py | 6 ++++++ > 3 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index 5a7e76eaa..b40a167eb 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ > # specified). > ovsdb_start_idltest () { > ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? > - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile --remote=punix:socket ${1:+--remote=$1} db || return $? > + SSL_FLAGS="" > + if [[ "${1::4}" == "pssl" ]]; then > + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ > + --certificate=$PKIDIR/testpki-cert2.pem \ > + --ca-cert=$PKIDIR/testpki-cacert.pem" > + fi > + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ > + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ > + db || return $? > on_exit 'kill `cat ovsdb-server.pid`' > } I like this approach more than OVSDB_CHECK_IDL_SSL_PY, where the server is started at the m4_define section, still I could move the server start also to CHECK_STREAM_OPEN_BLOCK directly. > > @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], > [AT_SETUP([Check stream open block - $1 - $3]) > AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) > AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) > + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) > + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) > + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) > + $PYTHON3 -c "import ssl" > + SSL_PRESENT=$? > + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) > + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) > + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) > AT_KEYWORDS([ovsdb server stream open_block $3]) > - AT_CHECK([ovsdb_start_idltest "ptcp:0:$4"]) > + PROTOCOL=$3 > + PROTOCOL=${PROTOCOL::3} > + LISTEN_PROTOCOL=p$PROTOCOL > + PKIDIR=$abs_top_builddir/tests > + AT_CHECK([ovsdb_start_idltest "$LISTEN_PROTOCOL:0:$4"]) > PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) > WRONG_PORT=$(($TCP_PORT + 101)) > - AT_CHECK([$2 tcp:$4:$TCP_PORT], [0], [ignore]) > - AT_CHECK([$2 tcp:$4:$WRONG_PORT], [1], [ignore], [ignore]) > + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > + $PKIDIR/testpki-cacert.pem], > + [0], [ignore]) > + AT_CHECK([$2 $PROTOCOL:$4:$WRONG_PORT \ > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > + $PKIDIR/testpki-cacert.pem], > + [1], [ignore], [ignore]) > OVSDB_SERVER_SHUTDOWN > - AT_CHECK([$2 tcp:$4:$TCP_PORT], [1], [ignore], [ignore]) > + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > + $PKIDIR/testpki-cacert.pem], > + [1], [ignore], [ignore]) > AT_CLEANUP]) > > CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [tcp], [127.0.0.1]) > @@ -2295,6 +2324,12 @@ CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > [tcp], [127.0.0.1]) > CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > [tcp6], [[[::1]]]) > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl], [127.0.0.1]) > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl6], [[[::1]]]) > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > + [ssl], [127.0.0.1]) > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > + [ssl6], [[[::1]]]) Is it fine to add the tests this way or should I build a new CHECK_STREAM_OPEN_BLOCK_SSL (and maybe CHECK_STREAM_OPEN_BLOCK_TCP) section and merge it together, like OVSDB_CHECK_IDL_SSL_PY to OVSDB_CHECK_IDL. Upside would be, that we check if python ssl module is installed only at ssl tests, but we would need to define all the checks and server management twice. > > # same as OVSDB_CHECK_IDL but uses Python IDL implementation with tcp > # with multiple remotes to assert the idl connects to the leader of the Raft cluster > diff --git a/tests/test-stream.c b/tests/test-stream.c > index 68ce2c544..fb4af58b9 100644 > --- a/tests/test-stream.c > +++ b/tests/test-stream.c > @@ -19,6 +19,7 @@ > #include "fatal-signal.h" > #include "openvswitch/vlog.h" > #include "stream.h" > +#include "stream-ssl.h" > #include "util.h" > > VLOG_DEFINE_THIS_MODULE(test_stream); > @@ -33,7 +34,16 @@ main(int argc, char *argv[]) > set_program_name(argv[0]); > > if (argc < 2) { > - ovs_fatal(0, "usage: %s REMOTE", argv[0]); > + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", > + argv[0]); > + } > + if (strncmp("ssl:", argv[1], 4) == 0) { > + if (argc < 5) { > + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", > + argv[0]); > + } > + stream_ssl_set_ca_cert_file(argv[4], false); > + stream_ssl_set_key_and_cert(argv[2], argv[3]); > } > > error = stream_open_block(stream_open(argv[1], &stream, DSCP_DEFAULT), > diff --git a/tests/test-stream.py b/tests/test-stream.py > index 93d63c019..73f26d0b9 100644 > --- a/tests/test-stream.py > +++ b/tests/test-stream.py > @@ -19,6 +19,12 @@ import ovs.stream > > def main(argv): > remote = argv[1] > + > + if remote.startswith("ssl"): > + ovs.stream.SSLStream.ssl_set_ca_cert_file(argv[4]) > + ovs.stream.SSLStream.ssl_set_certificate_file(argv[3]) > + ovs.stream.SSLStream.ssl_set_private_key_file(argv[2]) > + > err, stream = ovs.stream.Stream.open_block( > ovs.stream.Stream.open(remote), 10000) >
On 4/28/23 10:16, Stefan Hoffmann wrote: > On Thu, 2023-04-27 at 18:27 +0200, Stefan Hoffmann wrote: >> This tests stream.c and stream.py with ssl connection at >> CHECK_STREAM_OPEN_BLOCK. >> For the tests, ovsdb needs to be build with libssl. >> >> Signed-off-by: Stefan Hoffmann <stefan.hoffmann@cloudandheat.com> > > Some questions from myself: > > Are there other places, that should also get ssl or python checks and > should be included here? Maybe something (python) client related. Most of the tests are executed for both C and Python. Python IDL doesn't support some features that C implementation does, so some tests are not running for Python. There might be a few that are running for C only, but can technically be checked with Python as well, but it needs some careful read of the testsuite to find them. We also have some tests for json and jsonrpc that are implemented for both C and Python in the similar way, but they are mostly abstracted from the underlying stream implementation. We don't have a lot of negative tests, i.e. failure simulations, but these are hard to do. Mocking is a way, but it's only for Python, so should live in the pytest realm, I think. > > Besides that I add some comments to discuss my change below. > >> --- >> tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- >> tests/test-stream.c | 12 +++++++++++- >> tests/test-stream.py | 6 ++++++ >> 3 files changed, 57 insertions(+), 6 deletions(-) >> >> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at >> index 5a7e76eaa..b40a167eb 100644 >> --- a/tests/ovsdb-idl.at >> +++ b/tests/ovsdb-idl.at >> @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ >> # specified). >> ovsdb_start_idltest () { >> ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? >> - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile --remote=punix:socket ${1:+--remote=$1} db || return $? >> + SSL_FLAGS="" >> + if [[ "${1::4}" == "pssl" ]]; then >> + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ >> + --certificate=$PKIDIR/testpki-cert2.pem \ >> + --ca-cert=$PKIDIR/testpki-cacert.pem" We need to define the PKIDIR in this function for completeness, I think. Even though it is already defined in the CHECK_STREAM_OPEN_BLOCK, it's not defined in other users of ovsdb_start_idltest. >> + fi >> + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ >> + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ >> + db || return $? >> on_exit 'kill `cat ovsdb-server.pid`' >> } > > I like this approach more than OVSDB_CHECK_IDL_SSL_PY, where the server > is started at the m4_define section, still I could move the server > start also to CHECK_STREAM_OPEN_BLOCK directly. It's actually better to avoid shell scripting if possible. m4 macros are playing much more nicely with the autotest. Shell commands will not be visible in the testsuite log and it's hard to figure out what exactly failed in the shell script for that reason. In this particular case, the code is split into a function to avoid huge code duplication. Ideally, though, this whole function should be replaced with a macro definition and all the calls inside should be wrapped into AT_CHECK, the 'if' should be an m4_if and the ${1:+--remote=$1} should be an m4_if as well. And all the '\' should be 'dnl' instead ('\' breaks command logging in the autotest). Some things are just easier to write with shell scripts though. Some things are just legacy code that doesn't worth being re-written just for the looks or minor debugability improvements. > >> >> @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], >> [AT_SETUP([Check stream open block - $1 - $3]) >> AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) >> AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) >> + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) >> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) >> + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) >> + $PYTHON3 -c "import ssl" >> + SSL_PRESENT=$? >> + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) >> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) >> + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) We also need to check for a case where python is built without SSL support. OVSDB_CHECK_IDL_SSL_PY does that by trying to import ssl. >> AT_KEYWORDS([ovsdb server stream open_block $3]) >> - AT_CHECK([ovsdb_start_idltest "ptcp:0:$4"]) >> + PROTOCOL=$3 >> + PROTOCOL=${PROTOCOL::3} >> + LISTEN_PROTOCOL=p$PROTOCOL >> + PKIDIR=$abs_top_builddir/tests >> + AT_CHECK([ovsdb_start_idltest "$LISTEN_PROTOCOL:0:$4"]) >> PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) >> WRONG_PORT=$(($TCP_PORT + 101)) >> - AT_CHECK([$2 tcp:$4:$TCP_PORT], [0], [ignore]) >> - AT_CHECK([$2 tcp:$4:$WRONG_PORT], [1], [ignore], [ignore]) >> + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ >> + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ >> + $PKIDIR/testpki-cacert.pem], These ssl args can be stored in a variable and re-used instead of repeating them 3 times below. >> + [0], [ignore]) >> + AT_CHECK([$2 $PROTOCOL:$4:$WRONG_PORT \ >> + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ >> + $PKIDIR/testpki-cacert.pem], >> + [1], [ignore], [ignore]) >> OVSDB_SERVER_SHUTDOWN >> - AT_CHECK([$2 tcp:$4:$TCP_PORT], [1], [ignore], [ignore]) >> + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ >> + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ >> + $PKIDIR/testpki-cacert.pem], >> + [1], [ignore], [ignore]) >> AT_CLEANUP]) >> >> CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [tcp], [127.0.0.1]) >> @@ -2295,6 +2324,12 @@ CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], >> [tcp], [127.0.0.1]) >> CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], >> [tcp6], [[[::1]]]) >> +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl], [127.0.0.1]) >> +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl6], [[[::1]]]) >> +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], >> + [ssl], [127.0.0.1]) >> +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], >> + [ssl6], [[[::1]]]) > > Is it fine to add the tests this way or should I build a new > CHECK_STREAM_OPEN_BLOCK_SSL (and maybe CHECK_STREAM_OPEN_BLOCK_TCP) > section and merge it together, like OVSDB_CHECK_IDL_SSL_PY to > OVSDB_CHECK_IDL. > Upside would be, that we check if python ssl module is installed only > at ssl tests, but we would need to define all the checks and server > management twice. The reason why the OVSDB_CHECK_IDL consists of many separate implementations is that most of them are sightly different, i.e. use different flags, or different programs with different format of arguments. In the stream test we're designing test programs from scratch and it's easier to have formats exactly the same. We could have combined implementations of OVSDB_CHECK_IDL by adding more checks and arguments, but, in general, it's just a trade-off between code duplication and code complexity. As long as we do not copy-paste huge chunks of code, it's fine to duplicate a little for the sake of readability. > >> >> # same as OVSDB_CHECK_IDL but uses Python IDL implementation with tcp >> # with multiple remotes to assert the idl connects to the leader of the Raft cluster >> diff --git a/tests/test-stream.c b/tests/test-stream.c >> index 68ce2c544..fb4af58b9 100644 >> --- a/tests/test-stream.c >> +++ b/tests/test-stream.c >> @@ -19,6 +19,7 @@ >> #include "fatal-signal.h" >> #include "openvswitch/vlog.h" >> #include "stream.h" >> +#include "stream-ssl.h" >> #include "util.h" >> >> VLOG_DEFINE_THIS_MODULE(test_stream); >> @@ -33,7 +34,16 @@ main(int argc, char *argv[]) >> set_program_name(argv[0]); >> >> if (argc < 2) { >> - ovs_fatal(0, "usage: %s REMOTE", argv[0]); >> + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", >> + argv[0]); >> + } >> + if (strncmp("ssl:", argv[1], 4) == 0) { >> + if (argc < 5) { >> + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", >> + argv[0]); >> + } >> + stream_ssl_set_ca_cert_file(argv[4], false); >> + stream_ssl_set_key_and_cert(argv[2], argv[3]); >> } >> >> error = stream_open_block(stream_open(argv[1], &stream, DSCP_DEFAULT), >> diff --git a/tests/test-stream.py b/tests/test-stream.py >> index 93d63c019..73f26d0b9 100644 >> --- a/tests/test-stream.py >> +++ b/tests/test-stream.py >> @@ -19,6 +19,12 @@ import ovs.stream >> >> def main(argv): >> remote = argv[1] >> + >> + if remote.startswith("ssl"): >> + ovs.stream.SSLStream.ssl_set_ca_cert_file(argv[4]) >> + ovs.stream.SSLStream.ssl_set_certificate_file(argv[3]) >> + ovs.stream.SSLStream.ssl_set_private_key_file(argv[2]) We should, probably, add a semicolon to the match and have similar argument count checks here as we do in the C version. Best regards, Ilya Maximets.
On Fri, 2023-04-28 at 15:12 +0200, Ilya Maximets wrote: > On 4/28/23 10:16, Stefan Hoffmann wrote: > > On Thu, 2023-04-27 at 18:27 +0200, Stefan Hoffmann wrote: > > > This tests stream.c and stream.py with ssl connection at > > > CHECK_STREAM_OPEN_BLOCK. > > > For the tests, ovsdb needs to be build with libssl. > > > > > > Signed-off-by: Stefan Hoffmann <stefan.hoffmann@cloudandheat.com> > > > > Some questions from myself: > > > > Are there other places, that should also get ssl or python checks and > > should be included here? Maybe something (python) client related. > > Most of the tests are executed for both C and Python. Python IDL doesn't > support some features that C implementation does, so some tests are not > running for Python. There might be a few that are running for C only, but > can technically be checked with Python as well, but it needs some careful > read of the testsuite to find them. > > We also have some tests for json and jsonrpc that are implemented for both > C and Python in the similar way, but they are mostly abstracted from the > underlying stream implementation. > > We don't have a lot of negative tests, i.e. failure simulations, but > these are hard to do. Mocking is a way, but it's only for Python, so > should live in the pytest realm, I think. > > > > > Besides that I add some comments to discuss my change below. > > > > > --- > > > tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- > > > tests/test-stream.c | 12 +++++++++++- > > > tests/test-stream.py | 6 ++++++ > > > 3 files changed, 57 insertions(+), 6 deletions(-) > > > > > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > > > index 5a7e76eaa..b40a167eb 100644 > > > --- a/tests/ovsdb-idl.at > > > +++ b/tests/ovsdb-idl.at > > > @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ > > > # specified). > > > ovsdb_start_idltest () { > > > ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? > > > - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile --remote=punix:socket ${1:+--remote=$1} db || return $? > > > + SSL_FLAGS="" > > > + if [[ "${1::4}" == "pssl" ]]; then > > > + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ > > > + --certificate=$PKIDIR/testpki-cert2.pem \ > > > + --ca-cert=$PKIDIR/testpki-cacert.pem" > > We need to define the PKIDIR in this function for completeness, > I think. Even though it is already defined in the CHECK_STREAM_OPEN_BLOCK, > it's not defined in other users of ovsdb_start_idltest. > > > > + fi > > > + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ > > > + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ > > > + db || return $? > > > on_exit 'kill `cat ovsdb-server.pid`' > > > } > > > > I like this approach more than OVSDB_CHECK_IDL_SSL_PY, where the server > > is started at the m4_define section, still I could move the server > > start also to CHECK_STREAM_OPEN_BLOCK directly. > > It's actually better to avoid shell scripting if possible. m4 macros > are playing much more nicely with the autotest. Shell commands will > not be visible in the testsuite log and it's hard to figure out what > exactly failed in the shell script for that reason. > > In this particular case, the code is split into a function to avoid > huge code duplication. Ideally, though, this whole function should > be replaced with a macro definition and all the calls inside should > be wrapped into AT_CHECK, the 'if' should be an m4_if and the > ${1:+--remote=$1} should be an m4_if as well. And all the '\' should > be 'dnl' instead ('\' breaks command logging in the autotest). > Some things are just easier to write with shell scripts though. > Some things are just legacy code that doesn't worth being re-written > just for the looks or minor debugability improvements. Thanks for explaining. Should I introduce a m4 macro than, that we can use later to replace ovsdb_start_idltest or keep it, as it is for now? > > > > > > > > > @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], > > > [AT_SETUP([Check stream open block - $1 - $3]) > > > AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) > > > AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) > > > + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) > > > + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) > > > + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) > > > + $PYTHON3 -c "import ssl" > > > + SSL_PRESENT=$? > > > + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) > > > + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) > > > + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) > > We also need to check for a case where python is built without SSL > support. OVSDB_CHECK_IDL_SSL_PY does that by trying to import ssl. This is done 5 lines above, I copied that part from OVSDB_CHECK_IDL_SSL_PY. > > > > AT_KEYWORDS([ovsdb server stream open_block $3]) > > > - AT_CHECK([ovsdb_start_idltest "ptcp:0:$4"]) > > > + PROTOCOL=$3 > > > + PROTOCOL=${PROTOCOL::3} > > > + LISTEN_PROTOCOL=p$PROTOCOL > > > + PKIDIR=$abs_top_builddir/tests > > > + AT_CHECK([ovsdb_start_idltest "$LISTEN_PROTOCOL:0:$4"]) > > > PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) > > > WRONG_PORT=$(($TCP_PORT + 101)) > > > - AT_CHECK([$2 tcp:$4:$TCP_PORT], [0], [ignore]) > > > - AT_CHECK([$2 tcp:$4:$WRONG_PORT], [1], [ignore], [ignore]) > > > + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ > > > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > > > + $PKIDIR/testpki-cacert.pem], > > These ssl args can be stored in a variable and re-used instead of > repeating them 3 times below. > > > > + [0], [ignore]) > > > + AT_CHECK([$2 $PROTOCOL:$4:$WRONG_PORT \ > > > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > > > + $PKIDIR/testpki-cacert.pem], > > > + [1], [ignore], [ignore]) > > > OVSDB_SERVER_SHUTDOWN > > > - AT_CHECK([$2 tcp:$4:$TCP_PORT], [1], [ignore], [ignore]) > > > + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ > > > + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ > > > + $PKIDIR/testpki-cacert.pem], > > > + [1], [ignore], [ignore]) > > > AT_CLEANUP]) > > > > > > CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [tcp], [127.0.0.1]) > > > @@ -2295,6 +2324,12 @@ CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > > > [tcp], [127.0.0.1]) > > > CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > > > [tcp6], [[[::1]]]) > > > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl], [127.0.0.1]) > > > +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl6], [[[::1]]]) > > > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > > > + [ssl], [127.0.0.1]) > > > +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], > > > + [ssl6], [[[::1]]]) > > > > Is it fine to add the tests this way or should I build a new > > CHECK_STREAM_OPEN_BLOCK_SSL (and maybe CHECK_STREAM_OPEN_BLOCK_TCP) > > section and merge it together, like OVSDB_CHECK_IDL_SSL_PY to > > OVSDB_CHECK_IDL. > > Upside would be, that we check if python ssl module is installed only > > at ssl tests, but we would need to define all the checks and server > > management twice. > > The reason why the OVSDB_CHECK_IDL consists of many separate implementations > is that most of them are sightly different, i.e. use different flags, or > different programs with different format of arguments. In the stream test > we're designing test programs from scratch and it's easier to have formats > exactly the same. > > We could have combined implementations of OVSDB_CHECK_IDL by adding more > checks and arguments, but, in general, it's just a trade-off between code > duplication and code complexity. As long as we do not copy-paste huge chunks > of code, it's fine to duplicate a little for the sake of readability. > > > > > > > > > # same as OVSDB_CHECK_IDL but uses Python IDL implementation with tcp > > > # with multiple remotes to assert the idl connects to the leader of the Raft cluster > > > diff --git a/tests/test-stream.c b/tests/test-stream.c > > > index 68ce2c544..fb4af58b9 100644 > > > --- a/tests/test-stream.c > > > +++ b/tests/test-stream.c > > > @@ -19,6 +19,7 @@ > > > #include "fatal-signal.h" > > > #include "openvswitch/vlog.h" > > > #include "stream.h" > > > +#include "stream-ssl.h" > > > #include "util.h" > > > > > > VLOG_DEFINE_THIS_MODULE(test_stream); > > > @@ -33,7 +34,16 @@ main(int argc, char *argv[]) > > > set_program_name(argv[0]); > > > > > > if (argc < 2) { > > > - ovs_fatal(0, "usage: %s REMOTE", argv[0]); > > > + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", > > > + argv[0]); > > > + } > > > + if (strncmp("ssl:", argv[1], 4) == 0) { > > > + if (argc < 5) { > > > + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", > > > + argv[0]); > > > + } > > > + stream_ssl_set_ca_cert_file(argv[4], false); > > > + stream_ssl_set_key_and_cert(argv[2], argv[3]); > > > } > > > > > > error = stream_open_block(stream_open(argv[1], &stream, DSCP_DEFAULT), > > > diff --git a/tests/test-stream.py b/tests/test-stream.py > > > index 93d63c019..73f26d0b9 100644 > > > --- a/tests/test-stream.py > > > +++ b/tests/test-stream.py > > > @@ -19,6 +19,12 @@ import ovs.stream > > > > > > def main(argv): > > > remote = argv[1] > > > + > > > + if remote.startswith("ssl"): > > > + ovs.stream.SSLStream.ssl_set_ca_cert_file(argv[4]) > > > + ovs.stream.SSLStream.ssl_set_certificate_file(argv[3]) > > > + ovs.stream.SSLStream.ssl_set_private_key_file(argv[2]) > > We should, probably, add a semicolon to the match and have similar > argument count checks here as we do in the C version. > > Best regards, Ilya Maximets. Thanks, Stefan
On 4/28/23 15:27, Stefan Hoffmann wrote: > On Fri, 2023-04-28 at 15:12 +0200, Ilya Maximets wrote: >> On 4/28/23 10:16, Stefan Hoffmann wrote: >>> On Thu, 2023-04-27 at 18:27 +0200, Stefan Hoffmann wrote: >>>> This tests stream.c and stream.py with ssl connection at >>>> CHECK_STREAM_OPEN_BLOCK. >>>> For the tests, ovsdb needs to be build with libssl. >>>> >>>> Signed-off-by: Stefan Hoffmann <stefan.hoffmann@cloudandheat.com> >>> >>> Some questions from myself: >>> >>> Are there other places, that should also get ssl or python checks and >>> should be included here? Maybe something (python) client related. >> >> Most of the tests are executed for both C and Python. Python IDL doesn't >> support some features that C implementation does, so some tests are not >> running for Python. There might be a few that are running for C only, but >> can technically be checked with Python as well, but it needs some careful >> read of the testsuite to find them. >> >> We also have some tests for json and jsonrpc that are implemented for both >> C and Python in the similar way, but they are mostly abstracted from the >> underlying stream implementation. >> >> We don't have a lot of negative tests, i.e. failure simulations, but >> these are hard to do. Mocking is a way, but it's only for Python, so >> should live in the pytest realm, I think. >> >>> >>> Besides that I add some comments to discuss my change below. >>> >>>> --- >>>> tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- >>>> tests/test-stream.c | 12 +++++++++++- >>>> tests/test-stream.py | 6 ++++++ >>>> 3 files changed, 57 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at >>>> index 5a7e76eaa..b40a167eb 100644 >>>> --- a/tests/ovsdb-idl.at >>>> +++ b/tests/ovsdb-idl.at >>>> @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ >>>> # specified). >>>> ovsdb_start_idltest () { >>>> ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? >>>> - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile --remote=punix:socket ${1:+--remote=$1} db || return $? >>>> + SSL_FLAGS="" >>>> + if [[ "${1::4}" == "pssl" ]]; then >>>> + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ >>>> + --certificate=$PKIDIR/testpki-cert2.pem \ >>>> + --ca-cert=$PKIDIR/testpki-cacert.pem" >> >> We need to define the PKIDIR in this function for completeness, >> I think. Even though it is already defined in the CHECK_STREAM_OPEN_BLOCK, >> it's not defined in other users of ovsdb_start_idltest. >> >>>> + fi >>>> + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ >>>> + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ >>>> + db || return $? >>>> on_exit 'kill `cat ovsdb-server.pid`' >>>> } >>> >>> I like this approach more than OVSDB_CHECK_IDL_SSL_PY, where the server >>> is started at the m4_define section, still I could move the server >>> start also to CHECK_STREAM_OPEN_BLOCK directly. >> >> It's actually better to avoid shell scripting if possible. m4 macros >> are playing much more nicely with the autotest. Shell commands will >> not be visible in the testsuite log and it's hard to figure out what >> exactly failed in the shell script for that reason. >> >> In this particular case, the code is split into a function to avoid >> huge code duplication. Ideally, though, this whole function should >> be replaced with a macro definition and all the calls inside should >> be wrapped into AT_CHECK, the 'if' should be an m4_if and the >> ${1:+--remote=$1} should be an m4_if as well. And all the '\' should >> be 'dnl' instead ('\' breaks command logging in the autotest). >> Some things are just easier to write with shell scripts though. >> Some things are just legacy code that doesn't worth being re-written >> just for the looks or minor debugability improvements. > > Thanks for explaining. > Should I introduce a m4 macro than, that we can use later to replace > ovsdb_start_idltest or keep it, as it is for now? I'd say either keep the shell function as is or replace it entirely with a macro. See the OVSDB_CLUSTER_START_IDLTEST for example. Replacing will require changes in all the callers. And the macro should be defined outside of m4_divert_text. Both options are fine by me. In case of replacing, you may turn the change into a patch set with the first patch preforming some ground work on converting the function into a macro and the second patch adding SSL stream tests. Half-measure of introducing the macro but keeping the shell function doesn't sound great to me. > >> >>> >>>> >>>> @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], >>>> [AT_SETUP([Check stream open block - $1 - $3]) >>>> AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) >>>> AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) >>>> + $PYTHON3 -c "import ssl" >>>> + SSL_PRESENT=$? >>>> + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) >> >> We also need to check for a case where python is built without SSL >> support. OVSDB_CHECK_IDL_SSL_PY does that by trying to import ssl. > > This is done 5 lines above, I copied that part from > OVSDB_CHECK_IDL_SSL_PY. Oh, sorry. Missed that part. Best regards, Ilya Maximets.
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at index 5a7e76eaa..b40a167eb 100644 --- a/tests/ovsdb-idl.at +++ b/tests/ovsdb-idl.at @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ # specified). ovsdb_start_idltest () { ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile --remote=punix:socket ${1:+--remote=$1} db || return $? + SSL_FLAGS="" + if [[ "${1::4}" == "pssl" ]]; then + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ + --certificate=$PKIDIR/testpki-cert2.pem \ + --ca-cert=$PKIDIR/testpki-cacert.pem" + fi + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ + db || return $? on_exit 'kill `cat ovsdb-server.pid`' } @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], [AT_SETUP([Check stream open block - $1 - $3]) AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) + $PYTHON3 -c "import ssl" + SSL_PRESENT=$? + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) AT_KEYWORDS([ovsdb server stream open_block $3]) - AT_CHECK([ovsdb_start_idltest "ptcp:0:$4"]) + PROTOCOL=$3 + PROTOCOL=${PROTOCOL::3} + LISTEN_PROTOCOL=p$PROTOCOL + PKIDIR=$abs_top_builddir/tests + AT_CHECK([ovsdb_start_idltest "$LISTEN_PROTOCOL:0:$4"]) PARSE_LISTENING_PORT([ovsdb-server.log], [TCP_PORT]) WRONG_PORT=$(($TCP_PORT + 101)) - AT_CHECK([$2 tcp:$4:$TCP_PORT], [0], [ignore]) - AT_CHECK([$2 tcp:$4:$WRONG_PORT], [1], [ignore], [ignore]) + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ + $PKIDIR/testpki-cacert.pem], + [0], [ignore]) + AT_CHECK([$2 $PROTOCOL:$4:$WRONG_PORT \ + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ + $PKIDIR/testpki-cacert.pem], + [1], [ignore], [ignore]) OVSDB_SERVER_SHUTDOWN - AT_CHECK([$2 tcp:$4:$TCP_PORT], [1], [ignore], [ignore]) + AT_CHECK([$2 $PROTOCOL:$4:$TCP_PORT \ + $PKIDIR/testpki-privkey.pem $PKIDIR/testpki-cert.pem \ + $PKIDIR/testpki-cacert.pem], + [1], [ignore], [ignore]) AT_CLEANUP]) CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [tcp], [127.0.0.1]) @@ -2295,6 +2324,12 @@ CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], [tcp], [127.0.0.1]) CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], [tcp6], [[[::1]]]) +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl], [127.0.0.1]) +CHECK_STREAM_OPEN_BLOCK([C], [test-stream], [ssl6], [[[::1]]]) +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], + [ssl], [127.0.0.1]) +CHECK_STREAM_OPEN_BLOCK([Python3], [$PYTHON3 $srcdir/test-stream.py], + [ssl6], [[[::1]]]) # same as OVSDB_CHECK_IDL but uses Python IDL implementation with tcp # with multiple remotes to assert the idl connects to the leader of the Raft cluster diff --git a/tests/test-stream.c b/tests/test-stream.c index 68ce2c544..fb4af58b9 100644 --- a/tests/test-stream.c +++ b/tests/test-stream.c @@ -19,6 +19,7 @@ #include "fatal-signal.h" #include "openvswitch/vlog.h" #include "stream.h" +#include "stream-ssl.h" #include "util.h" VLOG_DEFINE_THIS_MODULE(test_stream); @@ -33,7 +34,16 @@ main(int argc, char *argv[]) set_program_name(argv[0]); if (argc < 2) { - ovs_fatal(0, "usage: %s REMOTE", argv[0]); + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", + argv[0]); + } + if (strncmp("ssl:", argv[1], 4) == 0) { + if (argc < 5) { + ovs_fatal(0, "usage: %s REMOTE [SSL_KEY] [SSL_CERT] [SSL_CA]", + argv[0]); + } + stream_ssl_set_ca_cert_file(argv[4], false); + stream_ssl_set_key_and_cert(argv[2], argv[3]); } error = stream_open_block(stream_open(argv[1], &stream, DSCP_DEFAULT), diff --git a/tests/test-stream.py b/tests/test-stream.py index 93d63c019..73f26d0b9 100644 --- a/tests/test-stream.py +++ b/tests/test-stream.py @@ -19,6 +19,12 @@ import ovs.stream def main(argv): remote = argv[1] + + if remote.startswith("ssl"): + ovs.stream.SSLStream.ssl_set_ca_cert_file(argv[4]) + ovs.stream.SSLStream.ssl_set_certificate_file(argv[3]) + ovs.stream.SSLStream.ssl_set_private_key_file(argv[2]) + err, stream = ovs.stream.Stream.open_block( ovs.stream.Stream.open(remote), 10000)
This tests stream.c and stream.py with ssl connection at CHECK_STREAM_OPEN_BLOCK. For the tests, ovsdb needs to be build with libssl. Signed-off-by: Stefan Hoffmann <stefan.hoffmann@cloudandheat.com> --- tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- tests/test-stream.c | 12 +++++++++++- tests/test-stream.py | 6 ++++++ 3 files changed, 57 insertions(+), 6 deletions(-)