feat(config): validate PORT and HEALTHCHECK_* values for config:set operations

Fixes #774
Fixes #777
Fixes #781

Author: helgi

rootfs/api/serializers.py

@@ -4,6 +4,7 @@
 
 import json
 import re
+from urllib.parse import urlparse
 
 from django.contrib.auth.models import User
 from django.utils import timezone
@@ -139,6 +140,39 @@ def validate_values(self, data):
                     "Config keys must start with a letter or underscore and "
                     "only contain [A-z0-9_]")
 
+            # Validate PORT
+            if key == 'PORT':
+                if not str(value).isnumeric():
+                    raise serializers.ValidationError('PORT can only be a numeric value')
+                elif int(value) not in range(1, 65536):
+                    # check if hte port is between 1 and 65535. One extra added for range()
+                    # http://kubernetes.io/docs/api-reference/v1/definitions/#_v1_serviceport
+                    raise serializers.ValidationError('PORT needs to be between 1 and 65535')
+
+            # Validate HEALTHCHECK_*
+            if key == 'HEALTHCHECK_URL':
+                # Only Path information is supported, not query / anchor or anything else
+                # Path is the only thing Kubernetes supports right now
+                # See https://github.com/deis/controller/issues/774
+                uri = urlparse(value)
+
+                if not uri.path:
+                    raise serializers.ValidationError(
+                        '{} is missing a URI path (such as /healthz). '
+                        'Without it no health check can be done'.format(key)
+                    )
+
+                # Disallow everything but path
+                # https://docs.python.org/3/library/urllib.parse.html
+                if uri.query or uri.fragment or uri.netloc:
+                    raise serializers.ValidationError(
+                        '{} can only be a URI path (such as /healthz) that does not contain '
+                        'other things such as query params'.format(key)
+                    )
+            elif key.startswith('HEALTHCHECK_') and not str(value).isnumeric():
+                # all other healthchecks are integers
+                raise serializers.ValidationError('{} can only be a numeric value'.format(key))
+
         return data
 
     def validate_memory(self, data):

rootfs/api/tests/test_config.py

@@ -276,6 +276,25 @@ def test_invalid_config_keys(self, mock_requests):
             resp = self.client.post(url, body)
             self.assertEqual(resp.status_code, 400)
 
+    def test_invalid_config_values(self, mock_requests):
+        """
+        Test that invalid config values are rejected.
+        Right now only PORT is checked
+        """
+        data = [
+            {'field': 'PORT', 'value': 'dog'},
+            {'field': 'PORT', 'value': 99999}
+        ]
+        url = '/v2/apps'
+        response = self.client.post(url)
+        self.assertEqual(response.status_code, 201, response.data)
+        app_id = response.data['id']
+        url = '/v2/apps/{app_id}/config'.format(**locals())
+        for row in data:
+            body = {'values': json.dumps({row['field']: row['value']})}
+            resp = self.client.post(url, body)
+            self.assertEqual(resp.status_code, 400, response.data)
+
     def test_admin_can_create_config_on_other_apps(self, mock_requests):
         """If a non-admin creates an app, an administrator should be able to set config
         values for that app.
@@ -727,33 +746,77 @@ def test_healthchecks(self, mock_requests):
         """
         Test that healthchecks can be applied
         """
-        url = '/v2/apps'
-        response = self.client.post(url)
+        response = self.client.post('/v2/apps')
         self.assertEqual(response.status_code, 201, response.data)
         app_id = response.data['id']
 
-        # Set healthcheck URL to get defaults set
-        body = {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': '25'})}
+        # Set a healthcheck option before URL is around (URL is required for full setting)
         resp = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
-            body
+            {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': '25'})}
         )
-        self.assertEqual(resp.status_code, 201)
+        self.assertEqual(resp.status_code, 201, response.data)
         self.assertIn('HEALTHCHECK_INITIAL_DELAY', resp.data['values'])
         self.assertEqual(resp.data['values']['HEALTHCHECK_INITIAL_DELAY'], '25')
 
         # Set healthcheck URL to get defaults set
-        body = {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
         resp = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
-            body
+            {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
         )
-        self.assertEqual(resp.status_code, 201)
+        self.assertEqual(resp.status_code, 201, response.data)
         self.assertIn('HEALTHCHECK_URL', resp.data['values'])
         self.assertEqual(resp.data['values']['HEALTHCHECK_URL'], '/health')
 
         # post a new build
-        url = "/v2/apps/{app_id}/builds".format(**locals())
-        body = {'image': 'quay.io/autotest/example'}
-        response = self.client.post(url, body)
+        response = self.client.post(
+            "/v2/apps/{app_id}/builds".format(**locals()),
+            {'image': 'quay.io/autotest/example'}
+        )
+        self.assertEqual(response.status_code, 201, response.data)
+
+    def test_healthchecks_validations(self, mock_requests):
+        """
+        Test that healthchecks validations work
+        """
+        response = self.client.post('/v2/apps')
         self.assertEqual(response.status_code, 201, response.data)
+        app_id = response.data['id']
+
+        # Set one of the values that require a numeric value to a string
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': 'horse'})}
+        )
+        self.assertEqual(resp.status_code, 400, response.data)
+
+        # test URL - Path is the only allowed thing
+        # Try setting various things such as query param
+
+        # query param
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'values': json.dumps({'HEALTHCHECK_URL': '/health?testing=0'})}
+        )
+        self.assertEqual(resp.status_code, 400, response.data)
+
+        # fragment
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'values': json.dumps({'HEALTHCHECK_URL': '/health#db'})}
+        )
+        self.assertEqual(resp.status_code, 400, response.data)
+
+        # netloc
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'values': json.dumps({'HEALTHCHECK_URL': 'http://someurl.com/health/'})}
+        )
+        self.assertEqual(resp.status_code, 400, response.data)
+
+        # no path
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'values': json.dumps({'HEALTHCHECK_URL': 'http://someurl.com'})}
+        )
+        self.assertEqual(resp.status_code, 400, response.data)