Closed (fixed)
Project:
Cloud
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
18 Jun 2020 at 18:45 UTC
Updated:
7 Jul 2020 at 01:14 UTC
Jump to comment: Most recent, Most recent file
Add more validations to Security Groups.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | interdiff-3153027-12.txt | 1.45 KB | baldwinlouie |
| #12 | 3153027-12.patch | 125.19 KB | baldwinlouie |
Comments
Comment #2
baldwinlouie commentedAttaching the first pass of the patch. It includes a bunch of things
- Validates whether the Security Group name is "default". This is a reserved term and errors out if sent to AWS
- Validates duplicate Security Group names.
- Validates duplicate permissions. It takes the to_port/from_port and ip_address, group_name, prefix_id, and checks the rule being added is a duplicate.
- Properly supporting prefix list id. Prefix_id is similar to Group, where you can have a IP permission against a prefix list id.
- Validates when adding a group_id based permission, that the group_id is in the same VPC as the Security Group.
- Validates when adding a group_id based permission, that the group_id actually exists.
- Validates ICMP ports
- Validates ICMPv6 ports
- Fixes a bug in IpPermission.php
return $this->get('peering_connection_id')->value();, which should be < return $this->get('peering_connection_id')->getValue();- Slightly refactor
Ec2Service.php- Refactor the code in
SecurityGroupEditForm.php- Added unit tests for Security Group Name
- Added unit tests for Port validation
There's a few things to note in the patch.
- I re-abled the following for Security
group_name. This way, I can add a constraint to the field. Because of this, you'll see references togroup_nameas$add[$i]['group_name[0][value]']in the Unit Tests.- In the tests, I had to remove "group" from any permission unit test. That is because the validation requires a valid security group. I'd have to rewrite all those tests to accommodate that.
- I can't add unit tests for duplicate permissions. That is because adding a second IP permission means using the "Add Rule" button, which is Ajax based.
Comment #4
baldwinlouie commentedUpdating with Coder fixes.
Comment #6
baldwinlouie commentedAttaching patch that fixes the tests.
Comment #7
baldwinlouie commentedNew patch that fixes the openstack tests.
Comment #8
baldwinlouie commentedOne more try. Coder complained about K8sCloudConfigTest.php
Comment #9
yas@baldwinlouie
The coder complaint was my mistake. I fixed and merged the patch at #3153103-10 before I came here.
Comment #10
baldwinlouie commented@yas, no problem!
Comment #11
baldwinlouie commentedUpdating the patch which fixes the issue with the
public function isEmpty()inIpPermission.phpComment #12
baldwinlouie commentedupdating the
public function isEmpty()to use a switch statement.Comment #15
yas