diff --git a/acct_mgt/app.py b/acct_mgt/app.py index f7ec782..f32b52b 100644 --- a/acct_mgt/app.py +++ b/acct_mgt/app.py @@ -2,12 +2,13 @@ import json import os -from flask import Flask, request, Response +from flask import Flask, make_response, request, Response from flask_httpauth import HTTPBasicAuth from . import defaults from . import kubeclient from . import moc_openshift +from . import exceptions ENVPREFIX = "ACCT_MGT_" @@ -61,6 +62,11 @@ def verify_password(username, password): and password == APP.config["ADMIN_PASSWORD"] ) + @APP.errorhandler(exceptions.ApiException) + def exception_handler(error): + msg = error.message if error.visible else "Internal Server Error" + return make_response({"msg": msg}, error.status_code) + @APP.route( "/users//projects//roles/", methods=["GET"] ) @@ -142,45 +148,23 @@ def get_moc_project(project_uuid): def create_moc_project(project_uuid, user_name=None): # first check the project_name is a valid openshift project name suggested_project_name = shift.cnvt_project_name(project_uuid) - if project_uuid != suggested_project_name: - # future work, handel colisons by suggesting a different valid - # project name - return Response( - response=json.dumps( - { - "msg": "ERROR: project name must match regex '[a-z0-9]([-a-z0-9]*[a-z0-9])?'", - "suggested name": suggested_project_name, - } - ), - status=400, - mimetype="application/json", - ) - if not shift.project_exists(project_uuid): - payload = request.get_json(silent=True) or {} - project_name = payload.pop("displayName", project_uuid) - annotations = payload.pop("annotations", {}) - result = shift.create_project( - project_uuid, project_name, user_name, annotations=annotations - ) - if result.status_code in (200, 201): - return Response( - response=json.dumps({"msg": f"project created ({project_uuid})"}), - status=200, - mimetype="application/json", - ) - return Response( - response=json.dumps( - {"msg": f"unable to create project ({project_uuid})"} - ), - status=400, - mimetype="application/json", + raise exceptions.BadRequest( + "project name must match regex '[a-z0-9]([-a-z0-9]*[a-z0-9])?'." + f" Suggested name: {suggested_project_name}." ) - return Response( - response=json.dumps({"msg": f"project already exists ({project_uuid})"}), - status=400, - mimetype="application/json", + + if shift.project_exists(project_uuid): + raise exceptions.Conflict("project already exists.") + + payload = request.get_json(silent=True) or {} + project_name = payload.pop("displayName", project_uuid) + annotations = payload.pop("annotations", {}) + + shift.create_project( + project_uuid, project_name, user_name, annotations=annotations ) + return {"msg": f"project created ({project_uuid})"} @APP.route("/projects/", methods=["DELETE"]) @AUTH.login_required diff --git a/acct_mgt/defaults.py b/acct_mgt/defaults.py index 3f6930a..07e6371 100644 --- a/acct_mgt/defaults.py +++ b/acct_mgt/defaults.py @@ -2,3 +2,4 @@ ADMIN_USERNAME = "admin" QUOTA_DEF_FILE = "quotas.json" +LIMIT_DEF_FILE = "limits.json" diff --git a/acct_mgt/exceptions.py b/acct_mgt/exceptions.py new file mode 100644 index 0000000..923b516 --- /dev/null +++ b/acct_mgt/exceptions.py @@ -0,0 +1,32 @@ +"""Defined exceptions used by acct-mgt.""" + + +class ApiException(Exception): + """Base exception class for errors. + + All exceptions subclassing ApiException will be caught from Flask's + error handler and return the appropriate status code and message. + The visible parameter controls whether the error message is visible + to the end user. + """ + + status_code = 500 + visible = True + default_message = "Internal Server Error." + + def __init__(self, message=None): + self.message = message or self.default_message + + +class BadRequest(ApiException): + """Exception class for invalid requests.""" + + status_code = 400 + default_message = "Invalid Request." + + +class Conflict(BadRequest): + """Exception class for requests that create already existing resources.""" + + status_code = 409 + default_message = "Resource already exists." diff --git a/acct_mgt/moc_openshift.py b/acct_mgt/moc_openshift.py index 3c3878d..370c991 100644 --- a/acct_mgt/moc_openshift.py +++ b/acct_mgt/moc_openshift.py @@ -4,10 +4,13 @@ import pprint import json import re +import sys import time from flask import Response +from . import exceptions + OPENSHIFT_ROLES = ["admin", "edit", "view"] @@ -54,6 +57,11 @@ def __init__(self, client, app): self.logger = app.logger self.id_provider = app.config["IDENTITY_PROVIDER"] self.quotafile = app.config["QUOTA_DEF_FILE"] + self.limitfile = app.config["LIMIT_DEF_FILE"] + + if not self.limitfile: + self.logger.error("No default limit file provided.") + sys.exit(1) @staticmethod def cnvt_project_name(project_name): @@ -328,6 +336,10 @@ def get_quota_definitions(self): return quota + def get_limit_definitions(self): + with open(self.limitfile, "r") as file: + return json.load(file) + @abc.abstractmethod def get_resourcequota_details(self, project_name) -> dict: pass @@ -362,7 +374,11 @@ def create_project(self, project_name, display_name, user_name, annotations=None "apiVersion": "project.openshift.io/v1", "metadata": {"name": project_name, "annotations": annotations}, } - return self.client.post(url, json=payload) + r = self.client.post(url, json=payload) + if r.status_code not in [200, 201]: + raise exceptions.ApiException(f"unable to create project ({project_name})") + self.create_limits(project_name) + return r def delete_project(self, project_name): # check project_name @@ -623,3 +639,23 @@ def get_resourcequota_details(self, project_name) -> dict: for rq_info in rq_data["items"]: rq_dict[rq_info["metadata"]["name"]] = rq_info["spec"] return rq_dict + + def create_limits(self, namespace, limits=None): + """ + namespace: namespace to create LimitRange + limits: dictionary of limits to create, or None for default + """ + url = f"/api/v1/namespaces/{namespace}/limitranges" + + payload = { + "apiVersion": "v1", + "kind": "LimitRange", + "metadata": {"name": f"{namespace.lower()}-limits"}, + "spec": {"limits": limits or self.get_limit_definitions()}, + } + r = self.client.post(url, json=payload) + if r.status_code not in [200, 201]: + raise exceptions.ApiException( + f"Unable to create default limits on project {namespace}." + ) + return r diff --git a/k8s/base/deployment.yaml b/k8s/base/deployment.yaml index 91b650b..a76308d 100644 --- a/k8s/base/deployment.yaml +++ b/k8s/base/deployment.yaml @@ -14,7 +14,9 @@ spec: - name: OPENSHIFT_URL value: https://kubernetes.default.svc - name: ACCT_MGT_QUOTA_DEF_FILE - value: /app/quota/quotas.json + value: /app/config/quotas.json + - name: ACCT_MGT_LIMIT_DEF_FILE + value: /app/config/limits.json envFrom: - secretRef: name: onboarding-credentials @@ -27,12 +29,12 @@ spec: containerPort: 8080 protocol: TCP volumeMounts: - - name: quota-vol - mountPath: /app/quota + - name: config-vol + mountPath: /app/config readOnly: true volumes: - - name: quota-vol + - name: config-vol configMap: - name: openshift-quota-definition + name: config-files serviceAccountName: onboarding-serviceaccount automountServiceAccountToken: true diff --git a/k8s/base/kustomization.yaml b/k8s/base/kustomization.yaml index 223fb9f..336b05d 100644 --- a/k8s/base/kustomization.yaml +++ b/k8s/base/kustomization.yaml @@ -15,6 +15,7 @@ resources: - service.yaml configMapGenerator: - - name: openshift-quota-definition + - name: config-files files: + - limits.json - quotas.json diff --git a/k8s/base/limits.json b/k8s/base/limits.json new file mode 100644 index 0000000..2e80f6c --- /dev/null +++ b/k8s/base/limits.json @@ -0,0 +1,13 @@ +[ + { + "type": "Container", + "default": { + "cpu": "200m", + "memory": "512Mi" + }, + "defaultRequest": { + "cpu": "100m", + "memory": "256Mi" + } + } +] diff --git a/tests/functional/test_project.py b/tests/functional/test_project.py index ca03d26..3214e76 100644 --- a/tests/functional/test_project.py +++ b/tests/functional/test_project.py @@ -70,16 +70,6 @@ def test_create_project_no_owner(session, suffix): session.delete(f"/projects/test-project-{suffix}") -# LKS: Cannot differentiate between "conflict with existing project" and -# "operation failed for some other reason" -def test_create_project_exists(session, a_project): - """Test that we cannot create a project with a conflicting name""" - - res = session.put(f"/projects/{a_project}/owner/test-owner") - assert res.status_code == 400 - - -@pytest.mark.xfail(reason="not supported by service") def test_create_project_exists_409(session, a_project): """Test that creating a project with a conflicting name results in a 409 CONFLICT error.""" diff --git a/tests/unit/test_app_project.py b/tests/unit/test_app_project.py index b90e4cd8..6d73b19 100644 --- a/tests/unit/test_app_project.py +++ b/tests/unit/test_app_project.py @@ -1,6 +1,7 @@ # pylint: disable=missing-module-docstring import json +from acct_mgt.exceptions import ApiException from .conftest import fake_200_response, fake_400_response @@ -23,7 +24,7 @@ def test_create_moc_project_bad_name(moc, client): res = client.put("/projects/Test%20Project") assert res.status_code == 400 assert "project name must match regex" in res.json["msg"] - assert res.json["suggested name"] == "test-project" + assert "test-project" in res.json["msg"] def test_create_moc_project_no_display_name(moc, client): @@ -72,17 +73,17 @@ def test_create_moc_project_exists(moc, client): moc.cnvt_project_name.return_value = "test-project" moc.project_exists.return_value = True res = client.put("/projects/test-project") - assert res.status_code == 400 + assert res.status_code == 409 assert "project already exists" in res.json["msg"] def test_create_moc_project_fails(moc, client): moc.cnvt_project_name.return_value = "test-project" moc.project_exists.return_value = False - moc.create_project.return_value = fake_400_response + moc.create_project.side_effect = ApiException("Error") res = client.put("/projects/test-project") - assert res.status_code == 400 - assert "unable to create project" in res.json["msg"] + assert res.status_code == 500 + assert "Error" in res.json["msg"] def test_delete_moc_project_exists(moc, client):