From 0e0dcfa7f24a1fac89d5add925a5a055a283bd95 Mon Sep 17 00:00:00 2001 From: Paul Lizer Date: Wed, 19 Aug 2026 23:01:09 -0400 Subject: [PATCH] Harden the broker API and add server-side pagination Server-side counterpart to the portal modernization. Fixes bugs found while reviewing api/app.py against the stored procedures, and adds the two endpoints the new UI needed. Bugs fixed: - The "No Limit" option never worked. The portal sends limit as the string "null"; the procedures declare @Limit INT, so SQL Server failed converting it and all three history endpoints returned 500. Limits are now coerced, and the portal no longer sends the sentinel. - Database connections leaked on every exception path: 27 get_db_connection() calls had only 7 finally blocks. All handlers now use a db_connection() context manager that always closes. - 18 handlers returned the raw str(e) to the caller, exposing driver errors, server names and schema detail. Failures now return {"error": ...} and the detail goes to logger.exception. - 14 print() calls became logger calls, including inside get_db_connection() and Key Vault retrieval, so database and secret failures actually reach Application Insights instead of being swallowed. - /api/scaling/rules returned 404 when no rules existed, which made the portal flash an error rather than render its empty state. /api/scaling/log and /api/scaling/rules/history returned a dict when empty and a list otherwise. All three now return a JSON array with 200. - TriggerScalingLogic ran without committing. pymssql does not autocommit, so the power-state updates and the activity-log insert were rolled back while the Azure power operations still went ahead, leaving Azure and the broker out of step and the scaling activity log permanently empty. - is_member_of_group_cached was defined but never called; token_required used the uncached path, so every authenticated request from the AVD and Linux hosts hit Microsoft Graph. Now wired up, and a Graph failure raises rather than returning False so a throttled call is never cached as a denial. - The scaling procedures relied on implicit MM/DD/YYYY date conversion, which depends on the session DATEFORMAT. They now convert explicitly with style 101, matching GetVmHistory, via TRY_CONVERT. Added: - GET /api/vms/summary, so the dashboard no longer fetches every VM row to compute eight counters. - Opt-in page/per_page pagination on the three history endpoints, backed by new paged procedures that return TotalCount via COUNT(*) OVER (). With neither parameter the response stays a bare array, because the scheduled task and older portal builds consume these as plain lists. - api/tests/ (44 tests) with pymssql and the Azure SDKs mocked, plus CI wiring. Verified by mutation testing: each fix was reverted in turn and the suite failed every time. - api/README.md covering the endpoint surface, auth model, consumer map, error envelope and pagination contract. Front end: - The dashboard uses the summary endpoint, and the history pages use server-side pagination, so whole result sets are no longer cached in the Flask session. That caching grew without bound and let two browser tabs clobber each other. - Both have fallbacks for an API deployed behind the portal. Removed: the unused pyodbc dependency and the /api/vms/available endpoint, which had no callers in the repo. External callers of that endpoint, if any, would need checking before deploying. Not addressed: the 54 Dependabot alerts on the default branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/front-end-tests.yml | 45 +- api/README.md | 166 +++ api/app.py | 1011 ++++++++++------- api/env.example | 4 + api/requirements-dev.txt | 2 + api/requirements.txt | 1 - api/tests/conftest.py | 238 ++++ api/tests/test_api_regressions.py | 340 ++++++ front_end/README.md | 8 +- front_end/app.py | 6 +- front_end/function_api.py | 172 ++- front_end/route_scaling_management.py | 352 +++--- front_end/route_vm_management.py | 166 +-- front_end/tests/conftest.py | 69 +- front_end/tests/test_ui_regressions.py | 108 +- .../034_create_procedure-GetVmSummary.sql | 21 + ..._alter_procedure-GetScalingActivityLog.sql | 32 + ...ter_procedure-GetVmScalingRulesHistory.sql | 34 + ...037_create_procedure-GetVmHistoryPaged.sql | 66 ++ ...e_procedure-GetScalingActivityLogPaged.sql | 54 + ...rocedure-GetVmScalingRulesHistoryPaged.sql | 54 + sql_queries/README.md | 14 +- 22 files changed, 2144 insertions(+), 819 deletions(-) create mode 100644 api/README.md create mode 100644 api/requirements-dev.txt create mode 100644 api/tests/conftest.py create mode 100644 api/tests/test_api_regressions.py create mode 100644 sql_queries/034_create_procedure-GetVmSummary.sql create mode 100644 sql_queries/035_alter_procedure-GetScalingActivityLog.sql create mode 100644 sql_queries/036_alter_procedure-GetVmScalingRulesHistory.sql create mode 100644 sql_queries/037_create_procedure-GetVmHistoryPaged.sql create mode 100644 sql_queries/038_create_procedure-GetScalingActivityLogPaged.sql create mode 100644 sql_queries/039_create_procedure-GetVmScalingRulesHistoryPaged.sql diff --git a/.github/workflows/front-end-tests.yml b/.github/workflows/front-end-tests.yml index 953ab13..522f0ee 100644 --- a/.github/workflows/front-end-tests.yml +++ b/.github/workflows/front-end-tests.yml @@ -1,17 +1,19 @@ -name: Front end tests +name: App tests on: pull_request: paths: - 'front_end/**' + - 'api/**' - '.github/workflows/front-end-tests.yml' push: paths: - 'front_end/**' + - 'api/**' - '.github/workflows/front-end-tests.yml' jobs: - test: + front-end-test: runs-on: ubuntu-latest permissions: contents: read @@ -40,3 +42,42 @@ jobs: API_URL: https://api.example.invalid MICROSOFT_PROVIDER_AUTHENTICATION_SECRET: secret run: pytest + + api-test: + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - uses: actions/checkout@v4 + + - name: Set up Python version + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Install dependencies + working-directory: api + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt -r requirements-dev.txt + + - name: Run pytest + working-directory: api + env: + TENANT_ID: tenant-id + CLIENT_ID: client-id + VM_SUBSCRIPTION_ID: subscription-id + VM_RESOURCE_GROUP: resource-group + AVD_HOST_GROUP_ID: avd-group-id + LINUX_HOST_GROUP_ID: linux-group-id + DOMAIN_NAME: example.invalid + VAULT_URL: https://vault.example.invalid + KEY_NAME: ssh-key + DB_SERVER: db.example.invalid + DB_DATABASE: LinuxBrokerTest + DB_USERNAME: api-user + DB_PASSWORD_NAME: db-password + MICROSOFT_PROVIDER_AUTHENTICATION_SECRET: provider-secret + NFS_SHARE: /mnt/test + run: pytest diff --git a/api/README.md b/api/README.md new file mode 100644 index 0000000..4130426 --- /dev/null +++ b/api/README.md @@ -0,0 +1,166 @@ +# Broker API + +This folder contains the Flask **Broker API** for the Linux Broker for AVD Access solution. It is the control-plane service used by the Service Management Portal, the scheduled scaling task, the AVD host broker, and the Linux host agents. For the full solution architecture and deployment model, see the repository [README](../README.md). + +## Purpose + +The API brokers Linux host checkouts, records VM state in Azure SQL, manages scaling rules, triggers scaling actions, and delivers the fleet-wide Linux host settings profile. It does not own the database schema; schema and stored procedure changes belong under the [`sql_queries`](../sql_queries/README.md) folder. + +## Endpoint Reference + +`token_required(...)` grants access when the bearer token has any listed delegated scope or app role. When a group is listed, membership in that configured group also grants access. + +| Method | Path | Required scopes, roles, or groups | Description | +| --- | --- | --- | --- | +| GET | `/health` | none | Checks database connectivity and returns API health and version. | +| GET | `/api/version` | none | Returns the API version string. | +| GET | `/api/vms` | `access_as_user`, `FullAccess`, `ScheduledTask` | Lists all broker VM records. | +| GET | `/api/vms/summary` | `access_as_user`, `FullAccess`, `ScheduledTask` | Returns dashboard counters: `TotalVMs`, `Available`, `CheckedOut`, `Maintenance`, `Released`, `PoweredOn`, `PoweredOff`, `Unreachable`, and `Ready`. | +| POST | `/api/vms/checkout` | `AvdHost`, `access_as_user`, `FullAccess`, or `AVD_HOST_GROUP_ID` membership | Checks out a ready Linux host and creates or updates the remote user. | +| POST | `/api/vms//update-attributes` | `ScheduledTask`, `access_as_user`, `FullAccess` | Updates VM power, network, or broker status fields. | +| POST | `/api/vms//delete` | `access_as_user`, `FullAccess` | Deletes a VM record. | +| POST | `/api/vms/add` | `access_as_user`, `FullAccess` | Adds a VM record. | +| GET | `/api/vms/` | `access_as_user`, `FullAccess` | Gets one VM record. | +| POST | `/api/vms//return` | `access_as_user`, `FullAccess` | Returns a checked-out VM and removes the remote user when possible. | +| POST | `/api/vms//release` | `LinuxHost`, `access_as_user`, `FullAccess`, or `LINUX_HOST_GROUP_ID` membership | Marks a host-side session released, with optional `username` and `leaseId` validation. | +| POST | `/api/vms/released` | `ScheduledTask`, `access_as_user`, `FullAccess` | Returns expired released VMs to the available pool and removes remote users. | +| POST | `/api/vms/history` | `access_as_user`, `FullAccess` | Returns VM history, optionally paged with `page` and `per_page`. | +| POST | `/api/scaling/log` | `access_as_user`, `FullAccess` | Returns scaling activity history, optionally paged with `page` and `per_page`. | +| POST | `/api/scaling/trigger` | `ScheduledTask`, `access_as_user`, `FullAccess` | Runs scaling logic and starts or stops Azure VMs as directed by SQL. | +| GET | `/api/scaling/rules` | `access_as_user`, `FullAccess` | Lists scaling rules; an empty rule set is `[]` with `200`. | +| GET | `/api/scaling/rules/` | `access_as_user`, `FullAccess` | Gets one scaling rule. | +| POST | `/api/scaling/rules/create` | `access_as_user`, `FullAccess` | Creates a scaling rule. | +| POST | `/api/scaling/rules//update` | `access_as_user`, `FullAccess` | Updates a scaling rule. | +| POST | `/api/scaling/rules//delete` | `access_as_user`, `FullAccess` | Deletes a scaling rule. | +| POST | `/api/scaling/rules/history` | `access_as_user`, `FullAccess` | Returns scaling rule history, optionally paged with `page` and `per_page`. | +| GET | `/api/hosts/settings` | `LinuxHost`, `access_as_user`, `FullAccess`, `ScheduledTask`, or `LINUX_HOST_GROUP_ID` membership | Returns the fleet-wide Linux host settings profile. | +| POST | `/api/hosts/settings/update` | `access_as_user`, `FullAccess` | Updates the fleet-wide Linux host settings profile. | +| POST | `/api/hosts/settings/apply` | `access_as_user`, `FullAccess`, `ScheduledTask` | Pushes the current settings profile to reachable hosts over SSH. | +| POST | `/api/hosts//settings/ack` | `LinuxHost`, `access_as_user`, `FullAccess`, or `LINUX_HOST_GROUP_ID` membership | Records the settings version applied by one host. | + +`/api/vms/available` is not present in `app.py`; do not add new callers for it. + +## Consumers + +These callers constrain response shapes and endpoint compatibility. + +| Consumer | Endpoints | +| --- | --- | +| `front_end` portal | VM, scaling, and host-settings endpoints. The dashboard prefers `/api/vms/summary`; history pages request `page` and `per_page`. | +| `task\function_app.py` | `/api/vms`, `/api/vms/released`, `/api/vms//update-attributes`, `/api/scaling/trigger` | +| Linux host release agent (`linux_host\...\release-session.sh`) | `/api/vms//release` | +| AVD host (`avd_host\...\Connect-LinuxBroker.ps1`) | `/api/vms/checkout` | +| Linux host settings agent | `/api/hosts/settings`, `/api/hosts//settings/ack` | + +## Authentication and Authorization + +Clients send Entra ID bearer tokens in the HTTP `Authorization` header. `token_required()` validates the token signature against the tenant JWKS, accepts audiences `CLIENT_ID` and `api://`, and accepts issuers: + +- `{AUTHORITY_HOST}/{TENANT_ID}/v2.0` +- `{AUTHORITY_HOST}/{TENANT_ID}/` +- `{STS_ISSUER_HOST}/{TENANT_ID}/` + +Authorization then checks delegated scopes in `scp`, app roles in `roles`, and optional group membership through Microsoft Graph `checkMemberGroups` using the token `oid`. + +Cloud endpoints are resolved in [`config.py`](config.py). `AZURE_CLOUD_NAME=AzurePublic` uses `login.microsoftonline.com`, `graph.microsoft.com`, and `sts.windows.net`. `AzureUSGovernment` uses `login.microsoftonline.us` and `graph.microsoft.us`. Any custom or sovereign cloud without a built-in profile must set `AZURE_AUTHORITY_HOST`, `GRAPH_ENDPOINT`, and `STS_ISSUER_HOST` explicitly. + +## Error Responses and Logging + +Handler failures use a JSON error envelope: + +```json +{"error": "Unable to retrieve virtual machines."} +``` + +Exception detail must not be returned in the response body. Log details with the `linuxbroker.api` logger; when `APPLICATIONINSIGHTS_CONNECTION_STRING` is set, that logger is configured for Azure Monitor. Authentication middleware and `/health` have their own fixed response shapes, but application handler errors should use the envelope. + +## Pagination Contract + +`/api/vms/history`, `/api/scaling/log`, and `/api/scaling/rules/history` support opt-in pagination. Supplying either `page` or `per_page` in the query string returns an envelope: + +```http +POST /api/vms/history?page=2&per_page=25 +Content-Type: application/json + +{"startdate":"08/01/2026","enddate":"08/19/2026"} +``` + +```json +{ + "items": [ + {"VMID": 42, "Hostname": "linux-01"} + ], + "page": 2, + "per_page": 25, + "total": 91, + "total_pages": 4 +} +``` + +When neither `page` nor `per_page` is present, the response remains a bare JSON array. Do not remove that default: `task\function_app.py` and older portal builds consume these endpoints as plain lists. The unpaged path also deliberately tolerates `"null"` for `limit`; older portal builds sent that sentinel for **No Limit**, and rolling deployments must not turn it into a SQL `INT` conversion failure. + +Empty collection responses are arrays with `200`, including `/api/scaling/rules`, `/api/scaling/log`, and `/api/scaling/rules/history`. + +## VM Summary + +`GET /api/vms/summary` returns fixed-size dashboard counters instead of requiring the portal to fetch every VM. `Ready` uses the same condition as checkout host selection: `VmStatus='Available'`, `PowerState='On'`, and `NetworkStatus='Reachable'`. + +## Configuration + +The API reads environment variables directly; it does not load `.env` files by itself. [`env.example`](env.example) shows the deployment settings. + +| Variable | Required | Purpose | +| --- | --- | --- | +| `SCM_DO_BUILD_DURING_DEPLOYMENT` | deployment | Enables App Service build during deployment. | +| `APPLICATIONINSIGHTS_CONNECTION_STRING` | optional | Enables Azure Monitor/OpenTelemetry export for `linuxbroker.api`. | +| `ApplicationInsightsAgent_EXTENSION_VERSION` | optional | App Service Application Insights extension version. | +| `APPLICATIONINSIGHTSAGENT_EXTENSION_ENABLED` | optional | Enables the App Service Application Insights extension. | +| `WEBSITE_HTTPLOGGING_RETENTION_DAYS` | optional | App Service HTTP log retention. | +| `VM_SUBSCRIPTION_ID` | required for scaling | Azure subscription used by `/api/scaling/trigger`. | +| `VM_RESOURCE_GROUP` | required for scaling | Resource group containing Linux host VMs. | +| `AVD_HOST_GROUP_ID` | required for AVD host group auth | Entra group whose members may call checkout. | +| `LINUX_HOST_GROUP_ID` | required for Linux host group auth | Entra group whose members may call release and host-settings ack/read endpoints. | +| `LINUX_HOST_ADMIN_LOGIN_NAME` | optional | SSH admin user prefix for remote host commands; defaults to `avdadmin`. | +| `DB_SERVER` | required | Azure SQL Server name or FQDN for `pymssql`. | +| `DB_DATABASE` | required | Azure SQL database name. | +| `DB_USERNAME` | required | SQL login name. | +| `DB_PASSWORD_NAME` | required | Key Vault secret name containing the SQL password. | +| `CLIENT_ID` | required | Broker API app registration client ID and accepted token audience. | +| `TENANT_ID` | required | Entra tenant used for token validation and Graph calls. | +| `AZURE_CLOUD_NAME` | optional | Cloud profile name; defaults to `AzurePublic`. | +| `AZURE_AUTHORITY_HOST` | required for `AzureCustom` | Login authority host override. | +| `GRAPH_ENDPOINT` | required for `AzureCustom` | Microsoft Graph endpoint override. | +| `STS_ISSUER_HOST` | required for `AzureCustom` | STS issuer host override. | +| `GRAPH_API_ENDPOINT` | optional | Legacy Graph scope setting in `config.py`; current token acquisition uses `GRAPH_ENDPOINT`. | +| `MICROSOFT_PROVIDER_AUTHENTICATION_SECRET` | required | Client secret used by the API to call Graph for group checks. | +| `DOMAIN_NAME` | required for SSH actions | DNS suffix used to build `@.`. | +| `VAULT_URL` | required | Key Vault URL for SQL password and SSH key retrieval. | +| `KEY_NAME` | required for SSH actions | Key Vault secret name containing the PEM SSH private key. | +| `NFS_SHARE` | required for checkout provisioning | NFS share argument passed to `create-user.sh`; used by code but not currently listed in `env.example`. | + +## Database Access + +Handlers call stored procedures rather than embedding schema logic in Python. `db_connection()` wraps `get_db_connection()` as a context manager so every acquired connection is closed on success or exception. + +Keep schema and procedure changes in numbered files under [`sql_queries`](../sql_queries/README.md). The deployment bootstrap applies those scripts in filename order and rewrites procedures to `CREATE OR ALTER PROCEDURE` for reruns. + +## Local Development and Tests + +Install runtime dependencies from this folder: + +```powershell +cd .\api +py -m venv .venv +.\.venv\Scripts\Activate.ps1 +pip install -r requirements.txt +python .\app.py +``` + +Set the required environment variables first. For local test runs, install the dev requirements and run pytest from the `api` folder: + +```powershell +pip install -r requirements.txt -r requirements-dev.txt +pytest +``` + +`api\tests\` contains pytest regression coverage for the hardened API paths, including connection cleanup, error envelopes, empty collections, VM summary, and paged history responses. diff --git a/api/app.py b/api/app.py index 707d362..81ad034 100644 --- a/api/app.py +++ b/api/app.py @@ -23,6 +23,7 @@ from azure.identity import DefaultAzureCredential from azure.mgmt.compute import ComputeManagementClient from functools import wraps +from contextlib import contextmanager from flask_caching import Cache from azure.keyvault.secrets import SecretClient from config import * @@ -31,8 +32,10 @@ # Flask App app = Flask(__name__) -app.config['VERSION'] = '0.158' +app.config['VERSION'] = '0.159' +# Backs is_member_of_group_cached, which keeps token validation off the Graph API on +# every request. cache = Cache(app, config={'CACHE_TYPE': 'simple'}) REMOTE_CREATE_USER_SCRIPT = '/usr/local/bin/create-user.sh' @@ -48,20 +51,18 @@ @app.route('/health', methods=['GET']) def health(): - conn = get_db_connection() - if not conn: - return jsonify({'status': 'unhealthy'}), 503 - try: - cursor = conn.cursor() - cursor.execute('SELECT 1') - cursor.fetchone() + with db_connection() as conn: + cursor = conn.cursor() + cursor.execute('SELECT 1') + cursor.fetchone() return jsonify({'status': 'healthy', 'version': app.config['VERSION']}), 200 - except Exception as e: - logger.error("Health check failed: %s", e) + except DatabaseUnavailable: + logger.error("Database connection failed during health check.") + return jsonify({'status': 'unhealthy'}), 503 + except Exception: + logger.exception("Health check failed.") return jsonify({'status': 'unhealthy'}), 503 - finally: - conn.close() # =============================== # Functions @@ -74,7 +75,7 @@ def retrieve_db_password_from_key_vault(): secret = secret_client.get_secret(DB_PASSWORD_NAME) db_password = secret.value except Exception as e: - print("Error retrieving password from Key Vault: %s", e) + logger.error("Error retrieving password from Key Vault: %s", e) db_password = None def get_db_connection(): @@ -82,7 +83,7 @@ def get_db_connection(): if db_password is None: retrieve_db_password_from_key_vault() if db_password is None: - print("Cannot connect to database without a password.") + logger.error("Cannot connect to database without a password.") return None try: conn = pymssql.connect( @@ -93,7 +94,7 @@ def get_db_connection(): ) return conn except pymssql.Error as e: - print("Error connecting to database: %s", e) + logger.error("Error connecting to database: %s", e) return None def refresh_db_password(interval=3600): @@ -115,7 +116,7 @@ def retrieve_pem_key_from_key_vault(vault_url, key_name): pem_file.write(pem_key) os.chmod(pem_file_path, required_permissions) except Exception as e: - print("Failed to write PEM key to file: %s", e) + logger.error("Failed to write PEM key to file: %s", e) raise else: current_permissions = oct(os.stat(pem_file_path).st_mode & 0o777) @@ -123,7 +124,7 @@ def retrieve_pem_key_from_key_vault(vault_url, key_name): try: os.chmod(pem_file_path, required_permissions) except Exception as e: - print("Failed to update permissions for %s: %s", pem_file_path, e) + logger.error("Failed to update permissions for %s: %s", pem_file_path, e) raise return pem_file_path @@ -148,33 +149,30 @@ def get_access_token(tenant_id, client_id, client_secret): def get_or_create_uid(username): try: - conn = get_db_connection() - if not conn: - logger.error("Database connection failed while resolving uid for %s", username) - return None - - cursor = conn.cursor(as_dict=True) - - # Check if user already exists - cursor.execute("SELECT uid FROM VmUsers WHERE username = %s", (username,)) - result = cursor.fetchone() - if result: - conn.close() - return result['uid'] - - # Assign a new UID starting from 2000 - cursor.execute("SELECT MAX(uid) AS max_uid FROM VmUsers") - max_uid = cursor.fetchone()['max_uid'] or 1999 - new_uid = max_uid + 1 - - # Insert new user - cursor.execute("INSERT INTO VmUsers (username, uid) VALUES (%s, %s)", (username, new_uid)) - conn.commit() - conn.close() + with db_connection() as conn: + cursor = conn.cursor(as_dict=True) + + # Check if user already exists + cursor.execute("SELECT uid FROM VmUsers WHERE username = %s", (username,)) + result = cursor.fetchone() + if result: + return result['uid'] + + # Assign a new UID starting from 2000 + cursor.execute("SELECT MAX(uid) AS max_uid FROM VmUsers") + max_uid = cursor.fetchone()['max_uid'] or 1999 + new_uid = max_uid + 1 + + # Insert new user + cursor.execute("INSERT INTO VmUsers (username, uid) VALUES (%s, %s)", (username, new_uid)) + conn.commit() return new_uid - except Exception as e: - logger.error("Failed to resolve uid for %s: %s", username, e) + except DatabaseUnavailable: + logger.error("Database connection failed while resolving uid for %s", username) + return None + except Exception: + logger.exception("Failed to resolve uid for %s.", username) return None def normalize_lease_id(value): @@ -198,6 +196,199 @@ def serialize_for_json(value): return value + +# =============================== +# Request plumbing + + +class DatabaseUnavailable(Exception): + """The API could not obtain a database connection.""" + + +class GroupCheckUnavailable(Exception): + """Group membership could not be determined (Graph unreachable, throttled, or + no token). + + Distinct from "the principal is not a member" so a transient Graph failure is + never cached as an authorization denial. + """ + + +@contextmanager +def db_connection(): + """Yield a database connection that is always closed. + + Most handlers previously called get_db_connection() and then conn.close() on the + success path only, so any exception in between leaked the connection until the + pool was exhausted. Using this as a context manager makes the close unconditional. + + Raises DatabaseUnavailable when a connection cannot be established, so callers do + not have to repeat the `if not conn` check. + """ + conn = get_db_connection() + if not conn: + raise DatabaseUnavailable("Could not establish a database connection.") + try: + yield conn + finally: + try: + conn.close() + except Exception: + logger.exception("Failed to close database connection.") + + +def error_response(message, status=500): + """Return a consistent JSON error envelope. + + Handlers used to return the raw str(e), which exposed driver errors, server names + and schema details to the caller. The detail belongs in Application Insights, not + in the response body. + """ + return jsonify({'error': message}), status + + +def coerce_optional_int(value, default=None, minimum=None, maximum=None): + """Parse an optional integer that may arrive as the string 'null'. + + The portal sends the literal string "null" for an unset limit. The stored + procedures declare @Limit as INT, so passing that string through made SQL Server + fail converting 'null' to int and the request 500'd -- which is why the portal's + "No Limit" option never worked. Anything unparseable is treated as unset rather + than as an error, so an old portal build keeps working during a rolling upgrade. + """ + if value is None: + return default + + if isinstance(value, bool): + return default + + if isinstance(value, str): + candidate = value.strip() + if candidate == '' or candidate.lower() in ('null', 'none', 'undefined'): + return default + else: + candidate = value + + try: + parsed = int(candidate) + except (TypeError, ValueError): + return default + + if minimum is not None and parsed < minimum: + return minimum + if maximum is not None and parsed > maximum: + return maximum + return parsed + + +def normalize_date_filter(value): + """Treat the portal's 'null' sentinel and blank strings as 'no filter'.""" + if value is None: + return None + if isinstance(value, str): + candidate = value.strip() + if candidate == '' or candidate.lower() in ('null', 'none'): + return None + return candidate + return value + + +# Matches the clamp inside the paged stored procedures. +MAX_PAGE_SIZE = 200 +DEFAULT_PAGE_SIZE = 50 + + +def run_history_query(proc, paged_proc, label): + """Shared implementation for the three date-filtered history endpoints. + + Pagination is opt-in: when neither `page` nor `per_page` is supplied the response + stays a bare JSON array, because the scheduled task and older portal builds consume + these endpoints as plain lists. + """ + try: + req_body = request.get_json(silent=True) or {} + + startdate = normalize_date_filter(req_body.get('startdate')) + enddate = normalize_date_filter(req_body.get('enddate')) + + raw_page = request.args.get('page', req_body.get('page')) + raw_per_page = request.args.get('per_page', req_body.get('per_page')) + wants_pagination = raw_page is not None or raw_per_page is not None + + if wants_pagination: + page = coerce_optional_int(raw_page, default=1, minimum=1) + per_page = coerce_optional_int( + raw_per_page, default=DEFAULT_PAGE_SIZE, minimum=1, maximum=MAX_PAGE_SIZE + ) + offset = (page - 1) * per_page + + # The paged procedures have no @Limit, so an operator-supplied limit is + # applied here as a cap on the overall result set. Without this the limit + # box in the portal would silently do nothing once paging was enabled. + limit = coerce_optional_int(req_body.get('limit'), default=None, minimum=1) + + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + f"EXEC {paged_proc} @StartDate = %s, @EndDate = %s, @Offset = %s, @PageSize = %s", + (startdate, enddate, offset, per_page), + ) + rows = cursor.fetchall() or [] + + # TotalCount rides along on each row, so an out-of-range page + # returns nothing and the total would otherwise read as 0 -- + # making "page 40 of 4" indistinguishable from "no matches" and + # collapsing the pager so the operator cannot navigate back. + if not rows and offset > 0: + cursor.execute( + f"EXEC {paged_proc} @StartDate = %s, @EndDate = %s, @Offset = %s, @PageSize = %s", + (startdate, enddate, 0, 1), + ) + probe = cursor.fetchall() or [] + total = int(probe[0].get('TotalCount') or 0) if probe else 0 + else: + total = int(rows[0].get('TotalCount') or 0) if rows else 0 + + items = [ + {key: value for key, value in row.items() if key != 'TotalCount'} + for row in rows + ] + + if limit is not None: + total = min(total, limit) + remaining = max(0, limit - offset) + items = items[:remaining] + + total_pages = (total + per_page - 1) // per_page if per_page else 0 + + return jsonify(serialize_for_json({ + 'items': items, + 'page': page, + 'per_page': per_page, + 'total': total, + 'total_pages': total_pages, + })), 200 + + # Unpaged path. `limit` may arrive as the string "null" from the portal. + limit = coerce_optional_int(req_body.get('limit'), default=None, minimum=1) + + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + f"EXEC {proc} @StartDate = %s, @EndDate = %s, @Limit = %s", + (startdate, enddate, limit), + ) + rows = cursor.fetchall() or [] + + return jsonify(serialize_for_json(rows)), 200 + + except DatabaseUnavailable: + logger.error("Database connection failed while reading %s.", label) + return error_response("Database connection failed.", 500) + except Exception: + logger.exception("Failed to read %s.", label) + return error_response(f"Unable to retrieve the {label}.", 500) + def get_remote_host_fqdn(hostname: str) -> str: linux_host_admin_login_name = LINUX_HOST_ADMIN_LOGIN_NAME or 'avdadmin' return f"{linux_host_admin_login_name}@{hostname}.{DOMAIN_NAME}" @@ -267,21 +458,16 @@ def release_vm_assignment(vmid, lease_id) -> bool: if not vmid or not normalized_lease_id: return False - conn = None try: - conn = get_db_connection() - if not conn: - logger.error("Database connection failed while releasing VMID %s after a failed checkout.", vmid) - return False - - with conn.cursor(as_dict=True) as cursor: - cursor.execute( - "EXEC ReturnVm @VMID = %s, @ExpectedLeaseId = %s", - (vmid, normalized_lease_id) - ) - row = cursor.fetchone() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + "EXEC ReturnVm @VMID = %s, @ExpectedLeaseId = %s", + (vmid, normalized_lease_id) + ) + row = cursor.fetchone() - conn.commit() + conn.commit() if not row: logger.error("Could not release VMID %s after a failed checkout; the lease no longer matches.", vmid) @@ -289,12 +475,12 @@ def release_vm_assignment(vmid, lease_id) -> bool: logger.info("Released VMID %s after a failed checkout.", vmid) return True - except Exception as e: - logger.error("Error releasing VMID %s after a failed checkout: %s", vmid, e) + except DatabaseUnavailable: + logger.error("Database connection failed while releasing VMID %s after a failed checkout.", vmid) + return False + except Exception: + logger.exception("Error releasing VMID %s after a failed checkout.", vmid) return False - finally: - if conn: - conn.close() def generate_secure_password(length=25) -> str: characters = string.ascii_letters + string.digits + string.punctuation @@ -304,8 +490,8 @@ def generate_secure_password(length=25) -> str: def is_member_of_group(service_principal_id, group_ids): access_token = get_access_token(TENANT_ID, CLIENT_ID, MICROSOFT_PROVIDER_AUTHENTICATION_SECRET) if not access_token: - print("Cannot acquire access token for Graph API.") - return False + logger.error("Cannot acquire access token for Graph API.") + raise GroupCheckUnavailable("Could not acquire a Graph API token.") headers = { 'Authorization': f'Bearer {access_token}', @@ -327,8 +513,11 @@ def is_member_of_group(service_principal_id, group_ids): else: return False else: - print("Graph API error: %s - %s", response.status_code, response.text) - return False + # Raise rather than return False: a throttled or failing Graph call is not + # evidence that the principal lacks membership, and returning False here + # would be memoized as a denial for the whole cache window. + logger.error("Graph API error: %s - %s", response.status_code, response.text) + raise GroupCheckUnavailable(f"Graph API returned {response.status_code}.") def delete_remote_user(hostname: str, username: str, lease_id: str = None) -> bool: normalized_lease_id = normalize_lease_id(lease_id) @@ -373,7 +562,8 @@ def delete_remote_user(hostname: str, username: str, lease_id: str = None) -> bo @cache.memoize(timeout=300) def is_member_of_group_cached(user_oid, group_ids): - return is_member_of_group(user_oid, group_ids) + # memoize needs hashable arguments, so the caller passes a tuple. + return is_member_of_group(user_oid, list(group_ids)) def token_required(required_permissions=None, required_group_ids=None): def decorator(f): @@ -388,10 +578,10 @@ def decorated(*args, **kwargs): if len(parts) == 2 and parts[0] == 'Bearer': token = parts[1] else: - print("Authorization header is malformed. Expected 'Bearer '.") + logger.error("Authorization header is malformed. Expected 'Bearer '.") if not token: - print("Token is missing in the request.") + logger.error("Token is missing in the request.") return jsonify({'message': 'Token is missing!'}), 401 try: @@ -456,23 +646,33 @@ def decorated(*args, **kwargs): is_in_group = False if required_group_ids: - is_in_group = is_member_of_group(user_oid, required_group_ids) + # Use the memoized wrapper: this runs on every authenticated + # request from the AVD and Linux hosts, and the uncached path was + # calling Microsoft Graph each time. + try: + is_in_group = is_member_of_group_cached(user_oid, tuple(required_group_ids)) + except GroupCheckUnavailable: + # Not cached, and reported as a dependency failure rather than + # a denial, so a Graph blip does not look like a permissions + # problem to the AVD and Linux host agents. + logger.exception("Could not verify group membership for %s.", user_oid) + return jsonify({'error': 'Unable to verify group membership. Please retry.'}), 503 if not (has_scope_permission or has_role_permission or is_in_group): - print("Access denied: insufficient scope or role permissions or group membership.") + logger.error("Access denied: insufficient scope or role permissions or group membership.") return jsonify({'message': 'Access denied: insufficient scope or role permissions or group membership.'}), 403 except jwt.ExpiredSignatureError: - print("Token has expired.") + logger.error("Token has expired.") return jsonify({'message': 'Token has expired.'}), 401 except jwt.InvalidAudienceError as e: - print("Invalid audience: %s", e) + logger.error("Invalid audience: %s", e) return jsonify({'message': 'Invalid audience.'}), 401 except jwt.InvalidIssuerError as e: - print("Invalid issuer: %s", e) + logger.error("Invalid issuer: %s", e) return jsonify({'message': 'Invalid issuer.'}), 401 except Exception as e: - print("Token validation error: %s", e) + logger.error("Token validation error: %s", e) return jsonify({'message': 'Token is invalid.'}), 401 return f(*args, **kwargs) @@ -604,53 +804,47 @@ def validate_host_settings(payload: dict): def fetch_host_settings(): """Read the single global settings profile.""" - conn = get_db_connection() - if not conn: - logger.error("Database connection failed while reading Linux host settings.") - return None - try: - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetLinuxHostSettings") - row = cursor.fetchone() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetLinuxHostSettings") + row = cursor.fetchone() if not row: logger.error("No Linux host settings profile exists.") return None return normalize_host_settings(row) - except Exception as e: - logger.error("Error reading Linux host settings: %s", e) + except DatabaseUnavailable: + logger.error("Database connection failed while reading Linux host settings.") + return None + except Exception: + logger.exception("Error reading Linux host settings.") return None - finally: - conn.close() def record_settings_applied(hostname: str, settings_version: int) -> bool: - conn = get_db_connection() - if not conn: - logger.error("Database connection failed while recording applied settings for %s.", hostname) - return False - try: - with conn.cursor(as_dict=True) as cursor: - cursor.execute( - "EXEC RecordHostSettingsApplied @Hostname = %s, @SettingsVersion = %s", - (hostname, settings_version) - ) - row = cursor.fetchone() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + "EXEC RecordHostSettingsApplied @Hostname = %s, @SettingsVersion = %s", + (hostname, settings_version) + ) + row = cursor.fetchone() - conn.commit() + conn.commit() if not row: logger.warning("No VM record matched hostname %s while recording applied settings.", hostname) return False return True - except Exception as e: - logger.error("Error recording applied settings for %s: %s", hostname, e) + except DatabaseUnavailable: + logger.error("Database connection failed while recording applied settings for %s.", hostname) + return False + except Exception: + logger.exception("Error recording applied settings for %s.", hostname) return False - finally: - conn.close() def apply_host_settings_to_host(hostname: str, settings: dict): """Push settings to one host over SSH. @@ -695,56 +889,50 @@ def get_version(): @token_required(['access_as_user', 'FullAccess', 'ScheduledTask']) def get_all_vms(): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - cursor = conn.cursor(as_dict=True) - cursor.execute("EXEC GetVms") - rows = cursor.fetchall() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetVms") + rows = cursor.fetchall() - if not rows: - return jsonify([]), 200 + return jsonify(serialize_for_json(rows or [])), 200 - return jsonify(serialize_for_json(rows)), 200 + except DatabaseUnavailable: + logger.error("Database connection failed while listing VMs.") + return error_response("Database connection failed.", 500) + except Exception: + logger.exception("Failed to list VMs.") + return error_response("Unable to retrieve virtual machines.", 500) - except Exception as e: - return f"An unexpected error occurred: {str(e)}", 500 +@app.route('/api/vms/summary', methods=['GET']) +@token_required(['access_as_user', 'FullAccess', 'ScheduledTask']) +def get_vm_summary(): + """Aggregate pool counters for the portal dashboard. -@app.route('/api/vms/available', methods=['GET']) -@token_required(['access_as_user', 'FullAccess']) -def get_available_vm(): + The dashboard previously fetched every VM row over HTTP just to count them, so the + payload grew with the pool. This returns a fixed-size object computed in SQL. + """ try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - cursor = conn.cursor(as_dict=True) - query = """ - SELECT TOP 1 * FROM dbo.VirtualMachines - WHERE PowerState = 'On' AND NetworkStatus = 'Reachable' AND VmStatus = 'Available' - """ - cursor.execute(query) - row = cursor.fetchone() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetVmSummary") + row = cursor.fetchone() - if not row: - return "No available VM found.", 404 - - available_vm = { - "VMID": row["VMID"], - "Hostname": row["Hostname"], - "IPAddress": row["IPAddress"], - "VmStatus": row["VmStatus"], - "NetworkStatus": row["NetworkStatus"], - "PowerState": row["PowerState"] - } + summary = serialize_for_json(row) if row else {} - return jsonify(available_vm), 200 + # The procedure always returns a row, but never let the dashboard 500 or render + # blanks if that ever changes. + fields = ('TotalVMs', 'Available', 'CheckedOut', 'Maintenance', 'Released', + 'PoweredOn', 'PoweredOff', 'Unreachable', 'Ready') + normalized = {field: int(summary.get(field) or 0) for field in fields} - except Exception as e: - return f"Error: {str(e)}", 500 + return jsonify(normalized), 200 + + except DatabaseUnavailable: + logger.error("Database connection failed while building the VM summary.") + return error_response("Database connection failed.", 500) + except Exception: + logger.exception("Failed to build the VM summary.") + return error_response("Unable to retrieve the virtual machine summary.", 500) @app.route('/api/vms/checkout', methods=['POST']) @token_required(['AvdHost', 'access_as_user', 'FullAccess'], required_group_ids=[AVD_HOST_GROUP_ID]) @@ -756,43 +944,40 @@ def checkout_vm(): avdhost = req_body.get('avdhost') if not username or not avdhost: - return "Please provide 'username' and 'avdhost' in the request body.", 400 + return error_response("Please provide 'username' and 'avdhost' in the request body.", 400) username = re.sub(r'[^a-zA-Z0-9_]', '', username) user_password = generate_secure_password() - conn = get_db_connection() - try: + with db_connection() as conn: with conn.cursor(as_dict=True) as cursor: cursor.callproc('CheckoutVm', (username, avdhost)) rows = cursor.fetchall() conn.commit() - finally: - conn.close() if not rows or 'Message' in rows[0]: - return "No available VM found. Please try again.", 409 + return error_response("No available VM found. Please try again.", 409) checked_out_vm = rows[0] vm_hostname = checked_out_vm.get('Hostname') lease_id = normalize_lease_id(checked_out_vm.get('LeaseId')) if not vm_hostname or not lease_id: - return "No hostname or LeaseId found for the checked-out VM.", 500 + return error_response("No hostname or LeaseId found for the checked-out VM.", 500) if not create_or_update_remote_user(vm_hostname, username, user_password, lease_id): release_vm_assignment(checked_out_vm.get("VMID"), lease_id) - return f"Failed to create or update user '{username}' on VM '{vm_hostname}'.", 500 + return error_response(f"Failed to create or update user '{username}' on VM '{vm_hostname}'.", 500) groups_to_add = ["tsusers", "appusers"] if not remote_group_exists(vm_hostname, "tsusers"): if not create_remote_group(vm_hostname, "tsusers"): - return f"Failed to create group 'tsusers' on VM '{vm_hostname}'.", 500 + return error_response(f"Failed to create group 'tsusers' on VM '{vm_hostname}'.", 500) if not remote_group_exists(vm_hostname, "appusers"): if not create_remote_group(vm_hostname, "appusers"): - return f"Failed to create group 'appusers' on VM '{vm_hostname}'.", 500 + return error_response(f"Failed to create group 'appusers' on VM '{vm_hostname}'.", 500) for group in groups_to_add: if not is_user_in_remote_group(vm_hostname, username, group): @@ -811,10 +996,15 @@ def checkout_vm(): return jsonify(serialize_for_json(response_data)), 200 except json.JSONDecodeError: - return "Invalid JSON data", 400 + return error_response("Invalid JSON data", 400) - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while checking out a VM.") + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to check out a VM.") + return error_response("Unable to check out a virtual machine.", 500) @app.route('/api/vms//update-attributes', methods=['POST']) @token_required(['ScheduledTask', 'access_as_user', 'FullAccess']) @@ -829,52 +1019,54 @@ def update_vm_attributes(vmid): if not any([powerstate, networkstatus, vmstatus]): return jsonify({'error': "Please provide at least one attribute to update."}), 400 - conn = get_db_connection() - if not conn: - return jsonify({'error': "Database connection failed."}), 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute( - "EXEC UpdateVmAttributes @VMID = %s, @PowerState = %s, @NetworkStatus = %s, @VmStatus = %s", - (vmid, powerstate, networkstatus, vmstatus) - ) - row = cursor.fetchone() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + "EXEC UpdateVmAttributes @VMID = %s, @PowerState = %s, @NetworkStatus = %s, @VmStatus = %s", + (vmid, powerstate, networkstatus, vmstatus) + ) + row = cursor.fetchone() conn.commit() - if not row: - return jsonify({'error': "VM not found or no attributes updated. Please try again."}), 404 - - conn.close() + + if not row: + return jsonify({'error': "VM not found or no attributes updated. Please try again."}), 404 return jsonify(row), 200 except json.JSONDecodeError: return jsonify({'error': "Invalid JSON data"}), 400 - except Exception as e: - return jsonify({'error': str(e)}), 500 + except DatabaseUnavailable: + logger.error("Database connection failed while updating VM %s attributes.", vmid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to update VM %s attributes.", vmid) + return error_response("Unable to update virtual machine attributes.", 500) @app.route('/api/vms//delete', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def delete_vm(vmid): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC DeleteVm @VMID = %s", (vmid,)) - row = cursor.fetchone() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC DeleteVm @VMID = %s", (vmid,)) + row = cursor.fetchone() - conn.commit() - conn.close() + conn.commit() if not row: - return f"VM with VMID {vmid} could not be deleted or was not found.", 404 + return error_response(f"VM with VMID {vmid} could not be deleted or was not found.", 404) return f"VM with VMID {vmid} has been successfully deleted.", 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while deleting VM %s.", vmid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to delete VM %s.", vmid) + return error_response("Unable to delete the virtual machine.", 500) @app.route('/api/vms/add', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) @@ -892,71 +1084,69 @@ def add_new_vm(): description = req_body.get('description', None) if not (hostname and ipaddress and powerstate and networkstatus and vmstatus): - return "Please provide 'hostname', 'ipaddress', 'powerstate', 'networkstatus', and 'vmstatus' in the request body.", 400 + return error_response("Please provide 'hostname', 'ipaddress', 'powerstate', 'networkstatus', and 'vmstatus' in the request body.", 400) - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute(""" - EXEC AddVm @Hostname = %s, @IPAddress = %s, @PowerState = %s, @NetworkStatus = %s, @VmStatus = %s, - @Username = %s, @AvdHost = %s, @Description = %s - """, (hostname, ipaddress, powerstate, networkstatus, vmstatus, username, avdhost, description)) + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute(""" + EXEC AddVm @Hostname = %s, @IPAddress = %s, @PowerState = %s, @NetworkStatus = %s, @VmStatus = %s, + @Username = %s, @AvdHost = %s, @Description = %s + """, (hostname, ipaddress, powerstate, networkstatus, vmstatus, username, avdhost, description)) - row = cursor.fetchone() + row = cursor.fetchone() - conn.commit() - conn.close() + conn.commit() if not row: - return "Failed to add new VM. Please try again.", 500 + return error_response("Failed to add new VM. Please try again.", 500) return jsonify({"NewVMID": row['NewVMID']}), 201 except json.JSONDecodeError: - return "Invalid JSON data", 400 + return error_response("Invalid JSON data", 400) - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while adding a VM.") + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to add a VM.") + return error_response("Unable to add the virtual machine.", 500) @app.route('/api/vms/', methods=['GET']) @token_required(['access_as_user', 'FullAccess']) def get_vm_details(vmid): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetVmDetails @VMID = %s", (vmid,)) - row = cursor.fetchone() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetVmDetails @VMID = %s", (vmid,)) + row = cursor.fetchone() if not row: - return f"VM with VMID {vmid} was not found.", 404 + return error_response(f"VM with VMID {vmid} was not found.", 404) return jsonify(serialize_for_json(row)), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while reading VM %s.", vmid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to read VM %s.", vmid) + return error_response("Unable to retrieve the virtual machine.", 500) @app.route('/api/vms//return', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def return_vm(vmid): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC ReturnVm @VMID = %s", (vmid,)) - row = cursor.fetchone() - conn.commit() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC ReturnVm @VMID = %s", (vmid,)) + row = cursor.fetchone() + conn.commit() if not row: - return f"VM with VMID {vmid} was not found or is not currently checked out.", 404 + return error_response(f"VM with VMID {vmid} was not found or is not currently checked out.", 404) hostname = row.get('Hostname') username = row.get('ReturnedUsername') @@ -971,8 +1161,13 @@ def return_vm(vmid): return jsonify(serialize_for_json(row)), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while returning VM %s.", vmid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to return VM %s.", vmid) + return error_response("Unable to return the virtual machine.", 500) @app.route('/api/vms//release', methods=['POST']) @token_required(['LinuxHost', 'access_as_user', 'FullAccess'], required_group_ids=[LINUX_HOST_GROUP_ID]) @@ -989,22 +1184,18 @@ def release_vm(hostname): if username: username = re.sub(r'[^a-zA-Z0-9_]', '', username) - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + "EXEC ReleaseVm @Hostname = %s, @LeaseId = %s, @Username = %s", + (hostname, lease_id, username) + ) + row = cursor.fetchone() - with conn.cursor(as_dict=True) as cursor: - cursor.execute( - "EXEC ReleaseVm @Hostname = %s, @LeaseId = %s, @Username = %s", - (hostname, lease_id, username) - ) - row = cursor.fetchone() - - conn.commit() - conn.close() + conn.commit() if not row: - return f"Failed to release VM with Hostname {hostname}. Please try again.", 500 + return error_response(f"Failed to release VM with Hostname {hostname}. Please try again.", 500) release_status = (row.get('ReleaseStatus') or '').strip() @@ -1020,22 +1211,23 @@ def release_vm(hostname): # NoActiveAssignment means the VM is already released, so the agent should stop retrying. return jsonify(serialize_for_json(row)), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while releasing VM with Hostname %s.", hostname) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to release VM with Hostname %s.", hostname) + return error_response("Unable to release the virtual machine.", 500) @app.route('/api/vms/released', methods=['POST']) @token_required(['ScheduledTask', 'access_as_user', 'FullAccess']) def return_released_vm_api(): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC ReturnReleasedVms") - rows = cursor.fetchall() - conn.commit() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC ReturnReleasedVms") + rows = cursor.fetchall() + conn.commit() if not rows: return "No VMs to return at this time.", 200 @@ -1054,39 +1246,23 @@ def return_released_vm_api(): return jsonify(serialize_for_json(rows)), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while returning released VMs.") + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to return released VMs.") + return error_response("Unable to return released virtual machines.", 500) @app.route('/api/vms/history', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def get_vm_history(): - try: - req_body = request.get_json() - - startdate = req_body.get('startdate', None) - enddate = req_body.get('enddate', None) - limit = req_body.get('limit', 100) - - startdate = None if startdate == 'null' else startdate - enddate = None if enddate == 'null' else enddate - - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetVmHistory @StartDate = %s, @EndDate = %s, @Limit = %s", (startdate, enddate, limit)) - rows = cursor.fetchall() - conn.close() - - return jsonify(serialize_for_json(rows)), 200 - - except json.JSONDecodeError: - return "Invalid JSON data", 400 - - except Exception as e: - return f"Error: {str(e)}", 500 + return run_history_query( + proc='GetVmHistory', + paged_proc='GetVmHistoryPaged', + label='VM history', + ) # =============================== # Scaling APIs @@ -1094,57 +1270,32 @@ def get_vm_history(): @app.route('/api/scaling/log', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def get_scaling_activity_log(): - try: - req_body = request.get_json() - - startdate = req_body.get('startdate', None) - enddate = req_body.get('enddate', None) - limit = req_body.get('limit', 100) - - startdate = None if startdate == 'null' else startdate - enddate = None if enddate == 'null' else enddate - - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetScalingActivityLog @StartDate = %s, @EndDate = %s, @Limit = %s", (startdate, enddate, limit)) - rows = cursor.fetchall() - - conn.close() - - if not rows: - return jsonify({"message": "No scaling activities found for the specified criteria."}), 200 - - return jsonify(rows), 200 - - except json.JSONDecodeError: - return "Invalid JSON data", 400 - - except Exception as e: - return f"Error: {str(e)}", 500 + return run_history_query( + proc='GetScalingActivityLog', + paged_proc='GetScalingActivityLogPaged', + label='scaling activity log', + ) @app.route('/api/scaling/trigger', methods=['POST']) @token_required(['ScheduledTask', 'access_as_user', 'FullAccess']) def trigger_scaling_logic(): try: if not VM_SUBSCRIPTION_ID or not VM_RESOURCE_GROUP: - return "Configuration error: missing Azure subscription or resource group.", 500 + return error_response("Configuration error: missing Azure subscription or resource group.", 500) credential = DefaultAzureCredential() compute_client = ComputeManagementClient(credential=credential, subscription_id=VM_SUBSCRIPTION_ID) - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC TriggerScalingLogic") - rows = cursor.fetchall() - - conn.close() - + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC TriggerScalingLogic") + rows = cursor.fetchall() + # TriggerScalingLogic updates PowerState on the selected VMs and inserts + # the activity-log row. pymssql does not autocommit, so without this the + # database rolled all of it back while the Azure power operations below + # still went ahead -- leaving Azure and the broker out of step and the + # scaling activity log permanently empty. + conn.commit() powered_on_vms = [] powered_off_vms = [] @@ -1164,8 +1315,13 @@ def trigger_scaling_logic(): return jsonify(response_payload), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while triggering scaling logic.") + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to trigger scaling logic.") + return error_response("Unable to trigger scaling logic.", 500) # =============================== # Scaling Rules APIs @@ -1174,43 +1330,43 @@ def trigger_scaling_logic(): @token_required(['access_as_user', 'FullAccess']) def get_scaling_rules(): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetScalingRules") - rows = cursor.fetchall() - conn.close() - - if not rows: - return "No scaling rules found.", 404 + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetScalingRules") + rows = cursor.fetchall() - return jsonify(rows), 200 + # An empty rule set is a valid state, not an error. Returning 404 here made the + # portal raise_for_status(), flash a failure and redirect, so its "no rules" + # empty state could never render. + return jsonify(serialize_for_json(rows or [])), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while listing scaling rules.") + return error_response("Database connection failed.", 500) + except Exception: + logger.exception("Failed to list scaling rules.") + return error_response("Unable to retrieve scaling rules.", 500) @app.route('/api/scaling/rules/', methods=['GET']) @token_required(['access_as_user', 'FullAccess']) def get_scaling_rule_details(ruleid): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetScalingRuleDetails @RuleID = %s", (ruleid,)) - row = cursor.fetchone() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC GetScalingRuleDetails @RuleID = %s", (ruleid,)) + row = cursor.fetchone() if not row: - return f"Scaling rule with RuleID {ruleid} was not found.", 404 + return error_response(f"Scaling rule with RuleID {ruleid} was not found.", 404) - return jsonify(row), 200 + return jsonify(serialize_for_json(row)), 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while reading scaling rule %s.", ruleid) + return error_response("Database connection failed.", 500) + except Exception: + logger.exception("Failed to read scaling rule %s.", ruleid) + return error_response("Unable to retrieve the scaling rule.", 500) @app.route('/api/scaling/rules/create', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) @@ -1226,40 +1382,41 @@ def create_scaling_rule(): scaledownincrement = req_body.get('scaledownincrement') if not all([minvms is not None, maxvms is not None, scaleupratio is not None, scaleupincrement is not None, scaledownratio is not None, scaledownincrement is not None]): - return ( + return error_response( "Please provide all required fields: 'minvms', 'maxvms', 'scaleupratio', " "'scaleupincrement', 'scaledownratio', 'scaledownincrement'.", - 400, + 400 ) - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute( - """ - EXEC CreateScalingRule @MinVMs = %s, @MaxVMs = %s, @ScaleUpRatio = %s, - @ScaleUpIncrement = %s, @ScaleDownRatio = %s, @ScaleDownIncrement = %s - """, - (minvms, maxvms, scaleupratio, scaleupincrement, scaledownratio, scaledownincrement), - ) - row = cursor.fetchone() - conn.commit() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute( + """ + EXEC CreateScalingRule @MinVMs = %s, @MaxVMs = %s, @ScaleUpRatio = %s, + @ScaleUpIncrement = %s, @ScaleDownRatio = %s, @ScaleDownIncrement = %s + """, + (minvms, maxvms, scaleupratio, scaleupincrement, scaledownratio, scaledownincrement), + ) + row = cursor.fetchone() + conn.commit() if not row: - return "Failed to create the scaling rule. Please try again.", 500 + return error_response("Failed to create the scaling rule. Please try again.", 500) new_rule_id = row.get('NewRuleID') return jsonify({"NewRuleID": new_rule_id}), 201 except json.JSONDecodeError: - return "Invalid JSON data", 400 + return error_response("Invalid JSON data", 400) - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while creating a scaling rule.") + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to create a scaling rule.") + return error_response("Unable to create the scaling rule.", 500) @app.route('/api/scaling/rules//update', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) @@ -1275,85 +1432,63 @@ def update_scaling_rule(ruleid): scaledownincrement = req_body.get('scaledownincrement') if not any([minvms is not None, maxvms is not None, scaleupratio is not None, scaleupincrement is not None, scaledownratio is not None, scaledownincrement is not None]): - return "Please provide at least one field to update.", 400 - - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor() as cursor: - cursor.execute( - """ - EXEC UpdateScalingRule @RuleID = %s, @MinVMs = %s, @MaxVMs = %s, @ScaleUpRatio = %s, - @ScaleUpIncrement = %s, @ScaleDownRatio = %s, @ScaleDownIncrement = %s - """, - (ruleid, minvms, maxvms, scaleupratio, scaleupincrement, scaledownratio, scaledownincrement), - ) - conn.commit() - conn.close() + return error_response("Please provide at least one field to update.", 400) + + with db_connection() as conn: + with conn.cursor() as cursor: + cursor.execute( + """ + EXEC UpdateScalingRule @RuleID = %s, @MinVMs = %s, @MaxVMs = %s, @ScaleUpRatio = %s, + @ScaleUpIncrement = %s, @ScaleDownRatio = %s, @ScaleDownIncrement = %s + """, + (ruleid, minvms, maxvms, scaleupratio, scaleupincrement, scaledownratio, scaledownincrement), + ) + conn.commit() return f"Scaling rule with RuleID {ruleid} updated successfully.", 200 except json.JSONDecodeError: - return "Invalid JSON data", 400 + return error_response("Invalid JSON data", 400) - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while updating scaling rule %s.", ruleid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to update scaling rule %s.", ruleid) + return error_response("Unable to update the scaling rule.", 500) @app.route('/api/scaling/rules//delete', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def delete_scaling_rule(ruleid): try: - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC DeleteScalingRule @RuleID = %s", (ruleid,)) - row = cursor.fetchone() - conn.commit() - conn.close() + with db_connection() as conn: + with conn.cursor(as_dict=True) as cursor: + cursor.execute("EXEC DeleteScalingRule @RuleID = %s", (ruleid,)) + row = cursor.fetchone() + conn.commit() if not row: - return f"Scaling rule with RuleID {ruleid} could not be deleted or was not found.", 404 + return error_response(f"Scaling rule with RuleID {ruleid} could not be deleted or was not found.", 404) return f"Scaling rule with RuleID {ruleid} has been successfully deleted.", 200 - except Exception as e: - return f"Error: {str(e)}", 500 + except DatabaseUnavailable: + logger.error("Database connection failed while deleting scaling rule %s.", ruleid) + return error_response("Database connection failed.", 500) + + except Exception: + logger.exception("Failed to delete scaling rule %s.", ruleid) + return error_response("Unable to delete the scaling rule.", 500) @app.route('/api/scaling/rules/history', methods=['POST']) @token_required(['access_as_user', 'FullAccess']) def get_scaling_rules_history(): - try: - req_body = request.get_json() - - startdate = req_body.get('startdate', None) - enddate = req_body.get('enddate', None) - limit = req_body.get('limit', 100) - - startdate = None if startdate == 'null' else startdate - enddate = None if enddate == 'null' else enddate - - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - with conn.cursor(as_dict=True) as cursor: - cursor.execute("EXEC GetVMScalingRulesHistory @StartDate = %s, @EndDate = %s, @Limit = %s", (startdate, enddate, limit)) - rows = cursor.fetchall() - conn.close() - - if not rows: - return jsonify({"message": "No scaling activities found for the specified criteria."}), 200 - - return jsonify(rows), 200 - - except json.JSONDecodeError: - return "Invalid JSON data", 400 - - except Exception as e: - return f"Error: {str(e)}", 500 + return run_history_query( + proc='GetVMScalingRulesHistory', + paged_proc='GetVmScalingRulesHistoryPaged', + label='scaling rules history', + ) # =============================== # Linux Host Settings APIs @@ -1408,11 +1543,7 @@ def update_host_settings(): ) }), 400 - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - try: + with db_connection() as conn: with conn.cursor(as_dict=True) as cursor: cursor.execute( "EXEC UpdateLinuxHostSettings " @@ -1441,14 +1572,16 @@ def update_host_settings(): row = cursor.fetchone() conn.commit() - finally: - conn.close() if not row: return jsonify({'error': 'Unable to update Linux host settings.'}), 500 return jsonify(normalize_host_settings(row)), 200 + except DatabaseUnavailable: + logger.error("Database connection failed while updating Linux host settings.") + return error_response("Database connection failed.", 500) + except Exception: logger.exception("Failed to update Linux host settings.") return jsonify({'error': 'Unable to update Linux host settings.'}), 500 @@ -1473,16 +1606,10 @@ def apply_host_settings(): if settings is None: return jsonify({'error': 'Unable to read Linux host settings.'}), 500 - conn = get_db_connection() - if not conn: - return "Database connection failed.", 500 - - try: + with db_connection() as conn: with conn.cursor(as_dict=True) as cursor: cursor.execute("EXEC GetVms") vms = cursor.fetchall() - finally: - conn.close() if requested_hostnames: wanted = {str(name).strip().lower() for name in requested_hostnames if str(name).strip()} @@ -1520,6 +1647,10 @@ def apply_host_settings(): 'Results': results }), 200 + except DatabaseUnavailable: + logger.error("Database connection failed while pushing Linux host settings.") + return error_response("Database connection failed.", 500) + except Exception: logger.exception("Failed to push Linux host settings.") return jsonify({'error': 'Unable to push Linux host settings.'}), 500 diff --git a/api/env.example b/api/env.example index 6438633..2747ea0 100644 --- a/api/env.example +++ b/api/env.example @@ -45,3 +45,7 @@ VAULT_URL="https://your_vault_name.vault.azure.net/" KEY_NAME="your_key_name" DB_PASSWORD_NAME="db_password_key_in_vault" +# NFS export mounted on the Linux hosts for user home directories. Passed to +# create-user.sh during checkout; leave empty if the hosts use local home directories. +NFS_SHARE="your_nfs_server:/export/home" + diff --git a/api/requirements-dev.txt b/api/requirements-dev.txt new file mode 100644 index 0000000..5acec5d --- /dev/null +++ b/api/requirements-dev.txt @@ -0,0 +1,2 @@ +pytest==8.3.4 +PyYAML==6.0.2 diff --git a/api/requirements.txt b/api/requirements.txt index 89a535a..4779919 100644 --- a/api/requirements.txt +++ b/api/requirements.txt @@ -1,7 +1,6 @@ Flask==2.2.2 gunicorn Werkzeug==2.2.2 -pyodbc==4.0.35 azure-monitor-opentelemetry==1.8.7 azure-identity==1.17.1 azure-mgmt-compute==33.0.0 diff --git a/api/tests/conftest.py b/api/tests/conftest.py new file mode 100644 index 0000000..6a72049 --- /dev/null +++ b/api/tests/conftest.py @@ -0,0 +1,238 @@ +import os +import sys +import types +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +API_ROOT = REPO_ROOT / "api" + +# The API reads these at import time. +_ENV = { + "TENANT_ID": "tenant-id", + "CLIENT_ID": "client-id", + "VM_SUBSCRIPTION_ID": "subscription-id", + "VM_RESOURCE_GROUP": "resource-group", + "AVD_HOST_GROUP_ID": "avd-group-id", + "LINUX_HOST_GROUP_ID": "linux-group-id", + "DOMAIN_NAME": "example.invalid", + "VAULT_URL": "https://vault.example.invalid", + "KEY_NAME": "ssh-key", + "DB_SERVER": "db.example.invalid", + "DB_DATABASE": "LinuxBrokerTest", + "DB_USERNAME": "api-user", + "DB_PASSWORD_NAME": "db-password", + "MICROSOFT_PROVIDER_AUTHENTICATION_SECRET": "provider-secret", + "NFS_SHARE": "/mnt/test", +} +for key, value in _ENV.items(): + os.environ.setdefault(key, value) + +sys.path.insert(0, str(API_ROOT)) +os.chdir(API_ROOT) + + +def _install_import_fakes(): + pymssql = types.ModuleType("pymssql") + pymssql.Error = Exception + pymssql.connect = lambda **kwargs: None + sys.modules.setdefault("pymssql", pymssql) + + jwt = types.ModuleType("jwt") + + class ExpiredSignatureError(Exception): + pass + + class InvalidAudienceError(Exception): + pass + + class InvalidIssuerError(Exception): + pass + + class RSAAlgorithm: + @staticmethod + def from_jwk(jwk): + return {"from_jwk": jwk} + + jwt.ExpiredSignatureError = ExpiredSignatureError + jwt.InvalidAudienceError = InvalidAudienceError + jwt.InvalidIssuerError = InvalidIssuerError + jwt.algorithms = types.SimpleNamespace(RSAAlgorithm=RSAAlgorithm) + jwt.get_unverified_header = lambda token: {"kid": "test-kid"} + jwt.decode = lambda *args, **kwargs: {"oid": "user-oid", "scp": "access_as_user"} + sys.modules.setdefault("jwt", jwt) + + azure = types.ModuleType("azure") + azure_monitor = types.ModuleType("azure.monitor") + azure_monitor_opentelemetry = types.ModuleType("azure.monitor.opentelemetry") + azure_monitor_opentelemetry.configure_azure_monitor = lambda **kwargs: None + azure_identity = types.ModuleType("azure.identity") + azure_identity.DefaultAzureCredential = lambda *args, **kwargs: object() + azure_mgmt = types.ModuleType("azure.mgmt") + azure_mgmt_compute = types.ModuleType("azure.mgmt.compute") + + class ComputeManagementClient: + def __init__(self, *args, **kwargs): + self.virtual_machines = types.SimpleNamespace( + begin_start=lambda *a, **k: None, + begin_power_off=lambda *a, **k: None, + ) + + azure_mgmt_compute.ComputeManagementClient = ComputeManagementClient + azure_keyvault = types.ModuleType("azure.keyvault") + azure_keyvault_secrets = types.ModuleType("azure.keyvault.secrets") + + class SecretClient: + def __init__(self, *args, **kwargs): + pass + + def get_secret(self, name): + return types.SimpleNamespace(value="fake-secret") + + azure_keyvault_secrets.SecretClient = SecretClient + + for module in ( + azure, azure_monitor, azure_monitor_opentelemetry, azure_identity, + azure_mgmt, azure_mgmt_compute, azure_keyvault, azure_keyvault_secrets, + ): + sys.modules.setdefault(module.__name__, module) + + +_install_import_fakes() + + +class FakeJwksResponse: + status_code = 200 + text = "jwks" + + def json(self): + return {"keys": [{"kid": "test-kid", "kty": "RSA", "use": "sig", "n": "n", "e": "e"}]} + + +class FakeCursor: + def __init__(self, db, as_dict=False): + self.db = db + self.as_dict = as_dict + self.proc = None + self.params = None + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def execute(self, sql, params=None): + self.proc = _proc_name(sql) + self.params = params + self.db.calls.append({"sql": sql, "proc": self.proc, "params": params}) + if self.proc in self.db.raise_on_execute: + raise RuntimeError(self.db.raise_on_execute[self.proc]) + + def fetchall(self): + # A queued sequence lets a test model calls that must differ, such as the + # out-of-range page probe re-querying the same procedure. + queue = self.db.fetchall_sequence.get(self.proc) + if queue: + return list(queue.pop(0)) + if self.proc in self.db.fetchall_rows: + return list(self.db.fetchall_rows[self.proc]) + return [] + + def fetchone(self): + if self.proc in self.db.fetchone_rows: + return self.db.fetchone_rows[self.proc] + rows = self.fetchall() + return rows[0] if rows else None + + +class FakeConnection: + def __init__(self, db): + self.db = db + self.closed = False + self.db.connections.append(self) + + def cursor(self, as_dict=False): + return FakeCursor(self.db, as_dict=as_dict) + + def commit(self): + self.db.commits += 1 + + def close(self): + self.closed = True + + +class FakeDb: + def __init__(self): + self.connections = [] + self.calls = [] + self.commits = 0 + self.raise_on_execute = {} + self.fetchall_sequence = {} + self.fetchall_rows = { + "GetVms": [{"VMID": 1, "Hostname": "linux-01"}], + "GetScalingRules": [{"RuleID": 1, "MinVMs": 1}], + "GetVmHistory": [{"VMID": 1, "Hostname": "linux-01"}], + "GetScalingActivityLog": [{"ActivityID": 1, "ActionTaken": "None"}], + "GetVMScalingRulesHistory": [{"RuleID": 1, "SysStartTime": "2026-08-01"}], + "GetVmHistoryPaged": [ + {"VMID": 2, "Hostname": "linux-02", "TotalCount": 3}, + {"VMID": 3, "Hostname": "linux-03", "TotalCount": 3}, + ], + "GetScalingActivityLogPaged": [ + {"ActivityID": 2, "ActionTaken": "Scale Up", "TotalCount": 2}, + ], + "GetVmScalingRulesHistoryPaged": [ + {"RuleID": 2, "MinVMs": 2, "TotalCount": 4}, + ], + } + self.fetchone_rows = { + "GetVmSummary": { + "TotalVMs": 5, + "Available": 1, + "CheckedOut": 1, + "Maintenance": 1, + "Released": 1, + "PoweredOn": 3, + "PoweredOff": 2, + "Unreachable": 1, + "Ready": 1, + } + } + + def connect(self): + return FakeConnection(self) + + def latest_call(self, proc): + return next(call for call in reversed(self.calls) if call["proc"] == proc) + + +def _proc_name(sql): + text = " ".join(str(sql).replace("\n", " ").split()) + if text.upper().startswith("EXEC "): + return text.split()[1] + return text + + +@pytest.fixture(scope="session") +def app_module(): + import app as module + module.app.config.update(TESTING=True) + return module + + +@pytest.fixture +def fake_db(app_module, monkeypatch): + db = FakeDb() + monkeypatch.setattr(app_module, "get_db_connection", db.connect) + return db + + +@pytest.fixture +def client(app_module, fake_db, monkeypatch): + for endpoint, view in list(app_module.app.view_functions.items()): + original = getattr(view, "__wrapped__", None) + if original is not None: + monkeypatch.setitem(app_module.app.view_functions, endpoint, original) + return app_module.app.test_client() diff --git a/api/tests/test_api_regressions.py b/api/tests/test_api_regressions.py new file mode 100644 index 0000000..7d7cef8 --- /dev/null +++ b/api/tests/test_api_regressions.py @@ -0,0 +1,340 @@ +import pytest + + +HISTORY_ENDPOINTS = [ + ("/api/vms/history", "GetVmHistory", "GetVmHistoryPaged"), + ("/api/scaling/log", "GetScalingActivityLog", "GetScalingActivityLogPaged"), + ("/api/scaling/rules/history", "GetVMScalingRulesHistory", "GetVmScalingRulesHistoryPaged"), +] + + +def test_coerce_optional_int_handles_nullish_junk_and_clamps(app_module): + coerce = app_module.coerce_optional_int + for value in (None, "", " ", "null", "NULL", "none", "undefined", "not-an-int", True): + assert coerce(value, default=None, minimum=1, maximum=200) is None + assert coerce("5", minimum=1, maximum=200) == 5 + assert coerce("0", minimum=1, maximum=200) == 1 + assert coerce("999", minimum=1, maximum=200) == 200 + + +@pytest.mark.parametrize("path,proc,_paged_proc", HISTORY_ENDPOINTS) +def test_history_null_limit_is_omitted_for_unpaged_queries(client, fake_db, path, proc, _paged_proc): + response = client.post(path, json={"startdate": "null", "enddate": "", "limit": "null"}) + assert response.status_code == 200 + assert response.get_json() == fake_db.fetchall_rows[proc] + assert fake_db.latest_call(proc)["params"] == (None, None, None) + + +@pytest.mark.parametrize("path,proc,_paged_proc", HISTORY_ENDPOINTS) +def test_history_unparseable_and_low_limit_are_safe(client, fake_db, path, proc, _paged_proc): + response = client.post(path, json={"limit": "junk"}) + assert response.status_code == 200 + assert fake_db.latest_call(proc)["params"][2] is None + + response = client.post(path, json={"limit": "0"}) + assert response.status_code == 200 + assert fake_db.latest_call(proc)["params"][2] == 1 + + +@pytest.mark.parametrize("path,proc", [ + ("/api/scaling/rules", "GetScalingRules"), + ("/api/scaling/log", "GetScalingActivityLog"), + ("/api/scaling/rules/history", "GetVMScalingRulesHistory"), +]) +def test_empty_collection_contracts_are_200_json_arrays(client, fake_db, path, proc): + fake_db.fetchall_rows[proc] = [] + response = client.get(path) if path == "/api/scaling/rules" else client.post(path, json={}) + assert response.status_code == 200 + assert response.is_json + assert response.get_json() == [] + + +@pytest.mark.parametrize("path,proc", [ + ("/api/vms", "GetVms"), + ("/api/scaling/rules", "GetScalingRules"), + ("/api/scaling/log", "GetScalingActivityLog"), +]) +def test_connection_closes_when_cursor_execute_raises(client, fake_db, path, proc): + fake_db.raise_on_execute[proc] = "forced cursor failure" + response = client.get(path) if path in {"/api/vms", "/api/scaling/rules"} else client.post(path, json={}) + assert response.status_code == 500 + assert fake_db.connections, "expected the handler to open a connection" + assert all(connection.closed for connection in fake_db.connections) + + +@pytest.mark.parametrize("path,method,proc", [ + ("/api/vms", "get", "GetVms"), + ("/api/vms/summary", "get", "GetVmSummary"), + ("/api/scaling/rules", "get", "GetScalingRules"), + ("/api/scaling/log", "post", "GetScalingActivityLog"), +]) +def test_database_errors_do_not_disclose_driver_details(client, fake_db, path, method, proc): + secret = "pymssql: login failed for user sa at db-prod-01" + fake_db.raise_on_execute[proc] = secret + response = getattr(client, method)(path, json={}) + assert response.status_code == 500 + body = response.get_data(as_text=True) + assert secret not in body + assert "db-prod-01" not in body + assert response.get_json()["error"] + + +def test_vm_summary_returns_integer_zeroes_when_procedure_has_no_row(client, fake_db): + fake_db.fetchone_rows["GetVmSummary"] = None + response = client.get("/api/vms/summary") + assert response.status_code == 200 + assert response.get_json() == { + "TotalVMs": 0, + "Available": 0, + "CheckedOut": 0, + "Maintenance": 0, + "Released": 0, + "PoweredOn": 0, + "PoweredOff": 0, + "Unreachable": 0, + "Ready": 0, + } + assert all(isinstance(value, int) for value in response.get_json().values()) + + +@pytest.mark.parametrize("path,_proc,paged_proc", HISTORY_ENDPOINTS) +def test_history_pagination_is_opt_in_and_strips_total_count(client, fake_db, path, _proc, paged_proc): + unpaged = client.post(path, json={}) + assert unpaged.status_code == 200 + assert isinstance(unpaged.get_json(), list) + + paged = client.post(f"{path}?page=2&per_page=2", json={}) + assert paged.status_code == 200 + payload = paged.get_json() + assert set(payload) == {"items", "page", "per_page", "total", "total_pages"} + assert payload["page"] == 2 + assert payload["per_page"] == 2 + assert payload["total"] == fake_db.fetchall_rows[paged_proc][0]["TotalCount"] + assert all("TotalCount" not in item for item in payload["items"]) + assert fake_db.latest_call(paged_proc)["params"] == (None, None, 2, 2) + + +@pytest.mark.parametrize("query,expected", [ + ("page=abc", (1, 50, 0, 50)), + ("per_page=999", (1, 200, 0, 200)), + ("page=0&per_page=-3", (1, 1, 0, 1)), +]) +def test_history_pagination_coerces_hostile_values(client, fake_db, query, expected): + page, per_page, offset, size = expected + response = client.post(f"/api/vms/history?{query}", json={}) + assert response.status_code == 200 + payload = response.get_json() + assert payload["page"] == page + assert payload["per_page"] == per_page + assert fake_db.latest_call("GetVmHistoryPaged")["params"] == (None, None, offset, size) + + +class _JwksResponse: + status_code = 200 + + def json(self): + return {"keys": [{"kid": "test-kid", "kty": "RSA", "use": "sig", "n": "n", "e": "e"}]} + + +def _call_token_required(app_module, monkeypatch, payload=None, decode_exception=None, header="Bearer token", group_member=True): + monkeypatch.setattr(app_module.requests, "get", lambda url: _JwksResponse()) + monkeypatch.setattr(app_module.jwt, "get_unverified_header", lambda token: {"kid": "test-kid"}) + + observed = {} + + def fake_decode(token, key, algorithms, audience, issuer): + observed["audience"] = audience + observed["issuer"] = issuer + if decode_exception: + raise decode_exception + return payload if payload is not None else {"oid": "user-oid", "scp": "Allowed"} + + monkeypatch.setattr(app_module.jwt, "decode", fake_decode) + monkeypatch.setattr(app_module, "is_member_of_group", lambda oid, groups: group_member and "matching-group" in groups) + + def protected(): + return app_module.jsonify({"ok": True}), 200 + + wrapped = app_module.token_required(["Allowed"], required_group_ids=["matching-group"])(protected) + headers = {"Authorization": header} if header is not None else {} + with app_module.app.test_request_context("/protected", headers=headers): + result = wrapped() + return result, observed + + +@pytest.mark.parametrize("payload", [ + {"oid": "user-oid", "scp": "Allowed Other"}, + {"oid": "user-oid", "roles": ["Allowed"]}, +]) +def test_token_required_accepts_matching_scope_or_role(app_module, monkeypatch, payload): + result, observed = _call_token_required(app_module, monkeypatch, payload=payload, group_member=False) + response, status = result + assert status == 200 + assert response.get_json() == {"ok": True} + assert observed["audience"] == [app_module.CLIENT_ID, app_module.APP_URI] + assert observed["issuer"] == [ + f"{app_module.AUTHORITY_HOST}/{app_module.TENANT_ID}/v2.0", + f"{app_module.AUTHORITY_HOST}/{app_module.TENANT_ID}/", + f"{app_module.STS_ISSUER_HOST}/{app_module.TENANT_ID}/", + ] + + +@pytest.mark.parametrize("exception_cls,message", [ + ("ExpiredSignatureError", "Token has expired."), + ("InvalidAudienceError", "Invalid audience."), + ("InvalidIssuerError", "Invalid issuer."), +]) +def test_token_required_rejects_expired_wrong_audience_and_wrong_issuer(app_module, monkeypatch, exception_cls, message): + exc = getattr(app_module.jwt, exception_cls)("boom") + result, _observed = _call_token_required(app_module, monkeypatch, decode_exception=exc) + response, status = result + assert status == 401 + assert response.get_json() == {"message": message} + + +@pytest.mark.parametrize("header", [None, "", "Basic token", "Bearer", "Bearer one two"]) +def test_token_required_rejects_missing_or_malformed_authorization(app_module, monkeypatch, header): + result, _observed = _call_token_required(app_module, monkeypatch, header=header) + response, status = result + assert status == 401 + assert response.get_json() == {"message": "Token is missing!"} + + +def test_token_required_rejects_token_without_oid(app_module, monkeypatch): + result, _observed = _call_token_required(app_module, monkeypatch, payload={"scp": "Allowed"}) + response, status = result + assert status == 403 + assert response.get_json() == {"message": "Token does not contain user ID (oid)."} + + +def test_token_required_rejects_insufficient_scope_role_and_group(app_module, monkeypatch): + result, _observed = _call_token_required( + app_module, + monkeypatch, + payload={"oid": "user-oid", "scp": "Other", "roles": ["Different"]}, + group_member=False, + ) + response, status = result + assert status == 403 + assert "Access denied" in response.get_json()["message"] + + + + + +# --------------------------------------------------------------------------- +# Regressions found in code review of the hardening change itself. + + +@pytest.mark.parametrize("path,_proc,paged_proc", HISTORY_ENDPOINTS) +def test_paged_history_reports_the_total_on_an_out_of_range_page(client, fake_db, path, _proc, paged_proc): + """A page past the end must still report the real total. + + TotalCount rides along on each row, so an empty page would otherwise report + total=0, making "page 40 of 4" indistinguishable from "no matches" and + collapsing the pager so the operator cannot navigate back. + """ + fake_db.fetchall_sequence[paged_proc] = [ + [], # the requested, out-of-range page + [{"VMID": 1, "TotalCount": 37}], # the probe that recovers the total + ] + + response = client.post(f"{path}?page=40&per_page=10", json={}) + assert response.status_code == 200 + + body = response.get_json() + assert body["items"] == [] + assert body["total"] == 37 + assert body["total_pages"] == 4 + assert body["page"] == 40 + + +def test_trigger_scaling_logic_commits_its_transaction(client, fake_db): + """TriggerScalingLogic updates PowerState and inserts the activity-log row. + + pymssql does not autocommit, so without an explicit commit the database rolled + all of that back while the Azure power operations still went ahead. + """ + before = fake_db.commits + response = client.post("/api/scaling/trigger", json={}) + assert response.status_code < 500 + assert fake_db.commits > before, "TriggerScalingLogic ran without committing" + + +def test_group_check_failure_is_not_cached_as_a_denial(app_module, monkeypatch): + """A Graph outage must surface as a dependency failure, not a 403. + + is_member_of_group returns False for "not a member" but raises + GroupCheckUnavailable when it cannot tell. If the unavailable case were returned + as False it would be memoized for the whole cache window, locking every AVD + checkout and Linux host release out for five minutes. + """ + app_module.cache.clear() + calls = {"n": 0} + + def exploding_graph(oid, groups): + calls["n"] += 1 + raise app_module.GroupCheckUnavailable("Graph API returned 429.") + + monkeypatch.setattr(app_module, "is_member_of_group", exploding_graph) + monkeypatch.setattr(app_module.requests, "get", lambda url: _JwksResponse()) + monkeypatch.setattr(app_module.jwt, "get_unverified_header", lambda token: {"kid": "test-kid"}) + monkeypatch.setattr(app_module.jwt.algorithms.RSAAlgorithm, "from_jwk", staticmethod(lambda key: "key")) + monkeypatch.setattr(app_module.jwt, "decode", lambda *a, **k: {"oid": "user-oid"}) + + def protected(): + return app_module.jsonify({"ok": True}), 200 + + wrapped = app_module.token_required(["Allowed"], required_group_ids=["matching-group"])(protected) + + statuses = [] + for _ in range(2): + with app_module.app.test_request_context("/protected", headers={"Authorization": "Bearer t"}): + _, status = wrapped() + statuses.append(status) + + # Reported as unavailable, not as a permissions denial. + assert statuses == [503, 503] + # And retried rather than served from the memo cache. + assert calls["n"] == 2 + + +def test_is_member_of_group_raises_rather_than_denying_on_graph_failure(app_module, monkeypatch): + """The function itself must distinguish "not a member" from "could not tell". + + Returning False on a Graph 429 would be memoized by is_member_of_group_cached and + served as an authorization denial for the whole cache window. + """ + class _Throttled: + status_code = 429 + text = "throttled" + + def json(self): + return {} + + monkeypatch.setattr(app_module, "get_access_token", lambda *a, **k: "graph-token") + monkeypatch.setattr(app_module.requests, "post", lambda *a, **k: _Throttled()) + + with pytest.raises(app_module.GroupCheckUnavailable): + app_module.is_member_of_group("user-oid", ["group-a"]) + + +def test_is_member_of_group_raises_when_no_graph_token(app_module, monkeypatch): + monkeypatch.setattr(app_module, "get_access_token", lambda *a, **k: None) + with pytest.raises(app_module.GroupCheckUnavailable): + app_module.is_member_of_group("user-oid", ["group-a"]) + + +def test_is_member_of_group_still_returns_false_for_a_real_non_member(app_module, monkeypatch): + """The genuine "not a member" answer must stay a plain False so it can be cached.""" + class _Ok: + status_code = 200 + text = "{}" + + def json(self): + return {"value": []} + + monkeypatch.setattr(app_module, "get_access_token", lambda *a, **k: "graph-token") + monkeypatch.setattr(app_module.requests, "post", lambda *a, **k: _Ok()) + + assert app_module.is_member_of_group("user-oid", ["group-a"]) is False diff --git a/front_end/README.md b/front_end/README.md index b6244c1..16cce54 100644 --- a/front_end/README.md +++ b/front_end/README.md @@ -11,7 +11,7 @@ The front end is a small Flask app with server-rendered Jinja templates and loca | `app.py` | Creates the Flask app, enables global CSRF protection, registers route modules, and defines shared error handlers. | | `config.py` | Reads cloud, Entra ID, and Broker API settings from environment variables. | | `function_authentication.py` | Provides the `@login_required` decorator used by authenticated pages. | -| `function_api.py` | Centralises authenticated Broker API helpers, request timeouts, JSON decoding, and VM summary aggregation. | +| `function_api.py` | Centralises authenticated Broker API helpers, request timeouts, JSON decoding, dashboard VM summary retrieval, and paged history calls. | | `route_authentication.py` | Implements sign in, token callback, and sign out. | | `route_user.py` | Implements the profile page. | | `route_vm_management.py` | Registers VM management routes with `register_route_vm_management(app)`. | @@ -25,6 +25,12 @@ The front end is a small Flask app with server-rendered Jinja templates and loca Routes are registered from `app.py` by calling `register_route_*(app)` functions. Add new VM pages to the VM route module and new scaling pages to the scaling route module unless the page is genuinely cross-cutting. +Current Broker API data flow: + +- The dashboard calls `GET /api/vms/summary` for aggregate counters. If that endpoint returns `404` or `405`, the portal falls back to `GET /api/vms` and counts client-side so rolling deployments keep working. +- VM history, scaling activity, and scaling rule history request one server-side page at a time with `page` and `per_page`. The Flask session stores only filter criteria, not full result sets, so result size is bounded and two browser tabs do not overwrite each other's data. +- The portal omits unset filters instead of sending the legacy `"null"` sentinel. + ## Shared Macro Library Import the shared macro library in every content template: diff --git a/front_end/app.py b/front_end/app.py index 0c7249b..fe13bea 100644 --- a/front_end/app.py +++ b/front_end/app.py @@ -12,7 +12,7 @@ from flask_session import Session from flask_wtf.csrf import CSRFProtect, CSRFError -from function_api import NotAuthenticated, api_get, api_post, summarize_vms +from function_api import NotAuthenticated, api_get, api_post, fetch_vm_summary, summarize_vms from route_authentication import register_route_authentication from route_user import register_route_user from route_vm_management import register_route_vm_management @@ -54,7 +54,7 @@ def index(): api_error = False try: - stats = summarize_vms(api_get('/vms')) + stats = fetch_vm_summary() except NotAuthenticated: return render_template('index.html', authenticated=False) except (requests.exceptions.RequestException, ValueError) as e: @@ -63,7 +63,7 @@ def index(): # Secondary panel: never let a scaling-log failure break the dashboard. try: - activity = api_post('/scaling/log', {"startdate": "null", "enddate": "null", "limit": "5"}) + activity = api_post('/scaling/log', {"limit": 5}) recent_activity = activity[:5] if isinstance(activity, list) else [] except (NotAuthenticated, requests.exceptions.RequestException, ValueError) as e: logger.warning("Unable to load recent scaling activity for dashboard: %s", e) diff --git a/front_end/function_api.py b/front_end/function_api.py index c081cba..f08af02 100644 --- a/front_end/function_api.py +++ b/front_end/function_api.py @@ -8,6 +8,7 @@ import requests from flask import session +from datetime import datetime from config import API_URL @@ -34,44 +35,77 @@ def api_get(path, timeout=DEFAULT_TIMEOUT): return response.json() -def api_post(path, payload=None, timeout=DEFAULT_TIMEOUT): +def api_post(path, payload=None, params=None, timeout=DEFAULT_TIMEOUT): response = requests.post( - f"{API_URL}{path}", headers=auth_headers(), json=payload, timeout=timeout + f"{API_URL}{path}", headers=auth_headers(), json=payload, + params=params, timeout=timeout ) response.raise_for_status() return response.json() +def build_history_payload(filters): + """Translate the stored filter bar values into the API's request body. + + The operator enters YYYY-MM-DD; the stored procedures expect MM/DD/YYYY. The + ignore flags win over whatever is in the date and limit boxes. + """ + filters = filters or {} + payload = {} + + if not filters.get("ignore_dates"): + for key in ("startdate", "enddate"): + raw = (filters.get(key) or "").strip() + if not raw: + continue + try: + payload[key] = datetime.strptime(raw, "%Y-%m-%d").strftime("%m/%d/%Y") + except ValueError: + # Validated on submit; skip rather than send something unparseable. + logger.warning("Ignoring unparseable %s filter value %r", key, raw) + + if not filters.get("ignore_limit"): + raw_limit = str(filters.get("limit") or "").strip() + if raw_limit: + try: + payload["limit"] = int(raw_limit) + except ValueError: + logger.warning("Ignoring unparseable limit filter value %r", raw_limit) + + return payload + + +def fetch_history_page(path, filters, page, per_page): + """Fetch one page from a history endpoint. + + Returns (rows, total_items, total_pages). Falls back to slicing client-side when + the API predates pagination and still answers with a bare list, so the portal + keeps working if it is deployed ahead of the API. + """ + payload = build_history_payload(filters) + result = api_post(path, payload, params={"page": page, "per_page": per_page}) + + if isinstance(result, dict) and "items" in result: + total = int(result.get("total") or 0) + total_pages = int(result.get("total_pages") or 0) + return result.get("items") or [], total, total_pages + + # Older API: a bare array (or the legacy {"message": ...} empty envelope). + rows = result if isinstance(result, list) else [] + total = len(rows) + total_pages = (total + per_page - 1) // per_page if per_page else 0 + start = (page - 1) * per_page + return rows[start:start + per_page], total, total_pages + + # Values are constrained by the VmStatus CHECK constraint in # sql_queries/003_create_table-virtual_machines.sql. VM_STATUSES = ("Available", "CheckedOut", "Maintenance", "Released") -def summarize_vms(vms): - """Aggregate a VM list into the counters shown on the dashboard.""" - vms = vms or [] - - def count(field, value): - return sum(1 for vm in vms if (vm or {}).get(field) == value) - - total = len(vms) - available = count("VmStatus", "Available") - checked_out = count("VmStatus", "CheckedOut") - maintenance = count("VmStatus", "Maintenance") - released = count("VmStatus", "Released") - unreachable = count("NetworkStatus", "Unreachable") - powered_on = count("PowerState", "On") - - # A VM is "ready" only when it is powered on, reachable and unassigned -- - # the same condition the API uses to pick a host for checkout. - ready = sum( - 1 - for vm in vms - if (vm or {}).get("VmStatus") == "Available" - and (vm or {}).get("PowerState") == "On" - and (vm or {}).get("NetworkStatus") == "Reachable" - ) - +def _build_stats(total, available, checked_out, maintenance, released, + unreachable, powered_on, ready): + """Shape the dashboard counters from raw counts.""" other = max(0, total - available - checked_out - maintenance - released) return { @@ -83,7 +117,7 @@ def count(field, value): "other": other, "unreachable": unreachable, "powered_on": powered_on, - "powered_off": total - powered_on, + "powered_off": max(0, total - powered_on), "ready": ready, "attention": maintenance + unreachable, "utilization": round((checked_out / total) * 100) if total else 0, @@ -98,3 +132,85 @@ def count(field, value): ) }, } + + +def summary_from_api(payload): + """Map the /vms/summary response onto the dashboard's counters.""" + payload = payload or {} + + def count(key): + try: + return int(payload.get(key) or 0) + except (TypeError, ValueError): + return 0 + + return _build_stats( + total=count("TotalVMs"), + available=count("Available"), + checked_out=count("CheckedOut"), + maintenance=count("Maintenance"), + released=count("Released"), + unreachable=count("Unreachable"), + powered_on=count("PoweredOn"), + ready=count("Ready"), + ) + + +def fetch_vm_summary(): + """Fetch dashboard counters, preferring the aggregate endpoint. + + Falls back to counting the full VM list client-side if /vms/summary does not + behave, so the portal keeps working when it is deployed ahead of the API. + + The fallback deliberately triggers on any HTTP error, not just 404. An API build + that predates this endpoint does not 404: Werkzeug matches /api/vms/summary + against the older `/api/vms/` rule, so it reaches GetVmDetails with + @VMID = 'summary', fails the int conversion in SQL, and returns 500. Keying the + fallback on 404 would therefore never fire against the very build it exists for. + + This cannot mask a real outage: if the broker or database is genuinely down, the + /vms fallback fails too and the caller still sees the error. + """ + try: + return summary_from_api(api_get('/vms/summary')) + except requests.exceptions.HTTPError as e: + status = getattr(e.response, 'status_code', None) + logger.info("Falling back to client-side VM counting (/vms/summary returned %s).", status) + return summarize_vms(api_get('/vms')) + + +def summarize_vms(vms): + """Aggregate a VM list into the counters shown on the dashboard.""" + vms = vms or [] + + def count(field, value): + return sum(1 for vm in vms if (vm or {}).get(field) == value) + + total = len(vms) + available = count("VmStatus", "Available") + checked_out = count("VmStatus", "CheckedOut") + maintenance = count("VmStatus", "Maintenance") + released = count("VmStatus", "Released") + unreachable = count("NetworkStatus", "Unreachable") + powered_on = count("PowerState", "On") + + # A VM is "ready" only when it is powered on, reachable and unassigned -- + # the same condition the API uses to pick a host for checkout. + ready = sum( + 1 + for vm in vms + if (vm or {}).get("VmStatus") == "Available" + and (vm or {}).get("PowerState") == "On" + and (vm or {}).get("NetworkStatus") == "Reachable" + ) + + return _build_stats( + total=total, + available=available, + checked_out=checked_out, + maintenance=maintenance, + released=released, + unreachable=unreachable, + powered_on=powered_on, + ready=ready, + ) diff --git a/front_end/route_scaling_management.py b/front_end/route_scaling_management.py index b582811..33e76dd 100644 --- a/front_end/route_scaling_management.py +++ b/front_end/route_scaling_management.py @@ -4,6 +4,7 @@ from flask import request, redirect, url_for, session, render_template, flash from datetime import datetime from function_authentication import login_required +from function_api import NotAuthenticated, fetch_history_page from config import API_URL logger = logging.getLogger(__name__) @@ -144,233 +145,146 @@ def delete_rule(ruleid): @login_required def scaling_activity_log(): if request.method == 'POST': - try: - startdate = request.form.get('startdate') - enddate = request.form.get('enddate') - limit = request.form.get('limit', 'null') - - ignore_dates = request.form.get('ignore_dates') - ignore_limit = request.form.get('ignore_limit') - filters = { - "startdate": startdate or "", - "enddate": enddate or "", - "limit": limit or "", - "ignore_dates": bool(ignore_dates), - "ignore_limit": bool(ignore_limit) - } - - logger.debug("Form data - StartDate: %s, EndDate: %s, Limit: %s", startdate, enddate, limit) - logger.debug("Flags - Ignore Dates: %s, Ignore Limit: %s", ignore_dates, ignore_limit) - - if ignore_limit: - limit = "null" + startdate = request.form.get('startdate') + enddate = request.form.get('enddate') + limit = request.form.get('limit', '') + + ignore_dates = request.form.get('ignore_dates') + ignore_limit = request.form.get('ignore_limit') + + # Validate up front so a bad date is reported against the form the operator + # is looking at rather than failing later inside the query. + if not ignore_dates: + for label, value in (("start", startdate), ("end", enddate)): + if not value: + continue + try: + datetime.strptime(value, '%Y-%m-%d') + except ValueError: + flash(f"Invalid {label} date format. Please use 'YYYY-MM-DD'.", "danger") + return redirect(url_for('scaling_activity_log')) + + # Only the criteria live in the session now; each page is fetched from the + # API on the GET, so the session cannot grow without bound and two tabs + # cannot overwrite each other's results. + session['scaling_activity_log_filters'] = { + "startdate": startdate or "", + "enddate": enddate or "", + "limit": limit if limit and limit != "null" else "", + "ignore_dates": bool(ignore_dates), + "ignore_limit": bool(ignore_limit), + } + session.pop('scaling_activity_log', None) + session.pop('scaling_activity_log_data', None) + + return redirect(url_for('scaling_activity_log')) + + filters = session.get('scaling_activity_log_filters') or { + "startdate": "", + "enddate": "", + "limit": "", + "ignore_dates": False, + "ignore_limit": False, + } - if ignore_dates: - startdate = "null" - enddate = "null" - else: - if startdate: - try: - startdate = datetime.strptime(startdate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid start date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('scaling_activity_log')) - else: - startdate = "null" - - if enddate: - try: - enddate = datetime.strptime(enddate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid end date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('scaling_activity_log')) - else: - enddate = "null" - - data = { - "startdate": startdate, - "enddate": enddate, - "limit": limit if limit else "null" - } - - logger.debug("Data for API request: %s", data) - - session['scaling_activity_log_data'] = data - session['scaling_activity_log_filters'] = filters - - access_token = session.get("access_token") - if not access_token: - return redirect(url_for('login')) - headers = {'Authorization': f'Bearer {access_token}'} - - response = requests.post(f"{API_URL}/scaling/log", headers=headers, json=data) - response.raise_for_status() - - log = response.json() - - if not log: - flash("No scaling activities found for the specified criteria.", "info") - else: - flash("Scaling activity log retrieved successfully!", "success") - - session['scaling_activity_log'] = log - - return redirect(url_for('scaling_activity_log')) - except requests.exceptions.RequestException as e: - flash("Unable to retrieve scaling activity log. Please try again later.", "danger") - logger.error("Failed to retrieve scaling activity log: %s", e) - return redirect(url_for('view_all_rules')) - except Exception as e: - flash("An unexpected error occurred. Please try again later.", "danger") - logger.error("Unexpected error in scaling_activity_log POST: %s", e) - return redirect(url_for('view_all_rules')) - else: - try: - log = session.get('scaling_activity_log', []) - filters = session.get('scaling_activity_log_filters') - if not filters: - stored_data = session.get('scaling_activity_log_data', {}) - filters = { - "startdate": "", - "enddate": "", - "limit": stored_data.get("limit", 100), - "ignore_dates": stored_data.get("startdate") == "null" and stored_data.get("enddate") == "null", - "ignore_limit": stored_data.get("limit") == "null" - } - page = max(1, int(request.args.get('page', 1))) - per_page = max(1, int(request.args.get('per_page', 10))) - total_items = len(log) - total_pages = (total_items + per_page - 1) // per_page - - start = (page - 1) * per_page - end = start + per_page - log_paginated = log[start:end] + try: + page = max(1, int(request.args.get('page', 1))) + except (TypeError, ValueError): + page = 1 + try: + per_page = min(200, max(1, int(request.args.get('per_page', 10)))) + except (TypeError, ValueError): + per_page = 10 - logger.debug("Page: %s, Per Page: %s, Total Pages: %s", page, per_page, total_pages) - logger.debug("Log items displayed: %s", len(log_paginated)) + try: + rows, total_items, total_pages = fetch_history_page( + '/scaling/log', filters, page, per_page + ) + except NotAuthenticated: + return redirect(url_for('login')) + except (requests.exceptions.RequestException, ValueError) as e: + flash("Unable to retrieve the scaling activity log. Please try again later.", "danger") + logger.error("Error retrieving scaling activity log: %s", e) + return redirect(url_for('view_all_rules')) - return render_template('scaling/scaling_activity_log.html', - log=log_paginated, - page=page, - total_pages=total_pages, - per_page=per_page, - filters=filters) - except Exception as e: - flash("An unexpected error occurred while displaying scaling activity log.", "danger") - logger.error("Unexpected error in scaling_activity_log GET: %s", e) - return redirect(url_for('view_all_rules')) + return render_template('scaling/scaling_activity_log.html', + log=rows, + page=page, + total_pages=total_pages, + per_page=per_page, + total_items=total_items, + filters=filters) @app.route('/scaling/rules/history', methods=['GET', 'POST']) @login_required def scaling_rules_history(): if request.method == 'POST': - try: - startdate = request.form.get('startdate') - enddate = request.form.get('enddate') - limit = request.form.get('limit', 'null') - - ignore_dates = request.form.get('ignore_dates') - ignore_limit = request.form.get('ignore_limit') - filters = { - "startdate": startdate or "", - "enddate": enddate or "", - "limit": limit or "", - "ignore_dates": bool(ignore_dates), - "ignore_limit": bool(ignore_limit) - } - - if ignore_limit: - limit = "null" - - if ignore_dates: - startdate = "null" - enddate = "null" - else: - if startdate: - try: - startdate = datetime.strptime(startdate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid start date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('scaling_rules_history')) - else: - startdate = "null" - - if enddate: - try: - enddate = datetime.strptime(enddate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid end date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('scaling_rules_history')) - else: - enddate = "null" - - data = { - "startdate": startdate, - "enddate": enddate, - "limit": limit if limit else "null" - } - - session['scaling_rules_history_data'] = data - session['scaling_rules_history_filters'] = filters - - access_token = session.get("access_token") - if not access_token: - return redirect(url_for('login')) - headers = {'Authorization': f'Bearer {access_token}'} - - response = requests.post(f"{API_URL}/scaling/rules/history", headers=headers, json=data) - response.raise_for_status() + startdate = request.form.get('startdate') + enddate = request.form.get('enddate') + limit = request.form.get('limit', '') + + ignore_dates = request.form.get('ignore_dates') + ignore_limit = request.form.get('ignore_limit') + + # Validate up front so a bad date is reported against the form the operator + # is looking at rather than failing later inside the query. + if not ignore_dates: + for label, value in (("start", startdate), ("end", enddate)): + if not value: + continue + try: + datetime.strptime(value, '%Y-%m-%d') + except ValueError: + flash(f"Invalid {label} date format. Please use 'YYYY-MM-DD'.", "danger") + return redirect(url_for('scaling_rules_history')) + + # Only the criteria live in the session now; each page is fetched from the + # API on the GET, so the session cannot grow without bound and two tabs + # cannot overwrite each other's results. + session['scaling_rules_history_filters'] = { + "startdate": startdate or "", + "enddate": enddate or "", + "limit": limit if limit and limit != "null" else "", + "ignore_dates": bool(ignore_dates), + "ignore_limit": bool(ignore_limit), + } + session.pop('scaling_rules_history', None) + session.pop('scaling_rules_history_data', None) + + return redirect(url_for('scaling_rules_history')) + + filters = session.get('scaling_rules_history_filters') or { + "startdate": "", + "enddate": "", + "limit": "", + "ignore_dates": False, + "ignore_limit": False, + } - history = response.json() - - if not history: - flash("No scaling rules history found for the specified criteria.", "info") - else: - flash("Scaling rules history retrieved successfully!", "success") - - session['scaling_rules_history'] = history - - return redirect(url_for('scaling_rules_history')) - except requests.exceptions.RequestException as e: - flash("Unable to retrieve scaling rules history. Please try again later.", "danger") - logger.error("Failed to retrieve scaling rules history: %s", e) - return redirect(url_for('view_all_rules')) - except Exception as e: - flash("An unexpected error occurred. Please try again later.", "danger") - logger.error("Unexpected error in scaling_rules_history POST: %s", e) - return redirect(url_for('view_all_rules')) - else: - try: - history = session.get('scaling_rules_history', []) - filters = session.get('scaling_rules_history_filters') - if not filters: - stored_data = session.get('scaling_rules_history_data', {}) - filters = { - "startdate": "", - "enddate": "", - "limit": stored_data.get("limit", 100), - "ignore_dates": stored_data.get("startdate") == "null" and stored_data.get("enddate") == "null", - "ignore_limit": stored_data.get("limit") == "null" - } - page = max(1, int(request.args.get('page', 1))) - per_page = max(1, int(request.args.get('per_page', 10))) - total_items = len(history) - total_pages = (total_items + per_page - 1) // per_page - - start = (page - 1) * per_page - end = start + per_page - history_paginated = history[start:end] + try: + page = max(1, int(request.args.get('page', 1))) + except (TypeError, ValueError): + page = 1 + try: + per_page = min(200, max(1, int(request.args.get('per_page', 10)))) + except (TypeError, ValueError): + per_page = 10 - logger.debug("Page: %s, Per Page: %s, Total Pages: %s", page, per_page, total_pages) - logger.debug("History items displayed: %s", len(history_paginated)) + try: + rows, total_items, total_pages = fetch_history_page( + '/scaling/rules/history', filters, page, per_page + ) + except NotAuthenticated: + return redirect(url_for('login')) + except (requests.exceptions.RequestException, ValueError) as e: + flash("Unable to retrieve the scaling rules history. Please try again later.", "danger") + logger.error("Error retrieving scaling rules history: %s", e) + return redirect(url_for('view_all_rules')) - return render_template('scaling/scaling_rules_history.html', - history=history_paginated, - page=page, - total_pages=total_pages, - per_page=per_page, - filters=filters) - except Exception as e: - flash("An unexpected error occurred while displaying scaling rules history.", "danger") - logger.error("Unexpected error in scaling_rules_history GET: %s", e) - return redirect(url_for('view_all_rules')) + return render_template('scaling/scaling_rules_history.html', + history=rows, + page=page, + total_pages=total_pages, + per_page=per_page, + total_items=total_items, + filters=filters) diff --git a/front_end/route_vm_management.py b/front_end/route_vm_management.py index ca1eeb7..a4c32bc 100644 --- a/front_end/route_vm_management.py +++ b/front_end/route_vm_management.py @@ -4,6 +4,7 @@ from flask import request, redirect, url_for, session, render_template, flash from datetime import datetime from function_authentication import login_required +from function_api import NotAuthenticated, fetch_history_page from config import API_URL logger = logging.getLogger(__name__) @@ -187,115 +188,72 @@ def return_vm(vmid): @login_required def vm_history(): if request.method == 'POST': - try: - startdate = request.form.get('startdate') - enddate = request.form.get('enddate') - limit = request.form.get('limit', 'null') - - ignore_dates = request.form.get('ignore_dates') - ignore_limit = request.form.get('ignore_limit') - - # Preserve exactly what the operator typed. The values below are - # rewritten into the API's MM/DD/YYYY (or "null") form, and the - # route then redirects, so without this the filter bar would come - # back blank on the following GET. - session['vm_history_filters'] = { - "startdate": startdate or "", - "enddate": enddate or "", - "limit": limit if limit and limit != "null" else "", - "ignore_dates": bool(ignore_dates), - "ignore_limit": bool(ignore_limit), - } - - if ignore_limit: - limit = "null" - - if ignore_dates: - startdate = "null" - enddate = "null" - else: - if startdate: - try: - startdate = datetime.strptime(startdate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid start date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('vm_history')) - else: - startdate = "null" - - if enddate: - try: - enddate = datetime.strptime(enddate, '%Y-%m-%d').strftime('%m/%d/%Y') - except ValueError: - flash("Invalid end date format. Please use 'YYYY-MM-DD'.", "danger") - return redirect(url_for('vm_history')) - else: - enddate = "null" - - data = { - "startdate": startdate, - "enddate": enddate, - "limit": limit if limit else "null" - } - - session['vm_history_data'] = data - - access_token = session.get("access_token") - if not access_token: - return redirect(url_for('login')) - headers = {'Authorization': f'Bearer {access_token}'} + startdate = request.form.get('startdate') + enddate = request.form.get('enddate') + limit = request.form.get('limit', '') - response = requests.post(f"{API_URL}/vms/history", headers=headers, json=data) - response.raise_for_status() - - vm_history = response.json() + ignore_dates = request.form.get('ignore_dates') + ignore_limit = request.form.get('ignore_limit') - if not vm_history: - flash("No VM history records found for the specified criteria.", "info") - else: - flash("VM history retrieved successfully!", "success") + # Validate before storing so a bad date is reported against the form the + # operator is looking at, rather than surfacing later as a query failure. + if not ignore_dates: + for label, value in (("start", startdate), ("end", enddate)): + if not value: + continue + try: + datetime.strptime(value, '%Y-%m-%d') + except ValueError: + flash(f"Invalid {label} date format. Please use 'YYYY-MM-DD'.", "danger") + return redirect(url_for('vm_history')) - session['vm_history'] = vm_history + # Only the criteria are stored. Results are fetched a page at a time on the + # GET, so the session no longer holds an unbounded result set and two + # browser tabs cannot clobber each other's results. + session['vm_history_filters'] = { + "startdate": startdate or "", + "enddate": enddate or "", + "limit": limit if limit and limit != "null" else "", + "ignore_dates": bool(ignore_dates), + "ignore_limit": bool(ignore_limit), + } + session.pop('vm_history', None) + session.pop('vm_history_data', None) - return redirect(url_for('vm_history')) - except requests.exceptions.RequestException as e: - flash("Unable to retrieve VM history. Please try again later.", "danger") - logger.error(f"Error retrieving VM history: {e}") - return redirect(url_for('view_all_vms')) - except Exception as e: - flash("An unexpected error occurred. Please try again later.", "danger") - logger.error(f"Unexpected error in vm_history POST: {e}") - return redirect(url_for('view_all_vms')) - else: - try: - vm_history = session.get('vm_history', []) - filters = session.get('vm_history_filters') or { - "startdate": "", - "enddate": "", - "limit": 20, - "ignore_dates": False, - "ignore_limit": False, - } - page = max(1, int(request.args.get('page', 1))) - per_page = max(1, int(request.args.get('per_page', 10))) + return redirect(url_for('vm_history')) - total_items = len(vm_history) - total_pages = (total_items + per_page - 1) // per_page + filters = session.get('vm_history_filters') or { + "startdate": "", + "enddate": "", + "limit": "", + "ignore_dates": False, + "ignore_limit": False, + } - start = (page - 1) * per_page - end = start + per_page - vm_history_paginated = vm_history[start:end] + try: + page = max(1, int(request.args.get('page', 1))) + except (TypeError, ValueError): + page = 1 + try: + per_page = min(200, max(1, int(request.args.get('per_page', 10)))) + except (TypeError, ValueError): + per_page = 10 - logger.debug(f"Displaying VM history page {page} of {total_pages}, items {start} to {end}") + try: + rows, total_items, total_pages = fetch_history_page( + '/vms/history', filters, page, per_page + ) + except NotAuthenticated: + return redirect(url_for('login')) + except (requests.exceptions.RequestException, ValueError) as e: + flash("Unable to retrieve VM history. Please try again later.", "danger") + logger.error("Error retrieving VM history: %s", e) + return redirect(url_for('view_all_vms')) - return render_template('vm/vm_history.html', - vm_history=vm_history_paginated, - page=page, - total_pages=total_pages, - per_page=per_page, - total_items=total_items, - filters=filters) - except Exception as e: - flash("An unexpected error occurred while displaying VM history.", "danger") - logger.error(f"Unexpected error in vm_history GET: {e}") - return redirect(url_for('view_all_vms')) + return render_template('vm/vm_history.html', + vm_history=rows, + page=page, + total_pages=total_pages, + per_page=per_page, + total_items=total_items, + filters=filters) diff --git a/front_end/tests/conftest.py b/front_end/tests/conftest.py index 4f78e64..40030d0 100644 --- a/front_end/tests/conftest.py +++ b/front_end/tests/conftest.py @@ -70,13 +70,19 @@ def json(self): def raise_for_status(self): if self.status_code >= 400: import requests - raise requests.exceptions.HTTPError(f"status {self.status_code}") + # Real requests attaches the response to the error; code that inspects + # e.response.status_code depends on it, so the stub must match. + raise requests.exceptions.HTTPError(f"status {self.status_code}", response=self) class FakeBrokerApi: def __init__(self): self.posts = [] - self.scaling_log_payload = [dict(LOG_ENTRY, ActivityID=i) for i in range(1, 6)] + # Number of rows the history endpoints report. + self.history_total = 120 + self.scaling_log_payload = [ + dict(LOG_ENTRY, ActivityID=i) for i in range(1, self.history_total + 1) + ] self.host_settings = dict(HOST_SETTINGS) self.apply_result = {"SettingsVersion": HOST_SETTINGS["SettingsVersion"], "TargetCount": 2, "SucceededCount": 2, @@ -85,10 +91,29 @@ def __init__(self): self.raise_get_paths = set() self.raise_post_paths = set() + # Mirrors GetVmSummary for the four seeded VMs in VMS: one Available (on, + # reachable -> ready), one CheckedOut, one Maintenance (off, unreachable), + # one Released. + self.vm_summary = { + "TotalVMs": 4, "Available": 1, "CheckedOut": 1, "Maintenance": 1, + "Released": 1, "PoweredOn": 3, "PoweredOff": 1, "Unreachable": 1, + "Ready": 1, + } + # Set to 404/405 to simulate an API that predates /vms/summary. + self.summary_status = None + + # Set True to simulate an API that predates pagination and answers with a + # bare list regardless of page/per_page. + self.legacy_history = False + def get(self, url, **kwargs): import requests if any(url.endswith(path) for path in self.raise_get_paths): raise requests.exceptions.RequestException("broker unavailable") + if url.endswith("/vms/summary"): + if self.summary_status is not None: + return FakeResponse({"error": "not found"}, status_code=self.summary_status) + return FakeResponse(self.vm_summary) if url.endswith("/hosts/settings"): return FakeResponse(self.host_settings) if re.search(r"/vms/\d+$", url): @@ -104,7 +129,8 @@ def get(self, url, **kwargs): def post(self, url, **kwargs): import requests - self.posts.append({"url": url, "json": kwargs.get("json")}) + params = kwargs.get("params") or {} + self.posts.append({"url": url, "json": kwargs.get("json"), "params": params}) if any(url.endswith(path) for path in self.raise_post_paths): raise requests.exceptions.RequestException("broker unavailable") if url.endswith("/hosts/settings/update"): @@ -112,17 +138,41 @@ def post(self, url, **kwargs): if url.endswith("/hosts/settings/apply"): return FakeResponse(self.apply_result) if url.endswith("/scaling/log"): - return FakeResponse(self.scaling_log_payload) + return self._history(self.scaling_log_payload, params) if url.endswith("/scaling/rules/history"): - return FakeResponse([dict(RULE, RuleID=i, SysStartTime="2026-08-01 10:00:00", SysEndTime=None) for i in range(1, 6)]) + rows = [dict(RULE, RuleID=i, SysStartTime="2026-08-01 10:00:00", SysEndTime=None) + for i in range(1, self.history_total + 1)] + return self._history(rows, params) if url.endswith("/vms/history"): - return FakeResponse(VMS) + rows = [dict(VMS[i % len(VMS)], VMID=i + 1) for i in range(self.history_total)] + return self._history(rows, params) if url.endswith("/scaling/rules/create"): return FakeResponse({"RuleID": 1}, status_code=201) if url.endswith("/vms/checkout"): return FakeResponse(VMS[0]) return FakeResponse({}) + def _history(self, rows, params): + """Mirror the API: a paged envelope when page/per_page are supplied, a bare + list otherwise.""" + if not isinstance(rows, list): + return FakeResponse(rows) + + if self.legacy_history or not params: + return FakeResponse(rows) + + page = int(params.get("page", 1) or 1) + per_page = int(params.get("per_page", 50) or 50) + start = (page - 1) * per_page + total = len(rows) + return FakeResponse({ + "items": rows[start:start + per_page], + "page": page, + "per_page": per_page, + "total": total, + "total_pages": (total + per_page - 1) // per_page if per_page else 0, + }) + @pytest.fixture(scope="session") def app(): @@ -162,13 +212,6 @@ def sign_in(client): sess["token_expiry"] = expiry -def seed_histories(client, count=120): - with client.session_transaction() as sess: - sess["vm_history"] = [dict(VMS[i % 4], VMID=i + 1) for i in range(count)] - sess["scaling_activity_log"] = [dict(LOG_ENTRY, ActivityID=i + 1) for i in range(count)] - sess["scaling_rules_history"] = [dict(RULE, RuleID=i + 1) for i in range(count)] - - def csrf_token(html): match = re.search(r'name="csrf_token"[^>]*value="([^"]+)"', html) assert match, "expected CSRF token in rendered form" diff --git a/front_end/tests/test_ui_regressions.py b/front_end/tests/test_ui_regressions.py index 43e56d8..a1ddf64 100644 --- a/front_end/tests/test_ui_regressions.py +++ b/front_end/tests/test_ui_regressions.py @@ -3,7 +3,7 @@ import pytest -from conftest import VMS, assert_checkbox_checked, assert_form_value, csrf_token, row_for_host, seed_histories +from conftest import VMS, assert_checkbox_checked, assert_form_value, csrf_token, row_for_host AUTHENTICATED_GET_ROUTES = [ @@ -105,8 +105,12 @@ def test_ignore_filter_checkboxes_survive_post_redirect_get(signed_in_client, br assert_form_value(body, "limit", "42") assert_checkbox_checked(body, "ignore_dates") assert_checkbox_checked(body, "ignore_limit") - # Whatever was typed, the API must still be told to ignore both. - assert broker_api.posts[-1]["json"] == {"startdate": "null", "enddate": "null", "limit": "null"} + # The ignore flags now mean "omit the filter" rather than sending the + # stringly-typed "null" sentinel that the API had to special-case. + sent = broker_api.posts[-1]["json"] + assert "startdate" not in sent + assert "enddate" not in sent + assert "limit" not in sent @pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) @@ -123,12 +127,58 @@ def test_ignore_flags_without_values_do_not_break(signed_in_client, broker_api, body = response.get_data(as_text=True) assert_checkbox_checked(body, "ignore_dates") assert_checkbox_checked(body, "ignore_limit") - assert broker_api.posts[-1]["json"] == {"startdate": "null", "enddate": "null", "limit": "null"} + sent = broker_api.posts[-1]["json"] + assert "startdate" not in sent + assert "enddate" not in sent + assert "limit" not in sent @pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) -def test_pagination_is_windowed_for_many_pages(signed_in_client, path): - seed_histories(signed_in_client, count=120) +def test_filters_are_sent_to_the_api_in_the_expected_format(signed_in_client, broker_api, path): + """The operator types YYYY-MM-DD; the stored procedures expect MM/DD/YYYY.""" + html = signed_in_client.get(path).get_data(as_text=True) + signed_in_client.post(path, data={ + "csrf_token": csrf_token(html), + "startdate": "2026-01-15", + "enddate": "2026-02-20", + "limit": "37", + }, follow_redirects=True) + + sent = broker_api.posts[-1]["json"] + assert sent["startdate"] == "01/15/2026" + assert sent["enddate"] == "02/20/2026" + assert sent["limit"] == 37 # a real int, not the string "37" + + +@pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) +def test_history_uses_server_side_pagination(signed_in_client, broker_api, path): + """Pages come from the API rather than a whole result set cached in the session.""" + response = signed_in_client.get(f"{path}?page=3&per_page=10") + assert response.status_code == 200 + + params = broker_api.posts[-1]["params"] + assert params["page"] == 3 + assert params["per_page"] == 10 + + with signed_in_client.session_transaction() as session: + # The old implementation stashed every row in the session. + assert "vm_history" not in session + assert "scaling_activity_log" not in session + assert "scaling_rules_history" not in session + + +@pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) +def test_history_falls_back_when_api_predates_pagination(signed_in_client, broker_api, path): + """During a rolling deploy the API may still answer with a bare list.""" + broker_api.legacy_history = True + response = signed_in_client.get(f"{path}?page=1&per_page=10") + assert response.status_code == 200 + assert "Unable to retrieve" not in response.get_data(as_text=True) + + +@pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) +def test_pagination_is_windowed_for_many_pages(signed_in_client, broker_api, path): + broker_api.history_total = 120 response = signed_in_client.get(f"{path}?page=6&per_page=10") assert response.status_code == 200 html = response.get_data(as_text=True) @@ -140,7 +190,6 @@ def test_pagination_is_windowed_for_many_pages(signed_in_client, path): @pytest.mark.parametrize("path", ["/vms/history", "/scaling/log", "/scaling/rules/history"]) @pytest.mark.parametrize("query", ["page=abc", "page=0", "page=999999", "per_page=-3"]) def test_hostile_pagination_query_strings_do_not_500(signed_in_client, path, query): - seed_histories(signed_in_client, count=20) response = signed_in_client.get(f"{path}?{query}", follow_redirects=True) assert response.status_code < 500 @@ -186,6 +235,51 @@ def test_dashboard_handles_non_list_activity_log(signed_in_client, broker_api): def test_dashboard_degrades_when_vm_api_is_unavailable(signed_in_client, broker_api): + broker_api.raise_get_paths.add("/vms/summary") + broker_api.raise_get_paths.add("/vms") + response = signed_in_client.get("/") + assert response.status_code == 200 + assert "Pool data unavailable" in response.get_data(as_text=True) + + +def test_dashboard_uses_the_summary_endpoint(signed_in_client, broker_api): + """The dashboard must not pull the whole VM list just to count it.""" + broker_api.vm_summary = { + "TotalVMs": 9, "Available": 4, "CheckedOut": 3, "Maintenance": 1, + "Released": 1, "PoweredOn": 7, "PoweredOff": 2, "Unreachable": 2, + "Ready": 3, + } + response = signed_in_client.get("/") + assert response.status_code == 200 + + html = response.get_data(as_text=True) + assert ">9<" in html.replace(" ", "").replace("\n", "") # total + assert "Pool overview" in html + # 3 of 9 checked out + assert "33% of the pool in use" in html + + +def test_dashboard_falls_back_when_api_predates_the_summary_endpoint(signed_in_client, broker_api): + """During a rolling deploy the portal can be newer than the API. + + An older API does not 404 on /api/vms/summary -- Werkzeug matches it against the + older /api/vms/ rule, which fails converting 'summary' to an int and + returns 500. The fallback must handle that, not just a clean 404. + """ + broker_api.summary_status = 500 + response = signed_in_client.get("/") + assert response.status_code == 200 + + html = response.get_data(as_text=True) + assert "Pool data unavailable" not in html + assert "Pool overview" in html + # Counted client-side from the four seeded VMs. + assert ">4<" in html.replace(" ", "").replace("\n", "") + + +def test_dashboard_still_reports_an_outage_when_both_paths_fail(signed_in_client, broker_api): + """The fallback must not mask a genuine broker outage.""" + broker_api.summary_status = 500 broker_api.raise_get_paths.add("/vms") response = signed_in_client.get("/") assert response.status_code == 200 diff --git a/sql_queries/034_create_procedure-GetVmSummary.sql b/sql_queries/034_create_procedure-GetVmSummary.sql new file mode 100644 index 0000000..19a8fed --- /dev/null +++ b/sql_queries/034_create_procedure-GetVmSummary.sql @@ -0,0 +1,21 @@ +-- Adds a dashboard summary aggregate for the VM pool. +-- +-- This lives after the VM table and later VM column additions so fresh deployments validate +-- every referenced column before the procedure is created. + +CREATE PROCEDURE [dbo].[GetVmSummary] +AS +BEGIN + SELECT + COUNT(*) AS TotalVMs, + COALESCE(SUM(CASE WHEN VmStatus = 'Available' THEN 1 ELSE 0 END), 0) AS Available, + COALESCE(SUM(CASE WHEN VmStatus = 'CheckedOut' THEN 1 ELSE 0 END), 0) AS CheckedOut, + COALESCE(SUM(CASE WHEN VmStatus = 'Maintenance' THEN 1 ELSE 0 END), 0) AS Maintenance, + COALESCE(SUM(CASE WHEN VmStatus = 'Released' THEN 1 ELSE 0 END), 0) AS Released, + COALESCE(SUM(CASE WHEN PowerState = 'On' THEN 1 ELSE 0 END), 0) AS PoweredOn, + COALESCE(SUM(CASE WHEN PowerState = 'Off' THEN 1 ELSE 0 END), 0) AS PoweredOff, + COALESCE(SUM(CASE WHEN NetworkStatus = 'Unreachable' THEN 1 ELSE 0 END), 0) AS Unreachable, + COALESCE(SUM(CASE WHEN VmStatus = 'Available' AND PowerState = 'On' AND NetworkStatus = 'Reachable' THEN 1 ELSE 0 END), 0) AS Ready + FROM dbo.VirtualMachines; +END +GO diff --git a/sql_queries/035_alter_procedure-GetScalingActivityLog.sql b/sql_queries/035_alter_procedure-GetScalingActivityLog.sql new file mode 100644 index 0000000..d2be4cc --- /dev/null +++ b/sql_queries/035_alter_procedure-GetScalingActivityLog.sql @@ -0,0 +1,32 @@ +-- Re-defines scaling activity history date filtering to parse MM/DD/YYYY explicitly. +-- +-- This must be separate from 013 because deployments run files in numeric order and reruns +-- are applied as CREATE OR ALTER PROCEDURE by the bootstrap script. + +CREATE PROCEDURE [dbo].[GetScalingActivityLog] + @StartDate NVARCHAR(10) = NULL, + @EndDate NVARCHAR(10) = NULL, + @Limit INT = NULL +AS +BEGIN + DECLARE @ConvertedStartDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@StartDate)), ''), 101); + DECLARE @ConvertedEndDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@EndDate)), ''), 101); + DECLARE @EffectiveLimit INT = CASE WHEN @Limit IS NULL OR @Limit < 1 THEN 1000 ELSE @Limit END; + + SELECT TOP (@EffectiveLimit) + ActivityID, + CheckTimestamp, + CurrentRunningVMs, + CurrentInUseVMs, + ActionTaken, + VMsPoweredOn, + VMsPoweredOff, + NewTotalVMs, + Outcome, + Notes + FROM dbo.VmScalingActivityLog + WHERE (@ConvertedStartDate IS NULL OR CheckTimestamp >= @ConvertedStartDate) + AND (@ConvertedEndDate IS NULL OR CheckTimestamp <= @ConvertedEndDate) + ORDER BY CheckTimestamp DESC, ActivityID DESC; +END +GO diff --git a/sql_queries/036_alter_procedure-GetVmScalingRulesHistory.sql b/sql_queries/036_alter_procedure-GetVmScalingRulesHistory.sql new file mode 100644 index 0000000..b874f21 --- /dev/null +++ b/sql_queries/036_alter_procedure-GetVmScalingRulesHistory.sql @@ -0,0 +1,34 @@ +-- Re-defines scaling rule history date filtering to parse MM/DD/YYYY explicitly. +-- +-- This must be separate from 021 because deployments run files in numeric order and reruns +-- are applied as CREATE OR ALTER PROCEDURE by the bootstrap script. + +CREATE PROCEDURE [dbo].[GetVmScalingRulesHistory] + @StartDate NVARCHAR(10) = NULL, + @EndDate NVARCHAR(10) = NULL, + @Limit INT = 100 +AS +BEGIN + DECLARE @ConvertedStartDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@StartDate)), ''), 101); + DECLARE @ConvertedEndDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@EndDate)), ''), 101); + DECLARE @EffectiveLimit INT = CASE WHEN @Limit IS NULL OR @Limit < 1 THEN 100 ELSE @Limit END; + + SELECT + RuleID, + MinVMs, + MaxVMs, + ScaleUpRatio, + ScaleUpIncrement, + ScaleDownRatio, + ScaleDownIncrement, + LastChecked, + SysStartTime, + SysEndTime + FROM dbo.VmScalingRulesHistory + WHERE (@ConvertedStartDate IS NULL OR SysStartTime >= @ConvertedStartDate) + AND (@ConvertedEndDate IS NULL OR SysEndTime <= @ConvertedEndDate) + ORDER BY SysStartTime DESC, SysEndTime DESC, RuleID DESC + OFFSET 0 ROWS + FETCH NEXT @EffectiveLimit ROWS ONLY; +END +GO diff --git a/sql_queries/037_create_procedure-GetVmHistoryPaged.sql b/sql_queries/037_create_procedure-GetVmHistoryPaged.sql new file mode 100644 index 0000000..3214691 --- /dev/null +++ b/sql_queries/037_create_procedure-GetVmHistoryPaged.sql @@ -0,0 +1,66 @@ +-- Adds a paged VM history reader with an in-band TotalCount for API pagination. +-- +-- This lives after 029 because it references the settings tracking columns that are added to +-- dbo.VirtualMachines and propagated to dbo.VirtualMachinesHistory by system versioning. + +CREATE PROCEDURE [dbo].[GetVmHistoryPaged] + @StartDate NVARCHAR(10) = NULL, + @EndDate NVARCHAR(10) = NULL, + @Offset INT = 0, + @PageSize INT = 50 +AS +BEGIN + -- Paging determines result size; there is intentionally no @Limit parameter here. + -- NULL dates, empty strings, or malformed MM/DD/YYYY strings become no filter. + DECLARE @ConvertedStartDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@StartDate)), ''), 101); + DECLARE @ConvertedEndDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@EndDate)), ''), 101); + DECLARE @SafeOffset INT = CASE WHEN @Offset IS NULL OR @Offset < 0 THEN 0 ELSE @Offset END; + DECLARE @SafePageSize INT = CASE WHEN @PageSize IS NULL OR @PageSize < 1 OR @PageSize > 200 THEN 50 ELSE @PageSize END; + + WITH MatchingRows AS ( + SELECT + VMID, + Hostname, + IPAddress, + PowerState, + NetworkStatus, + VmStatus, + Username, + AvdHost, + LeaseId, + CreateDate, + LastUpdateDate, + Description, + SysStartTime, + SysEndTime, + SettingsVersion, + SettingsAppliedDate, + COUNT(*) OVER () AS TotalCount + FROM dbo.VirtualMachinesHistory + WHERE (@ConvertedStartDate IS NULL OR SysStartTime >= @ConvertedStartDate) + AND (@ConvertedEndDate IS NULL OR SysEndTime <= @ConvertedEndDate) + ) + SELECT + VMID, + Hostname, + IPAddress, + PowerState, + NetworkStatus, + VmStatus, + Username, + AvdHost, + LeaseId, + CreateDate, + LastUpdateDate, + Description, + SysStartTime, + SysEndTime, + SettingsVersion, + SettingsAppliedDate, + TotalCount + FROM MatchingRows + ORDER BY SysStartTime DESC, SysEndTime DESC, VMID DESC + OFFSET @SafeOffset ROWS + FETCH NEXT @SafePageSize ROWS ONLY; +END +GO diff --git a/sql_queries/038_create_procedure-GetScalingActivityLogPaged.sql b/sql_queries/038_create_procedure-GetScalingActivityLogPaged.sql new file mode 100644 index 0000000..09b4cf1 --- /dev/null +++ b/sql_queries/038_create_procedure-GetScalingActivityLogPaged.sql @@ -0,0 +1,54 @@ +-- Adds a paged scaling activity reader with an in-band TotalCount for API pagination. +-- +-- This lives after 002 creates dbo.VmScalingActivityLog so fresh deployments validate the +-- referenced columns before the procedure is created. + +CREATE PROCEDURE [dbo].[GetScalingActivityLogPaged] + @StartDate NVARCHAR(10) = NULL, + @EndDate NVARCHAR(10) = NULL, + @Offset INT = 0, + @PageSize INT = 50 +AS +BEGIN + -- Paging determines result size; there is intentionally no @Limit parameter here. + -- NULL dates, empty strings, or malformed MM/DD/YYYY strings become no filter. + DECLARE @ConvertedStartDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@StartDate)), ''), 101); + DECLARE @ConvertedEndDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@EndDate)), ''), 101); + DECLARE @SafeOffset INT = CASE WHEN @Offset IS NULL OR @Offset < 0 THEN 0 ELSE @Offset END; + DECLARE @SafePageSize INT = CASE WHEN @PageSize IS NULL OR @PageSize < 1 OR @PageSize > 200 THEN 50 ELSE @PageSize END; + + WITH MatchingRows AS ( + SELECT + ActivityID, + CheckTimestamp, + CurrentRunningVMs, + CurrentInUseVMs, + ActionTaken, + VMsPoweredOn, + VMsPoweredOff, + NewTotalVMs, + Outcome, + Notes, + COUNT(*) OVER () AS TotalCount + FROM dbo.VmScalingActivityLog + WHERE (@ConvertedStartDate IS NULL OR CheckTimestamp >= @ConvertedStartDate) + AND (@ConvertedEndDate IS NULL OR CheckTimestamp <= @ConvertedEndDate) + ) + SELECT + ActivityID, + CheckTimestamp, + CurrentRunningVMs, + CurrentInUseVMs, + ActionTaken, + VMsPoweredOn, + VMsPoweredOff, + NewTotalVMs, + Outcome, + Notes, + TotalCount + FROM MatchingRows + ORDER BY CheckTimestamp DESC, ActivityID DESC + OFFSET @SafeOffset ROWS + FETCH NEXT @SafePageSize ROWS ONLY; +END +GO diff --git a/sql_queries/039_create_procedure-GetVmScalingRulesHistoryPaged.sql b/sql_queries/039_create_procedure-GetVmScalingRulesHistoryPaged.sql new file mode 100644 index 0000000..1266c5d --- /dev/null +++ b/sql_queries/039_create_procedure-GetVmScalingRulesHistoryPaged.sql @@ -0,0 +1,54 @@ +-- Adds a paged scaling rule history reader with an in-band TotalCount for API pagination. +-- +-- This lives after 001 creates the system-versioned rules table and its history table so fresh +-- deployments validate the referenced history columns before the procedure is created. + +CREATE PROCEDURE [dbo].[GetVmScalingRulesHistoryPaged] + @StartDate NVARCHAR(10) = NULL, + @EndDate NVARCHAR(10) = NULL, + @Offset INT = 0, + @PageSize INT = 50 +AS +BEGIN + -- Paging determines result size; there is intentionally no @Limit parameter here. + -- NULL dates, empty strings, or malformed MM/DD/YYYY strings become no filter. + DECLARE @ConvertedStartDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@StartDate)), ''), 101); + DECLARE @ConvertedEndDate DATETIME2 = TRY_CONVERT(DATETIME2, NULLIF(LTRIM(RTRIM(@EndDate)), ''), 101); + DECLARE @SafeOffset INT = CASE WHEN @Offset IS NULL OR @Offset < 0 THEN 0 ELSE @Offset END; + DECLARE @SafePageSize INT = CASE WHEN @PageSize IS NULL OR @PageSize < 1 OR @PageSize > 200 THEN 50 ELSE @PageSize END; + + WITH MatchingRows AS ( + SELECT + RuleID, + MinVMs, + MaxVMs, + ScaleUpRatio, + ScaleUpIncrement, + ScaleDownRatio, + ScaleDownIncrement, + LastChecked, + SysStartTime, + SysEndTime, + COUNT(*) OVER () AS TotalCount + FROM dbo.VmScalingRulesHistory + WHERE (@ConvertedStartDate IS NULL OR SysStartTime >= @ConvertedStartDate) + AND (@ConvertedEndDate IS NULL OR SysEndTime <= @ConvertedEndDate) + ) + SELECT + RuleID, + MinVMs, + MaxVMs, + ScaleUpRatio, + ScaleUpIncrement, + ScaleDownRatio, + ScaleDownIncrement, + LastChecked, + SysStartTime, + SysEndTime, + TotalCount + FROM MatchingRows + ORDER BY SysStartTime DESC, SysEndTime DESC, RuleID DESC + OFFSET @SafeOffset ROWS + FETCH NEXT @SafePageSize ROWS ONLY; +END +GO diff --git a/sql_queries/README.md b/sql_queries/README.md index afa6c95..416432a 100644 --- a/sql_queries/README.md +++ b/sql_queries/README.md @@ -86,9 +86,17 @@ The scripts do not contain `USE ` statements. The target database come - `031_create_procedure-UpdateLinuxHostSettings.sql`: updates the profile, bumping `SettingsVersion` only when a value actually changed - `032_create_procedure-RecordHostSettingsApplied.sql`: records the settings version a host has applied - `033_alter_procedure-GetVms.sql`: redefines `dbo.GetVms` to also return `SettingsVersion` and `SettingsAppliedDate` +- `034_create_procedure-GetVmSummary.sql`: returns one aggregate row for dashboard VM counters +- `035_alter_procedure-GetScalingActivityLog.sql`: redefines `dbo.GetScalingActivityLog` to parse optional `MM/DD/YYYY` date strings explicitly +- `036_alter_procedure-GetVmScalingRulesHistory.sql`: redefines `dbo.GetVmScalingRulesHistory` to parse optional `MM/DD/YYYY` date strings explicitly +- `037_create_procedure-GetVmHistoryPaged.sql`: returns paged VM history rows with `TotalCount` +- `038_create_procedure-GetScalingActivityLogPaged.sql`: returns paged scaling activity rows with `TotalCount` +- `039_create_procedure-GetVmScalingRulesHistoryPaged.sql`: returns paged scaling rule history rows with `TotalCount` `033` exists as its own file rather than being folded into `014` because `014` runs before `029` adds those columns, and SQL Server validates column references against existing tables when a procedure is created. +`034` through `039` are also additive/redefinition files so fresh deployments keep procedure validation in numeric schema order. The paged history procedures intentionally omit the legacy `@Limit` parameter: `@Offset` and `@PageSize` are the only result-size controls, and `NULL`/empty/malformed date strings are treated as no date filter. + ## Current Runtime Expectations The current code and deployment flow depend on the following SQL objects being present: @@ -216,7 +224,11 @@ WHERE name IN ( 'RegisterLinuxHostVm', 'GetLinuxHostSettings', 'UpdateLinuxHostSettings', - 'RecordHostSettingsApplied' + 'RecordHostSettingsApplied', + 'GetVmSummary', + 'GetVmHistoryPaged', + 'GetScalingActivityLogPaged', + 'GetVmScalingRulesHistoryPaged' ) ORDER BY name; ```