[4.7] FIX Site2SiteVPN on redundant VPC #1276

michaelandersen · 2015-12-22T11:39:13Z

This PR:

fixes the inability to setup more than one Site2Site VPN connection from a VPC
fixes starting of Site2Site VPN on redundant VPC
fixes Site2Site VPN state checking on redundant VPC
improves the vpc_vpn test to allow multple hypervisors
adds an integration test for Site2Site VPN on redundant VPC

Tested it on 4.7 single Xen server zone:

command:

nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone1-xen1.cfg -a tags=advanced,required_hardware=true /tmp/test_vpc_vpn.py

results:

Test Site 2 Site VPN Across redundant VPCs ... === TestName: test_01_redundant_vpc_site2site_vpn | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_01_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test Site 2 Site VPN Across VPCs ... === TestName: test_01_vpc_site2site_vpn | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 3 tests in 1490.076s

OK

also performed numerous manual inspections of state of VPN connections and connectivity between VPC's

wilderrodrigues · 2015-12-22T16:35:50Z

Jenkins error is not related:

wilderrodrigues · 2015-12-22T16:38:17Z

Did a code review on this PR and also talked to @michaelandersen on Slack about switching the MASTER router off. With the changes he applied on the Java file it will work fine.

Also checked the changes concerning the tests now covering KVM, Xen, VMware and HyperV.

Code LGTM and awesome to see it covered by an integration test. Thanks a lot!

👍

Will do some tests, but please check with @remibergsma @DaanHoogland @miguelaferreira to give a second LGTM based on tests because I'm on holidays. :)

Cheers,
Wilder

pdube · 2015-12-22T17:06:36Z

Good to see additional tests created. I would suggest adding a test that turns one of the routers off, and then seeing if the VPN connection works correctly. I quickly looked through the component testing and didn't find any similar tests.

Is there a timing issue possible where both routers would be in the same state? If so, it might be good to throw an exception before doing any operations (i.e. if the loop exits, and no routers were found to be MASTER).

Also, should the check to see if the router is the MASTER occur before persisting the vpn connection state on each router?

pdube · 2015-12-22T17:49:18Z

server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java

@@ -916,6 +916,9 @@ protected void updateSite2SiteVpnConnectionState(final List<DomainRouterVO> rout
                }
                continue;
            }
+            if (router.getIsRedundantRouter() && router.getRedundantState() != RedundantState.MASTER){


Should this check be done before the previous 'if'?

Not per se, as the previous if statement skips the iteration if the router to be checked is not in a running state. And running state is required to do any further state checking.

after some more thought: You could be right. if the BACKUP is not running and goes through the previous inspection AND has an attached vpn connection will be marked as disconnected.
need to investigate a bit to see what _s2sVpnMgr.getConnectionsForRouter(router) would return for a backup router. It the backup router does not have an attached vpn connection we are good as it will just skip any further vpn connection checking for that iteration.

ok @pdube so getConnectionsForRouter() returns vpn connections based on the vpcid of the router. As both the backup and master router have the same vpcid assigned and therefore will both return vpn connections, which will then be marked as disconnected if the backup router is not running, it makes sense to first exclude backup routers and then to exclude non-running routers.

I force pushed the change. @pdube could you test?

michaelandersen · 2015-12-22T19:50:24Z

As for:

Is there a timing issue possible where both routers would be in the same state? If so, it might be good to > throw an exception before doing any operations (i.e. if the loop exits, and no routers were found to be MASTER).

A dual MASTER state should not be possible, but even if it occurred it would, in this context worst case, trigger a vpn state check on both routers.

I would suggest adding a test that turns one of the routers off, and then seeing if the VPN connection works correctly.

This makes sense and i will manually verify and if possible automate this tomorrow.

DaanHoogland · 2015-12-23T14:58:40Z

test/integration/smoke/test_vpc_vpn.py

@@ -468,7 +500,7 @@ def get_ssh_client(self, virtual_machine, services, retries):

        return ssh_client

-    def create_natrule(self, vpc, vm, public_port, private_port, public_ip, network, services=None):
+    def _create_natrule(self, vpc, vm, public_port, private_port, public_ip, network, services=None):


why _create_natrule() and not create_natrule()?

a matter of style i guess, i consistently changed private methods/non-tests to be prefixed with an underscore.

michaelandersen · 2016-01-11T13:07:33Z

just ran the test again after adding the validateList method:

command:

nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone1-xen1.cfg -a tags=advanced,required_hardware=true test/integration/smoke/test_vpc_vpn.py

results:

Test Site 2 Site VPN Across redundant VPCs ... === TestName: test_01_redundant_vpc_site2site_vpn | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_01_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test Site 2 Site VPN Across VPCs ... === TestName: test_01_vpc_site2site_vpn | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 3 tests in 1567.984s

OK

michaelandersen · 2016-01-11T18:04:42Z

ping @DaanHoogland

michaelandersen · 2016-01-12T10:11:47Z

ping @sanju1010 @wido @terbolous

DaanHoogland · 2016-01-12T12:49:30Z

tested on a two kvm, as in the bubble, setup:

# less /tmp/MarvinLogs/test_vpc_vpn_Q1BTHP/results.txt
Test Site 2 Site VPN Across redundant VPCs ... === TestName: test_01_redundant_vpc_site2site_vpn | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_01_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test Site 2 Site VPN Across VPCs ... === TestName: test_01_vpc_site2site_vpn | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 3 tests in 2166.200s

OK

LGTM

wilderrodrigues · 2016-01-16T10:26:35Z

Ping @remibergsma

LGTM, please proceed with merge.

Cheers,
Wilder

[4.7] FIX Site2SiteVPN on redundant VPCThis PR: - fixes the inability to setup more than one Site2Site VPN connection from a VPC - fixes starting of Site2Site VPN on redundant VPC - fixes Site2Site VPN state checking on redundant VPC - improves the vpc_vpn test to allow multple hypervisors - adds an integration test for Site2Site VPN on redundant VPC Tested it on 4.7 single Xen server zone: command: ``` nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone1-xen1.cfg -a tags=advanced,required_hardware=true /tmp/test_vpc_vpn.py ``` results: ``` Test Site 2 Site VPN Across redundant VPCs ... === TestName: test_01_redundant_vpc_site2site_vpn | Status : SUCCESS === ok Test Remote Access VPN in VPC ... === TestName: test_01_vpc_remote_access_vpn | Status : SUCCESS === ok Test Site 2 Site VPN Across VPCs ... === TestName: test_01_vpc_site2site_vpn | Status : SUCCESS === ok ---------------------------------------------------------------------- Ran 3 tests in 1490.076s OK ``` also performed numerous manual inspections of state of VPN connections and connectivity between VPC's * pr/1276: Fix unable to setup more than one Site2Site VPN Connection FIX S2S VPN rVPC: Check only redundant routers in state MASTER PEP8 of integration/smoke/test_vpc_vpn Add S2S VPN test for Redundant VPC Make integration/smoke/test_vpc_vpn Hypervisor independant FIX VPN: non-working ipsec commands Signed-off-by: Remi Bergsma <github@remi.nl>

michaelandersen changed the title ~~FIX Site2SiteVPN on redundant VPC~~ [4.7] FIX Site2SiteVPN on redundant VPC Dec 22, 2015

pdube reviewed Dec 22, 2015
View reviewed changes

michaelandersen force-pushed the fix/site2sitevpn branch from f6364fc to 055d84e Compare December 22, 2015 19:38

michaelandersen mentioned this pull request Dec 23, 2015

Fixed issues with test_vpc_vpn script #1156

Closed

DaanHoogland reviewed Dec 23, 2015
View reviewed changes

michaelandersen force-pushed the fix/site2sitevpn branch 2 times, most recently from 7e988ff to 7513fdb Compare January 11, 2016 12:30

michaelandersen force-pushed the fix/site2sitevpn branch from 7513fdb to 1948e13 Compare January 11, 2016 17:56

michaelandersen force-pushed the fix/site2sitevpn branch from 1948e13 to d1e5230 Compare January 12, 2016 10:17

Michael Andersen added 6 commits January 12, 2016 11:24

FIX VPN: non-working ipsec commands

dfa924b

Make integration/smoke/test_vpc_vpn Hypervisor independant

fd83ca0

Add S2S VPN test for Redundant VPC

a42b046

PEP8 of integration/smoke/test_vpc_vpn

ecc8cc0

FIX S2S VPN rVPC: Check only redundant routers in state MASTER

230c9cf

Fix unable to setup more than one Site2Site VPN Connection

435a98c

michaelandersen force-pushed the fix/site2sitevpn branch from d1e5230 to 435a98c Compare January 12, 2016 10:25

asfgit merged commit 435a98c into apache:4.7 Jan 16, 2016

snyk-bot mentioned this pull request Jul 13, 2021

[Snyk] Security upgrade sass-lint from 1.12.1 to 1.13.0 hixio-mh/cloudstack#35

Open

This was referenced Sep 13, 2021

[Snyk] Security upgrade sass-lint from 1.12.1 to 1.13.0 hixio-mh/cloudstack#100

Open

[Snyk] Fix for 1 vulnerabilities hixio-mh/cloudstack#102

Open

snyk-bot mentioned this pull request Feb 14, 2022

[Snyk] Security upgrade sass-lint from 1.12.1 to 1.13.0 hixio-mh/cloudstack#186

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.7] FIX Site2SiteVPN on redundant VPC #1276

[4.7] FIX Site2SiteVPN on redundant VPC #1276

michaelandersen commented Dec 22, 2015

wilderrodrigues commented Dec 22, 2015

wilderrodrigues commented Dec 22, 2015

pdube commented Dec 22, 2015

pdube Dec 22, 2015

michaelandersen Dec 22, 2015

michaelandersen Dec 22, 2015

michaelandersen Dec 22, 2015

michaelandersen commented Dec 22, 2015

DaanHoogland Dec 23, 2015

michaelandersen Dec 23, 2015

michaelandersen commented Jan 11, 2016

michaelandersen commented Jan 11, 2016

michaelandersen commented Jan 12, 2016

DaanHoogland commented Jan 12, 2016

wilderrodrigues commented Jan 16, 2016

[4.7] FIX Site2SiteVPN on redundant VPC #1276

[4.7] FIX Site2SiteVPN on redundant VPC #1276

Conversation

michaelandersen commented Dec 22, 2015

wilderrodrigues commented Dec 22, 2015

wilderrodrigues commented Dec 22, 2015

pdube commented Dec 22, 2015

pdube Dec 22, 2015

Choose a reason for hiding this comment

michaelandersen Dec 22, 2015

Choose a reason for hiding this comment

michaelandersen Dec 22, 2015

Choose a reason for hiding this comment

michaelandersen Dec 22, 2015

Choose a reason for hiding this comment

michaelandersen commented Dec 22, 2015

DaanHoogland Dec 23, 2015

Choose a reason for hiding this comment

michaelandersen Dec 23, 2015

Choose a reason for hiding this comment

michaelandersen commented Jan 11, 2016

michaelandersen commented Jan 11, 2016

michaelandersen commented Jan 12, 2016

DaanHoogland commented Jan 12, 2016

wilderrodrigues commented Jan 16, 2016