From c5e96697f12fd877973b6bce812227f9964b0cdd Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 21 Feb 2024 11:42:45 +0000
Subject: [PATCH 1/2] Bugfix/namespace length (#1257)
* Add namespace field to App
Will be used to store the Cloud Platform namespace name. Adds migrations
that add the field, then a data migration to add values for existing apps,
then finally a third migration to require values when creating a new
object.
* Add namespace field to the register app form
Adds the field in the HTML.
Update the OIDC template to use the entered namespace when
creating the app IAM role.
* Add APP_ROLE_ARN secret after creating role
Updates the create app role task handler to pass a github api
token, so this can be used to create the APP_ROLE_ARN secret as
soon as the App role is created. This is required for apps that
will be deployed without Auth0 clients.
---
controlpanel/api/cluster.py | 4 ++-
.../api/migrations/0032_app_namespace.py | 17 ++++++++++++
.../0033_add_namespaces_values_to_apps.py | 20 ++++++++++++++
.../0034_remove_null_blank_from_namespace.py | 17 ++++++++++++
controlpanel/api/models/app.py | 7 +++--
controlpanel/api/tasks/handlers/app.py | 3 ++-
.../assume_roles/cloud_platform_oidc.json | 4 +--
controlpanel/frontend/forms.py | 16 +++++++++++-
.../frontend/jinja2/webapp-create.html | 26 ++++++++++++++++---
controlpanel/frontend/views/app.py | 2 --
controlpanel/frontend/views/apps_mng.py | 1 +
tests/api/cluster/test_app.py | 24 ++++++++++++-----
tests/api/models/test_app.py | 4 +--
tests/api/tasks/test_create_app_aws_role.py | 5 ++--
tests/frontend/test_forms.py | 16 ++++++++++++
tests/frontend/views/test_app.py | 1 +
16 files changed, 144 insertions(+), 23 deletions(-)
create mode 100644 controlpanel/api/migrations/0032_app_namespace.py
create mode 100644 controlpanel/api/migrations/0033_add_namespaces_values_to_apps.py
create mode 100644 controlpanel/api/migrations/0034_remove_null_blank_from_namespace.py
diff --git a/controlpanel/api/cluster.py b/controlpanel/api/cluster.py
index eb66629df..1a1df2459 100644
--- a/controlpanel/api/cluster.py
+++ b/controlpanel/api/cluster.py
@@ -499,7 +499,7 @@ def oidc_provider_statement(self):
"identity_provider_arn": iam_arn(
f"oidc-provider/{settings.OIDC_APP_EKS_PROVIDER}"
),
- "app_name": self.app.slug,
+ "app_namespace": self.app.namespace,
}
)
return json.loads(statement)
@@ -508,6 +508,8 @@ def create_iam_role(self):
assume_role_policy = deepcopy(BASE_ASSUME_ROLE_POLICY)
assume_role_policy["Statement"].append(self.oidc_provider_statement)
self.aws_role_service.create_role(self.iam_role_name, assume_role_policy)
+ for env in self.get_deployment_envs():
+ self._create_secrets(env_name=env)
def grant_bucket_access(self, bucket_arn, access_level, path_arns):
self.aws_role_service.grant_bucket_access(
diff --git a/controlpanel/api/migrations/0032_app_namespace.py b/controlpanel/api/migrations/0032_app_namespace.py
new file mode 100644
index 000000000..c0bb1d482
--- /dev/null
+++ b/controlpanel/api/migrations/0032_app_namespace.py
@@ -0,0 +1,17 @@
+# Generated by Django 4.2.7 on 2024-02-20 16:00
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+ dependencies = [
+ ("api", "0031_add_soft_delete_fields"),
+ ]
+
+ operations = [
+ migrations.AddField(
+ model_name="app",
+ name="namespace",
+ field=models.CharField(blank=True, max_length=63, null=True, unique=True),
+ ),
+ ]
diff --git a/controlpanel/api/migrations/0033_add_namespaces_values_to_apps.py b/controlpanel/api/migrations/0033_add_namespaces_values_to_apps.py
new file mode 100644
index 000000000..64dd9c9b6
--- /dev/null
+++ b/controlpanel/api/migrations/0033_add_namespaces_values_to_apps.py
@@ -0,0 +1,20 @@
+# Generated by Django 4.2.7 on 2024-02-20 16:01
+
+from django.db import migrations
+
+
+def add_namespaces(apps, schema_editor):
+ App = apps.get_model("api", "App")
+ for app in App.objects.all():
+ app.namespace = f"data-platform-app-{app.slug}"
+ app.save()
+
+
+class Migration(migrations.Migration):
+ dependencies = [
+ ("api", "0032_app_namespace"),
+ ]
+
+ operations = [
+ migrations.RunPython(code=add_namespaces, reverse_code=migrations.RunPython.noop)
+ ]
diff --git a/controlpanel/api/migrations/0034_remove_null_blank_from_namespace.py b/controlpanel/api/migrations/0034_remove_null_blank_from_namespace.py
new file mode 100644
index 000000000..c406a65fb
--- /dev/null
+++ b/controlpanel/api/migrations/0034_remove_null_blank_from_namespace.py
@@ -0,0 +1,17 @@
+# Generated by Django 4.2.7 on 2024-02-20 16:11
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+ dependencies = [
+ ("api", "0033_add_namespaces_values_to_apps"),
+ ]
+
+ operations = [
+ migrations.AlterField(
+ model_name="app",
+ name="namespace",
+ field=models.CharField(max_length=63, unique=True),
+ ),
+ ]
diff --git a/controlpanel/api/models/app.py b/controlpanel/api/models/app.py
index 54b270da2..93140edef 100644
--- a/controlpanel/api/models/app.py
+++ b/controlpanel/api/models/app.py
@@ -34,6 +34,9 @@ class App(TimeStampedModel):
# are not within the fields which will be searched frequently
app_conf = models.JSONField(null=True)
+ # Stores the Cloud Platform namespace name
+ namespace = models.CharField(unique=True, max_length=63)
+
# Non database field just for passing extra parameters
disable_authentication = False
connections = {}
@@ -254,9 +257,9 @@ def app_url_name(self, env_name):
if not format_pattern:
format_pattern = settings.APP_URL_NAME_PATTERN.get(self.DEFAULT_SETTING_KEY_WORD)
if format_pattern:
- return format_pattern.format(app_name=self.slug, env=env_name)
+ return format_pattern.format(app_name=self.namespace, env=env_name)
else:
- return self.slug
+ return self.namespace
def get_auth_client(self, env_name):
env_name = env_name or self.DEFAULT_AUTH_CATEGORY
diff --git a/controlpanel/api/tasks/handlers/app.py b/controlpanel/api/tasks/handlers/app.py
index accc6d68b..0b9d05f8d 100644
--- a/controlpanel/api/tasks/handlers/app.py
+++ b/controlpanel/api/tasks/handlers/app.py
@@ -28,5 +28,6 @@ class CreateAppAWSRole(BaseModelTaskHandler):
name = "create_app_aws_role"
def handle(self):
- cluster.App(self.object).create_iam_role()
+ task_user = User.objects.filter(pk=self.task_user_pk).first()
+ cluster.App(self.object, task_user.github_api_token).create_iam_role()
self.complete()
diff --git a/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json b/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json
index 9c13e16ba..a5e5b8b42 100644
--- a/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json
+++ b/controlpanel/api/templates/assume_roles/cloud_platform_oidc.json
@@ -9,8 +9,8 @@
"StringEquals": {
"{{ identity_provider }}:aud": "sts.amazonaws.com",
"{{ identity_provider }}:sub": [
- "system:serviceaccount:data-platform-app-{{ app_name }}-dev:data-platform-app-{{ app_name }}-dev-sa",
- "system:serviceaccount:data-platform-app-{{ app_name }}-prod:data-platform-app-{{ app_name }}-prod-sa"
+ "system:serviceaccount:{{ app_namespace }}-dev:{{ app_namespace }}-dev-sa",
+ "system:serviceaccount:{{ app_namespace }}-prod:{{ app_namespace }}-prod-sa"
]
}
}
diff --git a/controlpanel/frontend/forms.py b/controlpanel/frontend/forms.py
index 982c37d87..be8a27dd4 100644
--- a/controlpanel/frontend/forms.py
+++ b/controlpanel/frontend/forms.py
@@ -116,7 +116,7 @@ def _check_inputs_for_custom_connection(self, cleaned_data):
return auth0_conn_data
-class CreateAppForm(AppAuth0Form):
+class CreateAppForm(forms.Form):
repo_url = forms.CharField(
max_length=512,
@@ -148,8 +148,10 @@ class CreateAppForm(AppAuth0Form):
empty_label="Select",
required=False,
)
+ namespace = forms.CharField(required=True, max_length=63)
def __init__(self, *args, **kwargs):
+ self.request = kwargs.pop("request", None)
super().__init__(*args, **kwargs)
self.fields["existing_datasource_id"].queryset = self.get_datasource_queryset()
@@ -204,6 +206,18 @@ def clean_repo_url(self):
return repo_url
+ def clean_namespace(self):
+ """
+ Removes the env suffix if the user included it
+ """
+ namespace = self.cleaned_data["namespace"]
+ for suffix in ["-dev", "-prod"]:
+ if suffix in namespace:
+ namespace = namespace.removesuffix(suffix)
+ break
+
+ return namespace
+
class UpdateAppAuth0ConnectionsForm(AppAuth0Form):
env_name = forms.CharField(widget=forms.HiddenInput)
diff --git a/controlpanel/frontend/jinja2/webapp-create.html b/controlpanel/frontend/jinja2/webapp-create.html
index 30b62d58b..33ef3768c 100644
--- a/controlpanel/frontend/jinja2/webapp-create.html
+++ b/controlpanel/frontend/jinja2/webapp-create.html
@@ -73,13 +73,28 @@ {{ page_title }}
}) }}
{% endif %}
-
+
+ Cloud Platform namespace
+
+
+ {% set error_repo_msg = form.errors.get("namespace") %}
+ {% if error_repo_msg %}
+ {% set errorId = 'namespace-error' %}
+ {{ govukErrorMessage({
+ "id": errorId,
+ "html": error_repo_msg|join(". "),
+ }) }}
+ {% endif %}
+ Enter namespace with the -env suffix removed
+
+
{{ govukRadios({
"name": "connect_bucket",
"fieldset": {
@@ -89,7 +104,7 @@ {{ page_title }}
},
},
"hint": {
- "text": "Connect an existing app data source to your app, or create a new one.",
+ "text": "Connect an existing app data source to your app, or create a new one. If you don't need to connect to an S3 bucket, select 'Do this later'",
},
"items": [
{
@@ -100,6 +115,9 @@ {{ page_title }}
{
"value": "existing",
"html": existing_datasource_html|safe,
+ "hint": {
+ "text": "Only buckets that you have admin access to are displayed",
+ },
"checked": form.connect_bucket.value() == "existing"
},
{
diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py
index 7d252f0f7..1fa990525 100644
--- a/controlpanel/frontend/views/app.py
+++ b/controlpanel/frontend/views/app.py
@@ -141,8 +141,6 @@ def get_form_kwargs(self):
kwargs = FormMixin.get_form_kwargs(self)
kwargs.update(
request=self.request,
- all_connections_names=auth0.ExtendedAuth0().connections.get_all_connection_names(), # noqa: E501
- custom_connections=auth0.ExtendedConnections.custom_connections(),
)
return kwargs
diff --git a/controlpanel/frontend/views/apps_mng.py b/controlpanel/frontend/views/apps_mng.py
index c930b52f1..58331f48c 100644
--- a/controlpanel/frontend/views/apps_mng.py
+++ b/controlpanel/frontend/views/apps_mng.py
@@ -31,6 +31,7 @@ def register_app(self, user, app_data):
name=name,
repo_url=repo_url,
current_user=user,
+ namespace=app_data["namespace"],
)
self._add_app_to_users(new_app, user)
self._create_or_link_datasource(new_app, user, app_data)
diff --git a/tests/api/cluster/test_app.py b/tests/api/cluster/test_app.py
index 4b082bc24..c9b4c6e48 100644
--- a/tests/api/cluster/test_app.py
+++ b/tests/api/cluster/test_app.py
@@ -1,10 +1,9 @@
# Standard library
from copy import deepcopy
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, patch, call
# Third-party
import pytest
-from django.conf import settings
# First-party/Local
from controlpanel.api import cluster, models
@@ -13,7 +12,11 @@
@pytest.fixture
def app():
- return models.App(slug="test-app", repo_url="https://gitpub.example.com/test-repo")
+ return models.App(
+ slug="test-app",
+ repo_url="https://gitpub.example.com/test-repo",
+ namespace="test-namespace",
+ )
@pytest.fixture
@@ -65,8 +68,8 @@ def oidc_provider_statement(app, settings):
"StringEquals": {
f"{settings.OIDC_APP_EKS_PROVIDER}:aud": "sts.amazonaws.com",
f"{settings.OIDC_APP_EKS_PROVIDER}:sub": [
- f"system:serviceaccount:data-platform-app-{app.slug}-dev:data-platform-app-{app.slug}-dev-sa", # noqa
- f"system:serviceaccount:data-platform-app-{app.slug}-prod:data-platform-app-{app.slug}-prod-sa" # noqa
+ f"system:serviceaccount:{app.namespace}-dev:{app.namespace}-dev-sa", # noqa
+ f"system:serviceaccount:{app.namespace}-prod:{app.namespace}-prod-sa" # noqa
]
}
}
@@ -77,13 +80,22 @@ def test_oidc_provider_statement(app, oidc_provider_statement):
assert cluster.App(app).oidc_provider_statement == oidc_provider_statement
-def test_app_create_iam_role(aws_create_role, app, oidc_provider_statement):
+@patch("controlpanel.api.cluster.App.get_deployment_envs")
+@patch("controlpanel.api.cluster.App._create_secrets")
+def test_app_create_iam_role(
+ _create_secrets, get_deployment_envs, aws_create_role, app, oidc_provider_statement
+):
expected_assume_role = deepcopy(BASE_ASSUME_ROLE_POLICY)
expected_assume_role["Statement"].append(oidc_provider_statement)
+ get_deployment_envs.return_value = ["dev", "prod"]
cluster.App(app).create_iam_role()
aws_create_role.assert_called_with(app.iam_role_name, expected_assume_role)
+ _create_secrets.assert_has_calls([
+ call(env_name="dev"),
+ call(env_name="prod"),
+ ])
@pytest.fixture # noqa: F405
diff --git a/tests/api/models/test_app.py b/tests/api/models/test_app.py
index c9132c5b0..39277aea6 100644
--- a/tests/api/models/test_app.py
+++ b/tests/api/models/test_app.py
@@ -61,10 +61,10 @@ def test_slug_characters_replaced():
@pytest.mark.django_db
def test_slug_collisions_increments():
- app = App.objects.create(repo_url="git@github.com:org/foo-bar.git")
+ app = App.objects.create(repo_url="git@github.com:org/foo-bar.git", namespace="foo-bar")
assert "foo-bar" == app.slug
- app2 = App.objects.create(repo_url="https://www.example.com/org/foo-bar")
+ app2 = App.objects.create(repo_url="https://www.example.com/org/foo-bar", namespace="foo-bar-2")
assert "foo-bar-2" == app2.slug
diff --git a/tests/api/tasks/test_create_app_aws_role.py b/tests/api/tasks/test_create_app_aws_role.py
index a882a9845..b377323fa 100644
--- a/tests/api/tasks/test_create_app_aws_role.py
+++ b/tests/api/tasks/test_create_app_aws_role.py
@@ -1,5 +1,5 @@
# Standard library
-from unittest.mock import patch
+from unittest.mock import patch, MagicMock
# Third-party
import pytest
@@ -25,6 +25,7 @@ def test_cluster_not_called_without_valid_app(cluster, complete, users):
@pytest.mark.django_db
+@patch("controlpanel.api.auth0.ExtendedAuth0", new=MagicMock())
@patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete")
@patch("controlpanel.api.tasks.handlers.app.cluster")
def test_valid_app_and_user(cluster, complete, users):
@@ -32,6 +33,6 @@ def test_valid_app_and_user(cluster, complete, users):
create_app_aws_role(app.pk, users["superuser"].pk)
- cluster.App.assert_called_once_with(app)
+ cluster.App.assert_called_once_with(app, users["superuser"].github_api_token)
cluster.App.return_value.create_iam_role.assert_called_once()
complete.assert_called_once()
diff --git a/tests/frontend/test_forms.py b/tests/frontend/test_forms.py
index 8473182a8..91a9baff0 100644
--- a/tests/frontend/test_forms.py
+++ b/tests/frontend/test_forms.py
@@ -5,6 +5,7 @@
import pytest
from django.core.exceptions import ValidationError
from django.urls import reverse
+from mock import MagicMock
# First-party/Local
from controlpanel.api import aws
@@ -136,6 +137,7 @@ def test_create_app_form_clean_new_datasource(create_app_request_superuser):
"repo_url": "https://github.com/ministryofjustice/my_repo",
"connect_bucket": "new",
"new_datasource_name": "test-bucketname",
+ "namespace": "my-repo"
},
request=create_app_request_superuser,
)
@@ -151,6 +153,7 @@ def test_create_app_form_clean_new_datasource(create_app_request_superuser):
"deployment_envs": ["test"],
"repo_url": "https://github.com/ministryofjustice/my_repo",
"connect_bucket": "new",
+ "namespace": "my-repo"
},
request=create_app_request_superuser,
)
@@ -268,6 +271,7 @@ def test_create_app_form_clean_repo_url(create_app_request_superuser):
"repo_url": "https://github.com/ministryofjustice/my_repo",
"connect_bucket": "new",
"new_datasource_name": "test-bucketname",
+ "namespace": "my-repo"
},
request=create_app_request_superuser,
)
@@ -441,3 +445,15 @@ def test_ip_allowlist_form_missing_name():
}
f = forms.IPAllowlistForm(data)
assert f.errors["name"] == ["This field is required."]
+
+
+@pytest.mark.parametrize("env", ["dev", "prod"])
+@mock.patch(
+ "controlpanel.frontend.forms.CreateAppForm.get_datasource_queryset",
+ new=MagicMock,
+)
+def test_clean_namespace(env):
+ form = forms.CreateAppForm()
+ form.cleaned_data = {"namespace": f"my-namespace-{env}"}
+
+ assert form.clean_namespace() == "my-namespace"
diff --git a/tests/frontend/views/test_app.py b/tests/frontend/views/test_app.py
index 041a7f2fb..d25ed94ff 100644
--- a/tests/frontend/views/test_app.py
+++ b/tests/frontend/views/test_app.py
@@ -710,6 +710,7 @@ def test_register_app_with_creating_datasource(client, users):
repo_url=f"https://github.com/ministryofjustice/{test_app_name}",
connect_bucket="new",
new_datasource_name=test_bucket_name,
+ namespace="test-app-namespace",
)
response = client.post(reverse("create-app"), data)
From cb57411b05ce474ef34fd19d513b7411cb036a75 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 21 Feb 2024 15:14:51 +0000
Subject: [PATCH 2/2] Fix app_url_name by stripping data-platform-app prefix
(#1258)
---
controlpanel/api/models/app.py | 5 +++--
tests/api/models/test_app.py | 13 +++++++++++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/controlpanel/api/models/app.py b/controlpanel/api/models/app.py
index 93140edef..b5365c80e 100644
--- a/controlpanel/api/models/app.py
+++ b/controlpanel/api/models/app.py
@@ -254,12 +254,13 @@ def auth0_client_name(self, env_name=None):
def app_url_name(self, env_name):
format_pattern = settings.APP_URL_NAME_PATTERN.get(env_name.upper())
+ namespace = self.namespace.removeprefix("data-platform-app-")
if not format_pattern:
format_pattern = settings.APP_URL_NAME_PATTERN.get(self.DEFAULT_SETTING_KEY_WORD)
if format_pattern:
- return format_pattern.format(app_name=self.namespace, env=env_name)
+ return format_pattern.format(app_name=namespace, env=env_name)
else:
- return self.namespace
+ return namespace
def get_auth_client(self, env_name):
env_name = env_name or self.DEFAULT_AUTH_CATEGORY
diff --git a/tests/api/models/test_app.py b/tests/api/models/test_app.py
index 39277aea6..58d5e8d8c 100644
--- a/tests/api/models/test_app.py
+++ b/tests/api/models/test_app.py
@@ -209,3 +209,16 @@ def test_app_allowed_ip_ranges():
def test_iam_role_arn():
app = App(slug="example-app")
assert app.iam_role_arn == f"arn:aws:iam::{settings.AWS_DATA_ACCOUNT_ID}:role/test_app_example-app"
+
+
+@pytest.mark.parametrize("namespace, env, expected", [
+ ("data-platform-app-example", "dev", "example-dev"),
+ ("example", "dev", "example-dev"),
+ ("data-platform-example", "dev", "data-platform-example-dev"),
+ ("data-platform-app-example", "prod", "example"),
+ ("example", "prod", "example"),
+ ("data-platform-example", "prod", "data-platform-example"),
+])
+def test_app_url_name(namespace, env, expected):
+ app = App(namespace=namespace)
+ assert app.app_url_name(env_name=env) == expected