Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 5, 2025

What do these changes do?

  • 🎨 Reduces the access to the database per user to one request every 30 mins for authentication
  • ♻️ Upgrades auth repository funtions to asyncpg

Related issue/s

How to test

  • We generate a high volume of parallel requests to the same user account to validate the effectiveness of the caching mechanism.
  • By disabling the cache in the code (see w/o cache), we can directly observe the impact of database access on performance.
  • 🚨 Notably, performance degrades significantly with asyncpg, which raises concerns. This may suggest that there are still inefficiencies in the way we are using the library or misconfigurations in how we're using the new engine!!

image

Dev-ops

@pcrespov pcrespov added this to the Bazinga! milestone Jun 5, 2025
@pcrespov pcrespov self-assigned this Jun 5, 2025
@pcrespov pcrespov added a:webserver webserver's codebase. Assigning the area is particularly useful for bugs release Preparation for pre-release/release labels Jun 5, 2025
@pcrespov pcrespov changed the title Is7779/cache auth 🎨 webserver: cache authenticated calls Jun 5, 2025
@pcrespov pcrespov marked this pull request as ready for review June 5, 2025 09:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves authentication performance and reliability by reducing database requests via caching and migrating to the asyncpg engine for asynchronous database access. Key changes include:

  • Replacing the legacy db engine with the new asyncpg engine in authentication functions.
  • Increasing the caching time-to-live for burst requests from 5 seconds to 30 minutes.
  • Adjusting database connection contexts and query methods to work with the AsyncEngine.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
services/web/server/src/simcore_service_webserver/security/_authz_policy.py Updated to use asyncpg engine and increased cache TTL for authenticated calls.
services/web/server/src/simcore_service_webserver/security/_authz_db.py Migrated async database calls from aiopg to SQLAlchemy's AsyncEngine and adjusted query methods accordingly.
Comments suppressed due to low confidence (2)

services/web/server/src/simcore_service_webserver/security/_authz_policy.py:31

  • The cache TTL has been increased significantly from 5 seconds to 30 minutes. Please ensure that this duration aligns with the intended balance between performance improvements and authentication freshness.
_AUTHZ_BURST_CACHE_TTL: Final = (30 * _MINUTE)

services/web/server/src/simcore_service_webserver/security/_authz_policy.py:126

  • [nitpick] The debug log for invalid identity or permission has been removed. Consider documenting the rationale for the omission or adding an alternative diagnostic mechanism, if deemed necessary, to aid future troubleshooting.
if identity is None or permission is None:

@codecov
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.34%. Comparing base (ee22f37) to head (87419e6).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ee22f37) and HEAD (87419e6). Click for more details.

HEAD has 29 uploads less than BASE
Flag BASE (ee22f37) HEAD (87419e6)
unittests 32 3
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7826      +/-   ##
==========================================
- Coverage   86.72%   79.34%   -7.38%     
==========================================
  Files        1851      703    -1148     
  Lines       71967    33582   -38385     
  Branches     1218      176    -1042     
==========================================
- Hits        62410    26646   -35764     
+ Misses       9216     6878    -2338     
+ Partials      341       58     -283     
Flag Coverage Δ
integrationtests 64.18% <64.70%> (-0.02%) ⬇️
unittests 81.44% <76.47%> (-5.04%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.16% <ø> (-7.89%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 77.50% <ø> (-13.46%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 88.34% <ø> (-1.80%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 78.75% <76.47%> (-4.99%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee22f37...87419e6. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov requested a review from GitHK June 5, 2025 09:56
Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice, as you already said I can confirm this is not a security issue at all :)

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

Copy link
Contributor

@giancarloromeo giancarloromeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@pcrespov pcrespov force-pushed the is7779/cache-auth branch 2 times, most recently from e04eb7f to 1da293b Compare June 5, 2025 12:06
@pcrespov pcrespov enabled auto-merge (squash) June 5, 2025 12:08
@pcrespov pcrespov force-pushed the is7779/cache-auth branch from 1da293b to e43803d Compare June 5, 2025 12:56
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice! thanks a lot
how many connections does the asyncpg engine create?

@pcrespov pcrespov force-pushed the is7779/cache-auth branch from e43803d to 87419e6 Compare June 5, 2025 13:56
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 5, 2025

@pcrespov pcrespov merged commit 0a8019d into ITISFoundation:master Jun 5, 2025
90 of 95 checks passed
@pcrespov pcrespov deleted the is7779/cache-auth branch June 5, 2025 16:22
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 6, 2025
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:webserver webserver's codebase. Assigning the area is particularly useful for bugs release Preparation for pre-release/release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webserver: cache authenticated calls

5 participants