Better error messaging for ASGs with empty destinatinos in comma-delimited lists (#3783)

Author: geofffranks

app/messages/validators/security_group_rule_validator.rb

@@ -30,7 +30,7 @@ def validate(record)
       add_rule_error("protocol must be 'tcp', 'udp', 'icmp', or 'all'", record, index) unless valid_protocol(rule[:protocol])
 
       if valid_destination_type(rule[:destination], record, index)
-        rules = rule[:destination].split(',')
+        rules = rule[:destination].split(',', -1)
         add_rule_error("maximum destinations per rule exceeded - must be under #{MAX_DESTINATIONS_PER_RULE}", record, index) unless rules.length <= MAX_DESTINATIONS_PER_RULE
 
         rules.each do |d|
@@ -131,6 +131,7 @@ def valid_destination_type(destination, record, index)
   def validate_destination(destination, record, index)
     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?
+    add_rule_error('empty destination specified in comma-delimited list', record, index) if destination.empty?
 
     address_list = destination.split('-')
 

spec/unit/messages/validators/security_group_rule_validator_spec.rb

@@ -404,6 +404,84 @@ def self.name
             expect(subject.errors.full_messages).to include 'Rules[0]: maximum destinations per rule exceeded - must be under 6000'
           end
         end
+
+        context 'empty destinations in the front are invalid' do
+          let(:rules) do
+            [
+              {
+                protocol: 'udp',
+                destination: ',10.10.10.10,11.11.11.11',
+                ports: '8080'
+              }
+            ]
+          end
+
+          it 'throws an error for the missing destination' do
+            expect(subject).not_to be_valid
+            expect(subject.errors.full_messages.length).to equal(2)
+            expect(subject.errors.full_messages).to include 'Rules[0]: destination must contain valid CIDR(s), IP address(es), or IP address range(s)'
+            expect(subject.errors.full_messages).to include 'Rules[0]: empty destination specified in comma-delimited list'
+          end
+        end
+
+        context 'empty destinations in the middle are invalid' do
+          let(:rules) do
+            [
+              {
+                protocol: 'udp',
+                destination: '10.10.10.10,,11.11.11.11',
+                ports: '8080'
+              }
+            ]
+          end
+
+          it 'throws an error for the missing destination' do
+            expect(subject).not_to be_valid
+            expect(subject.errors.full_messages.length).to equal(2)
+            expect(subject.errors.full_messages).to include 'Rules[0]: destination must contain valid CIDR(s), IP address(es), or IP address range(s)'
+            expect(subject.errors.full_messages).to include 'Rules[0]: empty destination specified in comma-delimited list'
+          end
+        end
+
+        context 'empty destinations at the end are invalid' do
+          let(:rules) do
+            [
+              {
+                protocol: 'udp',
+                destination: '10.10.10.10,11.11.11.11,',
+                ports: '8080'
+              }
+            ]
+          end
+
+          it 'throws an error for the missing destination' do
+            expect(subject).not_to be_valid
+            expect(subject.errors.full_messages.length).to equal(2)
+            expect(subject.errors.full_messages).to include 'Rules[0]: destination must contain valid CIDR(s), IP address(es), or IP address range(s)'
+            expect(subject.errors.full_messages).to include 'Rules[0]: empty destination specified in comma-delimited list'
+          end
+        end
+
+        context 'multiple empty destinations are invalid' do
+          let(:rules) do
+            [
+              {
+                protocol: 'udp',
+                destination: ',10.10.10.10,,11.11.11.11,',
+                ports: '8080'
+              }
+            ]
+          end
+
+          it 'throws an error for each missing destination' do
+            expect(subject).not_to be_valid
+            expect(subject.errors.full_messages.length).to equal(6)
+            expect(subject.errors.full_messages).to include 'Rules[0]: destination must contain valid CIDR(s), IP address(es), or IP address range(s)'
+            expect(subject.errors.full_messages[0]).to eq 'Rules[0]: empty destination specified in comma-delimited list'
+            expect(subject.errors.full_messages[2]).to eq 'Rules[0]: empty destination specified in comma-delimited list'
+            expect(subject.errors.full_messages[4]).to eq 'Rules[0]: empty destination specified in comma-delimited list'
+          end
+        end
       end
     end