Merge pull request #83 from MITLibraries/IN-1052-add-guardrails-final-runs

In 1052 add guardrails final runs

Author: jonavellecuerdo

README.md

@@ -27,6 +27,8 @@ A Flask application run in AWS Lambda used to process invoices from Alma. Invoic
 ALMA_SAP_INVOICES_ECR_IMAGE_NAME=### The name of the ECR image for the Alma SAP Invoices CLI.
 ALMA_SAP_INVOICES_ECS_CLUSTER=### The full ARN of the ECS cluster to run the Alma SAP Invoices CLI ECS task.
 ALMA_SAP_INVOICES_ECS_TASK_DEFINITION=### The family and revision (formatted as <family>:<revision>) or full ARN of Alma SAP Invoices CLI ECS task. 
+ALMA_SAP_INVOICES_ECS_GROUPS=### Security group(s) for the Alma SAP Invoices ECS task (formatted as a comma-separated string excluding whitespaces, e.g. "sg-abc123,sg-def456").
+ALMA_SAP_INVOICES_ECS_SUBNETS=### Subnet(s) for the Alma SAP Invoices ECS task (formatted as a comma-separated string excluding whitespaces, e.g. "subnet-abc123,subnet-def456").
 ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG=### The network configuration for the Alma SAP Invoices ECS task.
 ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP=### The name of the log group containing logs from Alma SAP Invoices ECS task runs.
 LOGIN_DISABLED=### String variable representing a boolean to disable login (i.e., ignore 'login_required' decorator). Set to 'true' to disable login (recommended for unit testing and Dev); must be set to 'false' for Stage and Prod.

pyproject.toml

@@ -34,6 +34,7 @@ ignore = [
     "ANN102",
     "COM812",
     "D107",
+    "G004",
     "N812",
     "PTH",
 

tests/conftest.py

@@ -26,14 +26,12 @@ def _test_env(monkeypatch):
         "ALMA_SAP_INVOICES_ECS_CLUSTER",
         f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:cluster/mock-sapinvoices-ecs-test",
     )
+    monkeypatch.setenv("ALMA_SAP_INVOICES_ECS_GROUPS", "sg-abc123")
+    monkeypatch.setenv("ALMA_SAP_INVOICES_ECS_SUBNETS", "subnet-abc123,subnet-def456")
     monkeypatch.setenv(
         "ALMA_SAP_INVOICES_ECS_TASK_DEFINITION",
         f"arn:aws:ecs:us-east-1:{ACCOUNT_ID}:task-definition/mock-sapinvoices-ecs-test:1",
     )
-    monkeypatch.setenv(
-        "ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG",
-        '{"awsvpcConfiguration": {"securityGroups": ["sg-abc123"], "subnets": ["subnet-abc123"]}}',
-    )
     monkeypatch.setenv(
         "ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP", "mock-sapinvoices-ecs-test"
     )

tests/test_app.py

@@ -77,7 +77,7 @@ def test_app_log_activity_executed_runs_success(
     ) as mock_ecsclient_review_run:
         mock_ecsclient_review_run.return_value = "abc123"
         sapinvoices_client.get(
-            "/process-invoices/run/review", headers=mock_request_headers_oidc_data
+            "/process-invoices/run/review/execute", headers=mock_request_headers_oidc_data
         )
         assert (
             "Authenticated User executed a 'review' run (task ID = 'abc123')."

tests/test_aws/test_cloudwatch.py

@@ -42,29 +42,3 @@ def test_cloudwatchlogs_client_get_log_events_raise_error(
         match="No log streams found for task id 'DOES_NOT_EXIST'.",
     ):
         assert cloudwatchlogs_client.get_log_events(task_id="DOES_NOT_EXIST")
-
-
-def test_cloudwatchlogs_client_log_stream_exists_returns_true(
-    cloudwatchlogs_client,
-    mock_cloudwatchlogs_log_stream_review_run_task,
-):
-    # Note: Either 'review run' or 'final run' (task_id="abc002") mocked log stream
-    #       fixture will work for this test.
-    assert cloudwatchlogs_client.log_stream_exists(task_id="abc001") is True
-
-
-def test_cloudwatchlogs_client_log_stream_exists_returns_false(
-    cloudwatchlogs_client, mock_cloudwatchlogs_log_group
-):
-    assert cloudwatchlogs_client.log_stream_exists(task_id="DOES_NOT_EXIST") is False
-
-
-def test_cloudwatchlogs_client_get_log_streams_success(
-    cloudwatchlogs_client,
-    mock_cloudwatchlogs_log_stream_review_run_task,
-    mock_cloudwatchlogs_log_stream_final_run_task,
-):
-    assert cloudwatchlogs_client.get_log_streams() == [
-        "sapinvoices/mock-sapinvoices-ecs-test/abc001",
-        "sapinvoices/mock-sapinvoices-ecs-test/abc002",
-    ]

tests/test_aws/test_ecs.py

@@ -1,5 +1,4 @@
 # ruff: noqa: E501
-import json
 import os
 
 import pytest
@@ -17,9 +16,13 @@ def test_ecs_client_init_success(ecs_client):
     assert (
         ecs_client.task_definition == os.environ["ALMA_SAP_INVOICES_ECS_TASK_DEFINITION"]
     )
-    assert ecs_client.network_configuration == json.loads(
-        os.environ["ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG"]
-    )
+    assert ecs_client.network_configuration == {
+        "awsvpcConfiguration": {
+            "subnets": ["subnet-abc123", "subnet-def456"],
+            "securityGroups": ["sg-abc123"],
+            "assignPublicIp": "DISABLED",
+        }
+    }
     assert ecs_client.container == os.environ["ALMA_SAP_INVOICES_ECR_IMAGE_NAME"]
 
 

tests/test_config.py

@@ -2,7 +2,9 @@
 
 import pytest
 
-from webapp.exceptions import InvalidNetworkConfigurationError
+
+def test_config_check_required_env_vars_success(config):
+    config.check_required_env_vars()
 
 
 def test_config_check_required_env_vars_error(monkeypatch, config):
@@ -50,16 +52,11 @@ def test_config_env_access_error(config):
         _ = config.DOES_NOT_EXIST
 
 
-def test_config_env_access_raise_error_if_network_config_not_json(monkeypatch, config):
-    monkeypatch.setenv("ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG", "test")
-    with pytest.raises(InvalidNetworkConfigurationError, match="Expecting value"):
-        _ = config.ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG
-
-
-def test_config_env_access_raise_error_if_network_config_is_missing(monkeypatch, config):
-    monkeypatch.delenv("ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG")
-    with pytest.raises(
-        InvalidNetworkConfigurationError,
-        match="the JSON object must be str, bytes or bytearray, not NoneType",
-    ):
-        _ = config.ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG
+def test_config_env_access_network_config_success(monkeypatch, config):
+    assert config.ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG == {
+        "awsvpcConfiguration": {
+            "subnets": ["subnet-abc123", "subnet-def456"],
+            "securityGroups": ["sg-abc123"],
+            "assignPublicIp": "DISABLED",
+        }
+    }

webapp/app.py

@@ -1,14 +1,17 @@
 from __future__ import annotations
 
 import logging
+import time
 from typing import TYPE_CHECKING
 
 from flask import (
     Flask,
     Request,
+    abort,
     jsonify,
     redirect,
     render_template,
+    request,
     session,
     url_for,
 )
@@ -123,16 +126,40 @@ def process_invoices() -> str:
     @app.route("/process-invoices/run/<run_type>")
     @login_required
     def process_invoices_run(run_type: str) -> str | Response:
+        if run_type == "review":
+            return redirect(url_for("process_invoices_run_execute", run_type=run_type))
+        elif run_type == "final":  # noqa: RET505
+            return redirect(url_for("process_invoices_confirm_final_run"))
+        return abort(400, description=f"Invalid run type: '{run_type}'")
+
+    @app.route("/process-invoices/run/final/confirm")
+    @login_required
+    def process_invoices_confirm_final_run() -> str:
+        return render_template("process_invoices_confirm_final_run.html")
+
+    @app.route("/process-invoices/run/<run_type>/execute")
+    @login_required
+    def process_invoices_run_execute(run_type: str) -> str | Response:
         ecs_client = ECSClient()
         if active_tasks := ecs_client.get_active_tasks():
             return render_template(
                 "errors/error_400_cannot_run_multiple_tasks.html",
                 active_tasks=active_tasks,
             )
+
         if run_type == "review":
             task_arn = ecs_client.execute_review_run()
         elif run_type == "final":
-            task_arn = ecs_client.execute_final_run()
+            if request.referrer and request.referrer.endswith(
+                "process-invoices/run/final/confirm"
+            ):
+                task_arn = ecs_client.execute_final_run()
+            else:
+                return abort(
+                    400, description="Unable to confirm execution of 'final' run."
+                )
+        else:
+            return abort(400, description=f"Invalid run type: '{run_type}'")
         task_id = task_arn.split("/")[-1]  # type: ignore[union-attr]
         log_activity(f"executed a '{run_type}' run (task ID = '{task_id}').")
         return redirect(url_for("process_invoices_status", task_id=task_id))
@@ -157,12 +184,13 @@ def process_invoices_status(task_id: str) -> str:
     @app.route("/process-invoices/status/<task_id>/data")
     @login_required
     def process_invoices_status_data(task_id: str) -> Response:
+        t_0 = time.time()
         try:
             task_status, logs = get_task_status_and_logs(task_id)
         except ECSTaskLogStreamDoesNotExistError:
             task_status = "UNKNOWN"
             logs = ["Log stream does not exist."]
-
+        logger.info(f"Data route elapsed: {time.time()-t_0}")
         return jsonify({"status": task_status, "logs": logs})
 
     @app.route("/logout")

webapp/config.py

@@ -1,29 +1,27 @@
 # ruff: noqa:ANN401,  N802
 
-import json
 import logging
 import os
-from json import JSONDecodeError
 from typing import Any
 
 import sentry_sdk
 from sentry_sdk.integrations.aws_lambda import AwsLambdaIntegration
 
-from webapp.exceptions import InvalidNetworkConfigurationError
-
 logger = logging.getLogger("__name__")
 
 
 class Config:
     REQUIRED_ENV_VARS = (
+        "ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP",
         "ALMA_SAP_INVOICES_ECR_IMAGE_NAME",
         "ALMA_SAP_INVOICES_ECS_CLUSTER",
+        "ALMA_SAP_INVOICES_ECS_GROUPS",
+        "ALMA_SAP_INVOICES_ECS_SUBNETS",
         "ALMA_SAP_INVOICES_ECS_TASK_DEFINITION",
-        "ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG",
-        "ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP",
         "LOGIN_DISABLED",
         "SECRET_KEY",
         "WORKSPACE",
+        "SENTRY_DSN",
     )
     OPTIONAL_ENV_VARS = ("AWS_DEFAULT_REGION",)
 
@@ -34,62 +32,29 @@ def __getattr__(self, name: str) -> Any:
         message = f"'{name}' not a valid configuration variable"
         raise AttributeError(message)
 
-    def _getenv(self, name: str) -> Any:
-        if value := os.getenv(name):
-            return value
-        if name in self.REQUIRED_ENV_VARS:
-            message = f"'{name}' is a required environment variable."
-            raise AttributeError(message)
-        return None
-
     @property
     def ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG(self) -> dict:
-        network_config = os.getenv("ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG")
-        try:
-            return json.loads(network_config)  # type: ignore[arg-type]
-        except (TypeError, JSONDecodeError) as error:
-            raise InvalidNetworkConfigurationError(error) from error
-
-    @property
-    def ALMA_SAP_INVOICES_ECR_IMAGE_NAME(self) -> str:
-        return self._getenv("ALMA_SAP_INVOICES_ECR_IMAGE_NAME")
-
-    @property
-    def ALMA_SAP_INVOICES_ECS_CLUSTER(self) -> str:
-        return self._getenv("ALMA_SAP_INVOICES_ECS_CLUSTER")
-
-    @property
-    def ALMA_SAP_INVOICES_ECS_TASK_DEFINITION(self) -> str:
-        return self._getenv("ALMA_SAP_INVOICES_ECS_TASK_DEFINITION")
-
-    @property
-    def ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP(self) -> str:
-        return self._getenv("ALMA_SAP_INVOICES_CLOUDWATCH_LOG_GROUP")
+        security_groups = self.ALMA_SAP_INVOICES_ECS_GROUPS
+        subnets = self.ALMA_SAP_INVOICES_ECS_SUBNETS
+        return {
+            "awsvpcConfiguration": {
+                "subnets": subnets.split(","),
+                "securityGroups": security_groups.split(","),
+                "assignPublicIp": "DISABLED",
+            }
+        }
 
     @property
     def AWS_DEFAULT_REGION(self) -> str:
-        if region_name := self._getenv("AWS_DEFAULT_REGION"):
-            return region_name
-        return "us-east-1"
+        return os.getenv("AWS_DEFAULT_REGION", "us-east-1")
 
     @property
     def LOGIN_DISABLED(self) -> bool:
-        if self._getenv("LOGIN_DISABLED").lower() == "true":  # noqa: SIM103
-            return True
+        if login_disabled := os.getenv("LOGIN_DISABLED"):  # noqa: SIM102
+            if login_disabled.lower() == "true":
+                return True
         return False
 
-    @property
-    def SECRET_KEY(self) -> str:
-        return self._getenv("SECRET_KEY")
-
-    @property
-    def SENTRY_DSN(self) -> str:
-        return self._getenv("SENTRY_DSN")
-
-    @property
-    def WORKSPACE(self) -> str:
-        return self._getenv("WORKSPACE")
-
     def check_required_env_vars(self) -> None:
         """Method to raise exception if required env vars not set."""
         missing_vars = [var for var in self.REQUIRED_ENV_VARS if not os.getenv(var)]

webapp/exceptions.py

@@ -1,7 +1,3 @@
-class InvalidNetworkConfigurationError(Exception):
-    """Exception to raise when ALMA_SAP_INVOICES_ECS_NETWORK_CONFIG is invalid."""
-
-
 class ECSTaskLogStreamDoesNotExistError(Exception):
     """Exception to raise when a log stream is not found."""