Hi Ales, Thanks for the feedback and proposal. After some offline discussion the proposal would be: - Have the multinode test use the defaults from the ovn-fake-multinode i.e. use TLS. - Cert path is hardcoded in ovn-fake-multinode, so no need to detect the cert path in the multinode tests - Have the multinode test support an ENABLE_SSL option so that we can still run (locally, on laptop) the tests if the ovn-fake-multinode was started w/ ENABLE_SSL=no. - Change the ci to use the default (i.e. remove ENABLE_SSL=no from the build)
I'll send v2 based on this. Thanks Xavier On Fri, Jul 11, 2025 at 8:49 AM Ales Musil <[email protected]> wrote: > > > On Thu, Jul 10, 2025 at 5:30 PM Xavier Simonart <[email protected]> > wrote: > >> Hi Ales >> >> Thanks for the review and comments. >> I have some comments embedded below >> Thanks >> Xavier >> >> On Tue, Jul 8, 2025 at 1:39 PM Ales Musil <[email protected]> wrote: >> >>> >>> >>> On Wed, Jul 2, 2025 at 11:50 AM Xavier Simonart via dev < >>> [email protected]> wrote: >>> >>>> Test "ovn multinode - Transit Router basic functionality" stops >>>> ovn-controller, restarts it using local sb, and finally, restarts it >>>> using ovn-central sb using tcp. >>>> If the cluster was started with ENABLE_SSL="yes", then all following >>>> tests >>>> were failing. >>>> This patch ensures that ssl continue to be used if it was initially >>>> used. >>>> >>>> Fixes: 7c3f7f415f1d ("northd, controller: Flood ARP and NA packet on >>>> transit router.") >>>> Signed-off-by: Xavier Simonart <[email protected]> >>>> --- >>>> >>> >>> Hi Xavier, >>> >>> thank you for the patch. I can see the following comment >>> in the ovn-fake-multinode-tests.yml: >>> >>> "# Disable SSL/TLS for now. Revisit this if required." >>> >> Yes, that's true for the CI. >> But mutinode tests can also be run locally (on our laptop), and nothing >> in the multinode tests themselves (multinode.at) suggest that they >> should not use ENABLE_SSL. >> It also worked fine until recently. >> >>> >>> That kinda suggests that the CI wasn't meant to run with SSL enabled, >>> I'm fine with adding proper support I just wonder if we could do that >>> slightly differently. See down below. >>> >>> tests/multinode-macros.at | 43 ++++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 38 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/tests/multinode-macros.at b/tests/multinode-macros.at >>>> index 3f610f2e0..c349a36be 100644 >>>> --- a/tests/multinode-macros.at >>>> +++ b/tests/multinode-macros.at >>>> @@ -89,6 +89,37 @@ check_fake_multinode_setup_by_nodes() { >>>> on_exit "m_as $c ovs-ofctl dump-flows br-int > flow-${c}.txt" >>>> on_exit "m_as $c ovs-vsctl get open . external_ids > >>>> extids-${c}.txt" >>>> done >>>> + >>>> + # Only check ssl on one chassis - whole setup uses ssl or not. >>>> + if [[ -z "${REMOTE_PROT}" ]]; then >>>> + enable_ssl=$(m_as $c ps -efww | grep ovn-controller | grep >>>> private-key) >>>> + if [[ -n "$enable_ssl" ]]; then >>>> >>> >>> We have the 'ENABLE_SSL' env variable we could check. With that this can >>> be >>> actually part of 'multinode_setup_northd()' and >>> 'multinode_setup_controller()'. >>> >> I would be easier & nicer, but this is only true for the CI. If running >> locally, we might not have this variable available. >> So I do not think that we can rely on ENABLE_SSL, and this is why I think >> that we need to find out how the cluster was built. >> >>> >>> >>>> + private_key=$(m_as $c ps -efww | grep ovn-controller | sed >>>> -n 's/.*--private-key=\([[^ ]]*\).*/\1/p') >>>> + ssl_cert=$(m_as $c ps -efww | grep ovn-controller | sed -n >>>> 's/.*--certificate=\([[^ ]]*\).*/\1/p') >>>> + ssl_ca_cert=$(m_as $c ps -efww | grep ovn-controller | sed >>>> -n 's/.*--ca-cert=\([[^ ]]*\).*/\1/p') >>>> >>> >>> ovn-fake-multinode uses 'SSL_CERTS_PATH' env variable which is then >>> expanded to >>> proper arguments. I see as an option to utilize this env variable >>> (making it changeable >>> in ovn-fake-multinode) and setting it in ovn-fake-multinode-tests.yml. >>> We wouldn't >>> have to parse the arguments. >>> >> Same comment as above: this variable might not be available when we run >> the tests locally. >> We could also have the ovn-fake-multinode exporting the environment >> variables in the different containers. >> WDYT? >> > > Hi Xavier, > > that's a fair point, considering that we have pinned > ovn-fake-multinode version we could do the opposite, consider that > ENABLE_SSL is "true" which is the case for v0.4 and use the env > variable to do the setup without TLS. Then the defaults are aligned > and we don't have to detect anything special. You can always disable > it locally when needed. If the default ever changes we can adjust the > code when we move to a tag with a different default. WDYT? > > > >>> + CONTROLLER_SSL_ARGS="--ovn-controller-ssl-key=$private_key \ >>>> + --ovn-controller-ssl-cert=$ssl_cert \ >>>> + >>>> --ovn-controller-ssl-ca-cert=$ssl_ca_cert" >>>> + NORTHD_SSL_ARGS="--ovn-nb-db-ssl-key=$private_key \ >>>> + --ovn-nb-db-ssl-cert=$ssl_cert \ >>>> + --ovn-nb-db-ssl-ca-cert=$ssl_ca_cert \ >>>> + --ovn-sb-db-ssl-key=$private_key \ >>>> + --ovn-sb-db-ssl-cert=$ssl_cert \ >>>> + --ovn-sb-db-ssl-ca-cert=$ssl_ca_cert \ >>>> + --ovn-northd-ssl-key=$private_key \ >>>> + --ovn-northd-ssl-cert=$ssl_cert \ >>>> + --ovn-northd-ssl-ca-cert=$ssl_ca_cert" >>>> + >>>> + REMOTE_PROT=ssl >>>> + else >>>> + CONTROLLER_SSL_ARGS="" >>>> + NORTHD_SSL_ARGS="" >>>> + REMOTE_PROT=tcp >>>> + fi >>>> + export CONTROLLER_SSL_ARGS >>>> + export NORTHD_SSL_ARGS >>>> + export REMOTE_PROT >>>> + fi >>>> } >>>> >>>> check_fake_multinode_setup() { >>>> @@ -128,9 +159,10 @@ multinode_setup_northd() { >>>> >>>> multinode_cleanup_northd $c >>>> >>>> - m_as $c /usr/share/ovn/scripts/ovn-ctl start_northd >>>> - m_as $c ovn-nbctl set-connection ptcp:6641 >>>> - m_as $c ovn-sbctl set-connection ptcp:6642 >>>> + echo "Using ${NORTHD_SSL_ARGS} for northd". >>>> + m_as $c /usr/share/ovn/scripts/ovn-ctl start_northd >>>> ${NORTHD_SSL_ARGS} >>>> + m_as $c ovn-nbctl set-connection p${REMOTE_PROT}:6641 >>>> + m_as $c ovn-sbctl set-connection p${REMOTE_PROT}:6642 >>>> } >>>> >>>> # multinode_setup_controller NODE ENCAP_IP REMOTE_IP [ENCAP_TYPE] >>>> @@ -148,11 +180,12 @@ multinode_setup_controller() { >>>> m_as $c sh -c "rm -f /etc/openvswitch/*.db" >>>> >>>> m_as $c /usr/share/openvswitch/scripts/ovs-ctl start --system-id=$c >>>> - m_as $c /usr/share/ovn/scripts/ovn-ctl start_controller >>>> + echo "Using ${CONTROLLER_SSL_ARGS} for ovn-controller". >>>> + m_as $c /usr/share/ovn/scripts/ovn-ctl start_controller >>>> ${CONTROLLER_SSL_ARGS} >>>> >>>> m_as $c ovs-vsctl set open . external_ids:ovn-encap-ip=$encap_ip >>>> m_as $c ovs-vsctl set open . >>>> external-ids:ovn-encap-type=$encap_type >>>> - m_as $c ovs-vsctl set open . >>>> external-ids:ovn-remote=tcp:$remote_ip:6642 >>>> + m_as $c ovs-vsctl set open . >>>> external-ids:ovn-remote=${REMOTE_PROT}:$remote_ip:6642 >>>> m_as $c ovs-vsctl set open . >>>> external-ids:ovn-openflow-probe-interval=60 >>>> m_as $c ovs-vsctl set open . >>>> external-ids:ovn-remote-probe-interval=180000 >>>> m_as $c ovs-vsctl set open . >>>> external-ids:ovn-bridge-datapath-type=system >>>> -- >>>> 2.47.1 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >>> Thanks, >>> Ales >>> >> Thanks >> Xavier >> > > Thanks, > Ales > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
