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