Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/namespace length #1257

Merged
merged 5 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion controlpanel/api/cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(
Expand Down
17 changes: 17 additions & 0 deletions controlpanel/api/migrations/0032_app_namespace.py
Original file line number Diff line number Diff line change
@@ -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),
),
]
20 changes: 20 additions & 0 deletions controlpanel/api/migrations/0033_add_namespaces_values_to_apps.py
Original file line number Diff line number Diff line change
@@ -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)
]
Original file line number Diff line number Diff line change
@@ -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),
),
]
7 changes: 5 additions & 2 deletions controlpanel/api/models/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion controlpanel/api/tasks/handlers/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
}
}
Expand Down
16 changes: 15 additions & 1 deletion controlpanel/frontend/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -204,6 +206,18 @@ def clean_repo_url(self):

return repo_url

def clean_namespace(self):
"""
Removes the env suffix is the user included it
michaeljcollinsuk marked this conversation as resolved.
Show resolved Hide resolved
"""
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)
Expand Down
26 changes: 22 additions & 4 deletions controlpanel/frontend/jinja2/webapp-create.html
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,28 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
}) }}
{% endif %}

<input type="text" class="govuk-input" id="display_result_repo" name="repo_url" />
<input type="text" class="govuk-input" id="display_result_repo" name="repo_url" required />

<br/>
<br/>
</div>
<div class="govuk-form-group" id="container-element">

<label class="govuk-label govuk-label--m" for="display_result_repo">Cloud Platform namespace
</label>

{% 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 %}
<span class="govuk-hint">Enter namespace with the -env suffix removed</span>
<input type="text" class="govuk-input" id="id_namespace" name="namespace" required />

</div>


{{ govukRadios({
"name": "connect_bucket",
"fieldset": {
Expand All @@ -89,7 +104,7 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
},
},
"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": [
{
Expand All @@ -100,6 +115,9 @@ <h1 class="govuk-heading-xl">{{ page_title }}</h1>
{
"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"
},
{
Expand Down
2 changes: 0 additions & 2 deletions controlpanel/frontend/views/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions controlpanel/frontend/views/apps_mng.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
24 changes: 18 additions & 6 deletions tests/api/cluster/test_app.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
]
}
}
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/api/models/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ def test_slug_characters_replaced():

@pytest.mark.django_db
def test_slug_collisions_increments():
app = App.objects.create(repo_url="[email protected]:org/foo-bar.git")
app = App.objects.create(repo_url="[email protected]: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


Expand Down
5 changes: 3 additions & 2 deletions tests/api/tasks/test_create_app_aws_role.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Standard library
from unittest.mock import patch
from unittest.mock import patch, MagicMock

# Third-party
import pytest
Expand All @@ -25,13 +25,14 @@ 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):
app = mommy.make("api.App")

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()
3 changes: 3 additions & 0 deletions tests/frontend/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,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,
)
Expand All @@ -151,6 +152,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,
)
Expand Down Expand Up @@ -268,6 +270,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,
)
Expand Down
1 change: 1 addition & 0 deletions tests/frontend/views/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Loading