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 <amu...@redhat.com> wrote:
>
>
> On Wed, Jul 2, 2025 at 11:50 AM Xavier Simonart via dev <
> ovs-dev@openvswitch.org> 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 <xsimo...@redhat.com>
>> ---
>>
>
> 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?
>
> + 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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Thanks,
> Ales
>
Thanks
Xavier
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
