Add protocol number support for security group rules by ravening · Pull Request #3736 · apache/cloudstack

ravening · 2019-12-03T09:19:03Z

Description

Currently while creating ingress/egress rule for a security group,
we can specify only TCP/UDP/ICMP. Sometimes we need to add rules
for different protocol numbers or rules for all the above three
mentioned protocols.

In this new feature, users can specify the protocol number or select
"ALL" option which will apply rules for TCP/UDP/ICMP in one shot

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)

Screenshots (if appropriate):

Navigate to "Networks" and select "Security groups" from the drop-down menu on the top left
Select any security group name and click on either "ingress" or "egress" tab

How Has This Been Tested?

Adding a rule for a particular protocol number

Create a VM and note down the security group name to which it belongs to
Navigate to "Network" and select "Security groups" from the drop-down menu on the top left.
Select the security group which is obtained from step 1.
Click on either ingress or egress tab
Try to create a new rule by selecting "Protocol number" from the drop-down list.
For ex: Enter protocol number as 47 and cidr as "99.99.99.99/32"

Now get the hostname on which the VM is running and ssh to it.
Run the iptables-save command to ensure that there is a new rule for the protocol number you mentioned above.
The output should be as shown below

# iptables-save | grep 99.99.99
-A i-2-118-VM -s 99.99.99.99/32 -p gre -j ACCEPT

Adding a rule to all TCP/UDP/ICMP at a time

Create a VM and note down the security group name to which it belongs to
By default, there will be a drop action for the three protocols.
Navigate to "Network" and select "Security groups" from the drop-down menu on the top left.
Select the security group which is obtained from step 1.
Click on either ingress or egress tab
Try to create a new rule by selecting "ALL" from the drop-down list.

Now get the hostname on which the VM is running and ssh to it.
Run the iptables-save command to ensure that the below rule should be applied.

# iptables-save | grep i-2-118-VM
-A i-2-118-VM -s 88.88.88.88/32 -m state --state NEW -j ACCEPT

New integration test has been added in test/integration/component/test_protocol_number_security_group.py

Integration test can be run using

nosetests --with-marvin --marvin-config=<config file> test/integration/component/test_protocol_number_security_group.py

Currently while creating ingress/egress rule for a security group, we can specify only TCP/UDP/ICMP. Sometimes we need to add rules for different protocol number or rules for all the above three mentioned protocols. In this new feature users can specify the protocol number or select "ALL" option which will apply rules for TCP/UDP/ICMP

weizhouapache

I have reviewed all changes, and tested it manually.
The integration test works fine as well.

wido · 2019-12-03T10:26:34Z

+            else:
+                dport = " --dport " + str(start) + ":" + str(end)
+
+            if protocol != 'all' and protocol != 'icmp' and protocol != 'tcp' and protocol != 'udp':


For ip6tables it's not 'icmp' which is used for the protocol, it's icmpv6, so don't we need to add a check there?

In the UI, we only have options for "TCP", "UDP", "ICMP" and "ALL". There is no option for "ICMPv6" in UI. Once this is added in ui, we can have that check here as well.

@rakgenius I'm aware, but underneath we translate ICMP to ICMPv6 when the CIDR/IP is an IPv6 adress. That's what we need to check.

@wido the protocol is "icmp" in cloudstack, no matter the ip/cidr is ipv4 or ipv6.

look at the code below
https://github.com/apache/cloudstack/pull/3736/files/c27fdfee8c9f5ab06cf18dd6dc41ba356d75f4c4#diff-d10df45583c471e8de4a64128f95c802R1066-R1070

ravening · 2019-12-10T11:23:14Z

@wido @rhtyd @GabrielBrascher guys, can you review this when you get time?

weizhouapache · 2020-01-07T08:40:35Z

@DaanHoogland this PR has two approvals. could you kick off a trillian test ?

DaanHoogland · 2020-01-07T09:15:37Z

@blueorangutan package

blueorangutan · 2020-01-07T09:15:43Z

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

blueorangutan · 2020-01-07T09:34:56Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-543

DaanHoogland · 2020-01-07T09:43:21Z

@blueorangutan test

blueorangutan · 2020-01-07T09:43:43Z

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

blueorangutan · 2020-01-07T18:29:33Z

Trillian test result (tid-713)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29736 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3736-t713-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 77 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Currently while creating ingress/egress rule for a security group, we can specify only TCP/UDP/ICMP. Sometimes we need to add rules for different protocol number or rules for all the above three mentioned protocols. In this new feature users can specify the protocol number or select "ALL" option which will apply rules for TCP/UDP/ICMP

weizhouapache requested review from GabrielBrascher, wido and yadvr December 3, 2019 09:24

weizhouapache approved these changes Dec 3, 2019

View reviewed changes

wido reviewed Dec 3, 2019

View reviewed changes

svenvogel added type:enhancement component:security-group labels Dec 3, 2019

ravening requested a review from wido December 16, 2019 14:37

weizhouapache mentioned this pull request Jan 6, 2020

protocol number support in security group rules #3044

Closed

andrijapanicsb added this to the 4.14.0.0 milestone Jan 6, 2020

wido approved these changes Jan 6, 2020

View reviewed changes

andrijapanicsb merged commit dfd950c into apache:master Jan 8, 2020

yadvr mentioned this pull request Jun 8, 2020

[FEATURE] Support for protocol no. in SG apache/cloudstack-primate#406

Closed

Uh oh!

Conversation

ravening commented Dec 3, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

wido Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

ravening Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

wido Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

weizhouapache Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

ravening commented Dec 10, 2019

Uh oh!

weizhouapache commented Jan 7, 2020

Uh oh!

DaanHoogland commented Jan 7, 2020

Uh oh!

blueorangutan commented Jan 7, 2020

Uh oh!

blueorangutan commented Jan 7, 2020

Uh oh!

DaanHoogland commented Jan 7, 2020

Uh oh!

blueorangutan commented Jan 7, 2020

Uh oh!

blueorangutan commented Jan 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants