network: fix vm can be deployed on L2 network of other accounts #5784

weizhouapache · 2021-12-16T09:53:24Z

Description

This PR fixes a bug with integration test test_storage_policy.py
https://github.com/apache/cloudstack/blob/4.16.0.0/test/integration/smoke/test_storage_policy.py#L191-L208

in the test, root admin deploys a vm for another account on root admin's L2 network, it should fail but test passes.

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?

…ther users

weizhouapache · 2021-12-16T09:53:47Z

@blueorangutan package

blueorangutan · 2021-12-16T09:54:03Z

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

blueorangutan · 2021-12-16T10:44:52Z

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

sureshanaparti · 2021-12-16T11:03:53Z

@blueorangutan test

blueorangutan · 2021-12-16T11:05:03Z

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

DaanHoogland

clgtm

blueorangutan · 2021-12-16T20:07:08Z

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

Test	Result	Time (s)	Test File

GabrielBrascher

Thanks, @weizhouapache.
Code LGTM!

weizhouapache · 2021-12-22T08:50:16Z

this PR needs more love.
related to #2420

sureshanaparti · 2021-12-22T08:52:32Z

server/src/main/java/com/cloud/network/NetworkModelImpl.java

        // Perform account permission check
-        if ((network.getGuestType() != GuestType.Shared && network.getGuestType() != GuestType.L2) ||
-                (network.getGuestType() == GuestType.Shared && network.getAclType() == ACLType.Account)) {
+        if (network.getGuestType() != GuestType.Shared || network.getAclType() == ACLType.Account) {


@weizhouapache check the changes introduced at #2420, for L2 network.

weizhouapache · 2021-12-22T10:35:26Z

@nvazquez
the issue fixed by #2420 is not a real issue in my opinion.
#3158 has reverted part of #2420, this PR reverts rest of #2420.
could you please review it ?

weizhouapache · 2021-12-22T10:39:12Z

@blueorangutan package

blueorangutan · 2021-12-22T10:40:02Z

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

blueorangutan · 2021-12-22T12:06:09Z

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

weizhouapache · 2021-12-22T12:14:50Z

@blueorangutan test

blueorangutan · 2021-12-22T12:16:04Z

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

yadvr · 2021-12-30T10:57:19Z

@blueorangutan package

blueorangutan · 2021-12-30T10:58:04Z

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

blueorangutan · 2021-12-30T12:42:42Z

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

DaanHoogland · 2021-12-31T08:58:49Z

@blueorangutan package

blueorangutan · 2021-12-31T09:02:04Z

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

blueorangutan · 2022-01-03T09:51:40Z

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

weizhouapache · 2022-01-06T07:48:38Z

@blueorangutan package

blueorangutan · 2022-01-06T07:50:04Z

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

blueorangutan · 2022-01-06T09:51:47Z

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

weizhouapache · 2022-01-06T10:28:50Z

@blueorangutan package

blueorangutan · 2022-01-06T10:29:03Z

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

blueorangutan · 2022-01-06T11:03:17Z

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

weizhouapache · 2022-01-06T11:29:54Z

@blueorangutan package

yadvr · 2022-01-08T03:26:05Z

@blueorangutan package

blueorangutan · 2022-01-08T03:27:04Z

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

blueorangutan · 2022-01-08T07:05:09Z

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

sureshanaparti · 2022-01-10T07:37:23Z

@blueorangutan package

blueorangutan · 2022-01-10T07:38:04Z

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

blueorangutan · 2022-01-10T08:54:41Z

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

sureshanaparti · 2022-01-10T09:10:02Z

@weizhouapache can you check unit test error

[ERROR] addUserAccountFail(org.apache.cloudstack.cloudian.CloudianClientTest)  Time elapsed: 9.42 s  <<< ERROR!
org.apache.cloudstack.api.ServerApiException: Operation timed out, please try again.
	at org.apache.cloudstack.cloudian.CloudianClientTest.addUserAccountFail(CloudianClientTest.java:145)

weizhouapache · 2022-01-10T09:21:32Z

@blueorangutan package

blueorangutan · 2022-01-10T09:22:04Z

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

weizhouapache · 2022-01-10T09:22:39Z

@sureshanaparti it is not related to this PR. let me rebuild the package

blueorangutan · 2022-01-10T10:31:57Z

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

weizhouapache · 2022-01-10T10:44:22Z

@blueorangutan package

blueorangutan · 2022-01-10T10:45:04Z

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

blueorangutan · 2022-01-10T12:03:19Z

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

sureshanaparti · 2022-01-10T12:20:31Z

@blueorangutan test

blueorangutan · 2022-01-10T12:21:03Z

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

blueorangutan · 2022-01-10T21:55:07Z

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

Test	Result	Time (s)	Test File

…same domain (#5769) * Enhancement: create Shared networks and VPC private gateways by users * UI bug fix: pass correct domainid in CreateSharedNetworkForm * Update #5730: fix test failure with test_guest_vlan_range.py * Update #5730: fix test failure with test_persistent_network.py * Update #5730: Add since to new API commands and API parameters * Update #5730: Get first physical network for VPC private gateway if other ways do not work * Update #5730: code optimization (return !offering.isSpecifyVlan()) * Update #5730: fix hard-coded network offering id in test_pvlan.py * Update #5730: skip access check on the network owner if the owner is ROOT/system * Update #5730: overlap check on cidr/startip/endip * Update #5730: add methods to get accountid/domainid of shared networks * Update #5730: improve integration tests * Update #5730: update as per GutoVeronezi's comments * Network Sharing: give network access permission to other accounts within a domain * network: update ip in lb/pf/dnat tables when update vm nic ip * Update #5757: create 3 separated methods for DNAT/LB/PF update * travis: install python3-setuptools * Network Sharing: update integration test * Update #5769: Remove NetworkPermission.Ops * Update #5769: Update as per Daan's comments * Update #5769: Update as per Suresh's comments * Update #5769: fix UI bug that accounts/projects are not listed * Update #5769: fix domain admin can deploy vm on L2 network of other users * Update #5769: Remove method listPermittedNetworkIdsByDomains in NetworkPermissionDao * Update #5769: Skip network operation permissions check for root admin * UI: fix create Isolated/L2 network form * Update #5730: fix create Shared network form * Update #5769: fix domain admin can deploy vm on L2 network of other users * test: fix test_storage_policy.py * Update #5769: fix remove_nic in test_network_permissions.py * Update #5769: extract some codes to a method * Update #5769: fix add/remove nic by domain admin * Update #5769: allow domain admin to enable/disable static nat and create port forwarding rules * Update #5769: update integration test * Update #5769: fix unit test AssignLoadBalancerTest.java * Update #5769: allow normal users to share network permission to other users on UI * Update #5769: fix small UI bug with label * Update #5769: Support L2 network as associated network * test: sleep 30s after restarting mgt server in test_kubernetes_supported_versions.py to fix test failures with test_secondary_storage.py * Update #5784: revert part of changes in #2420 * Update #5757: invert if condition to reduce code indentation * Update #5769: fix regular user cannot create L2 network * Update #5769: Add associated nework id and name in private gateway response * Update #5769: list networks by networkfilter=Account on UI * Update #5769: fix ui issue when list private gateways or create shared network if no isolated networks * Update #5769: fix vue ui warnings * Update #5679: add BaseResponseWithAssociatedNetwork and extract method setResponseAssociatedNetworkInformation * Update #5679: extract some methods in VpcManagerImpl.java * Update #5679: Update smoke tests as per Daan's comments * Update #5769: fix vpc with private gateways cannot be removed when remove an acount * Update #5769: fix unit test failures after merging latest main * Update #5769: fix schema-41610to41700.sql * Update #5769: fix Request failed due to empty network offering list on UI * Update #5769: Throw exception when account is not found by name * Update #5769: display a warning message if network offering list is empty * Update #5769: fix an UI bug caused by previous commit b286cb7 * Update #5769: fix UI bugs due to vue3 merge * Update #5769: fix issue due to account type refactoring * Update #5769: fix ui bugs due to vue3 * Update #5769: fix issue due to vue3 upgrade * Update #5769: fix issue due to vue3 upgrade part 2 * Update #5769: fix issue due to vue3 upgrade part 3 * Update #5769: highlight default scope when create shared network on UI * Update #5769: fix domain list is not loaded on UI * Update #5769: fix restart/delete shared network by normal users * Update #5769: fix restart domain-scope shared network by domain admin * Update #5769: fix 3 UI bugs (1) double networks in list; (2) icon of first items in list; (3) account/project autoselect * Update #5769: fix 2 ui bugs; (1) selected project is not changed when change domain; (2) no network should be selected by default * Update #5769: fix update shared networks by domain admin/regular user * Update #5769: fix Flicking warning message about the empty network offerings * Update #5769: display associated network name in shared network info card * Update #5769: fix create private gateway form * Update #5769: fix network lists in project view * Update #5769: fix duplicated networks in network dropdown * Update #5769: fix failed to create shared network if associated L2 network is Setup * Update #5769: check AccessType.OperateEntry on network in its implementation * Revert "Update #5769: check AccessType.OperateEntry on network in its implementation" This reverts commit c42c489. * Update #5769: fix keyword search in list guest vlans

weizhouapache added 2 commits December 16, 2021 10:48

Update apache#5769: fix domain admin can deploy vm on L2 network of o…

9465ccd

…ther users

test: fix test_storage_policy.py

a553acd

DaanHoogland approved these changes Dec 16, 2021

View reviewed changes

sureshanaparti added this to the 4.16.1.0 milestone Dec 17, 2021

GabrielBrascher approved these changes Dec 17, 2021

View reviewed changes

sureshanaparti reviewed Dec 22, 2021

View reviewed changes

weizhouapache marked this pull request as draft December 22, 2021 08:58

Update apache#5784: revert part of changes in apache#2420

2e47236

weizhouapache requested a review from nvazquez December 22, 2021 10:33

weizhouapache marked this pull request as ready for review December 22, 2021 10:35

weizhouapache added a commit to shapeblue/cloudstack that referenced this pull request Dec 23, 2021

Update apache#5784: revert part of changes in apache#2420

c4fb45d

yadvr approved these changes Dec 30, 2021

View reviewed changes

weizhouapache added the component:networking label Jan 10, 2022

sureshanaparti merged commit fadd74a into apache:4.16 Jan 11, 2022

weizhouapache mentioned this pull request Feb 28, 2022

CloudStack allows deploying VMs on Networks that should not be accessed by VM's owner #6049

Closed

weizhouapache deleted the 4.16-fix-l2-network-permission-check branch December 9, 2022 08:18

network: fix vm can be deployed on L2 network of other accounts #5784

network: fix vm can be deployed on L2 network of other accounts #5784

Uh oh!

Conversation

weizhouapache commented Dec 16, 2021

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!

weizhouapache commented Dec 16, 2021

Uh oh!

blueorangutan commented Dec 16, 2021

Uh oh!

blueorangutan commented Dec 16, 2021

Uh oh!

sureshanaparti commented Dec 16, 2021

Uh oh!

blueorangutan commented Dec 16, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 16, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Dec 22, 2021

Uh oh!

sureshanaparti Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Dec 22, 2021

Uh oh!

weizhouapache commented Dec 22, 2021

Uh oh!

blueorangutan commented Dec 22, 2021

Uh oh!

blueorangutan commented Dec 22, 2021

Uh oh!

weizhouapache commented Dec 22, 2021

Uh oh!

blueorangutan commented Dec 22, 2021

Uh oh!

yadvr commented Dec 30, 2021

Uh oh!

blueorangutan commented Dec 30, 2021

Uh oh!

blueorangutan commented Dec 30, 2021

Uh oh!

DaanHoogland commented Dec 31, 2021

Uh oh!

blueorangutan commented Dec 31, 2021

Uh oh!

blueorangutan commented Jan 3, 2022

Uh oh!

weizhouapache commented Jan 6, 2022

Uh oh!

blueorangutan commented Jan 6, 2022

Uh oh!

blueorangutan commented Jan 6, 2022

Uh oh!

weizhouapache commented Jan 6, 2022

Uh oh!

blueorangutan commented Jan 6, 2022

Uh oh!

blueorangutan commented Jan 6, 2022

Uh oh!

weizhouapache commented Jan 6, 2022

Uh oh!

yadvr commented Jan 8, 2022

Uh oh!

blueorangutan commented Jan 8, 2022

Uh oh!

blueorangutan commented Jan 8, 2022

Uh oh!