Support from Acquia helps fund testing for Drupal Acquia logo

Comments

baldwinlouie created an issue. See original summary.

baldwinlouie’s picture

Status: Active » Needs review
FileSize
105.75 KB

Attaching 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 to group_name as $add[$i]['group_name[0][value]'] in the Unit Tests.

      ->setDisplayOptions(
        'form', [
          'type' => 'string_textfield',
        ]
      )

- 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.

Status: Needs review » Needs work

The last submitted patch, 2: 3153027-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

baldwinlouie’s picture

Status: Needs work » Needs review
FileSize
106.35 KB

Updating with Coder fixes.

Status: Needs review » Needs work

The last submitted patch, 4: 3153027-4.patch, failed testing. View results

baldwinlouie’s picture

FileSize
106.37 KB
3.31 KB

Attaching patch that fixes the tests.

baldwinlouie’s picture

Status: Needs work » Needs review
FileSize
118.66 KB
13.95 KB

New patch that fixes the openstack tests.

baldwinlouie’s picture

FileSize
119.31 KB

One more try. Coder complained about K8sCloudConfigTest.php

yas’s picture

@baldwinlouie

The coder complaint was my mistake. I fixed and merged the patch at #3153103-10 before I came here.

baldwinlouie’s picture

@yas, no problem!

baldwinlouie’s picture

FileSize
125.13 KB
8.41 KB

Updating the patch which fixes the issue with the public function isEmpty() in IpPermission.php

baldwinlouie’s picture

FileSize
125.19 KB
1.45 KB

updating the public function isEmpty() to use a switch statement.

  • yas committed 5efe72f on 8.x-1.x authored by baldwinlouie
    Issue #3153027 by baldwinlouie, yas: Add more validations in the add/...

  • yas committed cae8ef0 on 8.x-2.x authored by baldwinlouie
    Issue #3153027 by baldwinlouie, yas: Add more validations in the add/...
yas’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.