Merge pull request #1045 from kmala/bug

fix(whitelist): Handle empty whitelist from user gracefully

Author: kmala

rootfs/api/migrations/0018_auto_20160908_1748.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.10 on 2016-09-08 17:48
+from __future__ import unicode_literals
+
+import django.contrib.postgres.fields
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api', '0017_appsettings_autoscale'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='appsettings',
+            name='whitelist',
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=50), default=None, size=None),
+        ),
+    ]

rootfs/api/models/app.py

@@ -970,8 +970,13 @@ def whitelist(self, whitelist):
         service = self._fetch_service_config(self.id)
 
         try:
-            addresses = ",".join(address for address in whitelist)
-            service['metadata']['annotations']['router.deis.io/whitelist'] = addresses
+            if whitelist:
+                addresses = ",".join(address for address in whitelist)
+                service['metadata']['annotations']['router.deis.io/whitelist'] = addresses
+            elif 'router.deis.io/whitelist' in service['metadata']['annotations']:
+                service['metadata']['annotations'].pop('router.deis.io/whitelist', None)
+            else:
+                return
             self._scheduler.svc.update(self.id, self.id, data=service)
         except KubeException as e:
             raise ServiceUnavailable(str(e)) from e

rootfs/api/models/appsettings.py

@@ -19,7 +19,9 @@ class AppSettings(UuidAuditedModel):
     app = models.ForeignKey('App', on_delete=models.CASCADE)
     maintenance = models.NullBooleanField(default=None)
     routable = models.NullBooleanField(default=None)
-    whitelist = ArrayField(models.CharField(max_length=50), default=[])
+    # the default values is None to differentiate from user sending an empty whitelist
+    # and user just updating other fields meaning the values needs to be copied from prev release
+    whitelist = ArrayField(models.CharField(max_length=50), default=None)
     autoscale = JSONField(default={}, blank=True)
 
     class Meta:
@@ -70,13 +72,14 @@ def update_routable(self, previous_settings):
             self.summary += ["{} changed routablity from {} to {}".format(self.owner, old, new)]
 
     def update_whitelist(self, previous_settings):
-        # If no previous settings then assume it is the first record and do nothing
+        # If no previous settings then assume it is the first record and set as empty
+        # to prevent from database constraint violation
         if not previous_settings:
-            return
+            setattr(self, 'whitelist', [])
         old = getattr(previous_settings, 'whitelist', [])
-        new = getattr(self, 'whitelist', [])
+        new = getattr(self, 'whitelist', None)
         # if nothing changed copy the settings from previous
-        if len(new) == 0 and len(old) != 0:
+        if new is None and old is not None:
             setattr(self, 'whitelist', old)
         elif set(old) != set(new):
             self.app.whitelist(new)

rootfs/api/tests/test_app_settings.py

@@ -99,7 +99,7 @@ def test_settings_whitelist(self, mock_requests):
         app_id = self.create_app()
         app = App.objects.get(id=app_id)
         # add addresses to empty whitelist
-        addresses = ["1.2.3.4", "0.0.0.0/0"]
+        addresses = ["0.0.0.0/0"]
         whitelist = {'addresses': addresses}
         response = self.client.post(
             '/v2/apps/{app_id}/whitelist'.format(**locals()),
@@ -108,12 +108,34 @@ def test_settings_whitelist(self, mock_requests):
         self.assertEqual(set(response.data['addresses']),
                          set(app.appsettings_set.latest().whitelist), response.data)
         self.assertEqual(set(response.data['addresses']), set(addresses), response.data)
+
         # get the whitelist
         response = self.client.get('/v2/apps/{app_id}/whitelist'.format(**locals()))
         self.assertEqual(response.status_code, 200, response.data)
         self.assertEqual(set(response.data['addresses']),
                          set(app.appsettings_set.latest().whitelist), response.data)
         self.assertEqual(set(response.data['addresses']), set(addresses), response.data)
+
+        # delete an address from whitelist
+        whitelist = {'addresses': ["0.0.0.0/0"]}
+        addresses.remove("0.0.0.0/0")
+        response = self.client.delete(
+            '/v2/apps/{app_id}/whitelist'.format(**locals()),
+            whitelist)
+        self.assertEqual(response.status_code, 204, response.data)
+        self.assertEqual(set(addresses), set(app.appsettings_set.latest().whitelist))
+
+        # add addresses to empty whitelist
+        addresses = ["0.0.0.0/0"]
+        whitelist = {'addresses': addresses}
+        response = self.client.post(
+            '/v2/apps/{app_id}/whitelist'.format(**locals()),
+            whitelist)
+        self.assertEqual(response.status_code, 201, response.data)
+        self.assertEqual(set(response.data['addresses']),
+                         set(app.appsettings_set.latest().whitelist), response.data)
+        self.assertEqual(set(response.data['addresses']), set(addresses), response.data)
+
         # add addresses to non-empty whitelist
         whitelist = {'addresses': ["2.3.4.5"]}
         addresses.extend(["2.3.4.5"])
@@ -124,6 +146,7 @@ def test_settings_whitelist(self, mock_requests):
         self.assertEqual(set(response.data['addresses']),
                          set(app.appsettings_set.latest().whitelist), response.data)
         self.assertEqual(set(response.data['addresses']), set(addresses), response.data)
+
         # add exisitng addresses to whitelist
         response = self.client.post(
             '/v2/apps/{app_id}/whitelist'.format(**locals()),
@@ -135,14 +158,6 @@ def test_settings_whitelist(self, mock_requests):
             '/v2/apps/{app_id}/whitelist'.format(**locals()),
             whitelist)
         self.assertEqual(response.status_code, 422)
-        # delete an address from whitelist
-        whitelist = {'addresses': ["2.3.4.5"]}
-        addresses.remove("2.3.4.5")
-        response = self.client.delete(
-            '/v2/apps/{app_id}/whitelist'.format(**locals()),
-            whitelist)
-        self.assertEqual(response.status_code, 204, response.data)
-        self.assertEqual(set(addresses), set(app.appsettings_set.latest().whitelist))
         # pass invalid address
         whitelist = {'addresses': ["2.3.4.6.7"]}
         response = self.client.post(