fix: Games not stopping (#1557)

* create secret in aimmo game manager

* added migration for game tokens and not clear them

* fix tests

* moved game_creator to aimmo

* add more tests, but still in progress

* Merge branch 'development' into games-not-stopping

* fix tests

* add another test

* code review cleanup

Pipfile

@@ -10,6 +10,7 @@ pytest-pythonpath = "*"
 codeforlife-portal = "*"
 pytest-cov = "*"
 black = "*"
+django-test-migrations = "==1.1.0"
 
 [packages]
 aimmo = {editable = true,path = "."}

aimmo-game-creator/game_manager.py

@@ -250,26 +250,6 @@ def _delete_game_service(self, game_id):
             LOGGER.info("Removing service: {}".format(resource.metadata.name))
             self.api.delete_namespaced_service(resource.metadata.name, K8S_NAMESPACE)
 
-    def _create_game_secret(self, game_id):
-        name = KubernetesGameManager._create_game_name(game_id) + "-token"
-        try:
-            self.api.read_namespaced_secret(name, K8S_NAMESPACE)
-        except ApiException:
-            data = {"token": self._generate_game_token()}
-            self.secret_creator.create_secret(name, K8S_NAMESPACE, data)
-
-    def _delete_game_secret(self, game_id):
-        app_label = "app=aimmo-game"
-        game_label = "game_id={}".format(game_id)
-
-        resources = self.api.list_namespaced_secret(
-            namespace=K8S_NAMESPACE, label_selector=",".join([app_label, game_label])
-        )
-
-        for resource in resources.items:
-            LOGGER.info("Removing game secret: {}".format(resource.metadata.name))
-            self.api.delete_namespaced_secret(resource.metadata.name, K8S_NAMESPACE)
-
     def _create_game_server_allocation(
         self, game_id: int, game_data: dict, retry_count: int = 0
     ) -> str:
@@ -325,7 +305,6 @@ def _delete_game_server(self, game_id):
             )
 
     def create_game(self, game_id, game_data):
-        self._create_game_secret(game_id)
         game_server_name = self._create_game_server_allocation(game_id, game_data)
         self._create_game_service(game_id, game_server_name)
         self._add_path_to_ingress(game_id)
@@ -335,7 +314,6 @@ def delete_game(self, game_id):
         self._remove_path_from_ingress(game_id)
         self._delete_game_service(game_id)
         self._delete_game_server(game_id)
-        self._delete_game_secret(game_id)
 
     def delete_unknown_games(self):
         gameservers = self.custom_objects_api.list_namespaced_custom_object(

aimmo/__init__.py

@@ -1,11 +1 @@
-"""
-Sets up the AimmoAppConfig class. Since we don't want to reset the app config every
-time the Django server reloads when we make a code change, the app config is set only
-if the main thread isn't already running.
-"""
-import os
-
-if os.environ.get("RUN_MAIN", None) != "true":
-    default_app_config = "aimmo.apps.AimmoAppConfig"
-
 __version__ = "0.69.17b529"

aimmo/game_creator.py

@@ -0,0 +1,74 @@
+# -*- coding: utf-8 -*-
+# Code for Life
+#
+# Copyright (C) 2021, Ocado Innovation Limited
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# ADDITIONAL TERMS – Section 7 GNU General Public Licence
+#
+# This licence does not grant any right, title or interest in any “Ocado” logos,
+# trade names or the trademark “Ocado” or any other trademarks or domain names
+# owned by Ocado Innovation Limited or the Ocado group of companies or any other
+# distinctive brand features of “Ocado” as may be secured from time to time. You
+# must not distribute any modification of this program using the trademark
+# “Ocado” or claim any affiliation or association with Ocado or its employees.
+#
+# You are not authorised to use the name Ocado (or any of its trade names) or
+# the names of any author or contributor in advertising or for publicity purposes
+# pertaining to the distribution of this program, without the prior written
+# authorisation of Ocado.
+#
+# Any propagation, distribution or conveyance of this program must include this
+# copyright notice and these terms. You must not misrepresent the origins of this
+# program; modified versions of the program must be marked as such and not
+# identified as the original program.
+
+import secrets
+from aimmo.avatar_creator import create_avatar_for_user
+from aimmo.game_manager import GameManager
+
+NUM_BYTES_FOR_TOKEN_GENERATOR = 32
+TOKEN_MAX_LENGTH = 48
+
+
+def generate_game_token():
+    return secrets.token_urlsafe(nbytes=NUM_BYTES_FOR_TOKEN_GENERATOR)[
+        :TOKEN_MAX_LENGTH
+    ]
+
+
+def create_game(main_user, form):
+    """
+    Creates a Game by:
+    - saving the form
+    - setting default values
+    - adding users who can play the game
+    - creating an avatar for the main user.
+    - creating the game secret in game manager
+    :param main_user: The user who requested game creation, and is the game owner.
+    :param form: The form used to submit the creation of the game.
+    :param users_to_add_to_game: List of User objects who are able to play this game.
+    :return: The initialised Game object.
+    """
+    game = form.save(commit=False)
+    game.auth_token = generate_game_token()
+    game.generator = "Main"
+    game.owner = main_user
+    game.main_user = main_user
+    game.save()
+    create_avatar_for_user(main_user, game.id)
+    game_manager = GameManager()
+    game_manager.create_game_secret(game_id=game.id, token=game.auth_token)
+    return game

aimmo/game_manager.py

@@ -7,6 +7,7 @@
 from kubernetes.client import CoreV1Api
 from kubernetes.client.api.custom_objects_api import CustomObjectsApi
 from kubernetes.client.api_client import ApiClient
+from kubernetes.client.rest import ApiException
 
 LOGGER = logging.getLogger(__name__)
 
@@ -137,3 +138,43 @@ def recreate_game_server(
             game_id=game_id, game_data=game_data
         )
         self.patch_game_service(game_id=game_id, game_server_name=game_server_name)
+
+    def create_game_secret(self, game_id, token):
+        name = self.create_game_name(game_id) + "-token"
+        body = kubernetes.client.V1Secret(
+            kind="Secret",
+            string_data={"token": token},
+            metadata=kubernetes.client.V1ObjectMeta(
+                name=name,
+                namespace=K8S_NAMESPACE,
+                labels={"game_id": str(game_id), "app": "aimmo-game"},
+            ),
+        )
+        try:
+            self.api.read_namespaced_secret(name, K8S_NAMESPACE)
+        except ApiException:
+            try:
+                self.api.create_namespaced_secret(namespace=K8S_NAMESPACE, body=body)
+            except ApiException:
+                LOGGER.exception("Exception when calling create_namespaced_secret")
+        else:
+            try:
+                self.api.patch_namespaced_secret(
+                    name=name, namespace=K8S_NAMESPACE, body=body
+                )
+            except ApiException:
+                LOGGER.exception("Exception when calling patch_namespaced_secret")
+
+    def delete_game_secret(self, game_id):
+        app_label = "app=aimmo-game"
+        game_label = "game_id={}".format(game_id)
+
+        resources = self.api.list_namespaced_secret(
+            namespace=K8S_NAMESPACE, label_selector=",".join([app_label, game_label])
+        )
+
+        for resource in resources.items:
+            LOGGER.info("Removing game secret: {}".format(resource.metadata.name))
+            self.api.delete_namespaced_secret(
+                name=resource.metadata.name, namespace=K8S_NAMESPACE
+            )

aimmo/migrations/0016_game_class.py

@@ -10,6 +10,7 @@ class Migration(migrations.Migration):
 
     dependencies = [
         ("common", "0001_initial"),
+        ("portal", "0058_move_to_common_models"),
         ("aimmo", "0015_game_worksheet"),
     ]
 

aimmo/migrations/0025_generate_auth_token.py

@@ -0,0 +1,40 @@
+# Generated by Django 2.2.24 on 2021-07-12 10:50
+
+from django.db import migrations, models
+
+from aimmo.game_creator import generate_game_token
+from aimmo.game_manager import GameManager
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("aimmo", "0024_unique_class_per_game"),
+    ]
+
+    def generate_auth_token_for_games(apps, schema_editor):
+        Game = apps.get_model("aimmo", "Game")
+        for game in Game.objects.all():
+            game.auth_token = generate_game_token()
+            game.save()
+            game_manager = GameManager()
+            game_manager.create_game_secret(game_id=game.id, token=game.auth_token)
+
+    def clear_auth_token_for_games(apps, schema_editor):
+        Game = apps.get_model("aimmo", "Game")
+        for game in Game.objects.all():
+            game.auth_token = ""
+            game.save()
+            game_manager = GameManager()
+            game_manager.delete_game_secret(game_id=game.id)
+
+    operations = [
+        migrations.AlterField(
+            model_name="game",
+            name="auth_token",
+            field=models.CharField(max_length=48),
+        ),
+        migrations.RunPython(
+            generate_auth_token_for_games, reverse_code=clear_auth_token_for_games
+        ),
+    ]

aimmo/models.py

@@ -63,7 +63,7 @@ class Game(models.Model):
     STATUS_CHOICES = ((RUNNING, "running"), (STOPPED, "stopped"), (PAUSED, "paused"))
 
     name = models.CharField(max_length=100, blank=True, null=True)
-    auth_token = models.CharField(max_length=24, blank=True)
+    auth_token = models.CharField(max_length=48, blank=True)
     owner = models.ForeignKey(
         User,
         blank=True,

aimmo/tests/conftest.py

@@ -0,0 +1,11 @@
+from unittest.mock import MagicMock
+
+import pytest
+
+
+@pytest.fixture(autouse=True)
+def mock_game_manager(monkeypatch):
+    """Mock GameManager for tests that don't need it."""
+    monkeypatch.setattr(
+        "aimmo.migrations.0025_generate_auth_token.GameManager", MagicMock()
+    )

aimmo/tests/test_game_manager.py

@@ -1,9 +1,11 @@
 import time
-from unittest.mock import DEFAULT, MagicMock
+from unittest.mock import DEFAULT, MagicMock, PropertyMock
 
+import aimmo.game_manager
 import kubernetes
 import pytest
 from aimmo.game_manager import K8S_NAMESPACE, GameManager
+from kubernetes.client.exceptions import ApiException
 
 
 @pytest.fixture
@@ -117,3 +119,62 @@ def test_recreate_game_server(game_manager, game_id):
     game_manager.patch_game_service.assert_called_with(
         game_id=game_id, game_server_name=mock_game_server_name
     )
+
+
+def test_create_game_secret(game_manager, game_id):
+    token = "secret-token"
+    name = game_manager.create_game_name(game_id) + "-token"
+    expected_secret = kubernetes.client.V1Secret(
+        kind="Secret",
+        string_data={"token": token},
+        metadata=kubernetes.client.V1ObjectMeta(
+            name=name,
+            namespace=K8S_NAMESPACE,
+            labels={"game_id": str(game_id), "app": "aimmo-game"},
+        ),
+    )
+
+    game_manager.api.read_namespaced_secret = MagicMock()
+    game_manager.api.create_namespaced_secret = MagicMock()
+    game_manager.api.patch_namespaced_secret = MagicMock()
+    aimmo.game_manager.LOGGER.exception = MagicMock()
+
+    # Test create secret success
+    game_manager.api.read_namespaced_secret.side_effect = ApiException()
+    game_manager.create_game_secret(game_id=game_id, token=token)
+    game_manager.api.create_namespaced_secret.assert_called_with(
+        namespace=K8S_NAMESPACE, body=expected_secret
+    )
+
+    # Test create secret exception
+    game_manager.api.create_namespaced_secret.side_effect = ApiException()
+    game_manager.create_game_secret(game_id=game_id, token=token)
+    aimmo.game_manager.LOGGER.exception.assert_called()
+
+    # Test patch secret success
+    game_manager.api.read_namespaced_secret.side_effect = None
+    game_manager.create_game_secret(game_id=game_id, token=token)
+    game_manager.api.patch_namespaced_secret.assert_called_with(
+        name=name, namespace=K8S_NAMESPACE, body=expected_secret
+    )
+
+    # Test patch secret exception
+    game_manager.api.patch_namespaced_secret.side_effect = ApiException()
+    game_manager.create_game_secret(game_id=game_id, token=token)
+    aimmo.game_manager.LOGGER.exception.assert_called()
+
+
+def test_delete_game_secret(game_manager, game_id):
+    secret_name = "secret1"
+    mock_secret = MagicMock()
+    mock_secret.metadata.name = secret_name
+    game_manager.api.list_namespaced_secret = MagicMock()
+    type(game_manager.api.list_namespaced_secret.return_value).items = PropertyMock(
+        return_value=[mock_secret]
+    )
+    game_manager.api.delete_namespaced_secret = MagicMock()
+
+    game_manager.delete_game_secret(game_id=game_id)
+    game_manager.api.delete_namespaced_secret.assert_called_with(
+        name=secret_name, namespace=K8S_NAMESPACE
+    )