enable to create VPC portfowarding rules with source cidr #7081

RodrigoDLopez · 2023-01-11T16:52:19Z

Description

When a guest network is created, ACS allows the configuration of the firewall and port forwarding, load balancing, and VPN rules for all IPs associated with the network (including the source NAT). However, when creating a VPC, firewall configurations are possible only via ACLs; port forwarding and load balancing rules are only possible for other public IPs assigned to the network. Thus, situations, where it is necessary to combine firewall rules and port forwarding/load balancing in VPCs, are not possible.

To work around the situation, this PR implements an extension to allow the definition of source CIDR when creating port forwarding rules. If the cidrlist parameter is not informed, the current behavior is maintained. This attribute will only be used when creating port forwarding rules in VPCs, since in guest networks it is possible to combine firewall and port forwarding/load balancing rules directly on the public IPs allocated to that network.

(scc) > create portforwardingrule protocol=tcp privateport=22 publicport=22 openfirewall=false vmguestip=10.1.1.248 virtualmachineid=334d4279-a0e6-4c0a-a999-37aabda08acd ipaddressid=5cca136d-5425-44fa-b6af-aafc29df269e networkid=b61abc91-ee24-4578-bae6-fa2d06cf9370 cidrlist=192.168.200.170/32
{
  "portforwardingrule": {
    "cidrlist": "192.168.200.170/32",
    "fordisplay": true,
    "id": "210bab98-91f3-4911-8680-5189387a7135",
    "ipaddress": "172.16.200.51",
    "ipaddressid": "5cca136d-5425-44fa-b6af-aafc29df269e",
    "networkid": "b61abc91-ee24-4578-bae6-fa2d06cf9370",
    "privateendport": "22",
    "privateport": "22",
    "protocol": "tcp",
    "publicendport": "22",
    "publicport": "22",
    "state": "Active",
    "tags": [],
    "virtualmachinedisplayname": "pf-01",
    "virtualmachineid": "334d4279-a0e6-4c0a-a999-37aabda08acd",
    "virtualmachinename": "pf-01",
    "vmguestip": "10.1.1.248"
  }
}

Furthermore, it was observed that when removing a port forwarding rule, even if the ACS confirmed the removal of said rule, it remained active in the VR. This behavior has changed so that the rule is removed properly.

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

Major
Minor

How Has This Been Tested?

For testing purposes, I created a VPC: vpc-01 and a Tier: vpc-01-tier-01. Using this tier three instances PF01, PF02, PF03 was created and the port forward and ACL rules described below were created.

Firewall Rules Applied (ACL)

Origin	Start Port	End Port	Protocolo
0.0.0.0/0	22	22	TCP (Allow)
172.16.200.170/32	23	23	TCP (Allow)
172.16.200.170/32	24	24	TCP (Allow)
0.0.0.0/0	-----	-----	ALL (Deny)

Port forward rules

Private port	Public port	Protocol	Source CIDR	VM
22 - 22	22 - 22	TCP	---	PF01
22 - 22	23 - 23	TCP	---	PF02
22 - 22	24 - 24	TCP	---	PF03

This way, after applying the ACL rule that authorizes access to port 22 from any IP (0.0.0.0/0); access to other instances (PF02 and PF03) turns out to be exposed. After applying the port forward rule limiting access to a list of CIDRs, the observed result was that access to the resources/services of instances PF02 and PF03 was limited to the CIDR informed when creating the port forward rule

Regras de port forward

Private port	Public port	Protocol	Source CIDR	VM
22 - 22	22 - 22	TCP	---	A
22 - 22	23 - 23	TCP	172.16.200.170/32	B
22 - 22	24 - 24	TCP	172.16.200.170/32	C

codecov · 2023-01-11T17:50:13Z

Codecov Report

Attention: Patch coverage is 20.00000% with 100 lines in your changes missing coverage. Please review.

Project coverage is 15.78%. Comparing base (46201ee) to head (4a438b6).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...java/com/cloud/network/rules/RulesManagerImpl.java	0.00%	29 Missing ⚠️
.../network/rules/dao/PortForwardingRulesDaoImpl.java	0.00%	24 Missing ⚠️
...om/cloud/network/firewall/FirewallManagerImpl.java	52.94%	9 Missing and 7 partials ⚠️
...m/cloud/network/dao/FirewallRulesCidrsDaoImpl.java	0.00%	11 Missing ⚠️
.../com/cloud/network/rules/PortForwardingRuleVO.java	25.00%	6 Missing ⚠️
...a/com/cloud/agent/api/to/PortForwardingRuleTO.java	33.33%	3 Missing and 1 partial ⚠️
...and/user/firewall/UpdatePortForwardingRuleCmd.java	0.00%	4 Missing ⚠️
.../resource/virtualnetwork/model/ForwardingRule.java	40.00%	3 Missing ⚠️
...and/user/firewall/CreatePortForwardingRuleCmd.java	0.00%	2 Missing ⚠️
...a/com/cloud/network/router/CommandSetupHelper.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##               main    #7081     +/-   ##
===========================================
  Coverage     15.78%   15.78%             
- Complexity    12565    12571      +6     
===========================================
  Files          5627     5627             
  Lines        492260   492351     +91     
  Branches      63882    61358   -2524     
===========================================
+ Hits          77710    77728     +18     
- Misses       406076   406148     +72     
- Partials       8474     8475      +1

Flag	Coverage Δ
uitests	`4.04% <ø> (-0.01%)`	⬇️
unittests	`16.60% <20.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2023-01-11T18:09:08Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

3.4% Coverage
0.0% Duplication

DaanHoogland · 2023-01-12T12:03:55Z

@RodrigoDLopez can you please look at the sonar warning? They look serious in this case.

weizhouapache · 2023-01-13T07:33:27Z

Hi @RodrigoDLopez

Have you tested

create with a list of cidrs ?
same operation on isolated networks ?
create port forwarding rule with same public and same private port, but different cidr
create port forwarding rule with same public and different private port
update cidrlist of port forwarding rule ?
restart vpc with/without cleanup
restart vpc router

something are missing

ui changes
unit tests
marvin test

Please also ask one of your colleagues to test it.

systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py

assistanz247 · 2023-05-19T10:21:42Z

@RodrigoDLopez

Yes, our use case has been covered in the PR. Thanks a lot :-)

weizhouapache · 2023-05-22T06:54:21Z

@RodrigoDLopez

Yes, our use case has been covered in the PR. Thanks a lot :-)

cool, good to know it

github-actions · 2023-06-01T12:19:06Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

shwstppr · 2023-10-09T08:31:19Z

@RodrigoDLopez can you please address the merge conflicts and check review comments

DaanHoogland · 2023-11-15T14:48:27Z

ping @RodrigoDLopez

sureshanaparti · 2024-03-12T10:15:50Z

Hi @RodrigoDLopez It seems the issue #7483 is fixed with this PR. Can target this PR for 4.19.1?

DaanHoogland · 2024-06-06T12:09:03Z

@RodrigoDLopez can you look at the conflicts?

weizhouapache · 2024-06-06T14:07:18Z

...c/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java

@@ -107,8 +108,13 @@ public class CreatePortForwardingRuleCmd extends BaseAsyncCreateCmd implements P
                description = "the ID of the virtual machine for the port forwarding rule")
    private Long virtualMachineId;

-    @Parameter(name = ApiConstants.CIDR_LIST, type = CommandType.LIST, collectionType = CommandType.STRING, description = "the cidr list to forward traffic from. Multiple entries must be separated by a single comma character (,). This parameter is deprecated. Do not use.")
-    private List<String> cidrlist;
+    @Parameter(name = ApiConstants.CIDR_LIST,


I suggest we use SOURCE_CIDR_LIST

I'm keeping the param as cidrlist for now so that it matches the createLoadBalancerRule API and so that we don't add another parameter or change an existing param's name while we don't have a well-defined protocol to introduce breaking changes. However, I'll address it as "source CIDR list" in the UI and change its description to provide a better idea on what the parameter represents.

weizhouapache · 2024-06-06T14:07:33Z

...c/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java

+            collectionType = CommandType.STRING,
+            description = "the CIDR list to allow traffic, all other CIDRs will be blocked. " +
+                    "Multiple entries must be separated by a single comma character (,). This param will be used only for VPC's networks. By default, all CIDRs are allowed.")
+    private List<String> cidrList;


Also sourceCidrList

weizhouapache · 2024-06-06T14:13:43Z

...c/main/java/org/apache/cloudstack/api/command/user/firewall/CreatePortForwardingRuleCmd.java

+            type = CommandType.LIST,
+            collectionType = CommandType.STRING,
+            description = "the CIDR list to allow traffic, all other CIDRs will be blocked. " +
+                    "Multiple entries must be separated by a single comma character (,). This param will be used only for VPC's networks. By default, all CIDRs are allowed.")


I remember there are some changes for source cidr of load balancing rules. Does it work in isolated networks? If yes, this could be extended to support isolated networks as well.

This could be extended to also support isolated networks. However, I don't see a point in doing so, as a similar result can be accomplished by combining firewall and port forwarding rules when using isolated networks.

winterhazel · 2024-11-01T13:32:58Z

Hi @RodrigoDLopez It seems the issue #7483 is fixed with this PR. Can target this PR for 4.19.1?

@winterhazel , will you considder this?

My bad, I missed this message. Yes, I'll target 4.19.

github-actions · 2024-11-01T14:27:17Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2024-11-04T09:07:16Z

@blueorangutan package

blueorangutan · 2024-11-04T09:08:03Z

@DaanHoogland 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 · 2024-11-04T10:08:28Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11493

DaanHoogland · 2024-11-05T07:29:24Z

@blueorangutan test

blueorangutan · 2024-11-05T07:30:03Z

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

blueorangutan · 2024-11-05T20:59:34Z

[SF] Trillian test result (tid-11741)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 46174 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7081-t11741-kvm-ol8.zip
Smoke tests completed. 132 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_secure_vm_migration	`Error`	133.38	test_vm_life_cycle.py
test_01_secure_vm_migration	`Error`	133.38	test_vm_life_cycle.py

DaanHoogland · 2024-11-12T15:15:05Z

@RodrigoDLopez @winterhazel , are you guys testing this?

winterhazel · 2024-11-12T15:53:44Z

@RodrigoDLopez @winterhazel , are you guys testing this?

@DaanHoogland I already tested it in #7081 (comment), and consider this PR as ready for merge.

* 4.20: UI: Tooltip on the host information card to display the CPU speed in MHz and the memory value in MB (to 3 decimal places) (#9971) UI: Allow accounts of the `User` type to add other accounts or users to projects through UI (#9927) enable to create VPC portfowarding rules with source cidr (#7081) Add new column `last_id` to the table volumes (#9759) Allow VMWare import via another host (#9787) Linstor: add support for ISO block devices and direct download (#9792) get expunged VM data for job result (#9949) fix section divider display on auth page (#9966)

Co-authored-by: Lopez <rodrigo@scclouds.com.br> Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com>

* 4.20: UI: Tooltip on the host information card to display the CPU speed in MHz and the memory value in MB (to 3 decimal places) (apache#9971) UI: Allow accounts of the `User` type to add other accounts or users to projects through UI (apache#9927) enable to create VPC portfowarding rules with source cidr (apache#7081) Add new column `last_id` to the table volumes (apache#9759) Allow VMWare import via another host (apache#9787) Linstor: add support for ISO block devices and direct download (apache#9792) get expunged VM data for job result (apache#9949) fix section divider display on auth page (apache#9966)

boring-cyborg bot added component:api component:kubernetes component:networking component:virtual-router Python Warning... Python code Ahead! labels Jan 11, 2023

weizhouapache reviewed Jan 13, 2023

View reviewed changes

systemvm/debian/opt/cloud/bin/cs/CsNetfilter.py Outdated Show resolved Hide resolved

RodrigoDLopez mentioned this pull request May 18, 2023

VPC ACL Issue #7483

Closed

github-actions bot added the status:has-conflicts label Jun 1, 2023

DaanHoogland added this to the 4.19.0.0 milestone Jun 22, 2023

DaanHoogland changed the title ~~enable to create VPC porfowarding rules with source cidr~~ enable to create VPC portfowarding rules with source cidr Oct 9, 2023

DaanHoogland assigned RodrigoDLopez Nov 15, 2023

shwstppr modified the milestones: 4.19.0.0, 4.20.0.0 Dec 14, 2023

weizhouapache reviewed Jun 6, 2024

View reviewed changes

winterhazel self-assigned this Aug 26, 2024

JoaoJandre modified the milestones: 4.20.0.0, 4.21.0.0 Sep 10, 2024

boring-cyborg bot added the component:UI label Oct 21, 2024

github-actions bot removed the status:has-conflicts label Oct 21, 2024

Fix checkstyle

0eacf15

winterhazel force-pushed the Allow-source-CIDR-definition-in-port-forwarding-rules-for-VPCs branch from 4a438b6 to 0eacf15 Compare November 1, 2024 14:27

github-actions bot added the status:has-conflicts label Nov 1, 2024

RodrigoDLopez changed the base branch from main to 4.19 November 1, 2024 14:31

winterhazel removed the status:has-conflicts label Nov 1, 2024

DaanHoogland modified the milestones: 4.21.0.0, 4.19.2.0 Nov 4, 2024

apache deleted a comment from blueorangutan Nov 4, 2024

DaanHoogland added status:ready-for-merge and removed status:needs-testing labels Nov 13, 2024

DaanHoogland merged commit 4189bac into apache:4.19 Nov 28, 2024
1 check passed

DaanHoogland removed the status:ready-for-merge label Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable to create VPC portfowarding rules with source cidr #7081

enable to create VPC portfowarding rules with source cidr #7081

RodrigoDLopez commented Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 11, 2023 •

edited

Loading

sonarqubecloud bot commented Jan 11, 2023

DaanHoogland commented Jan 12, 2023

weizhouapache commented Jan 13, 2023

assistanz247 commented May 19, 2023

weizhouapache commented May 22, 2023

github-actions bot commented Jun 1, 2023

shwstppr commented Oct 9, 2023

DaanHoogland commented Nov 15, 2023

sureshanaparti commented Mar 12, 2024

DaanHoogland commented Jun 6, 2024

weizhouapache Jun 6, 2024

winterhazel Oct 22, 2024

weizhouapache Jun 6, 2024

weizhouapache Jun 6, 2024

winterhazel Oct 22, 2024 •

edited

Loading

winterhazel commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

DaanHoogland commented Nov 4, 2024

blueorangutan commented Nov 4, 2024

blueorangutan commented Nov 4, 2024

DaanHoogland commented Nov 5, 2024

blueorangutan commented Nov 5, 2024

blueorangutan commented Nov 5, 2024

DaanHoogland commented Nov 12, 2024

winterhazel commented Nov 12, 2024

enable to create VPC portfowarding rules with source cidr #7081

enable to create VPC portfowarding rules with source cidr #7081

Conversation

RodrigoDLopez commented Jan 11, 2023 • edited Loading

Description

Types of changes

Feature/Enhancement Scale

How Has This Been Tested?

codecov bot commented Jan 11, 2023 • edited Loading

Codecov Report

sonarqubecloud bot commented Jan 11, 2023

DaanHoogland commented Jan 12, 2023

weizhouapache commented Jan 13, 2023

assistanz247 commented May 19, 2023

weizhouapache commented May 22, 2023

github-actions bot commented Jun 1, 2023

shwstppr commented Oct 9, 2023

DaanHoogland commented Nov 15, 2023

sureshanaparti commented Mar 12, 2024

DaanHoogland commented Jun 6, 2024

weizhouapache Jun 6, 2024

Choose a reason for hiding this comment

winterhazel Oct 22, 2024

Choose a reason for hiding this comment

weizhouapache Jun 6, 2024

Choose a reason for hiding this comment

weizhouapache Jun 6, 2024

Choose a reason for hiding this comment

winterhazel Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

winterhazel commented Nov 1, 2024

github-actions bot commented Nov 1, 2024

DaanHoogland commented Nov 4, 2024

blueorangutan commented Nov 4, 2024

blueorangutan commented Nov 4, 2024

DaanHoogland commented Nov 5, 2024

blueorangutan commented Nov 5, 2024

blueorangutan commented Nov 5, 2024

DaanHoogland commented Nov 12, 2024

winterhazel commented Nov 12, 2024

RodrigoDLopez commented Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 11, 2023 •

edited

Loading

winterhazel Oct 22, 2024 •

edited

Loading