CLOUDSTACK-10090:createPortForwardingRule api call accepts 'halt' as … #2273
Conversation
…Protocol which Stops VR
| capabilities.put(Service.PortForwarding, null); | ||
|
|
||
| final Map<Capability, String> portForwardingCapabilities = new HashMap<Capability, String>(); | ||
| portForwardingCapabilities.put(Capability.SupportedProtocols, NetUtils.TCP_PROTO + "," + NetUtils.UDP_PROTO); |
There was a problem hiding this comment.
Why UDP? HAProxy cannot do UDP loadbalancing.
There was a problem hiding this comment.
@remibergsma
The changes here are for PF not for LB. Did I get you correctly ?
|
for PF protocol validation code changes LGTM |
|
Tested the fix and it seems to be working fine. Test LGTM. Before Applying fix : If createPortForwardingRule api is executed with Protocol as "Halt", then router goes into halt state. After Applying fix : If createPortForwardingRule api is executed with Protocol as "Halt", then it throws an error. Below is the screenshot :
|
|
tag:This is Ready to Merge |
|
@rhtyd Could you merge this please |
|
@mrunalinikankariya can you explain where/how this method is used and if putting a hardcoded protocol list can have undesirable effects, for example what if not both For merging, this needs to be tested first. |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1211 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@rhtyd |
|
Thanks @mrunalinikankariya for the explanation, however, I'm wondering if this might break use-cases where you would want to port-forward non-tcp/non-udp traffic such as |
|
Trillian test result (tid-1622)
|
|
@rhtyd We can add list of allowed/supported protocols in the port forwarding capabilities rest will be blocked. If esp, ah is used in PF these can be added as well. |
|
@jayapalu @mrunalinikankariya looks like there are several regression failures, this cannot be accepted as such, you can consider refactoring the logic to throw exception only for unsupported keywords/protocols |
|
@rhtyd @mrunalinikankariya Test failures are not related these changes. If we look at the changes there are cpvm, ssvm, iso failures which are unrelated to it. There are test setup failures as well. |
|
Okay @jayapalu rekicking tests. |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1632)
|
|
Tests LGTM, given the provided feedback and explanations I'll merge this now. Thanks @mrunalinikankariya and @jayapalu |
…Protocol which Stops VR
When we run the createPortForwardingRule API with input as Protocol as halt the PF rule is added however Halt is executed on VR. Hence the VR is stopped.
Following entry added to Firewall_Rules table and VirtualRouter went to halt(stopped)
mysql> select * from firewall_rules where id = 7
*************************** 1. row ***************************
id: 7
uuid: XXXXXXXXXXXXXXXXXXXXXXXXXXX
ip_address_id: 13
start_port: 222
end_port: 222
state: Revoke
protocol:
haltpurpose: PortForwarding
account_id: 2
domain_id: 1
network_id: 208
xid: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
created: 2017-09-04 04:48:16
icmp_code: NULL
icmp_type: NULL
related: NULL
type: User
vpc_id: NULL
traffic_type