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 <[email protected]>
>
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
