From 7c3730152ff736cff955e7dd4ac21db46e20fb7a Mon Sep 17 00:00:00 2001 From: Kristi Nikolla Date: Thu, 26 Jan 2023 10:19:46 -0500 Subject: [PATCH 1/2] Create base exceptions and use them This commit creates the ApiException class and registers an error handler with Flask. Moving forward this is the preferred mechanism for returning error codes to the end user. --- acct_mgt/app.py | 58 ++++++++++++-------------------- acct_mgt/exceptions.py | 32 ++++++++++++++++++ acct_mgt/moc_openshift.py | 7 +++- tests/functional/test_project.py | 10 ------ tests/unit/test_app_project.py | 11 +++--- 5 files changed, 65 insertions(+), 53 deletions(-) create mode 100644 acct_mgt/exceptions.py diff --git a/acct_mgt/app.py b/acct_mgt/app.py index b1b98d0..8c843e5 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/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..001a6d5 100644 --- a/acct_mgt/moc_openshift.py +++ b/acct_mgt/moc_openshift.py @@ -8,6 +8,8 @@ from flask import Response +from . import exceptions + OPENSHIFT_ROLES = ["admin", "edit", "view"] @@ -362,7 +364,10 @@ 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})") + return r def delete_project(self, project_name): # check project_name 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): From d62b27052fc4582d431a3be620814963ae44306e Mon Sep 17 00:00:00 2001 From: Kristi Nikolla Date: Thu, 26 Jan 2023 10:32:23 -0500 Subject: [PATCH 2/2] Create default limits Now when a project is created, default limits are applied. These default limits are loaded from the limits.json file through the config-files configmap into /app/config. - Renames the openshift-quota-definition configmap to config-files. - Changes the default directory for quotas.json to /app/config. --- acct_mgt/defaults.py | 1 + acct_mgt/moc_openshift.py | 31 +++++++++++++++++++++++++++++++ k8s/base/deployment.yaml | 12 +++++++----- k8s/base/kustomization.yaml | 3 ++- k8s/base/limits.json | 13 +++++++++++++ 5 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 k8s/base/limits.json 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/moc_openshift.py b/acct_mgt/moc_openshift.py index 001a6d5..370c991 100644 --- a/acct_mgt/moc_openshift.py +++ b/acct_mgt/moc_openshift.py @@ -4,6 +4,7 @@ import pprint import json import re +import sys import time from flask import Response @@ -56,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): @@ -330,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 @@ -367,6 +377,7 @@ def create_project(self, project_name, display_name, user_name, annotations=None 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): @@ -628,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" + } + } +]