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
