fix(container): typed port error

Author: duanhongyi

rootfs/api/models/app.py

@@ -23,7 +23,6 @@
 from api.exceptions import AlreadyExists, DryccException, ServiceUnavailable
 from api.utils import generate_app_name, apply_tasks
 from scheduler import KubeHTTPException, KubeException
-from scheduler.resources.pod import DEFAULT_CONTAINER_PORT
 from .gateway import Gateway, Route
 from .limit import LimitPlan
 from .config import Config
@@ -682,9 +681,9 @@ def _deploy(self, deploys, procfile_types, prev_release,
             err = '(app::deploy): {}'.format(e)
             self.log(err, logging.ERROR)
             raise ServiceUnavailable(err) from e
-        for procfile_type, value in deploys.items():
+        for procfile_type in deploys.keys():
             if procfile_type == PROCFILE_TYPE_WEB:  # http
-                target_port = int(value.get('envs', {}).get('PORT', DEFAULT_CONTAINER_PORT))
+                target_port = release.get_port(procfile_type)
                 self._create_default_ingress(target_port)
             service = self.service_set.filter(procfile_type=procfile_type).first()
             if not service:
@@ -953,14 +952,13 @@ def _build_env_vars(self, release, procfile_type):
         }
 
         default_env['SOURCE_VERSION'] = release.build.sha
-
-        # fetch application port and inject into ENV vars as needed
-        port = release.get_port()
-        if port:
-            default_env['PORT'] = port
         # merge envs on top of default to make envs win
         default_env.update(release.config.values)
         default_env.update(release.config.typed_values.get(procfile_type, {}))
+        # fetch application port and inject into ENV vars as needed
+        port = release.get_port(procfile_type)
+        if port:
+            default_env['PORT'] = port
         return default_env
 
     def _get_private_registry_config(self, image, registry=None):

rootfs/api/models/release.py

@@ -163,54 +163,15 @@ def new(self, user, config, build, summary=None, canary=False):
             build=build, version=new_version, summary=summary, canary=canary
         )
 
-    def get_port(self):
+    def get_port(self, procfile_type):
         """
         Get application port for a given release. If pulling from private registry
         then use default port or read from ENV var, otherwise attempt to pull from
         the container image
         """
-        try:
-            envs = self.config.values
-            creds = self.get_registry_auth()
-
-            if self.build.type == "buildpack":
-                self.log(
-                    'buildpack type detected. Defaulting to $PORT %s' % DEFAULT_CONTAINER_PORT)
-                return DEFAULT_CONTAINER_PORT
-
-            # application has registry auth - $PORT is required
-            if (creds is not None) or (settings.REGISTRY_LOCATION != 'on-cluster'):
-                if envs.get('PORT', None) is None:
-                    if not self.app.appsettings_set.latest().routable:
-                        return None
-                    raise DryccException(
-                        'PORT needs to be set in the application config '
-                        'when using a private registry'
-                    )
-
-                # User provided PORT
-                return int(envs.get('PORT'))
-
-            # If the user provides PORT
-            return int(envs.get('PORT', DEFAULT_CONTAINER_PORT))
-
-        except Exception as e:
-            raise DryccException(str(e)) from e
-
-    def get_registry_auth(self):
-        """
-        Gather login information for private registry if needed
-        """
-        auth = None
-        registry = self.config.registry
-        if registry.get('username', None):
-            auth = {
-                'username': registry.get('username', None),
-                'password': registry.get('password', None),
-                'email': self.owner.email
-            }
-
-        return auth
+        return int(self.config.typed_values.get(
+            procfile_type, {}).get(
+                'PORT', self.config.values.get('PORT', DEFAULT_CONTAINER_PORT)))
 
     def previous(self):
         """

rootfs/api/tests/test_release.py

@@ -15,7 +15,6 @@
 from api.models.build import Build
 from api.models.release import Release
 from scheduler import KubeHTTPException
-from api.exceptions import DryccException
 from api.tests import adapter, DryccTransactionTestCase
 import requests_mock
 
@@ -443,10 +442,7 @@ def test_release_get_port(self, mock_requests):
         release = app.release_set.latest()
 
         # when app is not routable, then it still return 5000
-        self.assertEqual(release.get_port(), 5000)
-
-        # when a buildpack type, default to 5000
-        self.assertEqual(release.get_port(), 5000)
+        self.assertEqual(release.get_port('web'), 5000)
 
         # switch to a dockerfile app or else it'll automatically default to 5000
         url = f'/v2/apps/{app_id}/builds'
@@ -459,11 +455,29 @@ def test_release_get_port(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
         release = app.release_set.latest()
+        self.assertEqual(release.get_port('web'), 8080)
 
-        # check that the port number returned is an int, not a string
-        self.assertEqual(release.get_port(), 8080)
+        url = f'/v2/apps/{app_id}/config'
+        body = {'typed_values': json.dumps({"web": {'PORT': '9000'}})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+        release = app.release_set.latest()
+        self.assertEqual(release.get_port('web'), 9000)
 
-        # TODO(bacongobbler): test dockerfile ports
+        # not web procfile
+        self.assertEqual(release.get_port('task'), 8080)
+        url = f'/v2/apps/{app_id}/config'
+        body = {'values': json.dumps({'PORT': '9000'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+        release = app.release_set.latest()
+        self.assertEqual(release.get_port('task'), 9000)
+        # set typed_values port
+        body = {'typed_values': json.dumps({"task": {'PORT': '9001'}})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+        release = app.release_set.latest()
+        self.assertEqual(release.get_port('task'), 9001)
 
     @override_settings(DRYCC_DEPLOY_HOOK_URLS=['http://drycc.rocks'])
     @mock.patch('api.models.release.logger')
@@ -575,45 +589,15 @@ def test_release_external_registry(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
         release = app.release_set.latest()
-
-        self.assertEqual(release.get_port(), 3000)
-
+        self.assertEqual(release.get_port('web'), 3000)
         self.assertEqual(release.get_deploy_image(PROCFILE_TYPE_WEB), 'test/autotest/example')
 
-    @override_settings(REGISTRY_LOCATION="off-cluster")
-    def test_release_external_registry_no_port(self, mock_requests):
-        """
-        Test that an exception is raised when registry if off-cluster but
-        no port is provided.
-        """
-        app_id = self.create_app()
-
-        app = App.objects.get(id=app_id)
-        url = f'/v2/apps/{app_id}/builds'
-        body = {'image': 'test/autotest/example', 'stack': 'container'}
+        url = f'/v2/apps/{app_id}/config'
+        body = {'typed_values': json.dumps({"web": {'PORT': '9000'}})}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
-        data = self.client.get(f"/v2/apps/{app_id}/releases/", body).json()
-        self.assertEqual(data["results"][0]["state"], "crashed", data)
-
-        with self.assertRaises(
-            DryccException,
-            msg='PORT needs to be set in the config when using a private registry'
-        ):
-            release = app.release_set.latest()
-            release.get_port()
-
-        # Set routable to false and port should be None instead of error
-        response = self.client.post(
-            f'/v2/apps/{app.id}/settings',
-            {'routable': False}
-        )
-        self.assertEqual(response.status_code, 201, response.data)
-        self.assertFalse(app.appsettings_set.latest().routable)
-
         release = app.release_set.latest()
-        port = release.get_port()
-        self.assertIsNone(port)
+        self.assertEqual(release.get_port('web'), 9000)
 
     def test_diff_procfile_types(self, mock_requests):
         app_id = self.create_app()