Add support for comma-delimited destinations in ASGs (#3644)

[#186770494](https://www.pivotaltracker.com/story/show/186770494)

Co-authored-by: Brandon Roberson <[email protected]>

Author: jrussett

app/messages/validators/security_group_rule_validator.rb

@@ -27,7 +27,12 @@ def validate(record)
 
       add_rule_error("protocol must be 'tcp', 'udp', 'icmp', or 'all'", record, index) unless valid_protocol(rule[:protocol])
 
-      validate_destination(rule[:destination], record, index) if valid_destination_type(rule[:destination], record, index)
+      if valid_destination_type(rule[:destination], record, index)
+        rule[:destination].split(',').each do |d|
+          validate_destination(d, record, index)
+        end
+      end
+
       validate_description(rule, record, index)
       validate_log(rule, record, index)
 
@@ -80,85 +85,66 @@ def validate_icmp_protocol(rule, record, index)
   end
 
   def valid_icmp_format(field)
-    field.is_a?(Integer) && field >= -1 && field <= 255
+    CloudController::ICMPRuleValidator.validate_icmp_control_message(field)
   end
 
   def valid_ports(ports)
     return false unless ports.is_a?(String)
-    return false if /[^\d\s\-,]/.match?(ports)
-
-    port_range = /\A\s*(\d+)\s*-\s*(\d+)\s*\z/.match(ports)
-    if port_range
-      left = port_range.captures[0].to_i
-      right = port_range.captures[1].to_i
-
-      return false if left >= right
-      return false unless port_in_range(left) && port_in_range(right)
-
-      return true
-    end
-
-    port_list = ports.split(',')
-    unless port_list.empty?
-      return false unless port_list.all? { |p| /\A\s*\d+\s*\z/.match(p) }
-      return false unless port_list.all? { |p| port_in_range(p.to_i) }
 
-      return true
-    end
-
-    false
-  end
-
-  def port_in_range(port)
-    port > 0 && port < 65_536
+    CloudController::TransportRuleValidator.validate_port(ports)
   end
 
   def valid_destination_type(destination, record, index)
+    error_message = 'destination must be a valid CIDR, IP address, or IP address range'
+    if CloudController::RuleValidator.comma_delimited_destinations_enabled?
+      error_message = 'nil destination; destination must be a comma-delimited list of valid CIDRs, IP addresses, or IP address ranges'
+    end
+
     if destination.nil?
-      add_rule_error('destination must be a valid CIDR, IP address, or IP address range', record, index)
+      add_rule_error(error_message, record, index)
       return false
     end
+
     unless destination.is_a?(String)
       add_rule_error('destination must be a string', record, index)
       return false
     end
+
     if /\s/ =~ destination
       add_rule_error('destination must not contain whitespace', record, index)
       return false
     end
 
+    if !CloudController::RuleValidator.comma_delimited_destinations_enabled? && !destination.index(',').nil?
+      add_rule_error(error_message, record, index)
+      return false
+    end
+
     true
   end
 
   def validate_destination(destination, record, index)
-    address_list = destination.split('-')
     error_message = 'destination must be a valid CIDR, IP address, or IP address range'
+    error_message = 'destination must contain valid CIDR(s), IP address(es), or IP address range(s)' if CloudController::RuleValidator.comma_delimited_destinations_enabled?
+
+    address_list = destination.split('-')
 
     if address_list.length == 1
-      add_rule_error(error_message, record, index) unless parse_ip(address_list.first)
+      add_rule_error(error_message, record, index) unless CloudController::RuleValidator.parse_ip(address_list.first)
 
     elsif address_list.length == 2
-      ipv4s = parse_ip(address_list)
-      return add_rule_error(error_message, record, index) unless ipv4s
+      ipv4s = CloudController::RuleValidator.parse_ip(address_list)
+      return add_rule_error('destination IP address range is invalid', record, index) unless ipv4s
 
       sorted_ipv4s = NetAddr.sort_IPv4(ipv4s)
-      add_rule_error(error_message, record, index) unless ipv4s.first == sorted_ipv4s.first
+      reversed_range_error = 'beginning of IP address range is numerically greater than the end of its range (range endpoints are inverted)'
+      add_rule_error(reversed_range_error, record, index) unless ipv4s.first == sorted_ipv4s.first
 
     else
       add_rule_error(error_message, record, index)
     end
   end
 
-  def parse_ip(val)
-    if val.is_a?(Array)
-      val.map { |ip| NetAddr::IPv4.parse(ip) }
-    else
-      NetAddr::IPv4Net.parse(val)
-    end
-  rescue NetAddr::ValidationError
-    nil
-  end
-
   def add_rule_error(message, record, index)
     record.errors.add("Rules[#{index}]:", message)
   end

config/cloud_controller.yml

@@ -261,6 +261,9 @@ default_staging_security_groups:
 default_running_security_groups:
 - dummy4
 
+security_groups:
+  enable_comma_delimited_destinations: false
+
 allowed_cors_domains:
 - http://*.appspot.com
 - http://*.inblue.net

docs/v3/source/includes/api_resources/_security_groups.erb

@@ -20,6 +20,12 @@
       "type": 8,
       "code": 0,
       "description": "Allow ping requests to private services"
+    },
+    {
+      "protocol": "tcp",
+      "destination": "1.1.1.1,2.2.2.2/24,10.0.0.0-10.0.0.255",
+      "ports": "80,443,8080",
+      "description": "Only valid if cc.security_groups.enable_comma_delimited_destinations is true"
     }
   ],
   "relationships": {

docs/v3/source/includes/resources/security_groups/_object.md.erb

@@ -26,7 +26,7 @@ Name | Type | Description
 | Name | Type | Description | Required | Default
 | ---- | ---- | ----------- | -------- | -------
 | **protocol** | _string_ | Protocol type Valid values are `tcp`, `udp`, `icmp`, or `all` | yes | N/A |
-| **destination** | _string_ | Destinations that the rule applies to Valid CIDR, IP address, or IP address range | yes | N/A |
+| **destination** | _string_ | The destination where the rule applies. Must be a singular Valid CIDR, IP address, or IP address range unless `cc.security_groups.enable_comma_delimited_destinations` is enabled. Then, the destination can be a comma-delimited string of CIDRs, IP addresses, or IP address ranges.  | yes | N/A |
 | **ports** | _string_ | Ports that the rule applies to; can be a single port (`9000`), a comma-separated list (`9000,9001`), or a range (`9000-9200`) | no | `null` |
 | **type** | _integer_ |[Type](https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-types) required for ICMP protocol; valid values are between -1 and 255 (inclusive), where -1 allows all | no | `null` |
 | **code** | _integer_ |[Code](https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-codes) required for ICMP protocol; valid values are between -1 and 255 (inclusive), where -1 allows all | no | `null` |

lib/cloud_controller/config_schemas/base/api_schema.rb

@@ -145,6 +145,10 @@ class ApiSchema < VCAP::Config
             default_staging_security_groups: [String],
             default_running_security_groups: [String],
 
+            security_groups: {
+              enable_comma_delimited_destinations: bool
+            },
+
             resource_pool: {
               maximum_size: Integer,
               minimum_size: Integer,

lib/cloud_controller/rule_validator.rb

@@ -10,7 +10,7 @@ def self.validate(rule)
       return errs unless errs.empty?
 
       destination = rule['destination']
-      errs << 'contains invalid destination' unless validate_destination(destination)
+      errs << 'contains invalid destination' unless validate_destination_type(destination) && validate_destination(destination)
 
       errs << 'contains invalid log value' if rule.key?('log') && !validate_boolean(rule['log'])
 
@@ -24,31 +24,59 @@ def self.validate_fields(rule)
         (rule.keys - (required_fields + optional_fields)).map { |key| "contains the invalid field '#{key}'" }
     end
 
+    def self.validate_destination_type(destination)
+      return false if destination.empty?
+
+      return false unless destination.is_a?(String)
+
+      return false if /\s/ =~ destination
+
+      true
+    end
+
     def self.validate_destination(destination)
-      return false if destination.empty? || /\s/ =~ destination
+      unless destination.index(',').nil?
+        return false unless comma_delimited_destinations_enabled?
 
-      address_list = destination.split('-')
+        destinations = destination.partition(',')
+        first_destination = destinations.first
+        remainder = destinations.last
+        return validate_destination(first_destination) && validate_destination(remainder)
+      end
 
-      return false if address_list.length > 2
+      address_list = destination.split('-')
 
       if address_list.length == 1
-        NetAddr::IPv4Net.parse(address_list.first)
-        return true
-      end
+        return true if parse_ip(address_list.first)
 
-      ipv4s = address_list.map do |address|
-        NetAddr::IPv4.parse(address)
+      elsif address_list.length == 2
+        ipv4s = parse_ip(address_list)
+        return false if ipv4s.nil?
+
+        sorted_ipv4s = NetAddr.sort_IPv4(ipv4s)
+        return true if ipv4s.first == sorted_ipv4s.first
       end
-      sorted_ipv4s = NetAddr.sort_IPv4(ipv4s)
-      return true if ipv4s.first == sorted_ipv4s.first
 
-      false
-    rescue NetAddr::ValidationError
       false
     end
 
     def self.validate_boolean(bool)
       !!bool == bool
     end
+
+    def self.parse_ip(val)
+      if val.is_a?(Array)
+        val.map { |ip| NetAddr::IPv4.parse(ip) }
+      else
+        NetAddr::IPv4Net.parse(val)
+      end
+    rescue NetAddr::ValidationError
+      nil
+    end
+
+    def self.comma_delimited_destinations_enabled?
+      config = VCAP::CloudController::Config.config
+      config.get(:security_groups, :enable_comma_delimited_destinations)
+    end
   end
 end

spec/unit/messages/security_group_create_message_spec.rb

@@ -145,6 +145,34 @@ module VCAP::CloudController
           end
         end
 
+        context 'when comma-delimited destinations are enabled' do
+          before do
+            TestConfig.config[:security_groups][:enable_comma_delimited_destinations] = true
+          end
+
+          context 'when rules are valid' do
+            let(:rules) do
+              [
+                {
+                  protocol: 'tcp',
+                  destination: '10.10.10.0/24,1.0.0.0-1.0.0.200',
+                  ports: '443,80,8080'
+                },
+                {
+                  protocol: 'icmp',
+                  destination: '10.10.10.0/24,1.1.1.1',
+                  type: 8,
+                  code: 0
+                }
+              ]
+            end
+
+            it 'is valid' do
+              expect(subject).to be_valid
+            end
+          end
+        end
+
         context 'when rules are invalid' do
           let(:rules) do
             [