api, server: fix add-remove vpn user without vpn owner by shwstppr · Pull Request #5850 · apache/cloudstack

shwstppr · 2022-01-11T10:12:29Z

Description

Fixes #5711

When the owner account does not have VPN access, VPN user should be added in Add state
While removing VPN user when the owner account has no VPN, VPN user should be deleted from the database.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Fixes apache#5711 ACS should not add a new user in Add state when the owner account does not have VPN access. While removing VPN user ACS should not fail completely when owner account ahs no VPN. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2022-01-19T11:36:16Z

@blueorangutan package

blueorangutan · 2022-01-19T11:37:05Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-19T12:23:48Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2249

sureshanaparti · 2022-01-19T12:24:27Z

@blueorangutan test

blueorangutan · 2022-01-19T12:25:04Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

sureshanaparti · 2022-01-20T07:54:42Z

@blueorangutan package

blueorangutan · 2022-01-20T07:55:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-20T08:35:48Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian ✔️ suse15. SL-JID 2260

shwstppr · 2022-01-24T07:22:22Z

@blueorangutan package

blueorangutan · 2022-01-24T07:23:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-01-24T08:13:10Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2282

shwstppr · 2022-01-24T08:53:23Z

@blueorangutan test

blueorangutan · 2022-01-24T08:54:03Z

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-02-08T09:59:35Z

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2514

shwstppr · 2022-02-08T10:06:05Z

@blueorangutan package

blueorangutan · 2022-02-08T10:07:02Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-08T10:45:56Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 2518

weizhouapache · 2022-02-08T10:54:18Z

@blueorangutan package

blueorangutan · 2022-02-08T10:55:03Z

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-08T11:34:15Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✔️ suse15. SL-JID 2521

DaanHoogland · 2022-02-08T13:53:08Z

@blueorangutan package

blueorangutan · 2022-02-08T13:54:04Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

weizhouapache · 2022-02-08T14:10:41Z


    List<? extends VpnUser> listVpnUsers(long vpnOwnerId, String userName);

+    void removeVpnUserWithoutValidVpnOwner(long id);


@shwstppr is removeVpnUserWithoutValidVpnOwner used somethere ?

@weizhouapache
--edit--
removed unused method

@shwstppr it uses another method removeVpnUserWithoutRemoteAccessVpn

@weizhouapache sorry realized that. I've removed the unused method now

@shwstppr good !

blueorangutan · 2022-02-08T14:42:58Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2531

DaanHoogland · 2022-02-08T15:34:55Z

@blueorangutan test

blueorangutan · 2022-02-08T15:38:03Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache

I have tested some scenarios, the following are good.

create vpn users for other accounts (I believe same results when create vpn users for same account by api)
(1) has no isolated network
(2) has isolated network in Allocated state, and no remote access vpn
(3) has isolated network in Implemented state, and no remote access vpn
(4) has isolated network in Implemented state, and remote access vpn is enabled
(5) has isolated network in Allocated state, and remote access vpn is enabled

I did not test
(1) has multiple isolated networks, but fail to apply vpn user in one of the VRs.

blueorangutan · 2022-02-09T02:08:03Z

Trillian test result (tid-3240)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36422 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5850-t3240-kvm-centos7.zip
Smoke tests completed. 78 look OK, 2 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.58	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.10	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.19	test_primary_storage.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	31.91	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	59.63	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	34.94	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	35.93	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	125.12	test_kubernetes_clusters.py

weizhouapache · 2022-02-09T06:24:17Z

@blueorangutan test

blueorangutan · 2022-02-09T06:25:03Z

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

sureshanaparti · 2022-02-09T06:28:35Z

I have tested some scenarios, the following are good.

create vpn users for other accounts (I believe same results when create vpn users for same account by api) (1) has no isolated network (2) has isolated network in Allocated state, and no remote access vpn (3) has isolated network in Implemented state, and no remote access vpn (4) has isolated network in Implemented state, and remote access vpn is enabled (5) has isolated network in Allocated state, and remote access vpn is enabled

I did not test (1) has multiple isolated networks, but fail to apply vpn user in one of the VRs.

Hi @shwstppr @weizhouapache what's the concensus here, is this PR good to go / merge?

weizhouapache · 2022-02-09T07:21:13Z

I have tested some scenarios, the following are good.
create vpn users for other accounts (I believe same results when create vpn users for same account by api) (1) has no isolated network (2) has isolated network in Allocated state, and no remote access vpn (3) has isolated network in Implemented state, and no remote access vpn (4) has isolated network in Implemented state, and remote access vpn is enabled (5) has isolated network in Allocated state, and remote access vpn is enabled
I did not test (1) has multiple isolated networks, but fail to apply vpn user in one of the VRs.

Hi @shwstppr @weizhouapache what's the concensus here, is this PR good to go / merge?

@sureshanaparti
it is good to me. I have approved it.

borisstoyanov

LGTM, manually tested

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti · 2022-02-09T11:19:16Z

@blueorangutan package

blueorangutan · 2022-02-09T11:21:02Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-09T11:57:41Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2538

blueorangutan · 2022-02-09T15:18:31Z

Trillian test result (tid-3257)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30690 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5850-t3257-kvm-centos7.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_nic	`Error`	145.22	test_nic.py

sureshanaparti · 2022-02-10T04:35:22Z

Trillian test result (tid-3257) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 30690 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5850-t3257-kvm-centos7.zip Smoke tests completed. 91 look OK, 1 have errors Only failed tests results shown below:

Test Result Time (s) Test File
test_01_nic Error 145.22 test_nic.py

Not noticed this test failure in the last run, seems to be intermittent and is unrelated to this PR changes.

* api, server: fix add-remove vpn user without vpn owner Fixes apache#5711 ACS should not add a new user in Add state when the owner account does not have VPN access. While removing VPN user ACS should not fail completely when owner account ahs no VPN. * change , fixes * remove unused method Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr mentioned this pull request Jan 11, 2022

Error while create VPN user for another account #5711

Closed

shwstppr self-assigned this Jan 11, 2022

sureshanaparti linked an issue Jan 11, 2022 that may be closed by this pull request

Error while create VPN user for another account #5711

Closed

sureshanaparti added this to the 4.16.1.0 milestone Jan 11, 2022

sureshanaparti requested review from GabrielBrascher, GutoVeronezi and weizhouapache January 11, 2022 11:10

sureshanaparti added the component:networking label Jan 11, 2022

sureshanaparti requested a review from yadvr January 11, 2022 11:11

apache deleted a comment from sureshanaparti Jan 19, 2022

apache deleted a comment from blueorangutan Jan 19, 2022

apache deleted a comment from sureshanaparti Jan 19, 2022

apache deleted a comment from shwstppr Jan 19, 2022

apache deleted a comment from blueorangutan Jan 19, 2022

weizhouapache reviewed Feb 8, 2022

View reviewed changes

weizhouapache approved these changes Feb 8, 2022

View reviewed changes

borisstoyanov approved these changes Feb 9, 2022

View reviewed changes

borisstoyanov removed their assignment Feb 9, 2022

remove unused method

f272fba

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti merged commit f88f934 into apache:4.16 Feb 10, 2022

DaanHoogland removed the status:needs-testing label Jan 28, 2026


		List<? extends VpnUser> listVpnUsers(long vpnOwnerId, String userName);

		void removeVpnUserWithoutValidVpnOwner(long id);

Conversation

shwstppr commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

shwstppr commented Jan 19, 2022

Uh oh!

blueorangutan commented Jan 19, 2022

Uh oh!

blueorangutan commented Jan 19, 2022

Uh oh!

sureshanaparti commented Jan 19, 2022

Uh oh!

blueorangutan commented Jan 19, 2022

Uh oh!

sureshanaparti commented Jan 20, 2022

Uh oh!

blueorangutan commented Jan 20, 2022

Uh oh!

blueorangutan commented Jan 20, 2022

Uh oh!

shwstppr commented Jan 24, 2022

Uh oh!

blueorangutan commented Jan 24, 2022

Uh oh!

blueorangutan commented Jan 24, 2022

Uh oh!

shwstppr commented Jan 24, 2022

Uh oh!

blueorangutan commented Jan 24, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

shwstppr commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

weizhouapache commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

DaanHoogland commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

weizhouapache Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

shwstppr Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizhouapache Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

shwstppr Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

weizhouapache Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

DaanHoogland commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Jan 11, 2022 •

edited

Loading

shwstppr Feb 9, 2022 •

edited

Loading