feat(admin-auth): Add TTL cache for admin roles #4358

davidtsuk · 2023-06-16T16:04:49Z

Currently, we set up a user's roles every time we make a request which is inefficient as it requires reading from a file and making multiple API requests to google. In order to improve performance, I've added a TTL cache with a 10 min TTL. Since it's unlikely for roles and groups to change, we should consider increasing this TTL in the future.

codecov · 2023-06-16T19:13:57Z

Codecov Report

Patch coverage: 54.01% and project coverage change: -0.33 ⚠️

Comparison is base (7b480a2) 91.05% compared to head (b070fe2) 90.72%.

❗ Current head b070fe2 differs from pull request most recent head f78fa66. Consider uploading reports for the commit f78fa66 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4358      +/-   ##
==========================================
- Coverage   91.05%   90.72%   -0.33%     
==========================================
  Files         793      797       +4     
  Lines       38886    39254     +368     
  Branches      226      245      +19     
==========================================
+ Hits        35408    35614     +206     
- Misses       3448     3608     +160     
- Partials       30       32       +2

Impacted Files	Coverage Δ
setup.py	`0.00% <0.00%> (ø)`
snuba/admin/auth_roles.py	`94.20% <ø> (ø)`
snuba/admin/static/api_client.tsx	`2.13% <0.00%> (-0.35%)`	⬇️
snuba/cli/dlq_consumer.py	`0.00% <0.00%> (ø)`
snuba/migrations/group_loader.py	`95.00% <ø> (ø)`
snuba/migrations/groups.py	`97.82% <ø> (ø)`
...allocation_policies/bytes_scanned_window_policy.py	`96.05% <ø> (-0.20%)`	⬇️
snuba/query/snql/discover_entity_selection.py	`98.11% <ø> (ø)`
snuba/settings/settings_self_hosted.py	`0.00% <0.00%> (ø)`
snuba/settings/settings_test.py	`100.00% <ø> (ø)`
... and 23 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

volokluev

This looks like it's caching the reading of the file (which shouldn't be that slow). Do we have evidence that this is the thing that needs to be cached? Is it caching the API calls somewhere that I don't see?

volokluev · 2023-06-16T19:58:56Z

snuba/admin/auth.py

@@ -68,7 +72,14 @@ def get_iam_roles_from_file(user: AdminUser) -> Sequence[str]:
 def _set_roles(user: AdminUser) -> AdminUser:
 # todo: depending on provider convert user email
 # to subset of DEFAULT_ROLES based on IAM roles
- iam_roles = get_iam_roles_from_file(user)
+ key = f"roles-{user.email}"


you're caching the file reading here. Is reading from the local file really taking that much time?

I think the function name is a bit misleading here, but this function reads the file and also makes a call to check whether a member is in a group and that's where the API is being called.

I think this function should be renamed get_iam_roles_for_user.

volokluev · 2023-06-16T21:50:19Z

snuba/admin/auth.py

@@ -68,7 +72,14 @@ def get_iam_roles_from_file(user: AdminUser) -> Sequence[str]:
 def _set_roles(user: AdminUser) -> AdminUser:
 # todo: depending on provider convert user email
 # to subset of DEFAULT_ROLES based on IAM roles
- iam_roles = get_iam_roles_from_file(user)
+ key = f"roles-{user.email}"


I think this function should be renamed get_iam_roles_for_user.

volokluev · 2023-06-16T21:57:46Z

tests/admin/clickhouse_migrations/test_api.py

@@ -359,6 +364,31 @@ def test_get_iam_roles(caplog: Any) -> None:
 tool_role,
 ]

+ iam_file.close()


This test is very large and should be split up into multiple tests.

You've also tested the happy path but the unhappy path is not tested. Example: what if the redis connection fails, does that mean nobody can access admin?

Also, is _set_roles a private module function? If so, can this be tested with the public API? If not, then maybe the API is not properly thought out.

volokluev · 2023-06-16T22:00:58Z

snuba/admin/auth.py

+ iam_roles_str = redis_client.get(key)
+ iam_roles = rapidjson.loads(iam_roles_str) if iam_roles_str else None
+ if not iam_roles:
+ iam_roles = get_iam_roles_from_file(user)
+ redis_client.set(key, rapidjson.dumps(iam_roles))
+ redis_client.expire(key, settings.ADMIN_ROLES_REDIS_TTL)
+


This implementation tightly couples authentication to caching. If your cache fails to update, you'll fail the request. this should be done in such a way that:

The distinction between cache and auth is clear

If the caching fails the app still works

volokluev · 2023-06-16T22:03:20Z

snuba/admin/auth.py

+ iam_roles = rapidjson.loads(iam_roles_str) if iam_roles_str else None
+ if not iam_roles:
+ iam_roles = get_iam_roles_from_file(user)
+ redis_client.set(key, rapidjson.dumps(iam_roles))


slight performance nit: set already has an expiration parameter

davidtsuk · 2023-06-19T14:26:44Z

Added additional tests and handling for redis failure.

evanh · 2023-06-19T20:15:33Z

snuba/redis.py

@@ -137,6 +138,9 @@ class RedisClientKey(Enum):
 RedisClientKey.OPTIMIZE: _initialize_specialized_redis_cluster(
 settings.REDIS_CLUSTERS["optimize"]
 ),
+ RedisClientKey.ADMIN_AUTH: _initialize_specialized_redis_cluster(
+ settings.REDIS_CLUSTERS["admin_auth"]


Is there a PR to add admin_auth to the prod settings as well?

not yet, I'll make a PR right now

That will need to go out before this can be merged.

this stuff is super inconvenient. I wonder if we can make it so that the prod_settings file can override individual keys of the default settings file, and does not have to redefine the entire variable

this stuff is super inconvenient. I wonder if we can make it so that the prod_settings file can override individual keys of the default settings file, and does not have to redefine the entire variable

@untitaker I made a draft PR to do this here.

davidtsuk · 2023-06-26T15:19:25Z

@evanh ops pr has been merged

sentry-io · 2023-06-26T17:51:04Z

Suspect Issues

This pull request has been deployed and Sentry has observed the following issues:

‼️ KeyError: 'admin_auth' 'admin_auth' KeyError snuba/redis.py <module> F... View Issue

Have questions? Reach out to us in the #proj-github-pr-comments channel.

_{Did you find this useful? React with a 👍 or 👎}

getsentry-bot · 2023-06-26T18:03:36Z

PR reverted: f65f5ad

This reverts commit a367a37. Co-authored-by: lynnagara <1779792+lynnagara@users.noreply.github.com>

davidtsuk added 5 commits June 16, 2023 11:57

Add TTL cache for admin roles

51f8261

Unpack args to redis

6c9461b

Add redis marker to tests

11c2c0f

Use strings to encode role array

ece3330

Fix test

d0c1aab

davidtsuk marked this pull request as ready for review June 16, 2023 19:25

davidtsuk requested a review from a team as a code owner June 16, 2023 19:25

volokluev reviewed Jun 16, 2023

View reviewed changes

davidtsuk requested a review from volokluev June 16, 2023 21:22

volokluev reviewed Jun 16, 2023

View reviewed changes

davidtsuk added 2 commits June 19, 2023 10:07

Clean up code

375a8d1

Add redis failure test

f78fa66

davidtsuk requested a review from volokluev June 19, 2023 14:25

evanh reviewed Jun 19, 2023

View reviewed changes

davidtsuk requested a review from evanh June 26, 2023 15:18

evanh approved these changes Jun 26, 2023

View reviewed changes

davidtsuk merged commit a367a37 into master Jun 26, 2023
20 checks passed

davidtsuk deleted the david/feat/admin-roles-cache branch June 26, 2023 15:59

lynnagara added the Trigger: Revert label Jun 26, 2023

getsentry-bot added a commit that referenced this pull request Jun 26, 2023

Revert "feat(admin-auth): Add TTL cache for admin roles (#4358)"

f65f5ad

This reverts commit a367a37. Co-authored-by: lynnagara <1779792+lynnagara@users.noreply.github.com>

rahul-kumar-saini restored the david/feat/admin-roles-cache branch June 26, 2023 18:48

davidtsuk mentioned this pull request Jun 26, 2023

feat(admin-auth): Add TTL cache for admin roles #4421

Merged

davidtsuk deleted the david/feat/admin-roles-cache branch June 26, 2023 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(admin-auth): Add TTL cache for admin roles #4358

feat(admin-auth): Add TTL cache for admin roles #4358

davidtsuk commented Jun 16, 2023

codecov bot commented Jun 16, 2023 •

edited

Loading

volokluev left a comment

volokluev Jun 16, 2023

davidtsuk Jun 16, 2023

volokluev Jun 16, 2023

volokluev Jun 16, 2023

volokluev Jun 16, 2023

volokluev Jun 16, 2023

volokluev Jun 16, 2023

davidtsuk commented Jun 19, 2023

evanh Jun 19, 2023

davidtsuk Jun 19, 2023

evanh Jun 19, 2023

untitaker Jun 19, 2023

davidtsuk Jun 20, 2023

davidtsuk commented Jun 26, 2023

sentry-io bot commented Jun 26, 2023

getsentry-bot commented Jun 26, 2023

feat(admin-auth): Add TTL cache for admin roles #4358

feat(admin-auth): Add TTL cache for admin roles #4358

Conversation

davidtsuk commented Jun 16, 2023

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

volokluev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtsuk commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtsuk commented Jun 26, 2023

sentry-io bot commented Jun 26, 2023

Suspect Issues

getsentry-bot commented Jun 26, 2023

codecov bot commented Jun 16, 2023 •

edited

Loading