New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global ACL for VPCs #7150
Global ACL for VPCs #7150
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7150 +/- ##
============================================
+ Coverage 29.12% 29.16% +0.04%
- Complexity 31030 31084 +54
============================================
Files 5193 5193
Lines 366322 366348 +26
Branches 53558 53557 -1
============================================
+ Hits 106694 106855 +161
+ Misses 245013 244832 -181
- Partials 14615 14661 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clg overall, will need extensive testing though
api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLListCmd.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general feature - could you add more marvin/integration tests for this?
@rohityadavcloud I added smoke tests for this feature. Let me know if you think this is enough. I used the manual tests I did as reference; however, there were methods that were not on the Marvin lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the test @BryanMLima , I have some doubts about it, hope you understand
@DaanHoogland thanks for the review, I'm not used to the Marvin tests so your suggestions made it clearer to me. Let me know if there is more to change. |
@blueorangutan package |
@DaanHoogland a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5613 |
@blueorangutan test |
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-6209)
|
server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
SonarCloud Quality Gate failed. |
@DaanHoogland could you trigger the integration tests again? The error seems unrelated to this PR. |
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7825 |
@blueorangutan test |
@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-8390)
|
@blueorangutan test |
@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-8391)
|
@BryanMLima can you look to make sure the redundant vpc test failures are not a problem? Did you test redundant VPCs? |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@BryanMLima any update on this? Also, merge conflicts here |
The Trillian tests are related to connection issues; this PR's changes should not impact this test, as it is not using global ACLs. |
@blueorangutan package |
@BryanMLima a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
@blueorangutan package |
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7863 |
@blueorangutan test |
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Test results from backend,
|
@DaanHoogland cc @borisstoyanov do we need any further testing or it good to merge? |
There was a test added to the smoke test suites, so I think we are done here; merging |
Description
Currently, ACS allows the creation of ACLs for VPCs; however, it is required to create an ACL per VPC. Therefore, even if multiple VPCs had the same ACL rules, it would be required to create multiple ACLs. This PR aims to change this behavior as to be able to create global ACLs which will be available to all VPCs, similar to the default ones:
default_allow
anddefault_deny
.This PR removed the requirement of the parameter
vpcId
of thecreateNetworkACLList
; therefore, if there is not a VPC ID in the parameter call, then it is a global ACL. It is important to note that only root admins can manipulate these global ACLs, but anyone can use them.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I performed the following tests in a local lab:
I created a global ACL with a root admin account.T0 - createNetworkAclList
I tried to create a global ACL with a domain admin account.
Expected? Yes
T1 - replaceNetworkaclList
I replaced the ACL for a network as root admin
I replaced the ACL for a network as domain admin
Expected? Yes
T2 - createNetworkAcl
I created an ACL rule as root admin
I tried to create an ACL rule as a domain admin
Expected? Yes
T3 - deleteNetworkAcl
I deleted an ACL rule as root admin
I tried to delete an ACL rule as domain admin
Expected? Yes
I tried to move an ACL rule as root admin, I used a screenshot as CMK has a problem to decode the response of API `moveNetworkAclItem` (not caused by this PR, this faulty behavior was already like this).T4 - moveNetworkAclItem
I tried to move an ACL rule as domain admin
Expected? Yes
T5 - updateNetworkAclItem
I updated an ACL rule as root admin
I tried to update an ACL rule as domain Admin
Expected? Yes
T6 - deleteNetworkAclList
I deleted an ACL rule as root admin
I tried to delete an ACL rule as domain admin
Expected? Yes
Furthermore, I created three VPCs and three VMs, each one attach to a tier of a VPC. After this, I created a global ACL allowing SSH connections. I tested the
ssh
command with all public IPs, previously created, of the VMs. All worked as expected. Furthermore, I changed the rule to deny any access to por 22 for the TCP protocol. Similarly, I tried to usedssh
with all 3 public IPs and all failed.