From b4912be57c42685851541c17f67831bd297b053d Mon Sep 17 00:00:00 2001 From: Matt McFarland Date: Tue, 25 Oct 2022 10:44:07 -0400 Subject: [PATCH] Fix middleware runtime error and release prep fixes (#130) * Temporarily use fork for starlette 0.21 release The 0.21 release resolves a frequent error on our fastapi version. See: https://github.com/encode/starlette/pull/1710 https://github.com/encode/starlette/pull/1715 * Disable FTP as function app deploy option Security controls * Trace request attributes before invoking middleware If an exception is raised in subsequent middlewares, added trace attributes will still be logged to Azure. This allows us to find requests that fail in the logs. * Make config cache thread safe cachetools cache is not thread safe and there were frequent exceptions logged indicating that cache updates during async calls were failing with key errors similar to those described in: https://github.com/tkem/cachetools/issues/80 Add a lock per table instance synchronizes cache updates across threads in. * Lint * Changelog --- CHANGELOG.md | 3 +++ deployment/terraform/resources/functions.tf | 1 + pccommon/pccommon/tables.py | 4 +++- pccommon/pccommon/tracing.py | 13 ++++++++----- pccommon/setup.py | 7 ++++++- pcfuncs/Dockerfile | 3 +++ pcfuncs/animation/animation.py | 1 - pctiler/Dockerfile | 3 +++ pctiler/pctiler/main.py | 2 +- requirements-dev.txt | 4 ++-- 10 files changed, 30 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0baf084a..8500ab1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Configure CORS correctly at the nginx-ingress level [#127](https://github.com/microsoft/planetary-computer-apis/pull/127) +- Make config cache access thread safe to prevent key errors [#130](https://github.com/microsoft/planetary-computer-apis/pull/130) +- Upgrade starlette (via fork) to prevent middleware errors [#130](https://github.com/microsoft/planetary-computer-apis/pull/130) +- Better request tracing [#130](https://github.com/microsoft/planetary-computer-apis/pull/130) ## [2022.3.0] diff --git a/deployment/terraform/resources/functions.tf b/deployment/terraform/resources/functions.tf index 1f5e0f07..d1c750b9 100644 --- a/deployment/terraform/resources/functions.tf +++ b/deployment/terraform/resources/functions.tf @@ -48,6 +48,7 @@ resource "azurerm_function_app" "pcfuncs" { site_config { linux_fx_version = "PYTHON|3.8" use_32_bit_worker_process = false + ftps_state = "Disabled" cors { allowed_origins = ["*"] diff --git a/pccommon/pccommon/tables.py b/pccommon/pccommon/tables.py index 5605f0b0..33f789a9 100644 --- a/pccommon/pccommon/tables.py +++ b/pccommon/pccommon/tables.py @@ -1,3 +1,4 @@ +from threading import Lock from typing import ( Any, Callable, @@ -58,6 +59,7 @@ def __init__( self._service_client: Optional[TableServiceClient] = None self._table_client: Optional[TableClient] = None self._cache: Cache = TTLCache(maxsize=1024, ttl=ttl or DEFAULT_TTL) + self._cache_lock: Lock = Lock() def _ensure_table_client(self) -> None: if not self._table_client: @@ -187,7 +189,7 @@ def update(self, partition_key: str, row_key: str, entity: M) -> None: } ) - @cachedmethod(cache=lambda self: self._cache) + @cachedmethod(cache=lambda self: self._cache, lock=lambda self: self._cache_lock) def get(self, partition_key: str, row_key: str) -> Optional[M]: with self as table_client: try: diff --git a/pccommon/pccommon/tracing.py b/pccommon/pccommon/tracing.py index 948f1415..80aabc1e 100644 --- a/pccommon/pccommon/tracing.py +++ b/pccommon/pccommon/tracing.py @@ -62,8 +62,7 @@ async def trace_request( # are slow. request.state.parent_span = span - response = await call_next(request) - + # Add request dimensions to the trace prior to calling the next middleware tracer.add_attribute_to_current_span( attribute_key="ref_id", attribute_value=request.headers.get(X_AZURE_REF), @@ -76,9 +75,6 @@ async def trace_request( attribute_key="request_ip", attribute_value=get_request_ip(request), ) - tracer.add_attribute_to_current_span( - attribute_key=HTTP_STATUS_CODE, attribute_value=response.status_code - ) tracer.add_attribute_to_current_span( attribute_key=HTTP_METHOD, attribute_value=str(request.method) ) @@ -103,6 +99,13 @@ async def trace_request( attribute_key="item", attribute_value=item_id ) + # Call next middleware + response = await call_next(request) + + # Include response dimensions in the trace + tracer.add_attribute_to_current_span( + attribute_key=HTTP_STATUS_CODE, attribute_value=response.status_code + ) return response else: return await call_next(request) diff --git a/pccommon/setup.py b/pccommon/setup.py index cb95fc2b..86e88a39 100644 --- a/pccommon/setup.py +++ b/pccommon/setup.py @@ -4,7 +4,12 @@ # Runtime requirements. inst_reqs = [ - "fastapi>=0.75.2", + # ---> + # TODO: restore fastapi release install after starlette dep upgraded to >= 0.21.0 + # "fastapi>=0.75.2", + "fastapi @ git+https://github.com/mmcfarland/fastapi/@982e7caf086bffeace8554da6d69e5f3082f14a3#egg=fastapi", + "starlette>=0.21.0,<0.22.0", + # <--- "opencensus-ext-azure==1.0.8", "opencensus-ext-logging==0.1.0", "orjson==3.5.2", diff --git a/pcfuncs/Dockerfile b/pcfuncs/Dockerfile index 37d1e619..9ffc6bc0 100644 --- a/pcfuncs/Dockerfile +++ b/pcfuncs/Dockerfile @@ -1,5 +1,8 @@ FROM mcr.microsoft.com/azure-functions/python:4-python3.8 +# git required for pip installs from git +RUN apt update && apt install -y git + ENV AzureWebJobsScriptRoot=/home/site/wwwroot \ AzureFunctionsJobHost__Logging__Console__IsEnabled=true diff --git a/pcfuncs/animation/animation.py b/pcfuncs/animation/animation.py index 3c158304..6670cc70 100644 --- a/pcfuncs/animation/animation.py +++ b/pcfuncs/animation/animation.py @@ -14,7 +14,6 @@ from PIL.Image import Image as PILImage from pyproj import CRS - from .constants import MAX_TILE_COUNT from .settings import AnimationSettings diff --git a/pctiler/Dockerfile b/pctiler/Dockerfile index e2e4bc01..e418eb8a 100644 --- a/pctiler/Dockerfile +++ b/pctiler/Dockerfile @@ -1,5 +1,8 @@ FROM python:3.9-slim +# git required for pip installs from git +RUN apt update && apt install -y git + # The devops Personal Access Token for accessing # Azure Artifacts. Note: This will be visible as # plain text in the docker build logs. Only use your diff --git a/pctiler/pctiler/main.py b/pctiler/pctiler/main.py index b6a1e59d..bb9cbdf7 100755 --- a/pctiler/pctiler/main.py +++ b/pctiler/pctiler/main.py @@ -7,7 +7,6 @@ from fastapi.openapi.utils import get_openapi from morecantile.defaults import tms as defaultTileMatrices from morecantile.models import TileMatrixSet -from pccommon.constants import X_REQUEST_ENTITY from starlette.middleware.cors import CORSMiddleware from titiler.core.errors import DEFAULT_STATUS_CODES, add_exception_handlers from titiler.core.middleware import ( @@ -18,6 +17,7 @@ from titiler.mosaic.errors import MOSAIC_STATUS_CODES from titiler.pgstac.db import close_db_connection, connect_to_db +from pccommon.constants import X_REQUEST_ENTITY from pccommon.logging import ServiceName, init_logging from pccommon.middleware import ( RequestTracingMiddleware, diff --git a/requirements-dev.txt b/requirements-dev.txt index 475c0c79..dc30db33 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -6,8 +6,8 @@ openapi-spec-validator==0.3.0 cachetools<=4.2. pytest==7.* pytest-asyncio==0.18.* -httpx==0.19.0 +httpx>=0.22.0 # Mypy types -types-cachetools \ No newline at end of file +types-cachetools